mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-30 02:49:39 +02:00
fix(config): enforce graph-scoped policies and query validation
This commit is contained in:
parent
fb442adb14
commit
845e32324c
12 changed files with 682 additions and 168 deletions
|
|
@ -306,7 +306,7 @@ pub struct ChangeRequest {
|
|||
/// Body for `POST /queries/{name}` — invokes the server-side stored query
|
||||
/// named in the path. The query source and name come from the registry,
|
||||
/// never the body; only the runtime inputs are supplied here.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, ToSchema)]
|
||||
#[derive(Debug, Clone, Default, Serialize, Deserialize, ToSchema)]
|
||||
pub struct InvokeStoredQueryRequest {
|
||||
/// JSON object whose keys match the stored query's declared parameters.
|
||||
#[serde(default)]
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ pub use graph_id::GraphId;
|
|||
pub use identity::{AuthSource, GraphKey, ResolvedActor, Scope, TenantId};
|
||||
pub use registry::{GraphHandle, GraphRegistry, InsertError, RegistryLookup, RegistrySnapshot};
|
||||
|
||||
use crate::queries::{QueryRegistry, check};
|
||||
use crate::queries::{QueryRegistry, check, format_check_breakages};
|
||||
|
||||
use std::collections::{HashMap, HashSet};
|
||||
use std::fs;
|
||||
|
|
@ -820,22 +820,6 @@ pub fn init_tracing() {
|
|||
let _ = tracing_subscriber::fmt().with_env_filter(filter).try_init();
|
||||
}
|
||||
|
||||
/// Format every breakage in a registry check report into a multi-line
|
||||
/// boot-abort message, naming each offending query.
|
||||
fn format_registry_breakages(label: &str, report: &queries::CheckReport) -> String {
|
||||
let joined = report
|
||||
.breakages
|
||||
.iter()
|
||||
.map(|b| format!("query '{}': {}", b.query, b.message))
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n ");
|
||||
format!(
|
||||
"graph '{label}': {} stored quer{} failed the schema check:\n {joined}",
|
||||
report.breakages.len(),
|
||||
if report.breakages.len() == 1 { "y" } else { "ies" }
|
||||
)
|
||||
}
|
||||
|
||||
/// Log each non-blocking advisory from a registry check report.
|
||||
fn log_registry_warnings(label: &str, report: &queries::CheckReport) {
|
||||
for warning in &report.warnings {
|
||||
|
|
@ -843,6 +827,19 @@ fn log_registry_warnings(label: &str, report: &queries::CheckReport) {
|
|||
}
|
||||
}
|
||||
|
||||
fn validate_registry_against_catalog(
|
||||
registry: &QueryRegistry,
|
||||
catalog: &Catalog,
|
||||
label: &str,
|
||||
) -> omnigraph::error::Result<()> {
|
||||
let report = check(registry, catalog);
|
||||
if report.has_breakages() {
|
||||
return Err(OmniError::manifest(format_check_breakages(label, &report)));
|
||||
}
|
||||
log_registry_warnings(label, &report);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Validate a loaded stored-query registry against the live schema and
|
||||
/// resolve it to an attachable handle. Refuses boot on any breakage
|
||||
/// (same posture as bad policy YAML), logs the non-blocking warnings,
|
||||
|
|
@ -855,11 +852,8 @@ fn validate_and_attach(
|
|||
catalog: &Catalog,
|
||||
label: &str,
|
||||
) -> Result<Option<Arc<QueryRegistry>>> {
|
||||
let report = check(&queries, catalog);
|
||||
if report.has_breakages() {
|
||||
bail!("{}", format_registry_breakages(label, &report));
|
||||
}
|
||||
log_registry_warnings(label, &report);
|
||||
validate_registry_against_catalog(&queries, catalog, label)
|
||||
.map_err(|err| color_eyre::eyre::eyre!(err.to_string()))?;
|
||||
Ok(if queries.is_empty() {
|
||||
None
|
||||
} else {
|
||||
|
|
@ -2214,13 +2208,26 @@ struct QueryNamePath {
|
|||
name: String,
|
||||
}
|
||||
|
||||
fn parse_optional_invoke_body(
|
||||
body: Bytes,
|
||||
) -> std::result::Result<InvokeStoredQueryRequest, ApiError> {
|
||||
if body.is_empty() {
|
||||
return Ok(InvokeStoredQueryRequest::default());
|
||||
}
|
||||
serde_json::from_slice::<Option<InvokeStoredQueryRequest>>(&body)
|
||||
.map(|request| request.unwrap_or_default())
|
||||
.map_err(|err| {
|
||||
ApiError::bad_request(format!("invalid stored-query invocation body: {err}"))
|
||||
})
|
||||
}
|
||||
|
||||
#[utoipa::path(
|
||||
post,
|
||||
path = "/queries/{name}",
|
||||
tag = "queries",
|
||||
operation_id = "invoke_query",
|
||||
params(("name" = String, Path, description = "Stored query name (the registry key)")),
|
||||
request_body = InvokeStoredQueryRequest,
|
||||
request_body = Option<InvokeStoredQueryRequest>,
|
||||
responses(
|
||||
(status = 200, description = "Read envelope (ReadOutput) or mutation envelope (ChangeOutput), serialized untagged", body = InvokeStoredQueryResponse),
|
||||
(status = 400, description = "Bad request (param type error; snapshot on a stored mutation)", body = ErrorOutput),
|
||||
|
|
@ -2249,8 +2256,9 @@ async fn server_invoke_query(
|
|||
Extension(handle): Extension<Arc<GraphHandle>>,
|
||||
actor: Option<Extension<ResolvedActor>>,
|
||||
Path(QueryNamePath { name }): Path<QueryNamePath>,
|
||||
Json(req): Json<InvokeStoredQueryRequest>,
|
||||
body: Bytes,
|
||||
) -> std::result::Result<Json<InvokeStoredQueryResponse>, ApiError> {
|
||||
let req = parse_optional_invoke_body(body)?;
|
||||
// A caller without `invoke_query` can't tell a denial from a missing
|
||||
// query: both 404 with this exact message, so the catalog can't be
|
||||
// probed without the grant. (A caller that holds invoke_query may still
|
||||
|
|
@ -2469,18 +2477,26 @@ async fn server_schema_apply(
|
|||
.map_err(ApiError::from_workload_reject)?;
|
||||
let result = {
|
||||
let db = &handle.engine;
|
||||
let registry = handle.queries.as_deref();
|
||||
let label = handle.key.graph_id.as_str().to_string();
|
||||
// Engine-layer policy enforcement (MR-722): pass the resolved
|
||||
// actor through so apply_schema_as can call enforce() with the
|
||||
// authoritative identity. With a policy installed in AppState,
|
||||
// engine-side enforcement re-checks the same decision the
|
||||
// HTTP-layer authorize_request just made above. PR #3 collapses
|
||||
// the redundancy.
|
||||
db.apply_schema_as(
|
||||
db.apply_schema_as_with_catalog_check(
|
||||
&request.schema_source,
|
||||
omnigraph::db::SchemaApplyOptions {
|
||||
allow_data_loss: request.allow_data_loss,
|
||||
},
|
||||
actor_id,
|
||||
|catalog| {
|
||||
if let Some(registry) = registry {
|
||||
validate_registry_against_catalog(registry, catalog, &label)?;
|
||||
}
|
||||
Ok(())
|
||||
},
|
||||
)
|
||||
.await
|
||||
.map_err(ApiError::from_omni)?
|
||||
|
|
|
|||
|
|
@ -315,6 +315,26 @@ pub fn check(registry: &QueryRegistry, catalog: &Catalog) -> CheckReport {
|
|||
report
|
||||
}
|
||||
|
||||
/// Format every breakage in a registry check report into a multi-line
|
||||
/// operator-facing message, naming each offending query.
|
||||
pub fn format_check_breakages(label: &str, report: &CheckReport) -> String {
|
||||
let joined = report
|
||||
.breakages
|
||||
.iter()
|
||||
.map(|b| format!("query '{}': {}", b.query, b.message))
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n ");
|
||||
format!(
|
||||
"graph '{label}': {} stored quer{} failed the schema check:\n {joined}",
|
||||
report.breakages.len(),
|
||||
if report.breakages.len() == 1 {
|
||||
"y"
|
||||
} else {
|
||||
"ies"
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
|
|
|||
|
|
@ -917,6 +917,34 @@ fn post_endpoints_have_request_body() {
|
|||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn invoke_stored_query_request_body_is_optional() {
|
||||
let doc = openapi_json();
|
||||
let request_body = &doc["paths"]["/queries/{name}"]["post"]["requestBody"];
|
||||
assert!(
|
||||
request_body.is_object(),
|
||||
"POST /queries/{{name}} should document its optional request body"
|
||||
);
|
||||
assert_eq!(
|
||||
request_body["required"].as_bool().unwrap_or(false),
|
||||
false,
|
||||
"stored-query invocation body should be optional"
|
||||
);
|
||||
let schema = &request_body["content"]["application/json"]["schema"];
|
||||
let ref_path = schema["$ref"]
|
||||
.as_str()
|
||||
.or_else(|| {
|
||||
schema["oneOf"]
|
||||
.as_array()
|
||||
.and_then(|schemas| schemas.iter().find_map(|schema| schema["$ref"].as_str()))
|
||||
})
|
||||
.unwrap();
|
||||
assert!(
|
||||
ref_path.contains("InvokeStoredQueryRequest"),
|
||||
"POST /queries/{{name}} requestBody should reference InvokeStoredQueryRequest, got {ref_path}"
|
||||
);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Serialization round-trip test
|
||||
// ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@ use axum::body::{Body, to_bytes};
|
|||
use axum::http::header::AUTHORIZATION;
|
||||
use axum::http::{Method, Request, StatusCode};
|
||||
use lance::index::DatasetIndexExt;
|
||||
use omnigraph::db::{Omnigraph, ReadTarget, SchemaApplyOptions};
|
||||
use omnigraph::db::{Omnigraph, ReadTarget};
|
||||
use omnigraph::error::OmniError;
|
||||
use omnigraph::loader::{LoadMode, load_jsonl};
|
||||
use omnigraph_policy::{PolicyChecker, PolicyEngine};
|
||||
|
|
@ -280,6 +280,28 @@ rules:
|
|||
branch_scope: any
|
||||
"#;
|
||||
|
||||
const STORED_QUERY_SCHEMA_APPLY_POLICY_YAML: &str = r#"
|
||||
version: 1
|
||||
groups:
|
||||
admins: [act-ragnor]
|
||||
protected_branches: [main]
|
||||
rules:
|
||||
- id: admins-can-invoke
|
||||
allow:
|
||||
actors: { group: admins }
|
||||
actions: [invoke_query]
|
||||
- id: admins-can-read
|
||||
allow:
|
||||
actors: { group: admins }
|
||||
actions: [read]
|
||||
branch_scope: any
|
||||
- id: admins-can-schema-apply
|
||||
allow:
|
||||
actors: { group: admins }
|
||||
actions: [schema_apply]
|
||||
target_branch_scope: protected
|
||||
"#;
|
||||
|
||||
const FIND_PERSON_GQ: &str =
|
||||
"query find_person($name: String) { match { $p: Person { name: $name } } return { $p.age } }";
|
||||
|
||||
|
|
@ -293,6 +315,22 @@ fn invoke_request(name: &str, token: &str, body: Value) -> Request<Body> {
|
|||
.unwrap()
|
||||
}
|
||||
|
||||
fn invoke_request_bytes(
|
||||
name: &str,
|
||||
token: &str,
|
||||
body: impl Into<Body>,
|
||||
content_type: Option<&str>,
|
||||
) -> Request<Body> {
|
||||
let mut builder = Request::builder()
|
||||
.uri(format!("/queries/{name}"))
|
||||
.method(Method::POST)
|
||||
.header("authorization", format!("Bearer {token}"));
|
||||
if let Some(content_type) = content_type {
|
||||
builder = builder.header("content-type", content_type);
|
||||
}
|
||||
builder.body(body.into()).unwrap()
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn invoke_stored_read_returns_rows() {
|
||||
let (_temp, app) = app_with_stored_queries(
|
||||
|
|
@ -312,6 +350,68 @@ async fn invoke_stored_read_returns_rows() {
|
|||
assert!(body["rows"].is_array(), "read envelope shape; body: {body}");
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn invoke_stored_read_accepts_absent_or_empty_body() {
|
||||
let no_param_query = "query list_people() { match { $p: Person } return { $p.name } }";
|
||||
let (_temp, app) = app_with_stored_queries(
|
||||
&[("list_people", no_param_query, false)],
|
||||
&[("act-invoke", "t-invoke")],
|
||||
INVOKE_POLICY_YAML,
|
||||
)
|
||||
.await;
|
||||
|
||||
let (status, body) = json_response(
|
||||
&app,
|
||||
invoke_request_bytes("list_people", "t-invoke", Body::empty(), None),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(status, StatusCode::OK, "body: {body}");
|
||||
assert_eq!(body["query_name"], "list_people");
|
||||
|
||||
let (status, body) = json_response(
|
||||
&app,
|
||||
invoke_request_bytes(
|
||||
"list_people",
|
||||
"t-invoke",
|
||||
Body::empty(),
|
||||
Some("application/json"),
|
||||
),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(status, StatusCode::OK, "body: {body}");
|
||||
|
||||
let (status, body) = json_response(
|
||||
&app,
|
||||
invoke_request_bytes(
|
||||
"list_people",
|
||||
"t-invoke",
|
||||
Body::from("{}"),
|
||||
Some("application/json"),
|
||||
),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(status, StatusCode::OK, "body: {body}");
|
||||
|
||||
let (status, body) = json_response(
|
||||
&app,
|
||||
invoke_request_bytes(
|
||||
"list_people",
|
||||
"t-invoke",
|
||||
Body::from("{"),
|
||||
Some("application/json"),
|
||||
),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(status, StatusCode::BAD_REQUEST, "body: {body}");
|
||||
assert!(
|
||||
body["error"]
|
||||
.as_str()
|
||||
.unwrap_or_default()
|
||||
.contains("invalid stored-query invocation body"),
|
||||
"malformed JSON should be rejected as bad request; body: {body}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn invoke_stored_mutation_double_gates_on_change() {
|
||||
let specs: &[(&str, &str, bool)] = &[(
|
||||
|
|
@ -787,6 +887,83 @@ async fn schema_apply_route_updates_graph_for_authorized_admin() {
|
|||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn schema_apply_route_rejects_stored_query_breakage_before_publish() {
|
||||
let (temp, app) = app_with_stored_queries(
|
||||
&[("find_person", FIND_PERSON_GQ, true)],
|
||||
&[("act-ragnor", "admin-token")],
|
||||
STORED_QUERY_SCHEMA_APPLY_POLICY_YAML,
|
||||
)
|
||||
.await;
|
||||
|
||||
let request = Request::builder()
|
||||
.method(Method::POST)
|
||||
.uri("/schema/apply")
|
||||
.header("content-type", "application/json")
|
||||
.header("authorization", "Bearer admin-token")
|
||||
.body(Body::from(
|
||||
serde_json::to_vec(&SchemaApplyRequest {
|
||||
schema_source: renamed_age_schema(),
|
||||
..Default::default()
|
||||
})
|
||||
.unwrap(),
|
||||
))
|
||||
.unwrap();
|
||||
let (status, payload) = json_response(&app, request).await;
|
||||
assert_eq!(status, StatusCode::BAD_REQUEST, "body: {payload}");
|
||||
let message = payload["error"].as_str().unwrap_or_default();
|
||||
assert!(
|
||||
message.contains("find_person") && message.contains("schema check"),
|
||||
"registry breakage should name the stored query; body: {payload}"
|
||||
);
|
||||
|
||||
let reopened = Omnigraph::open(graph_path(temp.path()).to_str().unwrap())
|
||||
.await
|
||||
.unwrap();
|
||||
let person = &reopened.catalog().node_types["Person"];
|
||||
assert!(person.properties.contains_key("age"));
|
||||
assert!(!person.properties.contains_key("years"));
|
||||
|
||||
let (invoke_status, invoke_body) = json_response(
|
||||
&app,
|
||||
invoke_request(
|
||||
"find_person",
|
||||
"admin-token",
|
||||
json!({ "params": { "name": "Alice" } }),
|
||||
),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(invoke_status, StatusCode::OK, "body: {invoke_body}");
|
||||
assert_eq!(invoke_body["row_count"], 1);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn schema_apply_route_noop_keeps_valid_stored_query_registry() {
|
||||
let (_temp, app) = app_with_stored_queries(
|
||||
&[("find_person", FIND_PERSON_GQ, true)],
|
||||
&[("act-ragnor", "admin-token")],
|
||||
STORED_QUERY_SCHEMA_APPLY_POLICY_YAML,
|
||||
)
|
||||
.await;
|
||||
|
||||
let request = Request::builder()
|
||||
.method(Method::POST)
|
||||
.uri("/schema/apply")
|
||||
.header("content-type", "application/json")
|
||||
.header("authorization", "Bearer admin-token")
|
||||
.body(Body::from(
|
||||
serde_json::to_vec(&SchemaApplyRequest {
|
||||
schema_source: fs::read_to_string(fixture("test.pg")).unwrap(),
|
||||
..Default::default()
|
||||
})
|
||||
.unwrap(),
|
||||
))
|
||||
.unwrap();
|
||||
let (status, payload) = json_response(&app, request).await;
|
||||
assert_eq!(status, StatusCode::OK, "body: {payload}");
|
||||
assert_eq!(payload["applied"], false);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn schema_apply_route_requires_schema_apply_policy_permission() {
|
||||
let (_temp, app) = app_for_graph_with_auth_tokens_and_policy(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue