mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-12 01:45:14 +02:00
mr-668: split PolicyEngine::load into kind-typed loaders
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) <noreply@anthropic.com>
This commit is contained in:
parent
67a46528ef
commit
4e2f18a95e
5 changed files with 228 additions and 13 deletions
|
|
@ -729,7 +729,7 @@ fn resolve_policy_engine(config: &OmnigraphConfig) -> Result<PolicyEngine> {
|
|||
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
|
||||
|
|
|
|||
|
|
@ -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<Self> {
|
||||
/// 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::"<graph_id>"` resource
|
||||
/// for every per-graph action evaluated against this engine.
|
||||
pub fn load_graph(path: &Path, graph_id: &str) -> Result<Self> {
|
||||
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<Self> {
|
||||
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<Entities> {
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Arc<GraphHandle>>
|
|||
|
||||
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<PolicyEngine> = Arc::new(policy);
|
||||
let checker = Arc::clone(&policy_arc) as Arc<dyn omnigraph_policy::PolicyChecker>;
|
||||
(Some(policy_arc), db.with_policy(checker))
|
||||
|
|
|
|||
|
|
@ -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()),
|
||||
|
|
|
|||
|
|
@ -64,7 +64,7 @@ fn additive_schema() -> String {
|
|||
fn install_policy(db: Omnigraph, dir_path: &Path) -> (Omnigraph, Arc<PolicyEngine>) {
|
||||
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<dyn PolicyChecker>);
|
||||
(db, engine)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue