diff --git a/clients/typescript/tests/client.test.ts b/clients/typescript/tests/client.test.ts index bd937b2883..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 permission in this scope; all requested nodes denied; 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: 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/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/denials/helpers.rs b/crates/sqlbuild-orchestration/tests/api/denials/helpers.rs new file mode 100644 index 0000000000..548a653b95 --- /dev/null +++ b/crates/sqlbuild-orchestration/tests/api/denials/helpers.rs @@ -0,0 +1,72 @@ +//! Masked denial responses, person requests, 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) +} + +/// 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 4d798f30a5..73b9d6681c 100644 --- a/crates/sqlbuild-orchestration/tests/api/test_contracts.rs +++ b/crates/sqlbuild-orchestration/tests/api/test_contracts.rs @@ -2,6 +2,8 @@ #[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"] @@ -40,6 +42,8 @@ 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"] 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_types.rs b/crates/sqlbuild-orchestration/tests/api/test_types.rs index 7d4430d6ec..b304469843 100644 --- a/crates/sqlbuild-orchestration/tests/api/test_types.rs +++ b/crates/sqlbuild-orchestration/tests/api/test_types.rs @@ -119,6 +119,15 @@ pub(crate) struct CliLoginTestCase { pub(crate) expected_exchanges: usize, } +/// 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) 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..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,17 +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 nodes customers, inventory, products; all requested nodes denied", - ), + 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 nodes customers, inventory, products; all requested nodes denied", - ), + expected_before: Err("missing view permission for this target"), expected_after: Ok(SUPPORT_ROLE), }, ]; @@ -302,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 { @@ -355,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 2c1f786811..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,7 +53,12 @@ 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 permission for this target" + for path, reason in [ + (catalog, listing_denied), + (unlisted, listing_denied), + ("/api/control/audit", "missing audit.view permission for this target"), + ]: with pytest.raises(HTTPError) as rejected: person_read(api, path, session=session) assert rejected.value.code == 403 @@ -61,9 +66,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()), @@ -93,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.