diff --git a/crates/feature-permissions/README.md b/crates/feature-permissions/README.md index 700545288..c6816a4bb 100644 --- a/crates/feature-permissions/README.md +++ b/crates/feature-permissions/README.md @@ -19,9 +19,11 @@ the same references. Installing a declaration validates its candidate graph atomically. Duplicate definitions, unknown references and cycles reject the update without changing the prior catalog. Duplicate assignment paths are deduplicated, with distinct provenance paths retained in deterministic order. Invalid assignment -references deny the whole assignment with diagnostics. Disabled contributions are -omitted independently; removing a Feature retires its handles and withdraws only -its declarations. Dangling references deny rather than recovering an old result. +references deny the whole assignment with diagnostics. Disabled positive +contributions are omitted independently. Explicit exclusions still traverse disabled +Features' declared keys and set edges, so disabling a Plugin cannot revive another +Feature's excluded operation. Removing a Feature retires its handles and withdraws +only its declarations. Dangling references deny rather than recovering an old result. Every change invalidates prior [`ResolvedPermissions`]. The Host re-resolves the current assignment before use. A resolution retains its actual catalog snapshot; diff --git a/crates/feature-permissions/src/registry.rs b/crates/feature-permissions/src/registry.rs index af9cb8d3e..14508e30d 100644 --- a/crates/feature-permissions/src/registry.rs +++ b/crates/feature-permissions/src/registry.rs @@ -317,11 +317,11 @@ impl PermissionHost { None => projection.diagnostics.push(PermissionDiagnostic::Unset), Some(a) if a.revoked => projection.diagnostics.push(PermissionDiagnostic::Revoked), Some(a) => { - let mut expansion = Expansion::new(self); + let mut expansion = Expansion::new(self, ExpansionPurpose::Grant); for reference in &a.grants { expansion.reference(reference, vec![]); } - let mut exclusions = Expansion::new(self); + let mut exclusions = Expansion::new(self, ExpansionPurpose::Exclusion); for reference in &a.exclusions { exclusions.reference(reference, vec![]); } @@ -391,7 +391,7 @@ impl PermissionHost { requested: &[PermissionKey], ) -> Result { self.check_resolution(ceiling)?; - let mut expansion = Expansion::new(self); + let mut expansion = Expansion::new(self, ExpansionPurpose::Grant); for key in requested { expansion.key(key, vec![]); } @@ -572,15 +572,25 @@ fn validate_graph( Ok(()) } +#[derive(PartialEq, Eq)] +enum ExpansionPurpose { + Grant, + // Disabling an owner withdraws positive contributions, not the meaning of + // an explicit deny. Its retained declaration still defines exclusion edges. + Exclusion, +} + struct Expansion<'a> { host: &'a PermissionHost, + purpose: ExpansionPurpose, allowed: BTreeMap>>, diagnostics: BTreeSet, } impl<'a> Expansion<'a> { - fn new(host: &'a PermissionHost) -> Self { + fn new(host: &'a PermissionHost, purpose: ExpansionPurpose) -> Self { Self { host, + purpose, allowed: BTreeMap::new(), diagnostics: BTreeSet::new(), } @@ -620,7 +630,9 @@ impl<'a> Expansion<'a> { .insert(PermissionDiagnostic::DisabledFeature( record.owner.clone(), )); - continue; + if self.purpose == ExpansionPurpose::Grant { + continue; + } } let mut next = path.clone(); next.push(OriginStep::Set(set.clone())); @@ -656,7 +668,9 @@ impl<'a> Expansion<'a> { if !self.host.catalog.features[&record.owner].enabled { self.diagnostics .insert(PermissionDiagnostic::DisabledFeature(record.owner.clone())); - return; + if self.purpose == ExpansionPurpose::Grant { + return; + } } if path.contains(&OriginStep::Key(key.clone())) { return; diff --git a/crates/feature-permissions/tests/resolution.rs b/crates/feature-permissions/tests/resolution.rs index 668d73666..320492f90 100644 --- a/crates/feature-permissions/tests/resolution.rs +++ b/crates/feature-permissions/tests/resolution.rs @@ -910,3 +910,125 @@ fn current_parent_execution_delegation_and_explicit_revocation_each_limit_childr PermissionError::RevokedGrant ); } + +fn assert_exclusion_survives_feature_disable(exclusion: PermissionReference) { + let mut host = host("workspace-a"); + let (_, ticket) = register(&mut host, "builtin:ticket", "ticket"); + host.declare( + &ticket, + declaration( + &[("ticket.read", &[]), ("ticket.keep", &[])], + &[( + "companion", + vec![keys("ticket", &["ticket.read", "ticket.keep"])], + )], + ), + ) + .unwrap(); + let (plugin, bundle) = register(&mut host, "plugin:bundle", "bundle"); + host.declare( + &bundle, + declaration( + &[ + ("bundle.block", &["ticket.read"]), + ("bundle.block.nested", &["bundle.block"]), + ("bundle.query", &[]), + ], + &[ + ("companion", vec![keys("bundle", &["bundle.query"])]), + ("blocked", vec![keys("bundle", &["bundle.block.nested"])]), + ("blocked-alias", vec![set("blocked")]), + ], + ), + ) + .unwrap(); + let unrestricted_policy = assignment(vec![set("companion")]); + let restricted_policy = PermissionAssignment { + exclusions: vec![exclusion], + ..unrestricted_policy.clone() + }; + // Issue before the deny is added, so a frozen child/Job cannot evade it. + let initial = host.resolve(Some(&unrestricted_policy)); + let requested = [key("ticket.read"), key("ticket.keep")]; + let child = host.issue_delegation(&initial, &requested).unwrap(); + let job = host + .issue_job_grant("job-a".into(), &initial, &requested) + .unwrap(); + + for enabled in [true, false, true] { + host.set_enabled(&plugin, enabled).unwrap(); + let unrestricted = host.resolve(Some(&unrestricted_policy)); + let restricted = host.resolve(Some(&restricted_policy)); + assert!( + !host + .allows(&ticket, &restricted, &key("ticket.read")) + .unwrap() + ); + assert!( + host.allows(&ticket, &restricted, &key("ticket.keep")) + .unwrap() + ); + assert_eq!( + host.allows(&bundle, &restricted, &key("bundle.query")) + .unwrap(), + enabled, + "disable must still withdraw the Plugin's positive contribution" + ); + assert!( + restricted + .projection() + .diagnostics + .contains(&PermissionDiagnostic::Excluded(key("ticket.read"))) + ); + assert_eq!( + restricted + .projection() + .diagnostics + .contains(&PermissionDiagnostic::DisabledFeature( + FeatureIdentity::parse("plugin:bundle").unwrap() + )), + !enabled + ); + assert_eq!( + host.issue_delegation(&restricted, &[key("ticket.read")]) + .unwrap_err(), + PermissionError::DelegationExceeded(key("ticket.read")) + ); + assert_eq!( + host.issue_job_grant("job-b".into(), &restricted, &[key("ticket.read")]) + .unwrap_err(), + PermissionError::DelegationExceeded(key("ticket.read")) + ); + for (execution, delegation) in [ + (&restricted, &unrestricted), + (&unrestricted, &restricted), + (&restricted, &restricted), + ] { + assert_keys( + &host.resolve_limited(execution, delegation, &child).unwrap(), + &["ticket.keep"], + ); + assert_keys( + &host + .resolve_job("job-a", execution, delegation, &child, &job) + .unwrap(), + &["ticket.keep"], + ); + } + } +} + +#[test] +fn disabled_feature_cannot_erase_direct_cross_feature_exclusion() { + assert_exclusion_survives_feature_disable(keys("bundle", &["bundle.block"])); +} + +#[test] +fn disabled_feature_cannot_erase_multilevel_cross_feature_exclusion() { + assert_exclusion_survives_feature_disable(keys("bundle", &["bundle.block.nested"])); +} + +#[test] +fn disabled_feature_cannot_erase_set_cross_feature_exclusion() { + assert_exclusion_survives_feature_disable(set("blocked-alias")); +}