Skip to content

Commit

Permalink
Add ability to set approvers on views in sdf
Browse files Browse the repository at this point in the history
This commit adds the ability to set approvers on the views with a new
set of sdf routes, backed by a new integration test among existing ones.

To support this change, we now divide ApprovalRequirements into two
separate categories: explicit and virtual.

- Explicit requirements are derived from ApprovalRequirementDefinition
  nodes (new to WorkspaceSnapshotGraphV4!) on the graph from any node
  with an outgoing HasApprovalRequirement edge
- Virtual requirements are derived from examining a list of changes on
  the fly and are useful for determining if meta changes to permissions
  require approvals themselves (e.g. if you add a
  ApprovalRequirementDefinition, we need to determine who can approve that
  change)

Both types of requirements share an inner "rule" type with covers
the universal elements of all approval requirements:

- The ID of the entity requiring an approval
- The kind of entity requiring an approval
- The minimum number of approvals for this rule
- The approvers of this rule, which can either be individual users or
  a "permission lookup" bag used to query SpiceDB for who can approve
  the requirement

Back to virtual requirements: not only are they useful for determining
meta changes to permissions, but also for creating requirements when
nodes are deleted and creating the checksum for new approvals involving
deleted nodes. In fact, that is why checksum calculatation now requires
the hashes alongside the IDs... you can't get the merkle tree hash for a
deleted node!

As a result of these changes, detecting changes between two graphs is
now fallible (we need to get the node weight in order to get the merkle
tree hash).

Other changes in this commit include, but are not limited to, the
following:

- Use UserPk instead of String for return type involving frontend
  approvals
- Re-order enum variants and their discriminants where possible when
  dealing with ApprovalRequirementDefinition and HasApprovalRequirement
- Rename "geometry" dummy data to "view" in deserialize graph
  integration test because it could be confusing (although it is
  entirely cosmetic for the purposes of the test)

Co-authored-by: Jacob Helwig <[email protected]>
Signed-off-by: Nick Gerace <[email protected]>
  • Loading branch information
nickgerace and jhelwig committed Jan 22, 2025
1 parent 8c08d1e commit 6e97c1a
Show file tree
Hide file tree
Showing 42 changed files with 1,699 additions and 274 deletions.
144 changes: 144 additions & 0 deletions lib/dal/src/approval_requirement.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
//! This module provides functionality for creating, updating, deleting, getting and listing
//! approval requirements when applying a change set.
#![warn(
bad_style,
clippy::missing_panics_doc,
clippy::panic,
clippy::panic_in_result_fn,
clippy::unwrap_in_result,
clippy::unwrap_used,
dead_code,
improper_ctypes,
missing_debug_implementations,
// TODO(nick): restore this and clean up the mess.
// missing_docs,
no_mangle_generic_items,
non_shorthand_field_patterns,
overflowing_literals,
path_statements,
patterns_in_fns_without_body,
unconditional_recursion,
unreachable_pub,
unused,
unused_allocation,
unused_comparisons,
unused_parens,
while_true
)]

use std::collections::{HashMap, HashSet};

use si_events::merkle_tree_hash::MerkleTreeHash;
use si_id::{ulid::Ulid, ApprovalRequirementDefinitionId, EntityId, UserPk};
use telemetry::prelude::*;
use thiserror::Error;

use crate::{
workspace_snapshot::{
graph::detector::Change, traits::approval_requirement::ApprovalRequirementExt,
},
DalContext, WorkspaceSnapshotError,
};

pub use crate::workspace_snapshot::traits::approval_requirement::{
ApprovalRequirementApprover, ApprovalRequirementRule,
};

#[allow(missing_docs)]
#[derive(Debug, Error)]
pub enum ApprovalRequirementError {
#[error("workspace snapshot error: {0}")]
WorkspaceSnapshot(#[from] WorkspaceSnapshotError),
}

type Result<T> = std::result::Result<T, ApprovalRequirementError>;

#[derive(Debug)]
pub struct ApprovalRequirementExplicit {
pub id: ApprovalRequirementDefinitionId,
pub rule: ApprovalRequirementRule,
}

#[derive(Debug)]
pub enum ApprovalRequirement {
Explicit(ApprovalRequirementExplicit),
Virtual(ApprovalRequirementRule),
}

impl ApprovalRequirement {
#[instrument(
name = "approval_requirement.new_definition",
level = "debug",
skip_all
)]
pub async fn new_definition(
ctx: &DalContext,
entity_id: impl Into<Ulid>,
minimum_approvers_count: usize,
approvers: HashSet<ApprovalRequirementApprover>,
) -> Result<ApprovalRequirementDefinitionId> {
ctx.workspace_snapshot()?
.new_definition(ctx, entity_id.into(), minimum_approvers_count, approvers)
.await
.map_err(Into::into)
}

#[instrument(
name = "approval_requirement.remove_definition",
level = "debug",
skip_all
)]
pub async fn remove_definition(
ctx: &DalContext,
id: ApprovalRequirementDefinitionId,
) -> Result<()> {
ctx.workspace_snapshot()?
.remove_definition(id)
.await
.map_err(Into::into)
}

#[instrument(
name = "approval_requirement.add_individual_approver_for_definition",
level = "debug",
skip_all
)]
pub async fn add_individual_approver_for_definition(
ctx: &DalContext,
id: ApprovalRequirementDefinitionId,
user_id: UserPk,
) -> Result<()> {
ctx.workspace_snapshot()?
.add_individual_approver_for_definition(ctx, id, user_id)
.await
.map_err(Into::into)
}

#[instrument(
name = "approval_requirement.remove_individual_approver_for_definition",
level = "debug",
skip_all
)]
pub async fn remove_individual_approver_for_definition(
ctx: &DalContext,
id: ApprovalRequirementDefinitionId,
user_id: UserPk,
) -> Result<()> {
ctx.workspace_snapshot()?
.remove_individual_approver_for_definition(ctx, id, user_id)
.await
.map_err(Into::into)
}

#[instrument(name = "approval_requirement.list", level = "debug", skip_all)]
pub async fn list(
ctx: &DalContext,
changes: &[Change],
) -> Result<(Vec<Self>, HashMap<EntityId, MerkleTreeHash>)> {
ctx.workspace_snapshot()?
.approval_requirements_for_changes(ctx, changes)
.await
.map_err(Into::into)
}
}
5 changes: 3 additions & 2 deletions lib/dal/src/change_set/approval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
use chrono::{DateTime, Utc};
use serde::{Deserialize, Serialize};
use si_data_pg::{PgError, PgRow};
use si_events::merkle_tree_hash::MerkleTreeHash;
use si_id::{ChangeSetApprovalId, ChangeSetId, EntityId, UserPk};
use telemetry::prelude::*;
use thiserror::Error;
Expand Down Expand Up @@ -88,11 +89,11 @@ impl ChangeSetApproval {
pub async fn new(
ctx: &DalContext,
status: ChangeSetApprovalStatus,
ids: Vec<EntityId>,
ids_with_hashes: Vec<(EntityId, MerkleTreeHash)>,
) -> Result<Self> {
let checksum = ctx
.workspace_snapshot()?
.calculate_checksum(ctx, ids)
.calculate_checksum(ctx, ids_with_hashes)
.await?;

let change_set_id = ctx.change_set_id();
Expand Down
3 changes: 3 additions & 0 deletions lib/dal/src/diagram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use std::{
use telemetry::prelude::*;
use thiserror::Error;

use crate::approval_requirement::ApprovalRequirementError;
use crate::workspace_snapshot::node_weight::NodeWeight;
use crate::{
attribute::{
Expand Down Expand Up @@ -46,6 +47,8 @@ use si_layer_cache::LayerDbError;
#[remain::sorted]
#[derive(Error, Debug)]
pub enum DiagramError {
#[error("approval requirement error: {0}")]
ApprovalRequirement(#[from] ApprovalRequirementError),
#[error("attribute prototype argument error: {0}")]
AttributePrototypeArgument(#[from] AttributePrototypeArgumentError),
#[error("attribute prototype not found")]
Expand Down
3 changes: 2 additions & 1 deletion lib/dal/src/diagram/geometry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,8 @@ impl Geometry {
| NodeWeight::SchemaVariant(_)
| NodeWeight::ManagementPrototype(_)
| NodeWeight::Geometry(_)
| NodeWeight::View(_) => {
| NodeWeight::View(_)
| NodeWeight::ApprovalRequirementDefinition(_) => {
return Err(DiagramError::GeometryCannotRepresentNodeWeight(
node_weight.into(),
))
Expand Down
14 changes: 14 additions & 0 deletions lib/dal/src/layer_db_types/content_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use strum::EnumDiscriminants;
use thiserror::Error;

use crate::action::prototype::ActionKind;
use crate::approval_requirement::ApprovalRequirementApprover;
use crate::validation::ValidationStatus;
use crate::{
action::ActionCompletionStatus, func::argument::FuncArgumentKind, prop::WidgetOptions,
Expand Down Expand Up @@ -60,6 +61,7 @@ pub enum ContentTypes {
ManagementPrototype(ManagementPrototypeContent),
Geometry(GeometryContent),
View(ViewContent),
ApprovalRequirementDefinition(ApprovalRequirementDefinitionContent),
}

macro_rules! impl_into_content_types {
Expand Down Expand Up @@ -117,6 +119,7 @@ impl_into_content_types!(Validation);
impl_into_content_types!(ManagementPrototype);
impl_into_content_types!(Geometry);
impl_into_content_types!(View);
impl_into_content_types!(ApprovalRequirementDefinition);

// Here we've broken the Foo, FooContent convention so we need to implement
// these traits manually
Expand Down Expand Up @@ -683,3 +686,14 @@ pub struct ManagementPrototypeContentV1 {
pub managed_schemas: Option<HashSet<SchemaId>>,
pub description: Option<String>,
}

#[derive(Debug, Clone, EnumDiscriminants, Serialize, Deserialize, PartialEq)]
pub enum ApprovalRequirementDefinitionContent {
V1(ApprovalRequirementDefinitionContentV1),
}

#[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
pub struct ApprovalRequirementDefinitionContentV1 {
pub minimum: usize,
pub approvers: HashSet<ApprovalRequirementApprover>,
}
1 change: 1 addition & 0 deletions lib/dal/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use tokio::time::Instant;

pub mod action;
pub mod actor_view;
pub mod approval_requirement;
pub mod attribute;
pub mod audit_logging;
pub mod authentication_prototype;
Expand Down
3 changes: 2 additions & 1 deletion lib/dal/src/schema/variant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2296,7 +2296,8 @@ impl SchemaVariant {
| EdgeWeightKindDiscriminants::SocketValue
| EdgeWeightKindDiscriminants::ValidationOutput
| EdgeWeightKindDiscriminants::Manages
| EdgeWeightKindDiscriminants::DiagramObject => {}
| EdgeWeightKindDiscriminants::DiagramObject
| EdgeWeightKindDiscriminants::HasApprovalRequirement => {}
}
}

Expand Down
Loading

0 comments on commit 6e97c1a

Please sign in to comment.