From 625556ae5873f112f44c4e6a14c4a35db54a4ebe Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 2 Sep 2026 16:15:23 -0400 Subject: [PATCH] feat(didbot-policy)!: carry before/after values in a write's diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diff now holds Change entries (path, before, after) instead of bare paths, with absence itself a value. This resolves the create/delete asymmetry a path-only diff forced onto evaluators (a delete always tripping a path guard regardless of changed_paths, while creates were trusted to report their own) semantically instead of by special-casing WriteAction: "may not change to a different value" is before.is_some() && after != before, correct on every action with no WriteAction::Delete arm. The empty-diff-for-delete allowance is gone; the contract is now exact, and it is the server's to enforce since it already holds both sides of the write. Subject::Write also gains agent_attributes (currently just the agent's handle), an explicit and extensible set the write path populates and evaluators read. It is what lets an "allowlist" policy — e.g. displayName may be set only to the agent's own handle — stay a deny rule: deny when the new value doesn't match, which subtracts from what is permitted like every other policy and reintroduces no allow variant. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: Ibfda3b5fbca36f08342b02e536e75ff857f95f73 --- Cargo.lock | 1 + crates/didbot-policy/Cargo.toml | 9 +- crates/didbot-policy/src/index.rs | 18 +- crates/didbot-policy/src/lib.rs | 244 ++++++++++++++++++++++++++-- crates/didbot-policy/src/subject.rs | 77 +++++++-- 5 files changed, 319 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ed797e20..05d9a5c7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1213,6 +1213,7 @@ dependencies = [ name = "didbot-policy" version = "0.1.0" dependencies = [ + "serde_json", "thiserror 2.0.20", ] diff --git a/crates/didbot-policy/Cargo.toml b/crates/didbot-policy/Cargo.toml index e7453618..400dffdb 100644 --- a/crates/didbot-policy/Cargo.toml +++ b/crates/didbot-policy/Cargo.toml @@ -9,10 +9,13 @@ repository.workspace = true publish.workspace = true [dependencies] -# For EvalError. Nothing else: this crate is the machinery underneath every -# evaluator, including out-of-process ones, and has no business reaching for -# I/O, async, or a serialization format of its own. +# For EvalError. thiserror.workspace = true +# For Change's before/after values. Already a workspace dependency, and the +# atproto record format this crate's writes ultimately describe is JSON, so +# this is the value type every evaluator already expects, not a new +# serialization format of this crate's own choosing. +serde_json.workspace = true [lints] workspace = true diff --git a/crates/didbot-policy/src/index.rs b/crates/didbot-policy/src/index.rs index 8e865dd3..0aca7bdc 100644 --- a/crates/didbot-policy/src/index.rs +++ b/crates/didbot-policy/src/index.rs @@ -12,7 +12,7 @@ use std::sync::Arc; use crate::outcome::Evaluator; use crate::policy::{Applicability, EvaluatorId, PolicyDeclaration, PolicyId, Scope}; -use crate::subject::{Subject, WriteAction}; +use crate::subject::{Change, Subject, WriteAction}; /// A trie over dotted changed-path segments. /// @@ -109,11 +109,17 @@ impl WriteIndex { /// lookups (the specific collection, the `Scope::Any` bucket) and, /// only when one of those exists, a walk bounded by the write's own /// changed paths — never by the number of policies registered. + /// + /// `changes` carries before/after values (see [`Change`]); this index + /// only ever reads `change.path` out of it — routing is on where a + /// write touched, not on what it changed to. A value predicate belongs + /// in the evaluator that a matched policy names, with the full + /// `Change` and [`Subject`] available to it. fn candidates( &self, collection: &str, action: WriteAction, - changed_paths: &[&str], + changes: &[Change<'_>], ) -> Vec { let mut out = Vec::new(); for trie in [ @@ -124,8 +130,8 @@ impl WriteIndex { .flatten() { let mut matched = trie.paths.here.clone(); - for path in changed_paths { - trie.paths.collect(path, &mut matched); + for change in changes { + trie.paths.collect(change.path, &mut matched); } matched.retain(|policy| { trie.actions @@ -222,7 +228,7 @@ impl PolicyTree { } => self .index .write_decision - .candidates(collection, *action, diff.changed_paths), + .candidates(collection, *action, diff.changes), Subject::Grant { client_id, .. } => self.index.grant_decision.candidates(client_id), } } @@ -242,7 +248,7 @@ impl PolicyTree { } => self .index .write_observation - .candidates(collection, *action, diff.changed_paths), + .candidates(collection, *action, diff.changes), Subject::Grant { client_id, .. } => self.index.grant_observation.candidates(client_id), } } diff --git a/crates/didbot-policy/src/lib.rs b/crates/didbot-policy/src/lib.rs index 9097afab..23f0e310 100644 --- a/crates/didbot-policy/src/lib.rs +++ b/crates/didbot-policy/src/lib.rs @@ -11,8 +11,11 @@ //! //! # What is here, and why //! -//! - [`Subject`] and [`WriteAction`] — what a policy is asked about. Two -//! shapes, one set of mechanics, per the epic's two trees. +//! - [`Subject`], [`WriteAction`], [`Diff`], [`Change`] and +//! [`AgentAttributes`] — what a policy is asked about. Two subject +//! shapes, one set of mechanics, per the epic's two trees; a write's +//! diff carries before/after values, not just paths, so a rule reads a +//! changed value rather than only knowing that a path changed. //! - [`Applicability`], [`Scope`], [`ActionSet`] and [`PolicyDeclaration`] — //! what a policy declares statically. This is what goes in the tree; a //! policy's actual judgement stays behind the [`Evaluator`] trait, opaque @@ -41,10 +44,14 @@ //! lexicon that carries it unsettled; this crate only consumes the //! [`PolicyDeclaration`] a wrapper resolves to, never the wrapper itself. //! -//! No position on what a delete means. [`WriteAction::Delete`] is routable — -//! a policy can declare it applies to deletes, or does not — but nothing -//! here decides what a delete's diff contains; that is left to whichever -//! evaluator a delete-aware policy names. +//! No position on what a delete means beyond what [`Diff`] already makes +//! exact: every path whose value differs between before and after, absence +//! included as a value. A rule that reads `before`/`after` resolves create +//! and delete correctly without ever branching on +//! [`WriteAction`] — see [`Change`]'s doc comment. `WriteAction` stays +//! useful for routing (a policy can still declare it applies only to +//! deletes) without being load-bearing for a value-shaped rule's +//! correctness. mod evaluate; mod index; @@ -56,7 +63,7 @@ pub use evaluate::{combine, evaluate, Verdict}; pub use index::{PolicyTree, PolicyTreeBuilder}; pub use outcome::{EvalError, Evaluator, Lifecycle, Observed, Outcome, Staleness}; pub use policy::{ActionSet, Applicability, EvaluatorId, PolicyDeclaration, PolicyId, Scope}; -pub use subject::{Diff, Subject, WriteAction}; +pub use subject::{AgentAttributes, Change, Diff, Subject, WriteAction}; #[cfg(test)] mod tests { @@ -96,15 +103,33 @@ mod tests { } } + /// A placeholder JSON value, for tests that only care which paths a + /// diff touched, not what changed at them. + fn dummy_value() -> &'static serde_json::Value { + static VALUE: std::sync::OnceLock = std::sync::OnceLock::new(); + VALUE.get_or_init(|| serde_json::Value::Bool(true)) + } + + /// Build a write [`Subject`] that touched exactly `paths`, each + /// reported as newly present — enough for index-routing tests, which + /// never inspect `before`/`after`. fn write<'a>(collection: &'a str, action: WriteAction, paths: &'a [&'a str]) -> Subject<'a> { + let changes: Vec> = paths + .iter() + .map(|&path| Change { + path, + before: None, + after: Some(dummy_value()), + }) + .collect(); + let changes: &'a [Change<'a>] = Box::leak(changes.into_boxed_slice()); Subject::Write { agent: "did:plc:agent", + agent_attributes: AgentAttributes::default(), client_id: "client-1", collection, action, - diff: Diff { - changed_paths: paths, - }, + diff: Diff { changes }, } } @@ -323,4 +348,203 @@ mod tests { Verdict::Reject { ref reason, .. } if reason == "b fires first" )); } + + /// A real value-reading evaluator: *"displayName may not change to a + /// different value once it has one"*. Written with no `WriteAction` + /// arm at all — before/after resolves create and delete on their own, + /// which is the whole point of carrying values in [`Diff`] instead of + /// bare paths. + struct NoRenaming; + + impl Evaluator for NoRenaming { + fn evaluate(&self, subject: &Subject<'_>, _policy: PolicyId) -> Result { + let Subject::Write { diff, .. } = subject else { + return Ok(Outcome::Allow); + }; + for change in diff.changes { + if change.path == "displayName" + && change.before.is_some() + && change.after != change.before + { + return Ok(Outcome::Reject { + reason: "displayName may not change".into(), + }); + } + } + Ok(Outcome::Allow) + } + fn observe(&self, _event: &Observed<'_>) {} + fn lifecycle(&self, _event: &Lifecycle<'_>) {} + fn staleness(&self) -> Staleness { + Staleness::Linearized + } + } + + fn tree_with_evaluator(evaluator: Arc) -> PolicyTree { + let mut builder = PolicyTree::builder(); + let evaluator_id = builder.register_evaluator(evaluator); + builder.add_policy(PolicyDeclaration { + applicability: Applicability::Write { + collections: Scope::Any, + actions: ActionSet::ALL, + path_prefixes: Scope::Only(vec!["displayName".into()]), + }, + observes: Applicability::Write { + collections: Scope::Any, + actions: ActionSet::ALL, + path_prefixes: Scope::Any, + }, + staleness: Staleness::Linearized, + evaluator: evaluator_id, + }); + builder.build() + } + + #[test] + fn before_after_resolves_create_and_delete_without_an_action_arm() { + let tree = tree_with_evaluator(Arc::new(NoRenaming)); + let old = serde_json::Value::String("alice".into()); + let new = serde_json::Value::String("bob".into()); + + // Create: no prior value, so nothing to protect. + let created = Change { + path: "displayName", + before: None, + after: Some(&new), + }; + let subject = Subject::Write { + agent: "did:plc:agent", + agent_attributes: AgentAttributes::default(), + client_id: "client-1", + collection: "app.bsky.actor.profile", + action: WriteAction::Create, + diff: Diff { + changes: &[created], + }, + }; + assert_eq!(evaluate(&subject, &tree), Verdict::Allow); + + // Update to a different value: denied. + let renamed = Change { + path: "displayName", + before: Some(&old), + after: Some(&new), + }; + let subject = Subject::Write { + agent: "did:plc:agent", + agent_attributes: AgentAttributes::default(), + client_id: "client-1", + collection: "app.bsky.actor.profile", + action: WriteAction::Update, + diff: Diff { + changes: &[renamed], + }, + }; + assert!(matches!(evaluate(&subject, &tree), Verdict::Reject { .. })); + + // Delete: the value existed and is now gone — also denied, with no + // `WriteAction::Delete` special case anywhere in `NoRenaming`. + let deleted = Change { + path: "displayName", + before: Some(&old), + after: None, + }; + let subject = Subject::Write { + agent: "did:plc:agent", + agent_attributes: AgentAttributes::default(), + client_id: "client-1", + collection: "app.bsky.actor.profile", + action: WriteAction::Delete, + diff: Diff { + changes: &[deleted], + }, + }; + assert!(matches!(evaluate(&subject, &tree), Verdict::Reject { .. })); + } + + /// An "allowlist" policy — *"displayName may be set only to the agent's + /// own handle"* — is a deny rule like any other: it denies whenever + /// the new value is not the handle. No allow variant is needed or + /// used; this is [`AgentAttributes`]'s worked example. + struct HandleOnly; + + impl Evaluator for HandleOnly { + fn evaluate(&self, subject: &Subject<'_>, _policy: PolicyId) -> Result { + let Subject::Write { + diff, + agent_attributes, + .. + } = subject + else { + return Ok(Outcome::Allow); + }; + for change in diff.changes { + if change.path != "displayName" { + continue; + } + let Some(after) = change.after else { + continue; + }; + let allowed = agent_attributes + .handle + .is_some_and(|handle| after.as_str() == Some(handle)); + if !allowed { + return Ok(Outcome::Reject { + reason: "displayName must be the agent's handle".into(), + }); + } + } + Ok(Outcome::Allow) + } + fn observe(&self, _event: &Observed<'_>) {} + fn lifecycle(&self, _event: &Lifecycle<'_>) {} + fn staleness(&self) -> Staleness { + Staleness::Linearized + } + } + + #[test] + fn an_allowlist_is_expressed_as_a_deny_rule() { + let tree = tree_with_evaluator(Arc::new(HandleOnly)); + let handle_value = serde_json::Value::String("alice.did.bot".into()); + let other_value = serde_json::Value::String("someone-else".into()); + + let matches_handle = Change { + path: "displayName", + before: None, + after: Some(&handle_value), + }; + let subject = Subject::Write { + agent: "did:plc:agent", + agent_attributes: AgentAttributes { + handle: Some("alice.did.bot"), + }, + client_id: "client-1", + collection: "app.bsky.actor.profile", + action: WriteAction::Update, + diff: Diff { + changes: &[matches_handle], + }, + }; + assert_eq!(evaluate(&subject, &tree), Verdict::Allow); + + let does_not_match = Change { + path: "displayName", + before: None, + after: Some(&other_value), + }; + let subject = Subject::Write { + agent: "did:plc:agent", + agent_attributes: AgentAttributes { + handle: Some("alice.did.bot"), + }, + client_id: "client-1", + collection: "app.bsky.actor.profile", + action: WriteAction::Update, + diff: Diff { + changes: &[does_not_match], + }, + }; + assert!(matches!(evaluate(&subject, &tree), Verdict::Reject { .. })); + } } diff --git a/crates/didbot-policy/src/subject.rs b/crates/didbot-policy/src/subject.rs index 634df60c..a32df0ae 100644 --- a/crates/didbot-policy/src/subject.rs +++ b/crates/didbot-policy/src/subject.rs @@ -23,24 +23,76 @@ pub enum WriteAction { Delete, } -/// The changed paths of a write, as the applicability index's key. +/// One field's value before and after a write, at a dotted path such as +/// `"displayName"` or `"profile.bio"`. +/// +/// `before` and `after` are `None` when the path was absent on that side — +/// absence is a value, not a missing entry. This is what lets a rule like +/// *"displayName may not change to a different value"* resolve without an +/// action special case: it is `before.is_some() && after != before`. A +/// create has `before: None` everywhere, so it never trips such a rule — +/// correct, there is no prior identity to protect. A delete has +/// `after: None` everywhere, so it always trips one guarding a path that +/// was present — also correct, with no [`WriteAction::Delete`] arm anywhere +/// in the rule. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Change<'a> { + /// The dotted field path this change is at. + pub path: &'a str, + /// The value before the write, or `None` if the path was absent. + pub before: Option<&'a serde_json::Value>, + /// The value after the write, or `None` if the path is now absent. + pub after: Option<&'a serde_json::Value>, +} + +/// What changed in a write, as the applicability index's key. /// /// The diff has to be computed to evaluate anything, so diffing *is* the /// indexing step: a write's changed paths are walked through the tree once, /// and a write matching no policy has paid only for that walk. /// -/// This is a minimal, storage-agnostic view — a list of dotted field paths — -/// deliberately not the richer diff type that `didbot-data`'s merkle search -/// tree produces. That type carries proof material this crate has no use -/// for, and coupling the policy core to one storage engine's diff shape -/// would be a dependency in the wrong direction. +/// The contract is exact: `changes` holds every path whose value differs +/// between before and after, where absence is a value — so a create lists +/// every path it introduces, a delete lists every path it removes, and +/// there is no case where an evaluator sees an empty diff for a write that +/// changed something. The server computes this, holding both sides of the +/// write already, so it is enforced once in the write path rather than +/// trusted from whatever produced the write. +/// +/// This is a minimal, storage-agnostic view, deliberately not the richer +/// diff type that `didbot-data`'s merkle search tree produces. That type +/// carries proof material this crate has no use for, and coupling the +/// policy core to one storage engine's diff shape would be a dependency in +/// the wrong direction. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub struct Diff<'a> { - /// Dotted field paths that changed, e.g. `"displayName"` or - /// `"profile.bio"`. Empty for a write whose evaluators only need to know - /// that it happened — a delete with nothing more specific to say, for - /// instance. - pub changed_paths: &'a [&'a str], + /// Every path that changed, each with its value before and after. + pub changes: &'a [Change<'a>], +} + +/// Attributes about the writing agent a policy may need beyond its DID, +/// explicit and extensible rather than a bag. +/// +/// This is the contract between this crate, the write path that populates +/// it, and the evaluators that read it: an evaluator names the attribute it +/// needs from here, and the write path knows, from this same type, what it +/// is obliged to fill in. A `None` means the write path could not resolve +/// the attribute for this write, not that the agent lacks one — an +/// evaluator that requires it should treat `None` the way it treats any +/// other missing precondition. +/// +/// Adding an allowlist-shaped policy — *"displayName may be set only to the +/// agent's handle or a small fixed list of values"* — does not reintroduce +/// an allow: it is expressed as *deny if the new value matches none of +/// them*, read off [`Change::after`](crate::subject::Change::after) and +/// this handle, which subtracts from what is permitted exactly like every +/// other policy. Monotonicity does not distinguish an allowlist from a +/// blocklist; both are deny rules, just aimed at different values. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct AgentAttributes<'a> { + /// The agent's current handle, if the write path resolved one for this + /// write. + pub handle: Option<&'a str>, } /// What one policy is asked to decide about. @@ -56,6 +108,9 @@ pub enum Subject<'a> { Write { /// The agent whose repository this is. agent: &'a str, + /// Other attributes of that agent a policy may reference — see + /// [`AgentAttributes`]. + agent_attributes: AgentAttributes<'a>, /// The application presenting the write, from its OAuth client id. client_id: &'a str, /// The collection (NSID) being written. -- 2.51.2