From 4e2f18a95ee971b06d5a6ee596b5afe065c0f431 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Wed, 27 May 2026 13:35:22 +0200 Subject: [PATCH] mr-668: split PolicyEngine::load into kind-typed loaders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-fix, every caller of `PolicyEngine::load(path, graph_id)` passed *some* `graph_id` argument — even when the policy was server-scoped and Cedar's resolution would never touch a Graph entity. The server-level loader at lib.rs passed the meaningless sentinel `"server"`. A graph policy file containing a `graph_list` rule compiled fine; a server policy file containing a `read` rule compiled fine. Both silently no-op'd at request time because the engine kind and the rule's resource kind disagreed. Correct-by-design fix: replace `load` with two kind-typed loaders. * `PolicyEngine::load_graph(path, graph_id)` — for per-graph policy files. Rejects any rule whose action `resource_kind()` is `Server`. * `PolicyEngine::load_server(path)` — for server-level policy files. Takes no `graph_id`: server-scoped actions resolve against the singleton `Omnigraph::Server::"root"` entity, never a Graph. Rejects any rule whose action `resource_kind()` is `Graph`. The old `load` is hard-deleted in the same commit because every in-tree consumer migrates here (no semver promise on the workspace crate, no external pinners). New `PolicyEngineKind` enum types the loader's intent; `validate_kind_alignment` is the load-time check that closes the "wrong action, wrong file, silent no-op" class — operators get a load-time error instead of confused-and- silent behavior at request time. Callsites migrated: * server lib.rs:374 (single-mode per-graph) → load_graph * server lib.rs:1065 (multi-mode server) → load_server * server lib.rs:1103 (multi-mode per-graph) → load_graph * CLI main.rs:732 (resolve_policy_engine) → load_graph * tests/server.rs ×5 (4 graph, 1 server) → load_graph/load_server * policy_engine_chassis.rs → load_graph Four new in-source tests pin the contract: both rejection paths and both positive paths. Closes the "operator puts an action in the wrong file and the rule silently never matches" class. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-cli/src/main.rs | 2 +- crates/omnigraph-policy/src/lib.rs | 221 +++++++++++++++++- crates/omnigraph-server/src/lib.rs | 6 +- crates/omnigraph-server/tests/server.rs | 10 +- .../omnigraph/tests/policy_engine_chassis.rs | 2 +- 5 files changed, 228 insertions(+), 13 deletions(-) diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 37e17f5..da121eb 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -729,7 +729,7 @@ fn resolve_policy_engine(config: &OmnigraphConfig) -> Result { let policy_file = config .resolve_policy_file() .ok_or_else(|| color_eyre::eyre::eyre!("policy.file must be set in omnigraph.yaml"))?; - PolicyEngine::load(&policy_file, &policy_graph_id(config)) + PolicyEngine::load_graph(&policy_file, &policy_graph_id(config)) } /// Open a local-URI graph and, when `policy.file` is configured in diff --git a/crates/omnigraph-policy/src/lib.rs b/crates/omnigraph-policy/src/lib.rs index 84c7c05..83d387a 100644 --- a/crates/omnigraph-policy/src/lib.rs +++ b/crates/omnigraph-policy/src/lib.rs @@ -115,6 +115,26 @@ pub enum PolicyResourceKind { Server, } +/// Which kind of policy file the caller is loading. Drives the +/// load-time validation that catches a "wrong action in wrong file" +/// mistake — a graph policy with `graph_list` rules, or a server +/// policy with `read` rules, both compile silently as Cedar but +/// never match any actual request. Typing the loader makes the +/// mistake a load-time error. +/// +/// Pairs with [`PolicyAction::resource_kind`]: every action's resource +/// kind must match the engine kind it's loaded under. +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub enum PolicyEngineKind { + /// Engine is loaded for a single graph; only actions whose + /// `resource_kind()` is `PolicyResourceKind::Graph` are allowed. + Graph, + /// Engine is loaded for server-level management endpoints; only + /// actions whose `resource_kind()` is `PolicyResourceKind::Server` + /// are allowed. + Server, +} + impl fmt::Display for PolicyAction { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.write_str(self.as_str()) @@ -415,11 +435,35 @@ impl PolicyCompiler { } impl PolicyEngine { - pub fn load(path: &Path, graph_id: &str) -> Result { + /// Load a per-graph policy file. Rejects rules whose actions are + /// server-scoped (e.g. `graph_list`) — those belong in a server + /// policy file, not a per-graph one. + /// + /// `graph_id` is the label of the graph this engine governs; + /// becomes the Cedar `Omnigraph::Graph::""` resource + /// for every per-graph action evaluated against this engine. + pub fn load_graph(path: &Path, graph_id: &str) -> Result { let config = PolicyConfig::load(path)?; + validate_kind_alignment(&config, PolicyEngineKind::Graph)?; PolicyCompiler::compile(&config, graph_id) } + /// Load a server-level policy file. Rejects rules whose actions + /// are per-graph (e.g. `read`, `change`) — those belong in a + /// per-graph policy file, not the server one. Takes no `graph_id`: + /// server-scoped actions resolve against the singleton + /// `Omnigraph::Server::"root"` entity, never a Graph. + pub fn load_server(path: &Path) -> Result { + let config = PolicyConfig::load(path)?; + validate_kind_alignment(&config, PolicyEngineKind::Server)?; + // The Graph entity created by the compiler is never referenced + // by a server-scoped rule, so the label below is purely a + // placeholder. Use the canonical SERVER_RESOURCE_ID so any + // future inspection of an unreachable Graph entity at least + // points at the right concept. + PolicyCompiler::compile(&config, SERVER_RESOURCE_ID) + } + /// Evaluate a request. `actor_id` is supplied as a separate /// argument (not inside `PolicyRequest`) so the type system enforces /// the "server-authoritative actor identity" invariant — clients @@ -550,6 +594,38 @@ impl PolicyEngine { } } +/// Reject any rule whose actions don't match the engine kind +/// being loaded. Closes the "wrong action in wrong file silently +/// no-ops" class — `graph_list` in a per-graph file or `read` in +/// a server file fails at load time instead of compiling cleanly +/// and never matching a request. +fn validate_kind_alignment(config: &PolicyConfig, kind: PolicyEngineKind) -> Result<()> { + let required = match kind { + PolicyEngineKind::Graph => PolicyResourceKind::Graph, + PolicyEngineKind::Server => PolicyResourceKind::Server, + }; + for rule in &config.rules { + for action in &rule.allow.actions { + if action.resource_kind() != required { + let (got, expected_file) = match action.resource_kind() { + PolicyResourceKind::Server => ("server-scoped", "server policy file"), + PolicyResourceKind::Graph => ("per-graph", "per-graph policy file"), + }; + bail!( + "policy rule '{}' uses {} action '{}' in a {:?} policy file; \ + move it to a {}", + rule.id, + got, + action, + kind, + expected_file + ); + } + } + } + Ok(()) +} + fn compile_entities(config: &PolicyConfig, graph_id: &str, schema: &Schema) -> Result { let mut group_entities = Vec::new(); for group in config.groups.keys() { @@ -909,8 +985,8 @@ impl PolicyChecker for PolicyEngine { #[cfg(test)] mod tests { use super::{ - PolicyAction, PolicyCompiler, PolicyConfig, PolicyExpectation, PolicyRequest, - PolicyTestCase, PolicyTestConfig, + PolicyAction, PolicyCompiler, PolicyConfig, PolicyEngine, PolicyExpectation, + PolicyRequest, PolicyTestCase, PolicyTestConfig, }; #[test] @@ -1275,4 +1351,143 @@ rules: assert!(allow.allowed); assert_eq!(allow.matched_rule_id.as_deref(), Some("team-read")); } + + // ─── MR-668 follow-up — load_graph / load_server kind alignment ─ + + /// A per-graph policy file containing a `graph_list` rule fails + /// at load time. Pre-fix, the file compiled cleanly and the rule + /// silently never matched (per-graph engine never gets a + /// `graph_list` check). Closes the "wrong action, wrong file, + /// silent no-op" class. + #[test] + fn load_graph_rejects_server_scoped_action() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("bad-graph-policy.yaml"); + std::fs::write( + &path, + r#" +version: 1 +groups: + admins: [act-andrew] +rules: + - id: misplaced-graph-list + allow: + actors: { group: admins } + actions: [graph_list] +"#, + ) + .unwrap(); + let err = match PolicyEngine::load_graph(&path, "g1") { + Ok(_) => panic!("expected server-scoped action in per-graph file to be rejected"), + Err(e) => e, + }; + let msg = err.to_string(); + assert!( + msg.contains("server-scoped") && msg.contains("graph_list"), + "expected server-scoped-in-graph-file rejection, got: {msg}" + ); + } + + /// A server policy file containing a `read` rule fails at load + /// time. Pre-fix, the file compiled cleanly and the rule silently + /// never matched (server engine never gets a `read` check). + #[test] + fn load_server_rejects_per_graph_action() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("bad-server-policy.yaml"); + std::fs::write( + &path, + r#" +version: 1 +groups: + team: [act-andrew] +rules: + - id: misplaced-read + allow: + actors: { group: team } + actions: [read] + branch_scope: any +"#, + ) + .unwrap(); + let err = match PolicyEngine::load_server(&path) { + Ok(_) => panic!("expected per-graph action in server file to be rejected"), + Err(e) => e, + }; + let msg = err.to_string(); + assert!( + msg.contains("per-graph") && msg.contains("read"), + "expected per-graph-in-server-file rejection, got: {msg}" + ); + } + + /// Positive case: a properly-shaped per-graph policy loads via + /// `load_graph` and authorizes as expected. Verifies the + /// kind-alignment check is permissive when the file is correct. + #[test] + fn load_graph_accepts_per_graph_only_policy() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("ok-graph-policy.yaml"); + std::fs::write( + &path, + r#" +version: 1 +groups: + team: [act-andrew] +rules: + - id: team-read + allow: + actors: { group: team } + actions: [read] + branch_scope: any +"#, + ) + .unwrap(); + let engine = PolicyEngine::load_graph(&path, "g1").unwrap(); + let decision = engine + .authorize( + "act-andrew", + &PolicyRequest { + action: PolicyAction::Read, + branch: Some("main".to_string()), + target_branch: None, + }, + ) + .unwrap(); + assert!(decision.allowed); + } + + /// Positive case: a properly-shaped server policy loads via + /// `load_server` and authorizes the `graph_list` action. + #[test] + fn load_server_accepts_server_only_policy() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("ok-server-policy.yaml"); + std::fs::write( + &path, + r#" +version: 1 +groups: + admins: [act-andrew] +rules: + - id: admins-list-graphs + allow: + actors: { group: admins } + actions: [graph_list] +"#, + ) + .unwrap(); + let engine = PolicyEngine::load_server(&path).unwrap(); + let decision = engine + .authorize( + "act-andrew", + &PolicyRequest { + action: PolicyAction::GraphList, + branch: None, + target_branch: None, + }, + ) + .unwrap(); + assert!(decision.allowed); + } } diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index c20aee7..65520a0 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -371,7 +371,7 @@ impl AppState { let uri = uri.into(); let db = Omnigraph::open(&uri).await?; let policy_engine = match policy_file { - Some(path) => Some(PolicyEngine::load(path, &uri)?), + Some(path) => Some(PolicyEngine::load_graph(path, &uri)?), None => None, }; if policy_engine.is_some() && bearer_tokens.is_empty() { @@ -1062,7 +1062,7 @@ async fn open_multi_graph_state( // resource-model refactor maps to the singleton // `Omnigraph::Server::"root"` entity at evaluation time. let server_policy = match server_policy_file { - Some(path) => Some(PolicyEngine::load(path, "server")?), + Some(path) => Some(PolicyEngine::load_server(path)?), None => None, }; @@ -1100,7 +1100,7 @@ async fn open_single_graph(cfg: GraphStartupConfig) -> Result> let (policy_arc, db) = match &cfg.policy_file { Some(path) => { - let policy = PolicyEngine::load(path, graph_id.as_str())?; + let policy = PolicyEngine::load_graph(path, graph_id.as_str())?; let policy_arc: Arc = Arc::new(policy); let checker = Arc::clone(&policy_arc) as Arc; (Some(policy_arc), db.with_policy(checker)) diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 9845fb3..78742f8 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -3592,7 +3592,7 @@ async fn ingest_per_actor_admission_cap_returns_429() { let policy_path = temp.path().join("policy.yaml"); fs::write(&policy_path, permit_all_policy_yaml(&["act-flooder"])).unwrap(); let policy_engine = - omnigraph_server::PolicyEngine::load(&policy_path, graph.to_string_lossy().as_ref()) + omnigraph_server::PolicyEngine::load_graph(&policy_path, graph.to_string_lossy().as_ref()) .unwrap(); let state = AppState::new_single( graph.to_string_lossy().to_string(), @@ -3716,7 +3716,7 @@ async fn engine_layer_policy_fires_via_direct_arc_omnigraph_from_new_single() { let policy_path = temp.path().join("policy.yaml"); fs::write(&policy_path, permit_all_policy_yaml(&["act-allowed"])).unwrap(); let policy_engine = - omnigraph_server::PolicyEngine::load(&policy_path, graph.to_string_lossy().as_ref()) + omnigraph_server::PolicyEngine::load_graph(&policy_path, graph.to_string_lossy().as_ref()) .unwrap(); let workload = omnigraph_server::workload::WorkloadController::new(100, 1_000_000_000); @@ -3940,7 +3940,7 @@ async fn build_parity_graph() -> (tempfile::TempDir, PathBuf, PathBuf) { } async fn sdk_change_decision(graph: &Path, policy_path: &Path, actor: &str) -> ParityDecision { - let policy = PolicyEngine::load(policy_path, graph.to_string_lossy().as_ref()).unwrap(); + let policy = PolicyEngine::load_graph(policy_path, graph.to_string_lossy().as_ref()).unwrap(); let db = Omnigraph::open(graph.to_str().unwrap()) .await .unwrap() @@ -4008,7 +4008,7 @@ async fn http_change_decision( } async fn sdk_merge_decision(graph: &Path, policy_path: &Path, actor: &str) -> ParityDecision { - let policy = PolicyEngine::load(policy_path, graph.to_string_lossy().as_ref()).unwrap(); + let policy = PolicyEngine::load_graph(policy_path, graph.to_string_lossy().as_ref()).unwrap(); let db = Omnigraph::open(graph.to_str().unwrap()) .await .unwrap() @@ -4878,7 +4878,7 @@ rules: "#, ) .unwrap(); - let server_policy = PolicyEngine::load(&policy_path, "server").unwrap(); + let server_policy = PolicyEngine::load_server(&policy_path).unwrap(); let tokens = vec![ ("act-andrew".to_string(), "andrew-token".to_string()), diff --git a/crates/omnigraph/tests/policy_engine_chassis.rs b/crates/omnigraph/tests/policy_engine_chassis.rs index b1f43d9..3953741 100644 --- a/crates/omnigraph/tests/policy_engine_chassis.rs +++ b/crates/omnigraph/tests/policy_engine_chassis.rs @@ -64,7 +64,7 @@ fn additive_schema() -> String { fn install_policy(db: Omnigraph, dir_path: &Path) -> (Omnigraph, Arc) { let policy_path = dir_path.join("policy.yaml"); fs::write(&policy_path, POLICY_YAML).unwrap(); - let engine = PolicyEngine::load(&policy_path, dir_path.to_str().unwrap()).unwrap(); + let engine = PolicyEngine::load_graph(&policy_path, dir_path.to_str().unwrap()).unwrap(); let engine = Arc::new(engine); let db = db.with_policy(Arc::clone(&engine) as Arc); (db, engine)