From cd36f220202351d8ca88c6ae31e49086078e7e1a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:05:24 +0000 Subject: [PATCH 1/2] fix: name no nodes in refused listings A refused location listing named the nodes the person could not view and read differently for a location that does not exist. Listing refusals now carry one fixed message with the decision ID, identical for unassigned and unknown locations. The audit event keeps the missing nodes. --- clients/typescript/tests/client.test.ts | 2 +- .../src/authorization/constants.rs | 4 + .../authorization/main/authorize_listing.rs | 13 +- .../src/authorization/mod.rs | 1 + .../tests/api/listing_denials/helpers.rs | 43 ++++++ .../tests/api/test_contracts.rs | 4 + .../tests/api/test_listing_denials.rs | 132 ++++++++++++++++++ .../tests/api/test_types.rs | 7 + .../tests/credentials/test_project_roles.rs | 8 +- .../publication/test_denial.py | 12 +- 10 files changed, 213 insertions(+), 13 deletions(-) create mode 100644 crates/sqlbuild-orchestration/src/authorization/constants.rs create mode 100644 crates/sqlbuild-orchestration/tests/api/listing_denials/helpers.rs create mode 100644 crates/sqlbuild-orchestration/tests/api/test_listing_denials.rs diff --git a/clients/typescript/tests/client.test.ts b/clients/typescript/tests/client.test.ts index bd937b2883..26781afc40 100644 --- a/clients/typescript/tests/client.test.ts +++ b/clients/typescript/tests/client.test.ts @@ -134,7 +134,7 @@ test("generated client preserves native scopes, replay, path encoding, SSE and b const decisionId: string | null | undefined = denied.error?.decision_id; assert.ok(decisionId); assert.deepEqual(denied.error, { - error: `missing permission in this scope; all requested nodes denied; decision ${decisionId}`, + error: `missing view on one or more nodes in this location; decision ${decisionId}`, decision_id: decisionId, }); const machine = await browser.GET(healthPath, { headers, params }); diff --git a/crates/sqlbuild-orchestration/src/authorization/constants.rs b/crates/sqlbuild-orchestration/src/authorization/constants.rs new file mode 100644 index 0000000000..8585acf319 --- /dev/null +++ b/crates/sqlbuild-orchestration/src/authorization/constants.rs @@ -0,0 +1,4 @@ +//! Fixed authorization texts that callers may see. + +/// A refused listing names no node, so a caller cannot learn nodes it cannot view or whether a location exists. +pub(crate) const LISTING_DENIED: &str = "missing view on one or more nodes in this location"; diff --git a/crates/sqlbuild-orchestration/src/authorization/main/authorize_listing.rs b/crates/sqlbuild-orchestration/src/authorization/main/authorize_listing.rs index 1801ec9f17..1dda1127d1 100644 --- a/crates/sqlbuild-orchestration/src/authorization/main/authorize_listing.rs +++ b/crates/sqlbuild-orchestration/src/authorization/main/authorize_listing.rs @@ -1,9 +1,10 @@ -//! A location-wide listing needs view coverage of every node in the current snapshot; it is never filtered. +//! A listing needs view of every node in the current snapshot; its refusal names no node and reads the same for unknown locations. use std::collections::BTreeSet; use uuid::Uuid; +use crate::authorization::constants::LISTING_DENIED; use crate::authorization::main::authorize_person::authorize_person; use crate::credentials::models::{Action, AuditContext, DefaultRole, PermissionScope}; use crate::store::errors::ControlError; @@ -30,5 +31,13 @@ pub(crate) async fn authorize_listing( target: Some(target.into()), nodes, }; - authorize_person(transaction, credentials, &scope, default_role).await + authorize_person(transaction, credentials, &scope, default_role) + .await + .map_err(|error| match error { + ControlError::Denied { decision_id, .. } => ControlError::Denied { + reason: LISTING_DENIED.into(), + decision_id, + }, + error => error, + }) } diff --git a/crates/sqlbuild-orchestration/src/authorization/mod.rs b/crates/sqlbuild-orchestration/src/authorization/mod.rs index d1ddfc554a..640a7b648c 100644 --- a/crates/sqlbuild-orchestration/src/authorization/mod.rs +++ b/crates/sqlbuild-orchestration/src/authorization/mod.rs @@ -1,4 +1,5 @@ //! Closed, scoped person permissions and serialized installation-administrator lifecycle. pub(crate) mod _helpers; +pub(crate) mod constants; pub mod main; diff --git a/crates/sqlbuild-orchestration/tests/api/listing_denials/helpers.rs b/crates/sqlbuild-orchestration/tests/api/listing_denials/helpers.rs new file mode 100644 index 0000000000..c33f11b046 --- /dev/null +++ b/crates/sqlbuild-orchestration/tests/api/listing_denials/helpers.rs @@ -0,0 +1,43 @@ +//! Masked denial responses and the audit detail behind them. + +use reqwest::header; +use serde_json::{Value, json}; +use uuid::Uuid; + +use crate::store::main::{begin::begin, commit::commit, query::query}; +use crate::store::models::{ControlStore, TransactionMode}; + +/// Status, the headers that could tell cases apart, and the body with its decision ID masked. +pub(crate) async fn masked(response: reqwest::Response) -> (Value, Uuid) { + let status = response.status().as_u16(); + let headers: Vec<(String, String)> = response + .headers() + .iter() + .filter(|(name, _)| *name != header::DATE) + .map(|(name, value)| (name.to_string(), value.to_str().unwrap_or("").to_owned())) + .collect(); + let text = response.text().await.expect("denial body"); + let body: Value = serde_json::from_str(&text).expect("JSON denial"); + let decision: Uuid = serde_json::from_value(body["decision_id"].clone()).expect("decision ID"); + ( + json!({"status": status, "headers": headers, + "body": text.replace(&decision.to_string(), "")}), + decision, + ) +} + +pub(crate) async fn audited_scope(store: &ControlStore, decision: Uuid) -> Value { + let transaction = begin(store, TransactionMode::Read).await.expect("audit"); + let (transaction, rows) = query( + transaction, + "SELECT scope_json, after_json FROM sqb_security_audit WHERE action='authorization.denied'", + vec![], + ) + .await + .expect("denials"); + commit(transaction).await.expect("close audit"); + rows.into_iter() + .find(|row| row["after_json"]["decision_id"] == json!(decision)) + .map(|row| json!([row["scope_json"]["nodes"], row["after_json"]["reason"]])) + .unwrap_or(Value::Null) +} diff --git a/crates/sqlbuild-orchestration/tests/api/test_contracts.rs b/crates/sqlbuild-orchestration/tests/api/test_contracts.rs index 25527b6ad0..ec5b9b4b9c 100644 --- a/crates/sqlbuild-orchestration/tests/api/test_contracts.rs +++ b/crates/sqlbuild-orchestration/tests/api/test_contracts.rs @@ -6,6 +6,8 @@ mod changes_helpers; mod fingerprint_reads_helpers; #[path = "helpers.rs"] pub(crate) mod helpers; +#[path = "listing_denials/helpers.rs"] +mod listing_denials_helpers; #[path = "node_sql/helpers.rs"] mod node_sql_helpers; #[path = "node_status/helpers.rs"] @@ -44,6 +46,8 @@ mod test_custom_roles; mod test_development; #[path = "test_external_sessions.rs"] mod test_external_sessions; +#[path = "test_listing_denials.rs"] +mod test_listing_denials; #[path = "test_location_health.rs"] mod test_location_health; #[path = "test_location_work.rs"] diff --git a/crates/sqlbuild-orchestration/tests/api/test_listing_denials.rs b/crates/sqlbuild-orchestration/tests/api/test_listing_denials.rs new file mode 100644 index 0000000000..75824910e4 --- /dev/null +++ b/crates/sqlbuild-orchestration/tests/api/test_listing_denials.rs @@ -0,0 +1,132 @@ +//! Refused listings name no node and read the same for an unassigned and an unknown location. + +use reqwest::header; +use serde_json::{Value, json}; +use uuid::Uuid; + +use crate::api::models::{ApiAuthentication, HumanLogin}; +use crate::api::tests::helpers::{start_api_with_authentication, stop_api}; +use crate::api::tests::listing_denials_helpers::{audited_scope, masked}; +use crate::api::tests::test_types::ListingDenialTestCase; +use crate::credentials::tests::location_helpers::create_location; +use crate::credentials::tests::role_helpers::role_fixture; +use crate::runs::tests::helpers::snapshot; +use crate::snapshots::main::publish_snapshot::publish_snapshot; +use crate::store::main::migrate_store::migrate_store; +use crate::store::tests::store_helpers::{close_test_store, open_test_store, run}; +use crate::store::tests::store_test_types::Backend; + +const LISTINGS: [&str; 6] = [ + "runs", + "catalog?schema=orders", + "statistics?schema=orders&name=orders", + "checks", + "checks/orders_not_null", + "maintenance", +]; + +const NODES: [&str; 4] = ["customers", "inventory", "orders", "products"]; + +#[test] +fn given_refused_listings_when_people_read_them_then_no_node_is_named_and_unknown_locations_look_unassigned() + { + let test_cases = [ + ListingDenialTestCase { + description: "SQLite listing denials", + backend: Backend::Sqlite, + expected_status: 403, + expected_error: "missing view on one or more nodes in this location; decision ", + }, + ListingDenialTestCase { + description: "PostgreSQL listing denials", + backend: Backend::Postgres, + expected_status: 403, + expected_error: "missing view on one or more nodes in this location; decision ", + }, + ]; + for test_case in test_cases { + run(async { + let Some(fixture) = open_test_store(test_case.backend).await else { + return; + }; + let store = &fixture.store; + migrate_store(store).await.expect("migrate"); + let roles = role_fixture(store).await; + let inventory = create_location(store, "inventory", "orders-admin") + .await + .expect("unassigned location"); + publish_snapshot(store, &snapshot(inventory)) + .await + .expect("unassigned snapshot"); + let api = start_api_with_authentication( + store, + ApiAuthentication { + login: HumanLogin::Password("https://control.example.com".into()), + ..ApiAuthentication::default() + }, + ) + .await; + let client = reqwest::Client::new(); + let read = |location: Uuid, listing: &str| { + client + .get(format!( + "{}/api/control/locations/{location}/targets/development/{listing}", + api.url + )) + .header(header::HOST, "control.example.com") + .header( + header::COOKIE, + format!("__Host-sqb-session={}", roles.member.bearer), + ) + .send() + }; + for listing in LISTINGS { + let (partial, partial_decision) = + masked(read(roles.location_id, listing).await.expect("partial")).await; + let (unassigned, unassigned_decision) = + masked(read(inventory, listing).await.expect("unassigned")).await; + let (unknown, unknown_decision) = + masked(read(Uuid::now_v7(), listing).await.expect("unknown")).await; + let body: Value = + serde_json::from_str(partial["body"].as_str().expect("body")).expect("JSON"); + assert_eq!( + ( + partial["status"].clone(), + body["error"].clone(), + unassigned == unknown, + partial == unassigned, + ), + ( + json!(test_case.expected_status), + json!(test_case.expected_error), + true, + true + ), + "{}: {listing} names no node and hides whether the location exists", + test_case.description + ); + assert!( + NODES + .iter() + .all(|node| !partial["body"].as_str().unwrap_or_default().contains(node)), + "{}: {listing} body names a node", + test_case.description + ); + assert_eq!( + ( + audited_scope(store, unassigned_decision).await[0].clone(), + audited_scope(store, unknown_decision).await[0].clone(), + audited_scope(store, partial_decision).await[1] + .as_str() + .is_some_and(|reason| reason.contains("inventory")), + ), + (json!(NODES), json!([]), true), + "{}: {listing} audits every refusal with the full detail", + test_case.description + ); + } + stop_api(api).await; + close_test_store(fixture).await; + }); + } +} diff --git a/crates/sqlbuild-orchestration/tests/api/test_types.rs b/crates/sqlbuild-orchestration/tests/api/test_types.rs index 7d4430d6ec..be4ad753ec 100644 --- a/crates/sqlbuild-orchestration/tests/api/test_types.rs +++ b/crates/sqlbuild-orchestration/tests/api/test_types.rs @@ -119,6 +119,13 @@ pub(crate) struct CliLoginTestCase { pub(crate) expected_exchanges: usize, } +pub(crate) struct ListingDenialTestCase { + pub(crate) description: &'static str, + pub(crate) backend: Backend, + pub(crate) expected_status: u16, + pub(crate) expected_error: &'static str, +} + pub(crate) struct CustomRoleApiTestCase { pub(crate) description: &'static str, pub(crate) backend: Backend, diff --git a/crates/sqlbuild-orchestration/tests/credentials/test_project_roles.rs b/crates/sqlbuild-orchestration/tests/credentials/test_project_roles.rs index 2182207428..db79fbbf9f 100644 --- a/crates/sqlbuild-orchestration/tests/credentials/test_project_roles.rs +++ b/crates/sqlbuild-orchestration/tests/credentials/test_project_roles.rs @@ -244,17 +244,13 @@ fn given_view_grant_on_part_of_the_location_when_listing_then_each_listing_is_de ProjectRoleTestCase { description: "SQLite location listings need view on every node", backend: Backend::Sqlite, - expected_before: Err( - "missing view on nodes customers, inventory, products; all requested nodes denied", - ), + expected_before: Err("missing view on one or more nodes in this location"), expected_after: Ok(SUPPORT_ROLE), }, ProjectRoleTestCase { description: "PostgreSQL location listings need view on every node", backend: Backend::Postgres, - expected_before: Err( - "missing view on nodes customers, inventory, products; all requested nodes denied", - ), + expected_before: Err("missing view on one or more nodes in this location"), expected_after: Ok(SUPPORT_ROLE), }, ]; diff --git a/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_denial.py b/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_denial.py index 2c1f786811..70784861e5 100644 --- a/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_denial.py +++ b/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_denial.py @@ -53,7 +53,13 @@ def test_given_denied_browser_requests_when_returned_then_only_durable_decision_ ) unlisted: str = catalog.replace(api.location_id, str(uuid4())) decisions: dict[str, str] = {} - for path in [catalog, unlisted, "/api/control/audit"]: + listing_denied: str = "missing view on one or more nodes in this location" + scope_denied: str = "missing permission in this scope; all requested nodes denied" + for path, reason in [ + (catalog, listing_denied), + (unlisted, listing_denied), + ("/api/control/audit", scope_denied), + ]: with pytest.raises(HTTPError) as rejected: person_read(api, path, session=session) assert rejected.value.code == 403 @@ -61,9 +67,7 @@ def test_given_denied_browser_requests_when_returned_then_only_durable_decision_ body: dict[str, Any] = json.load(rejected.value) assert set(body) == {"error", "decision_id"} decision: str = str(UUID(body["decision_id"])) - assert body["error"] == ( - "missing permission in this scope; all requested nodes denied; decision " + decision - ) + assert body["error"] == reason + "; decision " + decision decisions[decision] = "authorization.denied" request: dict[str, Any] = { "run_id": str(uuid4()), From 1f53d591dcfaa4d47d39f101c69481b15c6a70b1 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:34:49 +0000 Subject: [PATCH 2/2] fix: name no nodes in authorization denials Every refusal from person authorization now shows fixed text per action and its decision ID; the audit event keeps the missing nodes. Unknown jobs and triggers are refused only after the caller is authorized for the target, so an unknown object reads the same as a forbidden one. --- clients/typescript/tests/client.test.ts | 2 +- .../authorization/_helpers/custom_roles.rs | 24 +- .../src/authorization/constants.rs | 4 - .../authorization/main/authorize_listing.rs | 13 +- .../authorization/main/authorize_person.rs | 12 +- .../src/authorization/main/mod.rs | 1 + .../src/authorization/main/refuse_unknown.rs | 42 +++ .../src/authorization/mod.rs | 1 - .../declarations/_helpers/trigger_scope.rs | 27 +- .../main/change_person_trigger.rs | 2 +- .../src/runs/_helpers/requests/manual_job.rs | 30 +- .../{listing_denials => denials}/helpers.rs | 31 +- .../tests/api/test_contracts.rs | 8 +- .../tests/api/test_denials.rs | 268 ++++++++++++++++++ .../tests/api/test_listing_denials.rs | 132 --------- .../tests/api/test_types.rs | 6 +- .../tests/credentials/test_project_roles.rs | 36 +-- .../sqlbuild/references/docs/cli/server.md | 6 +- .../publication/test_denial.py | 8 +- .../publication/test_project_roles.py | 4 +- website/src/content/docs/docs/cli/server.mdx | 6 +- 21 files changed, 452 insertions(+), 211 deletions(-) delete mode 100644 crates/sqlbuild-orchestration/src/authorization/constants.rs create mode 100644 crates/sqlbuild-orchestration/src/authorization/main/refuse_unknown.rs rename crates/sqlbuild-orchestration/tests/api/{listing_denials => denials}/helpers.rs (57%) create mode 100644 crates/sqlbuild-orchestration/tests/api/test_denials.rs delete mode 100644 crates/sqlbuild-orchestration/tests/api/test_listing_denials.rs diff --git a/clients/typescript/tests/client.test.ts b/clients/typescript/tests/client.test.ts index 26781afc40..4c7c77a821 100644 --- a/clients/typescript/tests/client.test.ts +++ b/clients/typescript/tests/client.test.ts @@ -134,7 +134,7 @@ test("generated client preserves native scopes, replay, path encoding, SSE and b const decisionId: string | null | undefined = denied.error?.decision_id; assert.ok(decisionId); assert.deepEqual(denied.error, { - error: `missing view on one or more nodes in this location; decision ${decisionId}`, + error: `missing view permission for this target; decision ${decisionId}`, decision_id: decisionId, }); const machine = await browser.GET(healthPath, { headers, params }); diff --git a/crates/sqlbuild-orchestration/src/authorization/_helpers/custom_roles.rs b/crates/sqlbuild-orchestration/src/authorization/_helpers/custom_roles.rs index bfb34a400f..e62b97fb00 100644 --- a/crates/sqlbuild-orchestration/src/authorization/_helpers/custom_roles.rs +++ b/crates/sqlbuild-orchestration/src/authorization/_helpers/custom_roles.rs @@ -120,10 +120,18 @@ pub(crate) fn custom_coverage( (granting, missing) } -/// A denial names at most twenty uncovered nodes so the audited reason stays bounded. -pub(crate) fn denial_reason(action: Action, missing: &[String]) -> Result { +/// The audited reason naming up to twenty missing nodes, and node-free text for the caller. +pub(crate) fn denial_reason( + action: Action, + missing: &[String], +) -> Result<(String, String), ControlError> { + let action = action_name(action)?; + let shown = format!("missing {action} permission for this target"); if missing.is_empty() { - return Ok("missing permission in this scope; all requested nodes denied".into()); + return Ok(( + "missing permission in this scope; all requested nodes denied".into(), + shown, + )); } let listed: Vec<&str> = missing.iter().take(20).map(String::as_str).collect(); let more = missing.len() - listed.len(); @@ -132,10 +140,12 @@ pub(crate) fn denial_reason(action: Action, missing: &[String]) -> Result ControlError::Denied { - reason: LISTING_DENIED.into(), - decision_id, - }, - error => error, - }) + authorize_person(transaction, credentials, &scope, default_role).await } diff --git a/crates/sqlbuild-orchestration/src/authorization/main/authorize_person.rs b/crates/sqlbuild-orchestration/src/authorization/main/authorize_person.rs index 9489453bbf..a3b0d3cb82 100644 --- a/crates/sqlbuild-orchestration/src/authorization/main/authorize_person.rs +++ b/crates/sqlbuild-orchestration/src/authorization/main/authorize_person.rs @@ -96,6 +96,14 @@ pub(crate) async fn authorize_person( context.granting_role = "default:viewer".into(); return Ok((transaction, context)); } - let reason = denial_reason(scope.action, &missing)?; - deny(transaction, &context, json!(scope), &reason).await + let (audited, shown) = denial_reason(scope.action, &missing)?; + deny(transaction, &context, json!(scope), &audited) + .await + .map_err(|error| match error { + ControlError::Denied { decision_id, .. } => ControlError::Denied { + reason: shown, + decision_id, + }, + error => error, + }) } diff --git a/crates/sqlbuild-orchestration/src/authorization/main/mod.rs b/crates/sqlbuild-orchestration/src/authorization/main/mod.rs index 6ae84eb147..6c4ddfef68 100644 --- a/crates/sqlbuild-orchestration/src/authorization/main/mod.rs +++ b/crates/sqlbuild-orchestration/src/authorization/main/mod.rs @@ -11,3 +11,4 @@ pub(crate) mod publish_custom_roles; pub(crate) mod publish_group_roles; pub(crate) mod read_audit_events; pub mod read_security_audit; +pub(crate) mod refuse_unknown; diff --git a/crates/sqlbuild-orchestration/src/authorization/main/refuse_unknown.rs b/crates/sqlbuild-orchestration/src/authorization/main/refuse_unknown.rs new file mode 100644 index 0000000000..bafb0a9e6a --- /dev/null +++ b/crates/sqlbuild-orchestration/src/authorization/main/refuse_unknown.rs @@ -0,0 +1,42 @@ +//! An unknown object is refused like a forbidden one unless the person may act on the whole scope it was looked up in. + +use std::collections::BTreeSet; + +use uuid::Uuid; + +use crate::authorization::main::authorize_person::authorize_person; +use crate::credentials::models::{Action, AuditContext, DefaultRole, PermissionScope}; +use crate::store::errors::ControlError; +use crate::store::main::query::query; +use crate::store::models::{ControlTransaction, Parameter}; + +/// `location` is where the lookup happened, or `None` when the object alone named it. +pub(crate) async fn refuse_unknown( + transaction: ControlTransaction, + credentials: (&AuditContext, Uuid), + (action, location): (Action, Option<(Uuid, &str)>), + missing: ControlError, +) -> Result { + let (transaction, nodes) = match location { + Some((location_id, target)) => { + let (transaction, rows) = query(transaction, + "SELECT n.node_key FROM sqb_snapshot_nodes n JOIN sqb_current_snapshots c ON c.snapshot_id=n.snapshot_id \ + WHERE c.location_id=$1 AND c.target=$2", + vec![Parameter::Id(location_id), Parameter::Text(target.into())]).await?; + let nodes: BTreeSet = rows + .iter() + .filter_map(|row| row["node_key"].as_str().map(str::to_owned)) + .collect(); + (transaction, nodes) + } + None => (transaction, BTreeSet::new()), + }; + let scope = PermissionScope { + action, + location_id: location.map(|(location_id, _)| location_id), + target: location.map(|(_, target)| target.to_owned()), + nodes, + }; + authorize_person(transaction, credentials, &scope, DefaultRole::None).await?; + Err(missing) +} diff --git a/crates/sqlbuild-orchestration/src/authorization/mod.rs b/crates/sqlbuild-orchestration/src/authorization/mod.rs index 640a7b648c..d1ddfc554a 100644 --- a/crates/sqlbuild-orchestration/src/authorization/mod.rs +++ b/crates/sqlbuild-orchestration/src/authorization/mod.rs @@ -1,5 +1,4 @@ //! Closed, scoped person permissions and serialized installation-administrator lifecycle. pub(crate) mod _helpers; -pub(crate) mod constants; pub mod main; diff --git a/crates/sqlbuild-orchestration/src/declarations/_helpers/trigger_scope.rs b/crates/sqlbuild-orchestration/src/declarations/_helpers/trigger_scope.rs index 00b9691698..84e2af675a 100644 --- a/crates/sqlbuild-orchestration/src/declarations/_helpers/trigger_scope.rs +++ b/crates/sqlbuild-orchestration/src/declarations/_helpers/trigger_scope.rs @@ -2,6 +2,7 @@ use uuid::Uuid; +use crate::authorization::main::refuse_unknown::refuse_unknown; use crate::credentials::models::{Action, AuditContext, PermissionScope}; use crate::declarations::models::{TriggerChange, TriggerChangeRequest}; use crate::runs::main::frozen_job_request::frozen_job_request; @@ -14,15 +15,27 @@ use crate::store::models::{ControlTransaction, Parameter}; /// A group's scope is its frozen selection's write set, which covers every child's write set. pub(crate) async fn trigger_scope( transaction: ControlTransaction, - context: &AuditContext, + (context, credentials): (&AuditContext, Uuid), request: &TriggerChangeRequest, ) -> Result<(ControlTransaction, PermissionScope), ControlError> { + let action = match request.change { + TriggerChange::Enable | TriggerChange::Pause | TriggerChange::ResetDefault => { + Action::TriggerToggle + } + TriggerChange::SetCursor { .. } | TriggerChange::ResetCursor => Action::SensorResetCursor, + }; let (transaction, rows) = query(transaction, "SELECT location_id,target,job_key FROM sqb_triggers WHERE trigger_id=$1 AND removed_in_snapshot_id IS NULL", vec![Parameter::Id(request.trigger_id)]).await?; - let row = rows - .first() - .ok_or_else(|| ControlError::Invalid("current trigger not found".into()))?; + let Some(row) = rows.first() else { + return refuse_unknown( + transaction, + (context, credentials), + (action, None), + ControlError::Invalid("current trigger not found".into()), + ) + .await; + }; let job = JobRunRequest { run_id: Uuid::nil(), location_id: serde_json::from_value(row["location_id"].clone()) @@ -36,12 +49,6 @@ pub(crate) async fn trigger_scope( let (transaction, frozen) = frozen_job_request(transaction, context, &job).await?; let (transaction, nodes) = job_write_set(transaction, frozen.snapshot_id, &frozen.selection).await?; - let action = match request.change { - TriggerChange::Enable | TriggerChange::Pause | TriggerChange::ResetDefault => { - Action::TriggerToggle - } - TriggerChange::SetCursor { .. } | TriggerChange::ResetCursor => Action::SensorResetCursor, - }; Ok(( transaction, PermissionScope { diff --git a/crates/sqlbuild-orchestration/src/declarations/main/change_person_trigger.rs b/crates/sqlbuild-orchestration/src/declarations/main/change_person_trigger.rs index bab23514ed..25f8cc75ac 100644 --- a/crates/sqlbuild-orchestration/src/declarations/main/change_person_trigger.rs +++ b/crates/sqlbuild-orchestration/src/declarations/main/change_person_trigger.rs @@ -20,7 +20,7 @@ pub async fn change_person_trigger( ) -> Result { let (transaction, scope) = trigger_scope( begin(store, TransactionMode::Write).await?, - context, + (context, credentials), request, ) .await?; diff --git a/crates/sqlbuild-orchestration/src/runs/_helpers/requests/manual_job.rs b/crates/sqlbuild-orchestration/src/runs/_helpers/requests/manual_job.rs index a690f31d6d..531d0b8286 100644 --- a/crates/sqlbuild-orchestration/src/runs/_helpers/requests/manual_job.rs +++ b/crates/sqlbuild-orchestration/src/runs/_helpers/requests/manual_job.rs @@ -4,6 +4,7 @@ use serde_json::json; use uuid::Uuid; use crate::authorization::main::authorize_person::authorize_person; +use crate::authorization::main::refuse_unknown::refuse_unknown; use crate::credentials::main::audit::append_audit::append_audit; use crate::credentials::models::{Action, AuditContext, DefaultRole, PermissionScope}; use crate::runs::_helpers::requests::permissions::authorize_execution; @@ -14,7 +15,8 @@ use crate::runs::constants::MANUAL_JOB_AUDIT_TAG; use crate::runs::main::frozen_job_request::frozen_job_request; use crate::runs::models::{JobRunRequest, RunWindow}; use crate::store::errors::ControlError; -use crate::store::models::ControlTransaction; +use crate::store::main::query::query; +use crate::store::models::{ControlTransaction, Parameter}; pub(crate) async fn request_job_run_in( mut transaction: ControlTransaction, @@ -61,6 +63,10 @@ pub(crate) async fn request_job_run_in( if retry { return Ok((transaction, false)); } + let transaction = match credentials { + Some(credentials) => known_job(transaction, (&context, credentials), request).await?, + None => transaction, + }; let (mut transaction, frozen) = frozen_job_request(transaction, &context, request).await?; if let Some(credentials) = credentials { (transaction, context) = authorize_execution( @@ -78,3 +84,25 @@ pub(crate) async fn request_job_run_in( json!(null), json!({"request":request,"frozen":frozen}))).await?; Ok((transaction, true)) } + +/// A person's unknown job is refused like a forbidden one unless they may run every node of the target. +async fn known_job( + transaction: ControlTransaction, + credentials: (&AuditContext, Uuid), + request: &JobRunRequest, +) -> Result { + let (transaction, jobs) = query(transaction, + "SELECT j.job_key FROM sqb_jobs j JOIN sqb_current_snapshots c ON c.location_id=j.location_id \ + AND c.target=j.target AND c.snapshot_id=j.snapshot_id WHERE j.location_id=$1 AND j.target=$2 AND j.job_key=$3", + vec![Parameter::Id(request.location_id), Parameter::Text(request.target.clone()), Parameter::Text(request.job_key.clone())]).await?; + if jobs.is_empty() { + return refuse_unknown( + transaction, + credentials, + (Action::JobRun, Some((request.location_id, &request.target))), + ControlError::Invalid("published job not found in this location and target".into()), + ) + .await; + } + Ok(transaction) +} diff --git a/crates/sqlbuild-orchestration/tests/api/listing_denials/helpers.rs b/crates/sqlbuild-orchestration/tests/api/denials/helpers.rs similarity index 57% rename from crates/sqlbuild-orchestration/tests/api/listing_denials/helpers.rs rename to crates/sqlbuild-orchestration/tests/api/denials/helpers.rs index c33f11b046..548a653b95 100644 --- a/crates/sqlbuild-orchestration/tests/api/listing_denials/helpers.rs +++ b/crates/sqlbuild-orchestration/tests/api/denials/helpers.rs @@ -1,4 +1,4 @@ -//! Masked denial responses and the audit detail behind them. +//! Masked denial responses, person requests, and the audit detail behind them. use reqwest::header; use serde_json::{Value, json}; @@ -41,3 +41,32 @@ pub(crate) async fn audited_scope(store: &ControlStore, decision: Uuid) -> Value .map(|row| json!([row["scope_json"]["nodes"], row["after_json"]["reason"]])) .unwrap_or(Value::Null) } + +/// A person's browser request with session cookie, origin and CSRF proof. +pub(crate) async fn person_send( + client: &reqwest::Client, + (url, session, csrf): (&str, &str, &str), + body: Option, +) -> reqwest::Response { + let request = body.map_or_else(|| client.get(url), |body| client.post(url).json(&body)); + request + .header(header::HOST, "control.example.com") + .header(header::ORIGIN, "https://control.example.com") + .header(header::COOKIE, format!("__Host-sqb-session={session}")) + .header("x-sqb-csrf", csrf) + .send() + .await + .expect("person request") +} + +/// A job request body for a location and job key. +pub(crate) fn job(location_id: Uuid, job_key: &str) -> Value { + json!({"run_id": Uuid::now_v7(), "location_id": location_id, "target": "development", + "job_key": job_key, "reason": "Refresh orders", "priority": 0}) +} + +/// A trigger change body of the given kind. +pub(crate) fn trigger(trigger_id: Uuid, change: &str) -> Value { + json!({"event_id": Uuid::now_v7(), "trigger_id": trigger_id, "actor": "", + "reason": "Hold orders", "change": {"kind": change}}) +} diff --git a/crates/sqlbuild-orchestration/tests/api/test_contracts.rs b/crates/sqlbuild-orchestration/tests/api/test_contracts.rs index ec5b9b4b9c..c57b2e1c14 100644 --- a/crates/sqlbuild-orchestration/tests/api/test_contracts.rs +++ b/crates/sqlbuild-orchestration/tests/api/test_contracts.rs @@ -2,12 +2,12 @@ #[path = "changes/helpers.rs"] mod changes_helpers; +#[path = "denials/helpers.rs"] +mod denials_helpers; #[path = "fingerprint_reads/helpers.rs"] mod fingerprint_reads_helpers; #[path = "helpers.rs"] pub(crate) mod helpers; -#[path = "listing_denials/helpers.rs"] -mod listing_denials_helpers; #[path = "node_sql/helpers.rs"] mod node_sql_helpers; #[path = "node_status/helpers.rs"] @@ -42,12 +42,12 @@ mod test_cli_login; mod test_contract; #[path = "test_custom_roles.rs"] mod test_custom_roles; +#[path = "test_denials.rs"] +mod test_denials; #[path = "test_development.rs"] mod test_development; #[path = "test_external_sessions.rs"] mod test_external_sessions; -#[path = "test_listing_denials.rs"] -mod test_listing_denials; #[path = "test_location_health.rs"] mod test_location_health; #[path = "test_location_work.rs"] diff --git a/crates/sqlbuild-orchestration/tests/api/test_denials.rs b/crates/sqlbuild-orchestration/tests/api/test_denials.rs new file mode 100644 index 0000000000..838b41ebb7 --- /dev/null +++ b/crates/sqlbuild-orchestration/tests/api/test_denials.rs @@ -0,0 +1,268 @@ +//! Refusals name no node and read the same for a forbidden object and an unknown one. + +use serde_json::{Value, json}; +use uuid::Uuid; + +use crate::api::models::{ApiAuthentication, HumanLogin}; +use crate::api::tests::denials_helpers::{audited_scope, job, masked, person_send, trigger}; +use crate::api::tests::helpers::{start_api_with_authentication, stop_api}; +use crate::api::tests::test_types::{DenialRequest, DenialTestCase}; +use crate::credentials::tests::location_helpers::create_location; +use crate::credentials::tests::role_helpers::role_fixture; +use crate::declarations::main::list_triggers::list_triggers; +use crate::runs::main::request_job_run::request_job_run; +use crate::runs::models::JobRunRequest; +use crate::runs::tests::helpers::snapshot; +use crate::scheduler::tests::contracts::ticks::helpers::calendar_text; +use crate::snapshots::main::publish_snapshot::publish_snapshot; +use crate::store::main::migrate_store::migrate_store; +use crate::store::tests::store_helpers::{close_test_store, open_test_store, run}; +use crate::store::tests::store_test_types::Backend; + +/// Location-scoped reads and the action each needs. +const READS: [(&str, &str); 8] = [ + ("runs", "view"), + ("catalog?schema=orders", "view"), + ("statistics?schema=orders&name=orders", "view"), + ("checks", "view"), + ("checks/orders_not_null", "view"), + ("maintenance", "view"), + ("checks/orders_not_null/sql", "view.sql"), + ( + "checks/orders_not_null/results/01a1b000-0000-7000-8000-000000000000/evidence", + "view.evidence", + ), +]; + +const NODES: [&str; 4] = ["customers", "inventory", "orders", "products"]; + +#[test] +fn given_refused_requests_when_people_send_them_then_no_node_is_named_and_unknown_objects_look_forbidden() + { + let test_cases = [ + DenialTestCase { + description: "SQLite denials", + backend: Backend::Sqlite, + expected_status: 403, + }, + DenialTestCase { + description: "PostgreSQL denials", + backend: Backend::Postgres, + expected_status: 403, + }, + ]; + for test_case in test_cases { + run(async { + let Some(fixture) = open_test_store(test_case.backend).await else { + return; + }; + let store = &fixture.store; + migrate_store(store).await.expect("migrate"); + let roles = role_fixture(store).await; + let inventory = create_location(store, "inventory", "orders-admin") + .await + .expect("unassigned location"); + let mut published = snapshot(inventory); + published.documents.insert( + "jobs".into(), + json!({"orders": {"description": "Build orders", "selection": ["orders"]}}), + ); + published.documents.insert("triggers".into(), json!({"daily_orders": { + "description": "Refresh orders", "job_key": "orders", "starts_at": calendar_text(0), + "schedule": {"kind": "calendar", "times": [calendar_text(0), "2030-01-01T00:00:00Z"], + "refresh_at": "2029-01-01T00:00:00Z"}}})); + publish_snapshot(store, &published) + .await + .expect("unassigned snapshot"); + let trigger_id = list_triggers(store, inventory, "development") + .await + .expect("trigger")[0] + .trigger_id; + let queued = JobRunRequest { + run_id: Uuid::now_v7(), + location_id: inventory, + target: "development".into(), + job_key: "orders".into(), + reason: "Refresh orders".into(), + priority: 0, + window: None, + }; + assert!( + request_job_run(store, &roles.context, &queued) + .await + .expect("queued run") + ); + let api = start_api_with_authentication( + store, + ApiAuthentication { + login: HumanLogin::Password("https://control.example.com".into()), + ..ApiAuthentication::default() + }, + ) + .await; + let client = reqwest::Client::new(); + let member = (roles.member.bearer.as_str(), roles.member.csrf.as_str()); + let read = |location: Uuid, path: &str| { + format!( + "{}/api/control/locations/{location}/targets/development/{path}", + api.url + ) + }; + let runs = format!("{}/api/control/runs", api.url); + let triggers = format!("{}/api/control/triggers", api.url); + let cancel = format!("{}/api/control/runs/cancel", api.url); + let rerun = format!("{}/api/control/runs/rerun", api.url); + let unknown_run = Uuid::now_v7(); + let unknown_trigger = Uuid::now_v7(); + let mut cases: Vec<(String, &str, Vec)> = READS + .iter() + .map(|(path, action)| { + ( + (*path).to_owned(), + *action, + vec![ + (read(roles.location_id, path), None), + (read(inventory, path), None), + (read(Uuid::now_v7(), path), None), + ], + ) + }) + .collect(); + cases.extend([ + ( + "job run".to_owned(), + "job.run", + vec![ + (runs.clone(), Some(job(inventory, "orders"))), + (runs.clone(), Some(job(inventory, "returns"))), + (runs.clone(), Some(job(Uuid::now_v7(), "orders"))), + ], + ), + ( + "trigger pause".to_owned(), + "trigger.toggle", + vec![ + (triggers.clone(), Some(trigger(trigger_id, "pause"))), + (triggers.clone(), Some(trigger(unknown_trigger, "pause"))), + ], + ), + ( + "sensor cursor reset".to_owned(), + "sensor.reset_cursor", + vec![ + (triggers.clone(), Some(trigger(trigger_id, "reset_cursor"))), + ( + triggers.clone(), + Some(trigger(unknown_trigger, "reset_cursor")), + ), + ], + ), + ( + "run cancel".to_owned(), + "run.cancel", + vec![ + ( + cancel.clone(), + Some(json!({"run_id": queued.run_id, "event_id": Uuid::now_v7(), + "reason": "Stop orders"})), + ), + ( + cancel.clone(), + Some(json!({"run_id": unknown_run, "event_id": Uuid::now_v7(), + "reason": "Stop orders"})), + ), + ], + ), + ( + "rerun".to_owned(), + "run.rerun", + vec![ + ( + rerun.clone(), + Some( + json!({"run_id": Uuid::now_v7(), "source_run_id": queued.run_id, + "from_failure": false, "reason": "Retry orders", "priority": 0}), + ), + ), + ( + rerun.clone(), + Some( + json!({"run_id": Uuid::now_v7(), "source_run_id": unknown_run, + "from_failure": false, "reason": "Retry orders", "priority": 0}), + ), + ), + ], + ), + ]); + for (route, action, requests) in cases { + let mut responses: Vec<(Value, Uuid)> = Vec::new(); + for (url, body) in requests { + responses.push( + masked(person_send(&client, (&url, member.0, member.1), body).await).await, + ); + } + let first: Value = + serde_json::from_str(responses[0].0["body"].as_str().expect("body")) + .expect("JSON denial"); + assert_eq!( + ( + responses[0].0["status"].clone(), + first["error"].clone(), + responses + .iter() + .all(|(response, _)| *response == responses[0].0), + ), + ( + json!(test_case.expected_status), + json!(format!( + "missing {action} permission for this target; decision " + )), + true + ), + "{}: {route} reads the same for forbidden and unknown objects: {responses:?}", + test_case.description + ); + assert!( + NODES.iter().all(|node| !responses[0].0["body"] + .as_str() + .unwrap_or_default() + .contains(node)), + "{}: {route} names a node", + test_case.description + ); + for (_, decision) in &responses { + assert_ne!( + audited_scope(store, *decision).await, + Value::Null, + "{}: {route} audits every refusal", + test_case.description + ); + } + } + assert_eq!( + audited_scope( + store, + masked( + person_send( + &client, + (&read(inventory, "runs"), member.0, member.1), + None + ) + .await + ) + .await + .1 + ) + .await, + json!([ + NODES, + "missing view on nodes customers, inventory, orders, products; all requested nodes denied" + ]), + "{}: the audit keeps the missing nodes", + test_case.description + ); + stop_api(api).await; + close_test_store(fixture).await; + }); + } +} diff --git a/crates/sqlbuild-orchestration/tests/api/test_listing_denials.rs b/crates/sqlbuild-orchestration/tests/api/test_listing_denials.rs deleted file mode 100644 index 75824910e4..0000000000 --- a/crates/sqlbuild-orchestration/tests/api/test_listing_denials.rs +++ /dev/null @@ -1,132 +0,0 @@ -//! Refused listings name no node and read the same for an unassigned and an unknown location. - -use reqwest::header; -use serde_json::{Value, json}; -use uuid::Uuid; - -use crate::api::models::{ApiAuthentication, HumanLogin}; -use crate::api::tests::helpers::{start_api_with_authentication, stop_api}; -use crate::api::tests::listing_denials_helpers::{audited_scope, masked}; -use crate::api::tests::test_types::ListingDenialTestCase; -use crate::credentials::tests::location_helpers::create_location; -use crate::credentials::tests::role_helpers::role_fixture; -use crate::runs::tests::helpers::snapshot; -use crate::snapshots::main::publish_snapshot::publish_snapshot; -use crate::store::main::migrate_store::migrate_store; -use crate::store::tests::store_helpers::{close_test_store, open_test_store, run}; -use crate::store::tests::store_test_types::Backend; - -const LISTINGS: [&str; 6] = [ - "runs", - "catalog?schema=orders", - "statistics?schema=orders&name=orders", - "checks", - "checks/orders_not_null", - "maintenance", -]; - -const NODES: [&str; 4] = ["customers", "inventory", "orders", "products"]; - -#[test] -fn given_refused_listings_when_people_read_them_then_no_node_is_named_and_unknown_locations_look_unassigned() - { - let test_cases = [ - ListingDenialTestCase { - description: "SQLite listing denials", - backend: Backend::Sqlite, - expected_status: 403, - expected_error: "missing view on one or more nodes in this location; decision ", - }, - ListingDenialTestCase { - description: "PostgreSQL listing denials", - backend: Backend::Postgres, - expected_status: 403, - expected_error: "missing view on one or more nodes in this location; decision ", - }, - ]; - for test_case in test_cases { - run(async { - let Some(fixture) = open_test_store(test_case.backend).await else { - return; - }; - let store = &fixture.store; - migrate_store(store).await.expect("migrate"); - let roles = role_fixture(store).await; - let inventory = create_location(store, "inventory", "orders-admin") - .await - .expect("unassigned location"); - publish_snapshot(store, &snapshot(inventory)) - .await - .expect("unassigned snapshot"); - let api = start_api_with_authentication( - store, - ApiAuthentication { - login: HumanLogin::Password("https://control.example.com".into()), - ..ApiAuthentication::default() - }, - ) - .await; - let client = reqwest::Client::new(); - let read = |location: Uuid, listing: &str| { - client - .get(format!( - "{}/api/control/locations/{location}/targets/development/{listing}", - api.url - )) - .header(header::HOST, "control.example.com") - .header( - header::COOKIE, - format!("__Host-sqb-session={}", roles.member.bearer), - ) - .send() - }; - for listing in LISTINGS { - let (partial, partial_decision) = - masked(read(roles.location_id, listing).await.expect("partial")).await; - let (unassigned, unassigned_decision) = - masked(read(inventory, listing).await.expect("unassigned")).await; - let (unknown, unknown_decision) = - masked(read(Uuid::now_v7(), listing).await.expect("unknown")).await; - let body: Value = - serde_json::from_str(partial["body"].as_str().expect("body")).expect("JSON"); - assert_eq!( - ( - partial["status"].clone(), - body["error"].clone(), - unassigned == unknown, - partial == unassigned, - ), - ( - json!(test_case.expected_status), - json!(test_case.expected_error), - true, - true - ), - "{}: {listing} names no node and hides whether the location exists", - test_case.description - ); - assert!( - NODES - .iter() - .all(|node| !partial["body"].as_str().unwrap_or_default().contains(node)), - "{}: {listing} body names a node", - test_case.description - ); - assert_eq!( - ( - audited_scope(store, unassigned_decision).await[0].clone(), - audited_scope(store, unknown_decision).await[0].clone(), - audited_scope(store, partial_decision).await[1] - .as_str() - .is_some_and(|reason| reason.contains("inventory")), - ), - (json!(NODES), json!([]), true), - "{}: {listing} audits every refusal with the full detail", - test_case.description - ); - } - stop_api(api).await; - close_test_store(fixture).await; - }); - } -} diff --git a/crates/sqlbuild-orchestration/tests/api/test_types.rs b/crates/sqlbuild-orchestration/tests/api/test_types.rs index be4ad753ec..b304469843 100644 --- a/crates/sqlbuild-orchestration/tests/api/test_types.rs +++ b/crates/sqlbuild-orchestration/tests/api/test_types.rs @@ -119,11 +119,13 @@ pub(crate) struct CliLoginTestCase { pub(crate) expected_exchanges: usize, } -pub(crate) struct ListingDenialTestCase { +/// A refused request's URL and, for writes, its JSON body. +pub(crate) type DenialRequest = (String, Option); + +pub(crate) struct DenialTestCase { pub(crate) description: &'static str, pub(crate) backend: Backend, pub(crate) expected_status: u16, - pub(crate) expected_error: &'static str, } pub(crate) struct CustomRoleApiTestCase { diff --git a/crates/sqlbuild-orchestration/tests/credentials/test_project_roles.rs b/crates/sqlbuild-orchestration/tests/credentials/test_project_roles.rs index db79fbbf9f..387778bc48 100644 --- a/crates/sqlbuild-orchestration/tests/credentials/test_project_roles.rs +++ b/crates/sqlbuild-orchestration/tests/credentials/test_project_roles.rs @@ -19,7 +19,7 @@ use crate::store::main::migrate_store::migrate_store; use crate::store::tests::store_helpers::{close_test_store, open_test_store, run}; use crate::store::tests::store_test_types::Backend; -const ADHOC_DENIED: &str = "missing run.adhoc on nodes customers; all requested nodes denied"; +const ADHOC_DENIED: &str = "missing run.adhoc permission for this target"; #[test] fn given_published_selector_when_it_matches_more_nodes_then_coverage_grows_on_publish() { @@ -103,13 +103,13 @@ fn given_selector_resolving_to_no_nodes_when_published_then_it_grants_nothing() description: "SQLite grants nothing for an empty resolution", backend: Backend::Sqlite, expected_before: Ok(SUPPORT_ROLE), - expected_after: Err("missing permission in this scope; all requested nodes denied"), + expected_after: Err("missing view permission for this target"), }, ProjectRoleTestCase { description: "PostgreSQL grants nothing for an empty resolution", backend: Backend::Postgres, expected_before: Ok(SUPPORT_ROLE), - expected_after: Err("missing permission in this scope; all requested nodes denied"), + expected_after: Err("missing view permission for this target"), }, ]; for test_case in test_cases { @@ -149,8 +149,8 @@ fn given_selector_resolving_to_no_nodes_when_published_then_it_grants_nothing() .await .as_deref() .map_err(String::as_str), - Err("missing run.adhoc on nodes orders; all requested nodes denied"), - "{}: the denial names the uncovered node", + Err("missing run.adhoc permission for this target"), + "{}: the denial names no node", test_case.description ); close_test_store(fixture).await; @@ -165,22 +165,14 @@ fn given_grant_outside_a_triggers_job_when_the_member_overrides_it_then_both_tri ProjectRoleTestCase { description: "SQLite authorizes trigger overrides on the job's write set", backend: Backend::Sqlite, - expected_before: Err( - "missing trigger.toggle on nodes customers; all requested nodes denied", - ), - expected_after: Err( - "missing sensor.reset_cursor on nodes customers; all requested nodes denied", - ), + expected_before: Err("missing trigger.toggle permission for this target"), + expected_after: Err("missing sensor.reset_cursor permission for this target"), }, ProjectRoleTestCase { description: "PostgreSQL authorizes trigger overrides on the job's write set", backend: Backend::Postgres, - expected_before: Err( - "missing trigger.toggle on nodes customers; all requested nodes denied", - ), - expected_after: Err( - "missing sensor.reset_cursor on nodes customers; all requested nodes denied", - ), + expected_before: Err("missing trigger.toggle permission for this target"), + expected_after: Err("missing sensor.reset_cursor permission for this target"), }, ]; for test_case in test_cases { @@ -244,13 +236,13 @@ fn given_view_grant_on_part_of_the_location_when_listing_then_each_listing_is_de ProjectRoleTestCase { description: "SQLite location listings need view on every node", backend: Backend::Sqlite, - expected_before: Err("missing view on one or more nodes in this location"), + expected_before: Err("missing view permission for this target"), expected_after: Ok(SUPPORT_ROLE), }, ProjectRoleTestCase { description: "PostgreSQL location listings need view on every node", backend: Backend::Postgres, - expected_before: Err("missing view on one or more nodes in this location"), + expected_before: Err("missing view permission for this target"), expected_after: Ok(SUPPORT_ROLE), }, ]; @@ -298,13 +290,13 @@ fn given_role_removed_from_code_when_its_assignment_remains_then_it_grants_nothi description: "SQLite stale assignment grants nothing", backend: Backend::Sqlite, expected_before: Ok(SUPPORT_ROLE), - expected_after: Err("missing run.adhoc on nodes orders; all requested nodes denied"), + expected_after: Err("missing run.adhoc permission for this target"), }, ProjectRoleTestCase { description: "PostgreSQL stale assignment grants nothing", backend: Backend::Postgres, expected_before: Ok(SUPPORT_ROLE), - expected_after: Err("missing run.adhoc on nodes orders; all requested nodes denied"), + expected_after: Err("missing run.adhoc permission for this target"), }, ]; for test_case in test_cases { @@ -351,7 +343,7 @@ fn given_role_removed_from_code_when_its_assignment_remains_then_it_grants_nothi .await .as_deref() .map_err(String::as_str), - Err("missing permission in this scope; all requested nodes denied"), + Err("missing view permission for this target"), "{}: nor location-wide", test_case.description ); diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/server.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/server.md index dfa7f047ff..aee81e65cd 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/server.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/server.md @@ -172,5 +172,7 @@ permissions = ["view.evidence"] Assign a project role with `POST /api/admin/roles`, using the role key as `role`. Each publication replaces the location's project roles: new and changed grants apply at once, and removed roles stop applying. A selector covers newly matching nodes as soon as they are -published. Each change appears in the publication's change list as `role:`. A denial lists -the nodes the request was missing. +published. Each change appears in the publication's change list as `role:`. A denial names +only the missing permission, such as `missing view permission for this target`, and its decision +ID. It names no nodes, and an unknown location, job, trigger or run is refused with the same +response. The security audit event for that decision ID lists the nodes the request was missing. diff --git a/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_denial.py b/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_denial.py index 70784861e5..00906a3c9d 100644 --- a/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_denial.py +++ b/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_denial.py @@ -53,12 +53,11 @@ def test_given_denied_browser_requests_when_returned_then_only_durable_decision_ ) unlisted: str = catalog.replace(api.location_id, str(uuid4())) decisions: dict[str, str] = {} - listing_denied: str = "missing view on one or more nodes in this location" - scope_denied: str = "missing permission in this scope; all requested nodes denied" + listing_denied: str = "missing view permission for this target" for path, reason in [ (catalog, listing_denied), (unlisted, listing_denied), - ("/api/control/audit", scope_denied), + ("/api/control/audit", "missing audit.view permission for this target"), ]: with pytest.raises(HTTPError) as rejected: person_read(api, path, session=session) @@ -97,8 +96,7 @@ def test_given_denied_browser_requests_when_returned_then_only_durable_decision_ body["error"] == { "authorization.denied": ( - "missing permission in this scope; all requested nodes denied; decision " - + decision + "missing job.run permission for this target; decision " + decision ), "authentication.denied": "authentication rejected", }[action] diff --git a/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_project_roles.py b/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_project_roles.py index 61968b6f40..fc6221eb23 100644 --- a/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_project_roles.py +++ b/tests/integration/src/sqlbuild/compiler/compiled_project/publication/test_project_roles.py @@ -33,9 +33,7 @@ "Warning: role orders_support grant 2 selects no nodes (tag:support,tag:pii); " "it grants nothing in this snapshot" ), - expected_denial=( - "missing view.sql on nodes model:customers; all requested nodes denied" - ), + expected_denial="missing view.sql permission for this target", ) ], ids=lambda case: case.description, diff --git a/website/src/content/docs/docs/cli/server.mdx b/website/src/content/docs/docs/cli/server.mdx index 91be26c0d9..37144b9e86 100644 --- a/website/src/content/docs/docs/cli/server.mdx +++ b/website/src/content/docs/docs/cli/server.mdx @@ -164,5 +164,7 @@ permissions = ["view.evidence"] Assign a project role with `POST /api/admin/roles`, using the role key as `role`. Each publication replaces the location's project roles: new and changed grants apply at once, and removed roles stop applying. A selector covers newly matching nodes as soon as they are -published. Each change appears in the publication's change list as `role:`. A denial lists -the nodes the request was missing. +published. Each change appears in the publication's change list as `role:`. A denial names +only the missing permission, such as `missing view permission for this target`, and its decision +ID. It names no nodes, and an unknown location, job, trigger or run is refused with the same +response. The security audit event for that decision ID lists the nodes the request was missing.