From 4580ec011ae8028e4a48218c41d04fd36ed76537 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Thu, 28 May 2026 16:09:44 +0200 Subject: [PATCH] chore: address review cleanup comments --- crates/omnigraph-server/src/config.rs | 55 ++++++++++++------------- crates/omnigraph-server/tests/server.rs | 12 ++---- docs/user/policy.md | 4 +- docs/user/server.md | 7 ++-- 4 files changed, 37 insertions(+), 41 deletions(-) diff --git a/crates/omnigraph-server/src/config.rs b/crates/omnigraph-server/src/config.rs index 66ac364..1272a7b 100644 --- a/crates/omnigraph-server/src/config.rs +++ b/crates/omnigraph-server/src/config.rs @@ -210,23 +210,17 @@ impl OmnigraphConfig { } pub fn resolve_auth_env_file(&self) -> Option { - let path = self.auth.env_file.as_deref()?; - let path = Path::new(path); - Some(if path.is_absolute() { - path.to_path_buf() - } else { - self.base_dir.join(path) - }) + self.auth + .env_file + .as_deref() + .map(|path| self.resolve_config_path(path)) } pub fn resolve_policy_file(&self) -> Option { - let path = self.policy.file.as_deref()?; - let path = Path::new(path); - Some(if path.is_absolute() { - path.to_path_buf() - } else { - self.base_dir.join(path) - }) + self.policy + .file + .as_deref() + .map(|path| self.resolve_config_path(path)) } /// Resolve the per-graph policy file path for the named target, @@ -234,25 +228,21 @@ impl OmnigraphConfig { /// target is unknown or no per-graph `policy.file` is set. pub fn resolve_target_policy_file(&self, target_name: &str) -> Option { let target = self.graphs.get(target_name)?; - let path = target.policy.file.as_deref()?; - let path = Path::new(path); - Some(if path.is_absolute() { - path.to_path_buf() - } else { - self.base_dir.join(path) - }) + target + .policy + .file + .as_deref() + .map(|path| self.resolve_config_path(path)) } /// Resolve the server-level policy file path (used by management /// endpoints). Returns `None` if `server.policy.file` is not set. pub fn resolve_server_policy_file(&self) -> Option { - let path = self.server.policy.file.as_deref()?; - let path = Path::new(path); - Some(if path.is_absolute() { - path.to_path_buf() - } else { - self.base_dir.join(path) - }) + self.server + .policy + .file + .as_deref() + .map(|path| self.resolve_config_path(path)) } /// Resolve a raw config-supplied URI (which may be relative) to its @@ -328,6 +318,15 @@ impl OmnigraphConfig { self.base_dir.join(path).to_string_lossy().to_string() } } + + fn resolve_config_path(&self, value: &str) -> PathBuf { + let path = Path::new(value); + if path.is_absolute() { + path.to_path_buf() + } else { + self.base_dir.join(path) + } + } } pub fn default_config_path() -> PathBuf { diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 8f4aaee..a588cc7 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -5270,12 +5270,11 @@ rules: ); } - /// End-to-end: load an `omnigraph.yaml` with two graphs and serve - /// them. Both graphs must be queryable via cluster routes. - /// - /// Uses `_` placeholders for tempdirs so they live until end-of-test. + /// Loads an `omnigraph.yaml` with two graphs and verifies multi-mode + /// inference plus graph entry resolution. Cluster-route dispatch is + /// covered by the route tests above. #[tokio::test(flavor = "multi_thread")] - async fn server_settings_drive_multi_graph_startup_end_to_end() { + async fn server_settings_load_multi_graph_config_entries() { let cfg_dir = tempfile::tempdir().unwrap(); // Real graph storage dirs (the URIs in the config must point to // a graph init-able location). @@ -5310,9 +5309,6 @@ graphs: load_server_settings(Some(&config_path), None, None, None, true).unwrap(); assert!(matches!(settings.mode, ServerConfigMode::Multi { .. })); - // We don't actually call `serve()` (would bind a socket); we - // just confirm the settings are well-formed and the inferred - // mode lists both graphs. match settings.mode { ServerConfigMode::Multi { graphs, .. } => { assert_eq!(graphs.len(), 2); diff --git a/docs/user/policy.md b/docs/user/policy.md index 322e906..749d3be 100644 --- a/docs/user/policy.md +++ b/docs/user/policy.md @@ -79,7 +79,7 @@ cli: actor: act-andrew # default actor for CLI direct-engine writes ``` -Each per-graph rule must use exactly one of `branch_scope` or `target_branch_scope`. Server-scoped rules (`graph_list`) take neither — they have no branch context. +Each per-graph rule may use at most one of `branch_scope` or `target_branch_scope`. Server-scoped rules (`graph_list`) take neither — they have no branch context. `cli.actor` is the default actor identity for CLI direct-engine writes when `policy.file` is configured. Override per-invocation with `--as @@ -121,7 +121,7 @@ reaches `authorize_request()` without a matching policy permit. |---|---|---|---| | **Open** | no | no | Every request is permitted. Refuses to start unless `--unauthenticated` or `OMNIGRAPH_UNAUTHENTICATED=1` is set — the operator must explicitly opt in. | | **DefaultDeny** | yes | no | Every authenticated request for an action other than `read` is rejected with HTTP 403. Closes the "tokens but forgot the policy file" trap — an operator who sets up auth and forgot to point at a policy file used to ship the illusion of protection. | -| **PolicyEnabled** | yes | yes | Every request is evaluated by Cedar against the configured policy. | +| **PolicyEnabled** | yes | yes | Authenticated requests that reach a configured policy engine are evaluated by Cedar. Server-scoped actions still require `server.policy.file`. | The classifier is `classify_server_runtime_state` in `crates/omnigraph-server/src/lib.rs`; it returns `Err` for the "no diff --git a/docs/user/server.md b/docs/user/server.md index 5fb2dbe..1a1bdb8 100644 --- a/docs/user/server.md +++ b/docs/user/server.md @@ -117,9 +117,10 @@ endpoints (`/snapshot`, `/read`, `/export`, `/branches` GET, `/commits`, 1. `OMNIGRAPH_SERVER_BEARER_TOKENS_AWS_SECRET` — AWS Secrets Manager (build with `--features aws`) 2. `OMNIGRAPH_SERVER_BEARER_TOKENS_FILE` or `OMNIGRAPH_SERVER_BEARER_TOKENS_JSON` — JSON `{actor_id: token, …}` 3. `OMNIGRAPH_SERVER_BEARER_TOKEN` — single legacy token, actor `default` -- If no tokens and no policy are configured, startup refuses unless - `--unauthenticated` or `OMNIGRAPH_UNAUTHENTICATED=1` explicitly opts into - open local-dev mode. In that mode `/openapi.json` strips the security scheme. +- If no tokens are configured, startup refuses unless `--unauthenticated` or + `OMNIGRAPH_UNAUTHENTICATED=1` explicitly opts into open local-dev mode. A + policy file without tokens is also rejected at startup. In open mode + `/openapi.json` strips the security scheme. See [deployment.md](deployment.md) for token-source operational details.