diff --git a/.sqlx/query-6bc21fd940535409a6b59d230fed417ff92af5cb2f6c11a35944288b14fde343.json b/.sqlx/query-266d49e612499108de0c3742330a2e00b39906b01f43d835ec160751ed35c684.json similarity index 55% rename from .sqlx/query-6bc21fd940535409a6b59d230fed417ff92af5cb2f6c11a35944288b14fde343.json rename to .sqlx/query-266d49e612499108de0c3742330a2e00b39906b01f43d835ec160751ed35c684.json index c1ba203be75..d401fe34493 100644 --- a/.sqlx/query-6bc21fd940535409a6b59d230fed417ff92af5cb2f6c11a35944288b14fde343.json +++ b/.sqlx/query-266d49e612499108de0c3742330a2e00b39906b01f43d835ec160751ed35c684.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n with collections(id) as (\n select ls.id\n from unnest($2::text[]) as names(catalog_name)\n join live_specs ls on ls.catalog_name = names.catalog_name\n ),\n exp(id) as (\n select lsf.source_id as id\n from collections c\n join live_spec_flows lsf on c.id = lsf.target_id\n union\n select lsf.target_id as id\n from collections c\n join live_spec_flows lsf on c.id = lsf.source_id\n )\n select\n ls.id as \"id: Id\",\n ls.last_pub_id as \"last_pub_id: Id\",\n ls.last_build_id as \"last_build_id: Id\",\n ls.data_plane_id as \"data_plane_id: Id\",\n ls.catalog_name,\n ls.spec_type as \"spec_type?: CatalogType\",\n ls.spec as \"spec: TextJson>\",\n ls.built_spec as \"built_spec: TextJson>\",\n ls.inferred_schema_md5,\n (\n select max(capability) from internal.user_roles($1) r\n where starts_with(ls.catalog_name, r.role_prefix)\n ) as \"user_capability: Capability\",\n coalesce(\n (select json_agg(row_to_json(role_grants))\n from role_grants\n where starts_with(ls.catalog_name, subject_role)),\n '[]'\n ) as \"spec_capabilities!: Json>\",\n ls.dependency_hash\n from exp\n join live_specs ls on ls.id = exp.id\n where ls.spec is not null and not ls.catalog_name = any($3);\n ", + "query": "\n with collections(id) as (\n select ls.id\n from unnest($1::text[]) as names(catalog_name)\n join live_specs ls on ls.catalog_name = names.catalog_name\n ),\n exp(id) as (\n select lsf.source_id as id\n from collections c\n join live_spec_flows lsf on c.id = lsf.target_id\n union\n select lsf.target_id as id\n from collections c\n join live_spec_flows lsf on c.id = lsf.source_id\n )\n select\n ls.id as \"id: Id\",\n ls.last_pub_id as \"last_pub_id: Id\",\n ls.last_build_id as \"last_build_id: Id\",\n ls.data_plane_id as \"data_plane_id: Id\",\n ls.catalog_name,\n ls.spec_type as \"spec_type?: CatalogType\",\n ls.spec as \"spec: TextJson>\",\n ls.built_spec as \"built_spec: TextJson>\",\n ls.inferred_schema_md5,\n ls.dependency_hash\n from exp\n join live_specs ls on ls.id = exp.id\n where ls.spec is not null and not ls.catalog_name = any($2);\n ", "describe": { "columns": [ { @@ -62,62 +62,12 @@ }, { "ordinal": 9, - "name": "user_capability: Capability", - "type_info": { - "Custom": { - "name": "grant_capability", - "kind": { - "Enum": [ - "none", - "x_01", - "x_02", - "x_03", - "x_04", - "x_05", - "x_06", - "x_07", - "x_08", - "x_09", - "read", - "x_11", - "x_12", - "x_13", - "x_14", - "x_15", - "x_16", - "x_17", - "x_18", - "x_19", - "write", - "x_21", - "x_22", - "x_23", - "x_24", - "x_25", - "x_26", - "x_27", - "x_28", - "x_29", - "admin" - ] - } - } - } - }, - { - "ordinal": 10, - "name": "spec_capabilities!: Json>", - "type_info": "Json" - }, - { - "ordinal": 11, "name": "dependency_hash", "type_info": "Text" } ], "parameters": { "Left": [ - "Uuid", "TextArray", "TextArray" ] @@ -132,10 +82,8 @@ true, true, true, - null, - null, true ] }, - "hash": "6bc21fd940535409a6b59d230fed417ff92af5cb2f6c11a35944288b14fde343" + "hash": "266d49e612499108de0c3742330a2e00b39906b01f43d835ec160751ed35c684" } diff --git a/.sqlx/query-34dcc2c093232d6e00d1192a46fb2aedf85caf5c5625373ddd4311601dd1fa9c.json b/.sqlx/query-34dcc2c093232d6e00d1192a46fb2aedf85caf5c5625373ddd4311601dd1fa9c.json new file mode 100644 index 00000000000..99b7fa35208 --- /dev/null +++ b/.sqlx/query-34dcc2c093232d6e00d1192a46fb2aedf85caf5c5625373ddd4311601dd1fa9c.json @@ -0,0 +1,88 @@ +{ + "db_name": "PostgreSQL", + "query": "\n select\n coalesce(ls.id, '00:00:00:00:00:00:00:00'::flowid) as \"id!: Id\",\n coalesce(ls.last_pub_id, '00:00:00:00:00:00:00:00'::flowid) as \"last_pub_id!: Id\",\n coalesce(ls.last_build_id, '00:00:00:00:00:00:00:00'::flowid) as \"last_build_id!: Id\",\n coalesce(ls.data_plane_id, '00:00:00:00:00:00:00:00'::flowid) as \"data_plane_id!: Id\",\n names as \"catalog_name!: String\",\n ls.spec_type as \"spec_type?: CatalogType\",\n ls.spec as \"spec: TextJson>\",\n ls.built_spec as \"built_spec: TextJson>\",\n ls.inferred_schema_md5,\n ls.dependency_hash\n from unnest($1::text[]) names\n left outer join live_specs ls on ls.catalog_name = names\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id!: Id", + "type_info": "Macaddr8" + }, + { + "ordinal": 1, + "name": "last_pub_id!: Id", + "type_info": "Macaddr8" + }, + { + "ordinal": 2, + "name": "last_build_id!: Id", + "type_info": "Macaddr8" + }, + { + "ordinal": 3, + "name": "data_plane_id!: Id", + "type_info": "Macaddr8" + }, + { + "ordinal": 4, + "name": "catalog_name!: String", + "type_info": "Text" + }, + { + "ordinal": 5, + "name": "spec_type?: CatalogType", + "type_info": { + "Custom": { + "name": "catalog_spec_type", + "kind": { + "Enum": [ + "capture", + "collection", + "materialization", + "test" + ] + } + } + } + }, + { + "ordinal": 6, + "name": "spec: TextJson>", + "type_info": "Json" + }, + { + "ordinal": 7, + "name": "built_spec: TextJson>", + "type_info": "Json" + }, + { + "ordinal": 8, + "name": "inferred_schema_md5", + "type_info": "Text" + }, + { + "ordinal": 9, + "name": "dependency_hash", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "TextArray" + ] + }, + "nullable": [ + null, + null, + null, + null, + null, + true, + true, + true, + true, + true + ] + }, + "hash": "34dcc2c093232d6e00d1192a46fb2aedf85caf5c5625373ddd4311601dd1fa9c" +} diff --git a/.sqlx/query-6336a73b8d0dedacb47563fc753ca867a6e5a285c2862d552910767381170ab0.json b/.sqlx/query-6336a73b8d0dedacb47563fc753ca867a6e5a285c2862d552910767381170ab0.json deleted file mode 100644 index 84aea275149..00000000000 --- a/.sqlx/query-6336a73b8d0dedacb47563fc753ca867a6e5a285c2862d552910767381170ab0.json +++ /dev/null @@ -1,142 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n with user_roles as materialized (\n select role_prefix, capability from internal.user_roles($1)\n )\n select\n coalesce(ls.id, '00:00:00:00:00:00:00:00'::flowid) as \"id!: Id\",\n coalesce(ls.last_pub_id, '00:00:00:00:00:00:00:00'::flowid) as \"last_pub_id!: Id\",\n coalesce(ls.last_build_id, '00:00:00:00:00:00:00:00'::flowid) as \"last_build_id!: Id\",\n coalesce(ls.data_plane_id, '00:00:00:00:00:00:00:00'::flowid) as \"data_plane_id!: Id\",\n names as \"catalog_name!: String\",\n ls.spec_type as \"spec_type?: CatalogType\",\n ls.spec as \"spec: TextJson>\",\n ls.built_spec as \"built_spec: TextJson>\",\n ls.inferred_schema_md5,\n case when $3 then (\n select max(capability) from user_roles\n where starts_with(names, user_roles.role_prefix)\n ) else\n null\n end as \"user_capability: Capability\",\n case when $4 then coalesce(\n (select json_agg(row_to_json(role_grants))\n from role_grants\n where starts_with(names, subject_role)),\n '[]'\n ) else\n '[]'\n end as \"spec_capabilities!: Json>\",\n ls.dependency_hash\n from unnest($2::text[]) names\n left outer join live_specs ls on ls.catalog_name = names\n ", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "id!: Id", - "type_info": "Macaddr8" - }, - { - "ordinal": 1, - "name": "last_pub_id!: Id", - "type_info": "Macaddr8" - }, - { - "ordinal": 2, - "name": "last_build_id!: Id", - "type_info": "Macaddr8" - }, - { - "ordinal": 3, - "name": "data_plane_id!: Id", - "type_info": "Macaddr8" - }, - { - "ordinal": 4, - "name": "catalog_name!: String", - "type_info": "Text" - }, - { - "ordinal": 5, - "name": "spec_type?: CatalogType", - "type_info": { - "Custom": { - "name": "catalog_spec_type", - "kind": { - "Enum": [ - "capture", - "collection", - "materialization", - "test" - ] - } - } - } - }, - { - "ordinal": 6, - "name": "spec: TextJson>", - "type_info": "Json" - }, - { - "ordinal": 7, - "name": "built_spec: TextJson>", - "type_info": "Json" - }, - { - "ordinal": 8, - "name": "inferred_schema_md5", - "type_info": "Text" - }, - { - "ordinal": 9, - "name": "user_capability: Capability", - "type_info": { - "Custom": { - "name": "grant_capability", - "kind": { - "Enum": [ - "none", - "x_01", - "x_02", - "x_03", - "x_04", - "x_05", - "x_06", - "x_07", - "x_08", - "x_09", - "read", - "x_11", - "x_12", - "x_13", - "x_14", - "x_15", - "x_16", - "x_17", - "x_18", - "x_19", - "write", - "x_21", - "x_22", - "x_23", - "x_24", - "x_25", - "x_26", - "x_27", - "x_28", - "x_29", - "admin" - ] - } - } - } - }, - { - "ordinal": 10, - "name": "spec_capabilities!: Json>", - "type_info": "Json" - }, - { - "ordinal": 11, - "name": "dependency_hash", - "type_info": "Text" - } - ], - "parameters": { - "Left": [ - "Uuid", - "TextArray", - "Bool", - "Bool" - ] - }, - "nullable": [ - null, - null, - null, - null, - null, - true, - true, - true, - true, - null, - null, - true - ] - }, - "hash": "6336a73b8d0dedacb47563fc753ca867a6e5a285c2862d552910767381170ab0" -} diff --git a/.sqlx/query-87101040b0795647b0b9f144f24dc199f724caf45a9d9755a9fdc9fba9dc05e2.json b/.sqlx/query-e12384cbb271c0adffbeea930e0435851d0b21a93f5705f592796a5abe1542b5.json similarity index 63% rename from .sqlx/query-87101040b0795647b0b9f144f24dc199f724caf45a9d9755a9fdc9fba9dc05e2.json rename to .sqlx/query-e12384cbb271c0adffbeea930e0435851d0b21a93f5705f592796a5abe1542b5.json index c29fbf78e4b..70c86e1ebbf 100644 --- a/.sqlx/query-87101040b0795647b0b9f144f24dc199f724caf45a9d9755a9fdc9fba9dc05e2.json +++ b/.sqlx/query-e12384cbb271c0adffbeea930e0435851d0b21a93f5705f592796a5abe1542b5.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n WITH\n data_plane_ids AS (\n SELECT id\n FROM UNNEST($1::flowid[]) AS t(id)\n ),\n data_plane_names AS (\n SELECT name\n FROM UNNEST($2::text[]) AS t(name)\n -- User must be read-authorized to data-plane.\n WHERE EXISTS (\n SELECT 1\n FROM internal.user_roles($3, 'read') AS r\n WHERE starts_with(t.name, r.role_prefix)\n )\n )\n SELECT\n d.id AS \"control_id: Id\",\n d.data_plane_name,\n d.closed,\n d.hmac_keys,\n d.encrypted_hmac_keys AS \"encrypted_hmac_keys: models::RawValue\",\n d.data_plane_fqdn,\n d.broker_address,\n d.reactor_address,\n d.dekaf_address,\n d.dekaf_registry_address,\n d.ops_logs_name AS \"ops_logs_name: models::Collection\",\n d.ops_stats_name AS \"ops_stats_name: models::Collection\"\n FROM data_planes d\n WHERE\n d.id IN (select id from data_plane_ids) OR\n d.data_plane_name in (select name from data_plane_names)\n ", + "query": "\n WITH\n data_plane_ids AS (\n SELECT id\n FROM UNNEST($1::flowid[]) AS t(id)\n ),\n data_plane_names AS (\n -- Names have already passed the caller's user-authorization policy,\n -- so no in-SQL authorization check is needed here.\n SELECT name\n FROM UNNEST($2::text[]) AS t(name)\n )\n SELECT\n d.id AS \"control_id: Id\",\n d.data_plane_name,\n d.closed,\n d.hmac_keys,\n d.encrypted_hmac_keys AS \"encrypted_hmac_keys: models::RawValue\",\n d.data_plane_fqdn,\n d.broker_address,\n d.reactor_address,\n d.dekaf_address,\n d.dekaf_registry_address,\n d.ops_logs_name AS \"ops_logs_name: models::Collection\",\n d.ops_stats_name AS \"ops_stats_name: models::Collection\"\n FROM data_planes d\n WHERE\n d.id IN (select id from data_plane_ids) OR\n d.data_plane_name in (select name from data_plane_names)\n ", "describe": { "columns": [ { @@ -81,8 +81,7 @@ } } }, - "TextArray", - "Uuid" + "TextArray" ] }, "nullable": [ @@ -100,5 +99,5 @@ false ] }, - "hash": "87101040b0795647b0b9f144f24dc199f724caf45a9d9755a9fdc9fba9dc05e2" + "hash": "e12384cbb271c0adffbeea930e0435851d0b21a93f5705f592796a5abe1542b5" } diff --git a/.sqlx/query-f133c223a0df27245c413ef1ef8d94773a3c9ce12c6c34161874455e206e35dc.json b/.sqlx/query-f133c223a0df27245c413ef1ef8d94773a3c9ce12c6c34161874455e206e35dc.json deleted file mode 100644 index 858e7943170..00000000000 --- a/.sqlx/query-f133c223a0df27245c413ef1ef8d94773a3c9ce12c6c34161874455e206e35dc.json +++ /dev/null @@ -1,89 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "\n SELECT\n d.id AS \"control_id: Id\",\n d.data_plane_name,\n d.closed,\n d.hmac_keys,\n d.encrypted_hmac_keys AS \"encrypted_hmac_keys: models::RawValue\",\n d.data_plane_fqdn,\n d.broker_address,\n d.reactor_address,\n d.dekaf_address,\n d.dekaf_registry_address,\n d.ops_logs_name AS \"ops_logs_name: models::Collection\",\n d.ops_stats_name AS \"ops_stats_name: models::Collection\"\n FROM data_planes d\n WHERE data_plane_name = $1\n AND EXISTS (\n SELECT 1 FROM internal.user_roles($2, 'read') r\n WHERE starts_with($1, r.role_prefix)\n )\n ", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "control_id: Id", - "type_info": "Macaddr8" - }, - { - "ordinal": 1, - "name": "data_plane_name", - "type_info": "Text" - }, - { - "ordinal": 2, - "name": "closed", - "type_info": "Bool" - }, - { - "ordinal": 3, - "name": "hmac_keys", - "type_info": "TextArray" - }, - { - "ordinal": 4, - "name": "encrypted_hmac_keys: models::RawValue", - "type_info": "Json" - }, - { - "ordinal": 5, - "name": "data_plane_fqdn", - "type_info": "Text" - }, - { - "ordinal": 6, - "name": "broker_address", - "type_info": "Text" - }, - { - "ordinal": 7, - "name": "reactor_address", - "type_info": "Text" - }, - { - "ordinal": 8, - "name": "dekaf_address", - "type_info": "Text" - }, - { - "ordinal": 9, - "name": "dekaf_registry_address", - "type_info": "Text" - }, - { - "ordinal": 10, - "name": "ops_logs_name: models::Collection", - "type_info": "Text" - }, - { - "ordinal": 11, - "name": "ops_stats_name: models::Collection", - "type_info": "Text" - } - ], - "parameters": { - "Left": [ - "Text", - "Uuid" - ] - }, - "nullable": [ - false, - false, - false, - false, - false, - false, - false, - false, - true, - true, - false, - false - ] - }, - "hash": "f133c223a0df27245c413ef1ef8d94773a3c9ce12c6c34161874455e206e35dc" -} diff --git a/crates/agent/src/controlplane.rs b/crates/agent/src/controlplane.rs index fb14799a0b9..ce2b1bda85b 100644 --- a/crates/agent/src/controlplane.rs +++ b/crates/agent/src/controlplane.rs @@ -577,12 +577,16 @@ impl ControlPlane for PGControlPlane } async fn get_live_specs(&self, names: BTreeSet) -> anyhow::Result { + let snapshot = self.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let names = names.into_iter().collect::>(); let mut live = live_specs::get_live_specs( self.system_user_id, &names, None, // don't filter based on user capability &self.pool, + &snapshot, + None, ) .await?; @@ -655,6 +659,9 @@ impl ControlPlane for PGControlPlane logs_token, data_plane: data_plane.clone(), created_at, + snapshot, + // `filter_user_authz` is false, so no staleness anchor is consulted. + started_at: None, }; discovers_handler.discover(pool, req).await } @@ -666,6 +673,7 @@ impl ControlPlane for PGControlPlane draft: tables::DraftCatalog, default_data_plane: Option, ) -> anyhow::Result { + let snapshot = self.snapshot_watch.token(); let publication = DraftPublication { user_id: self.system_user_id, logs_token, @@ -673,7 +681,14 @@ impl ControlPlane for PGControlPlane detail, dry_run: false, default_data_plane_name: default_data_plane, - // skip authz checks for controller-initiated publications + // Controllers construct a fresh publication per poll, so they have + // no instant that stays fixed across attempts to anchor staleness + // on; they carry their own retry/backoff instead. + started_at: None, + snapshot: snapshot + .result() + .expect("authorization snapshot is not ready"), + // Skip user-to-catalog checks; spec-to-spec `RoleGrant` checks remain mandatory. verify_user_authz: false, initialize: NoopInitialize, finalize: PruneUnboundCollections, diff --git a/crates/agent/src/discovers.rs b/crates/agent/src/discovers.rs index a0bfdbc3583..6f5e76877a2 100644 --- a/crates/agent/src/discovers.rs +++ b/crates/agent/src/discovers.rs @@ -1,6 +1,6 @@ use anyhow::Context; use control_plane_api::{ - connector_tags, + Authorization, Snapshot, connector_tags, discovers::{Discover, DiscoverHandler, Row, fetch_discover}, draft, live_specs, proxy_connectors::DiscoverConnectors, @@ -19,6 +19,7 @@ pub enum JobStatus { PullFailed, DiscoverFailed, MergeFailed, + NotAuthorized, Success { #[serde(default, skip_serializing_if = "Option::is_none")] publication_id: Option, @@ -43,25 +44,36 @@ type ProcessResult = Result, } -pub struct DiscoverOutcome { - id: Id, - draft_id: Id, - result: ProcessResult, - status: JobStatus, +/// Outcome of evaluating a discover in `DiscoverExecutor::process`. +enum Processed { + Resolved(JobStatus, ProcessResult), + /// Control-plane state is not authoritative because the snapshot predates + /// the discover row. A refresh has been requested; retry after a short delay. + RetryStale, +} + +pub enum DiscoverOutcome { + Resolved { + id: Id, + draft_id: Id, + result: ProcessResult, + status: JobStatus, + }, + /// The control-plane snapshot was stale; the discover is left queued and + /// re-polled once refreshed state should be authoritative. + RetryStale, } impl automations::Outcome for DiscoverOutcome { @@ -69,12 +81,22 @@ impl automations::Outcome for DiscoverOutcome { self, txn: &'s mut sqlx::PgConnection, ) -> anyhow::Result { - let DiscoverOutcome { + let DiscoverOutcome::Resolved { id, draft_id, result, status, - } = self; + } = self + else { + // Leave the discover unresolved and re-poll once a refresh could + // have landed. If the Snapshot is still not authoritative by + // then, the poll defers again (see `Snapshot::taken_after`). + return Ok(automations::Action::Sleep( + Snapshot::STALE_RETRY_WAKE + .to_std() + .expect("wake interval is positive"), + )); + }; control_plane_api::draft::delete_errors(draft_id, txn) .await @@ -94,12 +116,13 @@ impl automations::Outcome for DiscoverOutcome { } } -fn precheck_failed(status: JobStatus) -> (JobStatus, ProcessResult) { - (status, Err(Vec::new())) +fn precheck_failed(status: JobStatus) -> Processed { + Processed::Resolved(status, Err(Vec::new())) } pub struct DiscoverExecutor { pub handler: DiscoverHandler, + pub snapshot_watch: std::sync::Arc>, } impl automations::Executor for DiscoverExecutor { @@ -119,23 +142,53 @@ impl automations::Executor for DiscoverExecutor { pool: &'s sqlx::PgPool, task_id: models::Id, _parent_id: Option, - _state: &'s mut Self::State, + state: &'s mut Self::State, inbox: &'s mut std::collections::VecDeque<(models::Id, Option)>, ) -> anyhow::Result { tracing::debug!(?inbox, %task_id, "executing discover task"); let row = fetch_discover(task_id, pool).await?; let draft_id = row.draft_id; assert_eq!(row.id, task_id); + let queued_at = row.updated_at; let time_queued = chrono::Utc::now().signed_duration_since(row.updated_at); - let (status, result) = self.process(row, pool).await?; - tracing::info!(id=%task_id, %time_queued, ?status, "finished"); + + // Pin one Snapshot for this poll: the deferral decision and every + // authorization decision of the discover observe the same view. + let snapshot = self.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); + + // A prior attempt could not classify under a stale Snapshot. Defer — + // without pre-flight checks or connector work — until this instance's + // Snapshot is authoritative for the recorded instant. + if let Some(anchor) = state.as_ref().and_then(|s| s.awaiting_snapshot_after) { + if !snapshot.taken_after(anchor) { + snapshot.revoke.cancel(); + inbox.clear(); + return Ok(DiscoverOutcome::RetryStale); + } + } + + let processed = self.process(row, pool, &snapshot).await?; inbox.clear(); - Ok(DiscoverOutcome { - id: task_id, - draft_id, - result, - status, - }) + match processed { + Processed::Resolved(status, result) => { + tracing::info!(id=%task_id, %time_queued, ?status, "finished"); + Ok(DiscoverOutcome::Resolved { + id: task_id, + draft_id, + result, + status, + }) + } + Processed::RetryStale => { + tracing::info!( + id=%task_id, %time_queued, + "control-plane snapshot is stale; rescheduling discover after refresh" + ); + state.get_or_insert_default().awaiting_snapshot_after = Some(queued_at); + Ok(DiscoverOutcome::RetryStale) + } + } } } @@ -145,7 +198,8 @@ impl DiscoverExecutor { &self, row: Row, pool: &sqlx::PgPool, - ) -> anyhow::Result<(JobStatus, ProcessResult)> { + snapshot: &Snapshot, + ) -> anyhow::Result { tracing::info!( %row.capture_name, %row.connector_tag_id, @@ -170,38 +224,42 @@ impl DiscoverExecutor { } else if !connector_tags::does_connector_exist(&row.image_name, pool).await? { return Ok(precheck_failed(JobStatus::ImageForbidden)); } - let maybe_data_plane = sqlx::query_as!( - tables::DataPlane, - r#" - SELECT - d.id AS "control_id: Id", - d.data_plane_name, - d.closed, - d.hmac_keys, - d.encrypted_hmac_keys AS "encrypted_hmac_keys: models::RawValue", - d.data_plane_fqdn, - d.broker_address, - d.reactor_address, - d.dekaf_address, - d.dekaf_registry_address, - d.ops_logs_name AS "ops_logs_name: models::Collection", - d.ops_stats_name AS "ops_stats_name: models::Collection" - FROM data_planes d - WHERE data_plane_name = $1 - AND EXISTS ( - SELECT 1 FROM internal.user_roles($2, 'read') r - WHERE starts_with($1, r.role_prefix) - ) - "#, - row.data_plane_name, - row.user_id, - ) - .fetch_optional(pool) - .await - .context("fetching data-plane")?; - let Some(data_plane) = maybe_data_plane else { - tracing::warn!(data_plane_name = ?row.data_plane_name, "data-plane not found or user may not be authorized"); + // Legacy `read` is what conveys deploy-level trust in a data plane; + // no narrow capability bit expresses that trust yet. + match snapshot.user_authorization( + row.user_id, + &row.data_plane_name, + models::Capability::Read, + Some(row.updated_at), + ) { + Authorization::Authorized => (), + Authorization::Denied => { + // The snapshot reflects the world after this discover was + // queued, so the denial is authoritative. + tracing::warn!(data_plane_name = ?row.data_plane_name, "user is not authorized to read data plane"); + return Ok(precheck_failed(JobStatus::NotAuthorized)); + } + Authorization::Stale => { + // The snapshot predates this discover's row, so a grant that + // would authorize the read may not be reflected yet. Request an + // early refresh and retry, rather than emitting a spurious + // NotAuthorized/NoDataPlane. + tracing::warn!(data_plane_name = ?row.data_plane_name, "data-plane read denied under a stale snapshot"); + snapshot.revoke.cancel(); + return Ok(Processed::RetryStale); + } + } + let data_plane = snapshot.data_plane_by_catalog_name(&row.data_plane_name); + + let Some(data_plane) = data_plane else { + tracing::warn!(data_plane_name = ?row.data_plane_name, "data-plane not found in control-plane snapshot"); + if !snapshot.taken_after(row.updated_at) { + // A plane registered after this Snapshot may already exist. + // Request an early refresh before making the absence terminal. + snapshot.revoke.cancel(); + return Ok(Processed::RetryStale); + } return Ok(precheck_failed(JobStatus::NoDataPlane)); }; @@ -214,8 +272,10 @@ impl DiscoverExecutor { row.update_only, row.logs_token, image_composed, - data_plane, + data_plane.clone(), pool, + &snapshot, + Some(row.updated_at), ) .await; @@ -225,7 +285,7 @@ impl DiscoverExecutor { }; match result { - Ok(output) if output.is_success() => Ok(( + Ok(output) if output.is_success() => Ok(Processed::Resolved( JobStatus::Success { publication_id: None, specs_unchanged: false, @@ -239,7 +299,17 @@ impl DiscoverExecutor { .iter() .map(tables::Error::to_draft_error) .collect::>(); - Ok((JobStatus::DiscoverFailed, Err(draft_errs))) + Ok(Processed::Resolved( + JobStatus::DiscoverFailed, + Err(draft_errs), + )) + } + Err(err) if validation::is_authz_snapshot_stale(&err) => { + // An authorization denial was evaluated against a Snapshot that + // isn't authoritative for this discover. Request an early refresh + // and retry, rather than reporting a spurious DiscoverFailed. + snapshot.revoke.cancel(); + Ok(Processed::RetryStale) } Err(err) => { let draft_errors = vec![models::draft_error::Error { @@ -250,7 +320,10 @@ impl DiscoverExecutor { catalog_name: row.capture_name.clone(), detail: format!("{:#}", err), }]; - Ok((JobStatus::DiscoverFailed, Err(draft_errors))) + Ok(Processed::Resolved( + JobStatus::DiscoverFailed, + Err(draft_errors), + )) } } } @@ -263,7 +336,7 @@ impl DiscoverExecutor { /// row, even if it differs from the endpoint on the drafted or live spec. All /// other specs in the given draft will be loaded as they are and used as the /// base for the merge after the discover completes. -async fn prepare_discover( +async fn prepare_discover<'a>( user_id: uuid::Uuid, draft_id: Id, capture_name: models::Capture, @@ -272,8 +345,10 @@ async fn prepare_discover( logs_token: uuid::Uuid, image_composed: String, data_plane: tables::DataPlane, - pool: &sqlx::PgPool, -) -> anyhow::Result { + pool: &'a sqlx::PgPool, + snapshot: &'a Snapshot, + started_at: Option, +) -> anyhow::Result> { let mut draft = draft::load_draft(draft_id, pool) .await .context("loading draft")?; @@ -287,11 +362,21 @@ async fn prepare_discover( // embedded in its control-plane Id — is carried on the Discover request, // so that re-discovers resolve connector feature-flag defaults as the // running task does. It's empty for a task which doesn't exist yet. - // Filter to only specs that the user can read. If they can't admin, then + // Filter to only specs that the user can view. If they can't edit, then // wait until they try to publish to surface that error. + // Use request-relative staleness: a denial from a Snapshot older than the + // queued discover is provisional, because a grant committed before queuing + // may be missing from it. let name = &[capture_name.to_string()]; - let live = - live_specs::get_live_specs(user_id, name, Some(models::Capability::Read), pool).await?; + let live = live_specs::get_live_specs( + user_id, + name, + Some(models::authz::Capability::CatalogRead.into()), + pool, + &snapshot, + started_at, + ) + .await?; let live_capture = live.captures.into_iter().next(); let created_at = live_capture .as_ref() @@ -362,6 +447,8 @@ async fn prepare_discover( reset_on_key_change, logs_token, created_at, + snapshot, + started_at, }) } @@ -442,7 +529,10 @@ mod test { dekaf_address: None, dekaf_registry_address: None, }; - + harness.refresh_snapshot().await; + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); + let started_at = tokens::now(); let result = super::prepare_discover( user_id, draft_id, @@ -453,6 +543,8 @@ mod test { image_composed.clone(), data_plane.clone(), &harness.pool, + &snapshot, + Some(started_at), ) .await .unwrap(); diff --git a/crates/agent/src/integration_tests/harness.rs b/crates/agent/src/integration_tests/harness.rs index a78cc34140f..7343ec1c819 100644 --- a/crates/agent/src/integration_tests/harness.rs +++ b/crates/agent/src/integration_tests/harness.rs @@ -129,7 +129,7 @@ pub struct UserDiscoverResult { } impl UserDiscoverResult { - async fn load(discover_id: Id, db: &sqlx::PgPool) -> UserDiscoverResult { + pub async fn load(discover_id: Id, db: &sqlx::PgPool) -> UserDiscoverResult { let discover = sqlx::query!( r#"select draft_id as "draft_id: Id", @@ -167,6 +167,24 @@ async fn load_draft_errors(draft_id: Id, db: &sqlx::PgPool) -> Vec<(String, Stri .collect::>() } +/// Owned counterpart of `TestHarness::refresh_snapshot_authoritative`, handed +/// out by `TestHarness::snapshot_refresher` to `'static` test fixtures which +/// cannot borrow the harness. +#[derive(Clone)] +pub struct SnapshotRefresher { + pool: sqlx::PgPool, + set_snapshot: Arc, +} + +impl SnapshotRefresher { + /// See `TestHarness::refresh_snapshot_authoritative`. + pub async fn refresh_authoritative(&self) { + let taken = tokens::now() + TestHarness::snapshot_settle(); + let snapshot = TestHarness::fetch_snapshot_at(&self.pool, taken).await; + (self.set_snapshot)(snapshot); + } +} + /// Facilitates writing integration tests. /// **Note:** integration tests require exclusive access to the database, /// so it's required to serialize test runs (see .config/nextest.toml). @@ -177,6 +195,11 @@ pub struct TestHarness { pub test_name: String, pub pool: sqlx::PgPool, pub publisher: Publisher, + /// Live authorization Snapshot watch. See the Snapshot testing model + /// documented above `fetch_snapshot`. + pub snapshot_watch: Arc>, + /// Manual write handle backing `snapshot_watch`; same reference. + set_snapshot: Arc, #[allow(dead_code)] // only here so we don't drop it until the harness is dropped pub builds_root: tempfile::TempDir, pub discover_handler: DiscoverHandler, @@ -241,6 +264,16 @@ impl HarnessBuilder { eprintln!("end of PUB-LOG"); }); + // Back the authorization Snapshot with a manually-driven watch (see + // the Snapshot testing model documented above `fetch_snapshot`). + let (snapshot_pending, snapshot_replace) = tokens::manual::(); + let set_snapshot: Arc = + Arc::new(move |snapshot| { + _ = snapshot_replace(Ok(snapshot)); + }); + set_snapshot(TestHarness::fetch_snapshot(&pool).await); + let snapshot_watch = snapshot_pending.ready_owned().await; + let mock_connectors = connectors::MockDiscoverConnectors::default(); let discover_handler = DiscoverHandler::new(mock_connectors.clone()); @@ -256,16 +289,13 @@ impl HarnessBuilder { ) .with_skip_all_tests(); - let snapshot_source = control_plane_api::snapshot::PgSnapshotSource::new(pool.clone()); - let snapshot_watch = tokens::watch(snapshot_source).ready_owned().await; - let control_plane = TestControlPlane::new(PGControlPlane::new( pool.clone(), system_user_id, publisher.clone(), discover_handler.clone(), logs_tx.clone(), - snapshot_watch, + snapshot_watch.clone(), 1.0, // auto_discover_probability publication_cooldown, crate::controllers::ControllerConfig::default(), @@ -280,6 +310,8 @@ impl HarnessBuilder { test_name, pool, publisher, + snapshot_watch, + set_snapshot, builds_root, discover_handler, control_plane, @@ -293,6 +325,9 @@ impl HarnessBuilder { }; harness.truncate_tables().await; harness.setup_test_connectors().await; + // The Snapshot was taken before `truncate_tables` cleared grants; re-fetch + // so authorization sees the truncated baseline rather than stale grants. + harness.refresh_snapshot().await; harness } @@ -484,6 +519,14 @@ impl TestHarness { del_tenants as ( delete from tenants ), + -- Storage mappings must be cleared too: `provision_tenant` (and the + -- beta-onboard directive) insert a tenant's mapping with `on conflict + -- do nothing`, so a mapping left over from an earlier run — including + -- one whose `data_planes` captured a developer's live local stack — + -- would silently survive and be read by the next test. + del_storage_mappings as ( + delete from storage_mappings + ), del_user_grants as ( -- preserve the system user's role grants delete from user_grants where user_id != $1 @@ -523,6 +566,19 @@ impl TestHarness { ), del_daily_stats as ( delete from catalog_stats_daily + ), + -- Clear data-planes too, so every test starts from a deterministic + -- baseline regardless of any data-planes a developer's live local + -- stack has registered in this shared database (e.g. a running + -- `mise run local:stack` registers `ops/dp/public/-cluster`). + -- `setup_test_connectors` re-inserts the single `ops/dp/public/test` + -- plane the tests expect. `data_plane_private_links` is deleted first + -- to satisfy its foreign key onto `data_planes`. + del_data_plane_private_links as ( + delete from internal.data_plane_private_links + ), + del_data_planes as ( + delete from data_planes ) delete from catalog_stats_monthly;"#, system_user_id @@ -547,6 +603,92 @@ impl TestHarness { &mut self.control_plane } + // The harness's Snapshot testing model: + // + // Production refreshes the authorization Snapshot through + // `PgSnapshotSource`'s timer-gated polling loop. The harness backs the + // same watch with a manual writer (`set_snapshot`) instead: + // + // - Refreshes are explicit. Nothing refreshes on a timer, and + // `MIN_REFRESH_INTERVAL` never gates a test. Grant-mutating helpers + // either refresh the watch (`add_role_grant`) or deliberately leave it + // holding the pre-grant world (`add_role_grant_unobserved`). + // + // - Observed *state* and the authoritative *timestamp* are controlled + // separately. A refresh always fetches current Postgres state, but + // stamps it with a caller-chosen `taken`. Staleness compares `taken` + // against an operation's freshness anchor (`Snapshot::taken_after`, + // allowing `TEMPORAL_SKEW`), and tests compress wall-clock time into + // milliseconds — a `taken = now()` Snapshot still reads as stale for a + // row written moments earlier. `refresh_snapshot_authoritative` / + // `refresh_snapshot_stale` push `taken` clear of the skew in either + // direction. + // + // Individual helpers below document only how they differ. + + // Current Postgres state, stamped `taken = now()`. + async fn fetch_snapshot(pool: &sqlx::PgPool) -> control_plane_api::Snapshot { + Self::fetch_snapshot_at(pool, tokens::now()).await + } + + // Current Postgres state with a caller-chosen `taken` — the same query + // `PgSnapshotSource` runs, minus its cool-off. + async fn fetch_snapshot_at( + pool: &sqlx::PgPool, + taken: tokens::DateTime, + ) -> control_plane_api::Snapshot { + let mut decrypted_hmac_keys = std::collections::HashMap::new(); + let data = control_plane_api::snapshot::try_fetch(pool, &mut decrypted_hmac_keys) + .await + .expect("failed to fetch authorization snapshot"); + control_plane_api::Snapshot::new(taken, data) + } + + /// Re-fetches the Snapshot at `taken = now()`, making grant changes + /// written directly to Postgres visible. + pub async fn refresh_snapshot(&self) { + self.refresh_snapshot_at(tokens::now()).await + } + + /// Refreshes with an exact `taken`. Prefer `refresh_snapshot_authoritative` + /// / `refresh_snapshot_stale` unless a test needs a precise instant. + pub async fn refresh_snapshot_at(&self, taken: tokens::DateTime) { + let snapshot = Self::fetch_snapshot_at(&self.pool, taken).await; + (self.set_snapshot)(snapshot); + } + + /// An owned handle which performs `refresh_snapshot_authoritative`, for + /// injecting a refresh from within `'static` test fixtures — such as a + /// `DiscoverConnectors` impl — which cannot borrow the harness. + pub fn snapshot_refresher(&self) -> SnapshotRefresher { + SnapshotRefresher { + pool: self.pool.clone(), + set_snapshot: self.set_snapshot.clone(), + } + } + + /// Refreshes with `taken` pushed far enough forward to be authoritative + /// for everything written up to now: any denial it produces is definitive + /// rather than retryable. + pub async fn refresh_snapshot_authoritative(&self) { + self.refresh_snapshot_at(tokens::now() + Self::snapshot_settle()) + .await + } + + /// The inverse: current grant state stamped in the past, so any denial it + /// produces reads as provisional and retries. Models production's window + /// where a write has landed in Postgres but the Snapshot predates it. + pub async fn refresh_snapshot_stale(&self) { + self.refresh_snapshot_at(tokens::now() - Self::snapshot_settle()) + .await + } + + // Margin pushing `taken` clear of `TEMPORAL_SKEW` in either direction. + // Any multiple > 1 works; 4 leaves obvious headroom. + fn snapshot_settle() -> chrono::TimeDelta { + control_plane_api::Snapshot::TEMPORAL_SKEW * 4 + } + /// Setup a new tenant with the given name, and return the id of the user /// who has `admin` capabilities to it. Performs essentially the same setup /// as the beta onboarding directive, so the user_grants, role_grants, @@ -559,13 +701,32 @@ impl TestHarness { "full_name": format!("Full ({tenant}) Name"), }); - control_plane_api::directives::beta_onboard::provision_test_tenant( + let user_id = control_plane_api::directives::beta_onboard::provision_test_tenant( &self.pool, tenant, &email, meta, ) - .await + .await; + // Grants were just written; re-sync the authorization Snapshot. + self.refresh_snapshot().await; + user_id } pub async fn add_role_grant(&mut self, subject: &str, object: &str, capability: Capability) { + self.add_role_grant_unobserved(subject, object, capability) + .await; + // Re-sync the authorization Snapshot with the new grant. + self.refresh_snapshot().await; + } + + /// Writes a role grant to Postgres *without* re-syncing the authorization + /// Snapshot, modelling production's window between a grant landing in the + /// database and the next Snapshot refresh observing it. Authorization run + /// during that window sees the pre-grant world. + pub async fn add_role_grant_unobserved( + &mut self, + subject: &str, + object: &str, + capability: Capability, + ) { sqlx::query!( r#" insert into role_grants (subject_role, object_role, capability) @@ -581,6 +742,19 @@ impl TestHarness { } pub async fn add_user_grant(&mut self, user_id: Uuid, role: &str, capability: Capability) { + self.add_user_grant_unobserved(user_id, role, capability) + .await; + // Re-sync the authorization Snapshot with the new grant. + self.refresh_snapshot().await; + } + + /// The `add_role_grant_unobserved` counterpart for user grants. + pub async fn add_user_grant_unobserved( + &mut self, + user_id: Uuid, + role: &str, + capability: Capability, + ) { let mut txn = self.pool.begin().await.unwrap(); control_plane_api::grants::upsert_user_grant( user_id, @@ -594,21 +768,37 @@ impl TestHarness { txn.commit().await.unwrap(); } + /// Rewrites `catalog_name`'s `last_pub_id` so the spec reads as published + /// long before any event in the current test. Compressed test time means + /// everything is otherwise "just published", which sidesteps the common + /// production shape of an old spec whose *authorization* changes now. The + /// id sits a few days past the Estuary epoch — old, but non-zero, because + /// a zero id means "never published". + pub async fn age_live_spec(&self, catalog_name: &str) { + let updated = sqlx::query( + "update live_specs set last_pub_id = '00:08:00:00:00:00:00:00'::flowid + where catalog_name = $1", + ) + .bind(catalog_name) + .execute(&self.pool) + .await + .unwrap(); + assert_eq!( + 1, + updated.rows_affected(), + "expected to age exactly one live spec named {catalog_name}" + ); + } + pub async fn assert_specs_touched_since(&mut self, prev_specs: &tables::LiveCatalog) { - let user_id = self.control_plane().inner.system_user_id; let owned_names: Vec = prev_specs .all_spec_names() .map(|n| (*n).to_owned()) .collect(); - let specs = control_plane_api::live_specs::fetch_live_specs( - user_id, - &owned_names, - false, /* don't fetch user capabilities */ - false, /* don't fetch spec capabilities */ - &self.pool, - ) - .await - .expect("failed to query live specs"); + + let specs = control_plane_api::live_specs::fetch_live_specs(&owned_names, &self.pool) + .await + .expect("failed to query live specs"); assert_eq!( prev_specs.spec_count(), specs.len(), @@ -1130,12 +1320,14 @@ impl TestHarness { task_types::PUBLICATIONS => Server::new().register(PublicationsExecutor { publisher: self.publisher.clone(), pg_pool: self.pool.clone(), + snapshot_watch: self.snapshot_watch.clone(), runtime_v2_new_captures: self.runtime_v2_new_captures, runtime_v2_new_materializations: self.runtime_v2_new_materializations, runtime_v2_new_derivations: self.runtime_v2_new_derivations, }), task_types::DISCOVERS => Server::new().register(DiscoverExecutor { handler: self.discover_handler.clone(), + snapshot_watch: self.snapshot_watch.clone(), }), task_types::APPLIED_DIRECTIVES => Server::new().register(self.directive_exec.clone()), task_types::TENANT_ALERT_EVALS => Server::new().register( @@ -1241,6 +1433,110 @@ impl TestHarness { UserDiscoverResult::load(disco_id, &self.pool).await } + /// Registers an additional data-plane beyond the `ops/dp/public/test` one + /// that `setup_test_connectors` creates. Tests use this when they need a + /// plane that *exists* but that a given user has no grant to read, which is + /// the only way to distinguish an authorization denial from a missing plane. + pub async fn add_data_plane(&self, data_plane_name: &str) { + sqlx::query!( + r##"insert into data_planes ( + data_plane_name, + data_plane_fqdn, + ops_logs_name, + ops_stats_name, + ops_l1_inferred_name, + ops_l1_stats_name, + ops_l1_events_name, + ops_l2_inferred_transform, + ops_l2_stats_transform, + ops_l2_events_transform, + broker_address, + reactor_address, + hmac_keys, + enable_l2 + ) values ( + $1, + $2, + 'ops/logs', + 'ops/stats', + 'ops/L1/inferred', + 'ops/L1/stats', + 'ops/L1/events', + 'from-L1-inferred', + 'from-L1-stats', + 'from-L1-events', + 'broker:address', + 'reactor:address', + '{secret-key}', + false + );"##, + data_plane_name as &str, + format!("{}.dp.estuary-data.com", data_plane_name.replace('/', "-")) as String, + ) + .execute(&self.pool) + .await + .expect("failed to insert data-plane"); + } + + /// Inserts a queued `discovers` row for a caller-chosen `data_plane_name` + /// and returns its id, without running it. Unlike `user_discover`, the + /// data-plane is a parameter so tests can drive authorization outcomes + /// (which are evaluated against `data_plane_name`). + pub async fn queue_discover( + &self, + image_name: &str, + image_tag: &str, + capture_name: &str, + draft_id: Id, + data_plane_name: &str, + ) -> Id { + let connector_tag = sqlx::query!( + r##"select ct.id as "id: Id" + from connectors c + join connector_tags ct on c.id = ct.connector_id + where c.image_name = $1 and ct.image_tag = $2;"##, + image_name, + image_tag + ) + .fetch_one(&self.pool) + .await + .expect("querying for connector_tags id"); + + let config_json = TextJson(models::RawValue::from_str("{}").unwrap()); + let disco = sqlx::query!( + r##"insert into discovers ( + capture_name, + connector_tag_id, + draft_id, + endpoint_config, + update_only, + data_plane_name + ) values ($1, $2, $3, $4, false, $5) + returning id as "id: Id";"##, + capture_name as &str, + connector_tag.id as Id, + draft_id as Id, + config_json as TextJson, + data_plane_name as &str, + ) + .fetch_one(&self.pool) + .await + .unwrap(); + disco.id + } + + pub async fn discover_job_status(&self, discover_id: Id) -> crate::discovers::JobStatus { + let row = sqlx::query!( + r#"select job_status as "job_status: TextJson" + from discovers where id = $1;"#, + discover_id as Id, + ) + .fetch_one(&self.pool) + .await + .expect("failed to query discover"); + row.job_status.0 + } + pub async fn fail_shard(&mut self, shard: &ShardRef) { let fields = serde_json::from_value(serde_json::json!({ "eventType": "shardFailure", @@ -1295,16 +1591,16 @@ impl TestHarness { .await } - /// Runs a publication by inserting into the `publications` table and - /// waiting for the publications handler to process it. Returns - /// a `ScenarioResult` (a hold over from the old publications tests, which - /// were ported over) describing the results of the publication. - async fn async_publication( + /// Inserts a queued `publications` row (creating the draft if one wasn't + /// supplied) and returns its id, *without* running it. `async_publication` + /// runs the task to completion; tests that need to control what the + /// authorization Snapshot looks like between polls drive it themselves. + pub async fn queue_publication( &mut self, user_id: Uuid, detail: impl Into, draft: Either, - ) -> ScenarioResult { + ) -> Id { let detail = detail.into(); let draft_id = match draft { Either::L(catalog) => self.create_draft(user_id, detail.clone(), catalog).await, @@ -1319,24 +1615,75 @@ impl TestHarness { &mut txn, user_id, draft_id, - detail.clone(), + detail, "ops/dp/public/test".to_string(), ) .await .expect("failed to create publication"); txn.commit().await.expect("failed to commit transaction"); + pub_id + } + /// Runs exactly one poll of the publications task `pub_id` and returns the + /// resulting `ScenarioResult`. A result whose status is still `Queued` means + /// the executor rescheduled rather than resolving — today that happens only + /// for a stale authorization Snapshot. + pub async fn poll_publication_once(&mut self, pub_id: Id) -> ScenarioResult { let task_id = self .run_automation_task(automations::task_types::PUBLICATIONS) .await .expect("expected a publication task to have run"); - assert_eq!( - task_id, pub_id, - "automations task id should match the publication that was just created" - ); + assert_eq!(task_id, pub_id, "an unexpected publication task ran"); + self.get_publication_result(pub_id.into()).await + } + + /// Runs a publication by inserting into the `publications` table and + /// waiting for the publications handler to process it. Returns a + /// `ScenarioResult` (a hold over from the old publications tests, which + /// were ported over) describing the results of the publication. + async fn async_publication( + &mut self, + user_id: Uuid, + detail: impl Into, + draft: Either, + ) -> ScenarioResult { + let detail = detail.into(); + let pub_id = self.queue_publication(user_id, detail, draft).await; + + // A stale-Snapshot publication reschedules (Action::Sleep) rather + // than resolving. Mimic production's re-poll-after-refresh loop, + // bounded so a genuine failure to converge still surfaces. + let mut attempts = 0; + let pub_result = loop { + let task_id = self + .run_automation_task(automations::task_types::PUBLICATIONS) + .await + .expect("expected a publication task to have run"); + assert_eq!( + task_id, pub_id, + "automations task id should match the publication that was just created" + ); + + let pub_result = self.get_publication_result(pub_id.into()).await; + if pub_result.status.r#type != publications::StatusType::Queued { + break pub_result; + } - let pub_result = self.get_publication_result(pub_id.into()).await; - assert_ne!(publications::StatusType::Queued, pub_result.status.r#type); + attempts += 1; + assert!( + attempts < 5, + "publication kept rescheduling on a stale authorization snapshot" + ); + // Compressed test time never advances past the skew on its own, + // so model production's elapsed wait explicitly (see the Snapshot + // testing model above `fetch_snapshot`). + self.refresh_snapshot_authoritative().await; + self.set_min_task_wake_at(pub_id).await; + }; + assert!( + attempts == 0 || !pub_result.status.is_success(), + "an authorized publication resolved only after {attempts} deferral(s)" + ); pub_result } @@ -1589,7 +1936,7 @@ impl TestHarness { maybe_claims: control_plane_api::MaybeControlClaims::with_verified(verified), original_uri: axum::http::Uri::from_static("/graphql"), pg_pool: self.pool.clone(), - refresh: app.snapshot.token(), + refresh: app.snapshot_watch.token(), retry_after: tokens::DateTime::UNIX_EPOCH, started: tokens::now(), locale: control_plane_api::Locale::EnUS, @@ -2139,6 +2486,7 @@ impl ControlPlane for TestControlPlane { let mocks = self.mocks.lock().unwrap(); mocks.build_failures.clone() }; + let snapshot = self.inner.snapshot_watch.token(); let publication = DraftPublication { user_id: self.inner.system_user_id, detail, @@ -2146,6 +2494,11 @@ impl ControlPlane for TestControlPlane { logs_token, dry_run: false, default_data_plane_name: data_plane_name, + // Mirrors the production controller path, which has no queued row. + started_at: None, + snapshot: snapshot + .result() + .expect("authorization snapshot is not ready"), verify_user_authz: false, initialize: NoopInitialize, finalize, @@ -2224,7 +2577,7 @@ impl ControlPlane for TestControlPlane { } } -enum Either { +pub enum Either { L(L), R(R), } diff --git a/crates/agent/src/integration_tests/locking_retries.rs b/crates/agent/src/integration_tests/locking_retries.rs index af66b7b4749..b28d18b5553 100644 --- a/crates/agent/src/integration_tests/locking_retries.rs +++ b/crates/agent/src/integration_tests/locking_retries.rs @@ -23,7 +23,8 @@ async fn test_publication_concurrent_commits() { "beavers/dams": minimal_capture(None, &["beavers/dens"]), } })); - + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); // Try to reproduce a scenario where multiple different publications all try to commit // concurrently. We'll expect exactly one of them to succeed, and the others to fail. for test_iteration in 0..5 { @@ -41,6 +42,8 @@ async fn test_publication_concurrent_commits() { None, true, 0, + &snapshot, + None, ) .await .unwrap(); @@ -55,6 +58,8 @@ async fn test_publication_concurrent_commits() { None, true, 0, + &snapshot, + None, ) .await .unwrap(); @@ -68,6 +73,8 @@ async fn test_publication_concurrent_commits() { None, true, 0, + &snapshot, + None, ) .await .unwrap(); @@ -110,6 +117,8 @@ async fn test_publication_optimistic_locking_failures() { "mice/also-new": minimal_capture(Some(Id::new([8, 7, 6, 5, 4, 3, 2, 1])), &["mice/does-not-exist"]), } })); + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let naughty_pub_id = Id::new([8; 8]); // If a user explicitly sets `expectPubId` in the model, then a mismatch gets returned as a // build error, before we even try to commit. @@ -124,6 +133,8 @@ async fn test_publication_optimistic_locking_failures() { None, true, 0, + &snapshot, + None, ) .await .expect("build failed"); @@ -157,7 +168,8 @@ async fn test_publication_optimistic_locking_failures() { "mice/capture": minimal_capture(None, &["mice/cheese", "mice/seeds"]), } }); - + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let will_fail_pub = Id::new([9; 8]); let will_fail_build = harness .publisher @@ -170,10 +182,13 @@ async fn test_publication_optimistic_locking_failures() { None, true, 0, + &snapshot, + None, ) .await .expect("build a failed"); - + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let will_commit_pub = Id::new([10; 8]); let will_commit_build = harness .publisher @@ -186,6 +201,8 @@ async fn test_publication_optimistic_locking_failures() { None, true, 0, + snapshot, + None, ) .await .expect("build b failed"); @@ -228,6 +245,8 @@ async fn test_publication_optimistic_locking_failures() { "mice/capture": minimal_capture(None, &["mice/cheese", "mice/seeds"]), } })); + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let will_fail_build = harness .publisher .build( @@ -239,6 +258,8 @@ async fn test_publication_optimistic_locking_failures() { None, true, 0, + snapshot, + None, ) .await .expect("cheese build failed"); @@ -253,6 +274,8 @@ async fn test_publication_optimistic_locking_failures() { "mice/capture": minimal_capture(None, &["mice/cheese", "mice/seeds"]), } })); + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let will_commit_build = harness .publisher .build( @@ -264,6 +287,8 @@ async fn test_publication_optimistic_locking_failures() { None, true, 0, + snapshot, + None, ) .await .expect("seeds build failed"); @@ -402,6 +427,8 @@ async fn test_injected_ops_collections_are_not_locked() { "owls/capture": minimal_capture(None, &["owls/hoots"]), } })); + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let build = harness .publisher .build( @@ -413,6 +440,8 @@ async fn test_injected_ops_collections_are_not_locked() { None, true, 0, + snapshot, + None, ) .await .expect("owls build failed"); @@ -498,6 +527,8 @@ async fn test_injected_ops_collections_are_not_locked() { } } })); + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let reader_build = harness .publisher .build( @@ -509,6 +540,8 @@ async fn test_injected_ops_collections_are_not_locked() { None, true, 0, + snapshot, + None, ) .await .expect("reader build failed"); diff --git a/crates/agent/src/integration_tests/source_captures.rs b/crates/agent/src/integration_tests/source_captures.rs index fdd47013444..0e9778eb7ce 100644 --- a/crates/agent/src/integration_tests/source_captures.rs +++ b/crates/agent/src/integration_tests/source_captures.rs @@ -420,10 +420,24 @@ async fn test_source_capture_no_annotations() { } } })); + + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let pub_id = Id::new([0, 0, 0, 0, 0, 0, 0, 9]); let built = harness .publisher - .build(user_id, pub_id, None, draft, Uuid::new_v4(), None, false, 0) + .build( + user_id, + pub_id, + None, + draft, + Uuid::new_v4(), + None, + false, + 0, + snapshot, + None, + ) .await .expect("build failed"); assert!(built.has_errors()); diff --git a/crates/agent/src/integration_tests/unknown_connectors.rs b/crates/agent/src/integration_tests/unknown_connectors.rs index 0de6930a24b..8c167f42e21 100644 --- a/crates/agent/src/integration_tests/unknown_connectors.rs +++ b/crates/agent/src/integration_tests/unknown_connectors.rs @@ -41,10 +41,23 @@ async fn test_forbidden_connector() { } } })); + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); let pub_id = Id::new([0, 0, 0, 0, 0, 0, 0, 9]); let built = harness .publisher - .build(user_id, pub_id, None, draft, Uuid::new_v4(), None, true, 0) + .build( + user_id, + pub_id, + None, + draft, + Uuid::new_v4(), + None, + true, + 0, + snapshot, + None, + ) .await .expect("build failed"); assert!(built.has_errors()); diff --git a/crates/agent/src/integration_tests/user_discovers.rs b/crates/agent/src/integration_tests/user_discovers.rs index a6c8b9f9ccb..8a2559da004 100644 --- a/crates/agent/src/integration_tests/user_discovers.rs +++ b/crates/agent/src/integration_tests/user_discovers.rs @@ -1,9 +1,20 @@ use super::{spec_fixture, wrap_connector_schema}; use crate::{ ControlPlane, - integration_tests::harness::{TestHarness, UserDiscoverResult, draft_catalog, set_of}, + discovers::JobStatus, + integration_tests::harness::{ + SnapshotRefresher, TestHarness, UserDiscoverResult, connectors::MockDiscoverConnectors, + draft_catalog, set_of, + }, }; +use control_plane_api::{ + discovers::{Discover, DiscoverHandler}, + proxy_connectors::DiscoverConnectors, +}; +use models::Id; +use proto_flow::capture; use proto_flow::capture::response::{Discovered, discovered::Binding}; +use uuid::Uuid; #[tokio::test] async fn test_user_discovers() { @@ -333,6 +344,942 @@ async fn test_user_discovers() { } } +// A data-plane that exists but that no tenant in these tests is granted to +// read. Registering it (rather than using a bogus name) is what separates an +// authorization denial from `NoDataPlane`. +const FOREIGN_DATA_PLANE: &str = "dogs/dp/private/test"; + +/// Queues a discover for `capture_name` against `FOREIGN_DATA_PLANE`, which the +/// caller's tenant has no grant to read. +async fn queue_foreign_dp_discover(harness: &mut TestHarness, user_id: Uuid, name: &str) -> Id { + let draft_id = harness + .create_draft(user_id, name, Default::default()) + .await; + harness + .queue_discover("source/test", ":test", name, draft_id, FOREIGN_DATA_PLANE) + .await +} + +/// A discover whose data-plane authorization is denied by a Snapshot that +/// predates the discover row must be retried, not failed: a grant that would +/// authorize it may simply not be reflected in that Snapshot yet. The row stays +/// queued and the task reschedules. +#[tokio::test] +async fn test_discover_reschedules_on_stale_data_plane_authz() { + let mut harness = TestHarness::init("test_discover_reschedules_on_stale_data_plane").await; + let user_id = harness.setup_tenant("cats").await; + harness.add_data_plane(FOREIGN_DATA_PLANE).await; + + let disco_id = queue_foreign_dp_discover(&mut harness, user_id, "cats/capture-stale").await; + harness.refresh_snapshot_stale().await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran, "expected the stale discover to run"); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::Queued + ), + "stale-snapshot discover should stay queued (rescheduled), got: {:?}", + harness.discover_job_status(disco_id).await, + ); +} + +/// The converse: once the Snapshot is authoritative for the discover row, the +/// same denial is definitive and resolves terminally rather than looping. +/// This is the anchor's other boundary: changes committed after the queued +/// discover carry no observation guarantee, so an authoritative denial is +/// terminal regardless of what commits later. +#[tokio::test] +async fn test_discover_unauthorized_data_plane_is_terminal() { + let mut harness = TestHarness::init("test_discover_unauthorized_data_plane").await; + let user_id = harness.setup_tenant("cats").await; + harness.add_data_plane(FOREIGN_DATA_PLANE).await; + + let disco_id = queue_foreign_dp_discover(&mut harness, user_id, "cats/capture-authz").await; + harness.refresh_snapshot_authoritative().await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!( + Some(disco_id), + ran, + "expected the authoritative discover to run" + ); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::NotAuthorized + ), + "authoritative unauthorized discover should resolve NotAuthorized, got: {:?}", + harness.discover_job_status(disco_id).await, + ); +} + +/// After a stale-Snapshot denial, the discover executor persists the instant +/// an authoritative Snapshot must postdate (in `internal.tasks`, so whichever +/// agent instance dequeues the next poll applies the same criterion) and +/// defers re-polls without pre-flight checks or connector work. Once the +/// local Snapshot postdates that instant, the retry proceeds and succeeds. +#[tokio::test] +async fn test_discover_defers_polls_until_authoritative_snapshot() { + let mut harness = TestHarness::init("test_discover_defers_polls").await; + let user_id = harness.setup_tenant("cats").await; + harness.add_data_plane(FOREIGN_DATA_PLANE).await; + + harness.refresh_snapshot_stale().await; + harness + .add_role_grant_unobserved("cats/", "dogs/dp/private/", models::Capability::Read) + .await; + + let capture_name = "cats/capture-deferred"; + let disco_id = queue_foreign_dp_discover(&mut harness, user_id, capture_name).await; + harness.discover_handler.connectors.mock_discover( + capture_name, + Ok((spec_fixture(), single_binding_response("acorns"))), + ); + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::Queued + ), + "discover should reschedule while the grant is unobserved, got: {:?}", + harness.discover_job_status(disco_id).await, + ); + + let state: serde_json::Value = harness.get_task_state(disco_id).await; + assert!( + state + .get("awaiting_snapshot_after") + .is_some_and(|v| v.is_string()), + "the executor should record the instant a Snapshot must postdate, got: {state}" + ); + + // Because the anchor is persisted, the re-poll may be dequeued by a + // *different* agent instance whose own local Snapshot is stale — one whose + // revoke token the original attempt never cancelled. Model that handoff by + // replacing the watch with another stale Snapshot bearing a fresh token. + harness.refresh_snapshot_stale().await; + let handoff_revoke = harness + .snapshot_watch + .token() + .result() + .unwrap() + .revoke + .clone(); + assert!(!handoff_revoke.is_cancelled()); + + // A re-poll under the still-stale Snapshot defers, leaving the row queued. + harness.set_min_task_wake_at(disco_id).await; + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::Queued + ), + "a re-poll under a still-stale Snapshot should defer, got: {:?}", + harness.discover_job_status(disco_id).await, + ); + + // The deferring poll must request a refresh of the Snapshot it observed: + // no prior cancellation covers this instance's Snapshot, and without one + // the task would idle until the watch's ordinary refresh interval. + assert!( + handoff_revoke.is_cancelled(), + "a deferring poll should cancel the stale Snapshot it observed" + ); + + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(disco_id).await; + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + let status = harness.discover_job_status(disco_id).await; + assert!( + matches!(status, JobStatus::Success { .. }), + "discover should succeed once the Snapshot postdates the anchor, got: {status:?}", + ); +} + +/// The second, independent stale path through `DiscoverExecutor::process`: the +/// data-plane check passes, but `prepare_discover`'s `get_live_specs` denies the +/// discover's own capture against a Snapshot older than that capture. The error +/// arrives as `AuthorizationSnapshotStale` from `control-plane-api` and must be +/// mapped to a reschedule rather than a `DiscoverFailed` status. +#[tokio::test] +async fn test_discover_reschedules_on_stale_live_spec_authz() { + let mut harness = TestHarness::init("test_discover_stale_live_spec_authz").await; + let cats_user = harness.setup_tenant("cats").await; + let dogs_user = harness.setup_tenant("dogs").await; + + // Publish a capture owned by `cats`. + let capture_name = "cats/capture-owned"; + let pub_result = harness + .user_publication( + cats_user, + "publish cats capture", + draft_catalog(serde_json::json!({ + "captures": { capture_name: minimal_capture() }, + })), + ) + .await; + assert!( + pub_result.status.is_success(), + "setup publication failed: {:?}", + pub_result.errors + ); + + // `dogs` may read the shared data-plane (every tenant is granted + // `ops/dp/public/`), so the data-plane precheck passes and we reach the + // live-spec authorization inside `prepare_discover`. `dogs` has no grant to + // `cats/`, and the Snapshot predates the capture's publication. + let draft_id = harness + .create_draft(dogs_user, "cross-tenant discover", Default::default()) + .await; + let disco_id = harness + .queue_discover( + "source/test", + ":test", + capture_name, + draft_id, + "ops/dp/public/test", + ) + .await; + harness.refresh_snapshot_stale().await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::Queued + ), + "stale live-spec authorization should reschedule, got: {:?}", + harness.discover_job_status(disco_id).await, + ); + + // With an authoritative Snapshot the denial stops being retryable and the + // discover reaches a terminal status instead of looping forever. + harness.refresh_snapshot_authoritative().await; + harness.discover_handler.connectors.mock_discover( + capture_name, + Ok((spec_fixture(), single_binding_response("kibble"))), + ); + harness.set_min_task_wake_at(disco_id).await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + // Once authoritative, the denial stops being retryable. `get_live_specs` + // falls back to its pre-existing behavior of silently omitting the + // unreadable spec, so the discover proceeds as if the capture were new + // rather than looping. Documenting the concrete status (not merely + // "terminal") keeps that silent drop visible. + let status = harness.discover_job_status(disco_id).await; + assert!( + matches!(status, JobStatus::Success { .. }), + "discover should reach a terminal status once the Snapshot is authoritative, got: {status:?}", + ); +} + +/// A collection which a capture binding targets across tenant lines. Its owner +/// (`dogs`) publishes it with a user-set projection; the discovering user +/// (`cats`) needs a read grant to it for the merge to see it. +const SHARED_COLLECTION: &str = "dogs/shared/data"; + +/// Publishes `SHARED_COLLECTION` under `dogs`, ages it so its publication time +/// cannot mask request-relative staleness, and drafts a `cats` capture whose +/// binding targets it. Returns the draft id for `queue_discover`. +async fn setup_shared_collection_discover( + harness: &mut TestHarness, + cats_user: Uuid, + dogs_user: Uuid, + capture_name: &str, +) -> Id { + // The writer capture isn't incidental: a draft holding only a collection + // with no writer builds to zero specs and is reported as an empty draft. + let pub_result = harness + .user_publication( + dogs_user, + "publish shared collection", + draft_catalog(serde_json::json!({ + "collections": { + SHARED_COLLECTION: { + // Wrapped as a connector-managed schema: the merge + // refuses to update collections whose schemas + // auto-discover doesn't manage. + "schema": wrap_connector_schema(serde_json::json!({ + "type": "object", + "properties": { "id": { "type": "string" } }, + "required": ["id"] + })), + "key": ["/id"], + "projections": { "id_projection": "/id" } + } + }, + "captures": { + "dogs/shared/writer": { + "endpoint": { + "connector": { "image": "source/test:test", "config": {} } + }, + "bindings": [ + { "resource": { "id": "data" }, "target": SHARED_COLLECTION } + ] + } + }, + })), + ) + .await; + assert!( + pub_result.status.is_success(), + "setup publication failed: {:?} {:?}", + pub_result.status, + pub_result.errors + ); + harness.age_live_spec(SHARED_COLLECTION).await; + + harness + .create_draft( + cats_user, + "shared collection discover", + draft_catalog(serde_json::json!({ + "captures": { + capture_name: { + "endpoint": { + "connector": { "image": "source/test:test", "config": {} } + }, + "bindings": [ + { "resource": { "id": "data" }, "target": SHARED_COLLECTION } + ], + } + }, + })), + ) + .await +} + +/// Asserts the drafted `SHARED_COLLECTION` was merged *from the live +/// collection*: it expects the live, nonzero publication id and keeps the +/// owner's projection. A collection drafted from scratch — what a silently +/// dropped authorization produces — has `expect_pub_id: zero` and no +/// projections, so each assertion is discriminating on its own. +fn assert_live_collection_preserved(draft: &tables::DraftCatalog) { + let drafted = draft + .collections + .get_by_key(&models::Collection::new(SHARED_COLLECTION)) + .expect("the target collection should be drafted"); + assert!( + drafted.expect_pub_id.is_some_and(|id| !id.is_zero()), + "the drafted collection should expect the live publication id, got: {:?}", + drafted.expect_pub_id, + ); + let model = drafted.model.as_ref().expect("drafted collection model"); + assert!( + model + .projections + .contains_key(&models::Field::new("id_projection")), + "the live collection's projection should be preserved, got: {:?}", + model.projections, + ); +} + +/// The merge phase fetches the capture's target collections with the user's +/// `CatalogRead` capability, and its staleness anchor must be the discover request — +/// not the target collection's own age. This is the late-observation race for +/// a *collection*: the grant to `dogs/shared/` commits before the discover is +/// queued, but the Snapshot predates both. Judged spec-relatively the (aged) +/// collection makes the denial authoritative, and it is silently dropped: the +/// discover "succeeds", re-drafting the collection from scratch with a zeroed +/// publication id. Judged request-relatively the denial is provisional, the +/// discover reschedules, and a refreshed Snapshot — guaranteed to include the +/// pre-queue grant — preserves the live collection. +#[tokio::test] +async fn test_discover_reschedules_on_stale_collection_authz() { + let mut harness = TestHarness::init("test_discover_stale_collection_authz").await; + let cats_user = harness.setup_tenant("cats").await; + let dogs_user = harness.setup_tenant("dogs").await; + + let capture_name = "cats/capture-shared"; + let draft_id = + setup_shared_collection_discover(&mut harness, cats_user, dogs_user, capture_name).await; + + // The Snapshot holds the pre-grant world; the grant and the discover row + // both come after it, in that order. + harness.refresh_snapshot_stale().await; + harness + .add_role_grant_unobserved("cats/", "dogs/shared/", models::Capability::Read) + .await; + + let disco_id = harness + .queue_discover( + "source/test", + ":test", + capture_name, + draft_id, + "ops/dp/public/test", + ) + .await; + harness.discover_handler.connectors.mock_discover( + capture_name, + Ok((spec_fixture(), single_binding_response("data"))), + ); + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::Queued + ), + "a stale denial of the binding's target collection should reschedule, got: {:?}", + harness.discover_job_status(disco_id).await, + ); + + // The Snapshot catches up, observing the grant: the discover completes and + // the merge is based on the live collection. + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(disco_id).await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + + let result = UserDiscoverResult::load(disco_id, &harness.pool).await; + assert!( + result.job_status.is_success(), + "discover should succeed once the grant is observed, got: {:?} with errors: {:?}", + result.job_status, + result.errors, + ); + assert_live_collection_preserved(&result.draft); +} + +/// The complement of `test_discover_reschedules_on_stale_live_spec_authz`, +/// and the reported scenario end to end: an *existing* capture with +/// non-default bindings and settings, whose reader is granted access just +/// before queuing a re-discover — after the Snapshot was taken. The capture +/// is aged, so a spec-relative staleness anchor would call the stale denial +/// authoritative, silently filter the live capture, and "succeed" with a +/// starter baseline — `expect_pub_id: 0`, no bindings, default settings. +/// Anchored to the discover request the denial is provisional: the first +/// poll reschedules, and a refreshed Snapshot — guaranteed to include the +/// pre-queue grant — preserves the live capture: its nonzero publication id, +/// its binding, and its non-default interval. Each assertion is +/// discriminating on its own, because the starter baseline has none of them. +#[tokio::test] +async fn test_discover_preserves_live_capture_after_late_grant() { + let mut harness = TestHarness::init("test_discover_capture_late_grant").await; + let cats_user = harness.setup_tenant("cats").await; + let dogs_user = harness.setup_tenant("dogs").await; + + // Publish a capture owned by `cats` with distinctive, non-default + // properties: a bound collection and a 42-minute interval. The collection + // schema is connector-managed so a re-discover may merge into it. The + // capture and collection live under *different* sub-prefixes: `dogs` gets + // an observed grant to the collection below, so that only the capture's + // own authorization rides on the late grant — otherwise the (correctly + // request-anchored) collection check would also reschedule and mask a + // regression of the capture anchor. + let capture_name = "cats/in/capture-owned"; + let pub_result = harness + .user_publication( + cats_user, + "publish cats capture", + draft_catalog(serde_json::json!({ + "collections": { + "cats/data/noms": { + "schema": wrap_connector_schema(serde_json::json!({ + "type": "object", + "properties": { "id": { "type": "string" } }, + "required": ["id"] + })), + "key": ["/id"] + } + }, + "captures": { + capture_name: { + "endpoint": { + "connector": { "image": "source/test:test", "config": {} } + }, + "bindings": [ + { "resource": { "id": "noms" }, "target": "cats/data/noms" } + ], + "interval": "42m" + } + }, + })), + ) + .await; + assert!( + pub_result.status.is_success(), + "setup publication failed: {:?} {:?}", + pub_result.status, + pub_result.errors + ); + // Age the capture: a spec-relative anchor would now judge any recent + // Snapshot authoritative for it, which is exactly the regression this + // test discriminates against. + harness.age_live_spec(capture_name).await; + // The collection grant is observed from the start. + harness + .add_role_grant("dogs/", "cats/data/", models::Capability::Read) + .await; + + let draft_id = harness + .create_draft(dogs_user, "late-grant re-discover", Default::default()) + .await; + + // The Snapshot observes the collection grant but not the capture's, + // which commits after it and just before the discover is queued. + harness.refresh_snapshot_stale().await; + harness + .add_role_grant_unobserved("dogs/", "cats/in/", models::Capability::Read) + .await; + + let disco_id = harness + .queue_discover( + "source/test", + ":test", + capture_name, + draft_id, + "ops/dp/public/test", + ) + .await; + harness.discover_handler.connectors.mock_discover( + capture_name, + Ok((spec_fixture(), single_binding_response("noms"))), + ); + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::Queued + ), + "a stale denial of the existing capture should reschedule, got: {:?}", + harness.discover_job_status(disco_id).await, + ); + + // The Snapshot observes the grant; the discover completes against the + // live capture rather than a starter baseline. + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(disco_id).await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + + let result = UserDiscoverResult::load(disco_id, &harness.pool).await; + assert!( + result.job_status.is_success(), + "discover should succeed once the grant is observed, got: {:?} with errors: {:?}", + result.job_status, + result.errors, + ); + + let drafted = result + .draft + .captures + .get_by_key(&models::Capture::new(capture_name)) + .expect("the capture should be drafted"); + assert!( + drafted.expect_pub_id.is_some_and(|id| !id.is_zero()), + "the drafted capture should expect the live publication id, got: {:?}", + drafted.expect_pub_id, + ); + let model = drafted.model.as_ref().expect("drafted capture model"); + assert_eq!( + vec!["cats/data/noms"], + model + .bindings + .iter() + .map(|b| b.target.as_str()) + .collect::>(), + "the live capture's binding should be preserved", + ); + assert_eq!( + std::time::Duration::from_secs(42 * 60), + model.interval, + "the live capture's non-default interval should be preserved", + ); + assert!( + model.auto_discover.is_none(), + "a preserved live capture must not gain the starter's auto_discover", + ); +} + +/// A `DiscoverConnectors` which revokes the grant authorizing the discover's +/// target collection — and pushes a refreshed, authoritative Snapshot into +/// the watch — while the connector RPC is in flight, before answering with +/// the underlying mock. This is production's shape: connector RPCs run for +/// seconds, and grant changes and Snapshot refreshes land freely within them. +#[derive(Clone)] +struct RevokeMidRpc { + pool: sqlx::PgPool, + refresher: SnapshotRefresher, + inner: MockDiscoverConnectors, +} + +impl DiscoverConnectors for RevokeMidRpc { + async fn discover<'a>( + &'a self, + data_plane: &'a tables::DataPlane, + task: &'a models::Capture, + logs_token: Uuid, + request: capture::Request, + ) -> anyhow::Result<(capture::response::Spec, capture::response::Discovered)> { + sqlx::query( + "delete from role_grants where subject_role = 'cats/' and object_role = 'dogs/shared/'", + ) + .execute(&self.pool) + .await?; + self.refresher.refresh_authoritative().await; + self.inner + .discover(data_plane, task, logs_token, request) + .await + } +} + +/// One discover must evaluate authorization against exactly one Snapshot: the +/// executor pins the watch's Snapshot once, and it rides the `Discover` +/// through the connector RPC into the merge. A grant change landing while the +/// RPC is in flight must not cause the capture baseline (resolved before the +/// RPC) and the collection baselines (resolved after it) to come from +/// different views. +/// +/// `RevokeMidRpc` deletes the authorizing grant and refreshes the watch from +/// inside the RPC. The merge still resolves `SHARED_COLLECTION` under the +/// pinned pre-revocation Snapshot, so the discover succeeds and preserves the +/// live collection. The guard discover then pins the post-revocation Snapshot +/// and shows the same merge silently drops the collection — so the first +/// result is attributable to pinning alone. +#[tokio::test] +async fn test_discover_uses_one_snapshot_across_connector_rpc() { + let mut harness = TestHarness::init("test_discover_one_snapshot_across_rpc").await; + let cats_user = harness.setup_tenant("cats").await; + let dogs_user = harness.setup_tenant("dogs").await; + + let capture_name = "cats/capture-shared"; + let draft_id = + setup_shared_collection_discover(&mut harness, cats_user, dogs_user, capture_name).await; + // Snapshot A: the grant written and observed, stamped authoritative. + harness + .add_role_grant("cats/", "dogs/shared/", models::Capability::Read) + .await; + harness.refresh_snapshot_authoritative().await; + + let mut mock = MockDiscoverConnectors::default(); + mock.mock_discover( + capture_name, + Ok((spec_fixture(), single_binding_response("data"))), + ); + let handler = DiscoverHandler::new(RevokeMidRpc { + pool: harness.pool.clone(), + refresher: harness.snapshot_refresher(), + inner: mock, + }); + + // Pin Snapshot A and assemble the request, as `DiscoverExecutor::process` + // and `prepare_discover` do. + let draft = control_plane_api::draft::load_draft(draft_id, &harness.pool) + .await + .unwrap(); + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); + let data_plane = snapshot + .data_plane_by_catalog_name("ops/dp/public/test") + .expect("test data-plane exists") + .clone(); + let output = handler + .discover( + &harness.pool, + Discover { + capture_name: models::Capture::new(capture_name), + data_plane, + logs_token: Uuid::new_v4(), + user_id: cats_user, + filter_user_authz: true, + update_only: false, + reset_on_key_change: false, + draft, + created_at: String::new(), + snapshot, + started_at: Some(tokens::now()), + }, + ) + .await + .expect("discover should not error"); + assert!( + output.is_success(), + "the pinned pre-revocation Snapshot should authorize the merge, got: {:?}", + output.draft.errors, + ); + assert_live_collection_preserved(&output.draft); + + // Guard: the same discover pinning the post-revocation Snapshot cannot see + // the live collection: the merge silently drops it and re-drafts it from + // scratch. The extra refresh stamps the Snapshot authoritative for this + // discover's `started_at`, making the denial terminal rather than stale. + let started_at = tokens::now(); + harness.refresh_snapshot_authoritative().await; + + let mut mock = MockDiscoverConnectors::default(); + mock.mock_discover( + capture_name, + Ok((spec_fixture(), single_binding_response("data"))), + ); + let handler = DiscoverHandler::new(mock); + let draft = control_plane_api::draft::load_draft(draft_id, &harness.pool) + .await + .unwrap(); + let snapshot = harness.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); + let data_plane = snapshot + .data_plane_by_catalog_name("ops/dp/public/test") + .expect("test data-plane exists") + .clone(); + let output = handler + .discover( + &harness.pool, + Discover { + capture_name: models::Capture::new(capture_name), + data_plane, + logs_token: Uuid::new_v4(), + user_id: cats_user, + filter_user_authz: true, + update_only: false, + reset_on_key_change: false, + draft, + created_at: String::new(), + snapshot, + started_at: Some(started_at), + }, + ) + .await + .expect("guard discover should not error"); + assert!( + output.is_success(), + "an authoritative denial silently drops the collection, got: {:?}", + output.draft.errors, + ); + let drafted = output + .draft + .collections + .get_by_key(&models::Collection::new(SHARED_COLLECTION)) + .expect("the target collection should be drafted"); + assert_eq!( + Some(models::Id::zero()), + drafted.expect_pub_id, + "under the revoked view the live collection is invisible and re-drafted from scratch", + ); +} + +/// A data-plane registration that lands after the current Snapshot must be +/// observable to a discover queued afterward. Until the Snapshot catches up, +/// the missing plane is provisional: the discover stays queued and requests an +/// early refresh. It proceeds once an authoritative Snapshot includes the +/// registration. +#[tokio::test] +async fn test_discover_succeeds_after_late_data_plane_registration() { + let mut harness = TestHarness::init("test_discover_late_data_plane_registration").await; + let user_id = harness.setup_tenant("cats").await; + let data_plane_name = "ops/dp/public/late-registration"; + let capture_name = "cats/capture-late-data-plane"; + + // Snapshot A includes the tenant's grant to `ops/dp/public/`, but not the + // concrete plane which is registered immediately afterward. + harness.refresh_snapshot_stale().await; + let token = harness.snapshot_watch.token(); + let snapshot = token.result().expect("snapshot should be ready"); + assert!( + !snapshot.revoke.is_cancelled(), + "a freshly-published Snapshot should not already be revoked" + ); + harness.add_data_plane(data_plane_name).await; + + let draft_id = harness + .create_draft(user_id, "late data-plane discover", Default::default()) + .await; + let disco_id = harness + .queue_discover( + "source/test", + ":test", + capture_name, + draft_id, + data_plane_name, + ) + .await; + harness.discover_handler.connectors.mock_discover( + capture_name, + Ok((spec_fixture(), single_binding_response("acorns"))), + ); + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::Queued + ), + "a plane missing from a stale Snapshot should reschedule, got: {:?}", + harness.discover_job_status(disco_id).await, + ); + assert!( + snapshot.revoke.is_cancelled(), + "a stale missing-plane decision must request an early Snapshot refresh" + ); + + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(disco_id).await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + let status = harness.discover_job_status(disco_id).await; + assert!( + matches!(status, JobStatus::Success { .. }), + "discover should succeed once the plane is observed, got: {status:?}", + ); +} + +/// An authorized plane that remains absent after an authoritative refresh must +/// resolve as `NoDataPlane`. The stale first poll is provisional, while the +/// authoritative second poll is terminal; neither may invoke the connector. +#[tokio::test] +async fn test_discover_missing_data_plane_is_terminal_after_refresh() { + let mut harness = TestHarness::init("test_discover_missing_data_plane").await; + let user_id = harness.setup_tenant("cats").await; + let capture_name = "cats/capture-missing-dp"; + let data_plane_name = "ops/dp/public/does-not-exist"; + + // `setup_tenant` grants `cats/ -> ops/dp/public/ read`, so this name passes + // authorization; Snapshot A and Postgres both lack the concrete plane. + harness.refresh_snapshot_stale().await; + let draft_id = harness + .create_draft(user_id, "missing dp discover", Default::default()) + .await; + let disco_id = harness + .queue_discover( + "source/test", + ":test", + capture_name, + draft_id, + data_plane_name, + ) + .await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::Queued + ), + "a plane missing from a stale Snapshot should reschedule, got: {:?}", + harness.discover_job_status(disco_id).await, + ); + assert!( + harness + .discover_handler + .connectors + .last_discover_request(capture_name) + .is_none(), + "the connector must not be invoked while plane existence is unknown" + ); + + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(disco_id).await; + + let ran = harness + .run_automation_task(automations::task_types::DISCOVERS) + .await; + assert_eq!(Some(disco_id), ran); + assert!( + matches!( + harness.discover_job_status(disco_id).await, + JobStatus::NoDataPlane + ), + "a plane missing from an authoritative Snapshot should be NoDataPlane, got: {:?}", + harness.discover_job_status(disco_id).await, + ); + assert!( + harness + .discover_handler + .connectors + .last_discover_request(capture_name) + .is_none(), + "the connector must not be invoked for a missing plane" + ); +} + +/// `JobStatus::NotAuthorized` is new, and job statuses round-trip through a JSON +/// column. Pin its serialized form so a rename can't silently orphan rows that +/// were written with the old spelling. +#[test] +fn test_job_status_not_authorized_serde() { + let encoded = serde_json::to_value(&JobStatus::NotAuthorized).unwrap(); + assert_eq!(serde_json::json!({"type": "notAuthorized"}), encoded); + assert!(matches!( + serde_json::from_value::(encoded).unwrap(), + JobStatus::NotAuthorized + )); +} + +/// A discovered response with a single enabled binding, sufficient for a +/// discover to merge and succeed. +fn single_binding_response(name: &str) -> Discovered { + Discovered { + bindings: vec![Binding { + recommended_name: name.to_string(), + document_schema_json: document_schema(1), + resource_config_json: format!(r#"{{"id": "{name}"}}"#).into(), + key: vec!["/id".to_string()], + disable: false, + resource_path: Vec::new(), + is_fallback_key: false, + }], + } +} + +fn minimal_capture() -> serde_json::Value { + serde_json::json!({ + "endpoint": { + "connector": { + "image": "source/test:test", + "config": {} + } + }, + "bindings": [], + }) +} + fn document_schema(version: usize) -> bytes::Bytes { serde_json::to_string(&serde_json::json!({ "type": "object", diff --git a/crates/agent/src/integration_tests/user_publications.rs b/crates/agent/src/integration_tests/user_publications.rs index e31394f4707..b9d4e6852ba 100644 --- a/crates/agent/src/integration_tests/user_publications.rs +++ b/crates/agent/src/integration_tests/user_publications.rs @@ -1,9 +1,10 @@ use super::harness::{ - TestHarness, draft_catalog, get_collection_generation_id, mock_inferred_schema, set_of, + Either, TestHarness, draft_catalog, get_collection_generation_id, mock_inferred_schema, set_of, }; use crate::{ ControlPlane, controllers::ControllerState, integration_tests::harness::InjectBuildError, }; +use control_plane_api::publications; use models::{Capability, CatalogType, Id, status::AlertType}; #[tokio::test] @@ -428,6 +429,506 @@ async fn successful_user_publication_clears_background_publication_failed_alert( harness.assert_alert_resolved(fired_alert.alert.id).await; } +/// A draft that materializes `cats/noms`, which `dogs` may only publish once it +/// holds both a user grant and a role grant to `cats/`. +fn dogs_materialize_cats_draft() -> tables::DraftCatalog { + draft_catalog(serde_json::json!({ + "materializations": { + "dogs/materialize": { + "endpoint": { + "connector": { + "image": "materialize/test:test", + "config": {} + } + }, + "bindings": [ + { + "resource": { "table": "dog_noms" }, + "source": "cats/noms" + } + ] + } + } + })) +} + +/// Publishes `cats/noms` and returns the `dogs` user id. Shared setup for the +/// stale-authorization publication tests below. +async fn setup_cross_tenant_publication(harness: &mut TestHarness) -> uuid::Uuid { + let cats_user = harness.setup_tenant("cats").await; + + // The capture isn't incidental: a draft holding only a collection with no + // writer builds to zero specs and is reported as an empty draft. + let result = harness + .user_publication( + cats_user, + "publish cats/noms", + draft_catalog(serde_json::json!({ + "collections": { + "cats/noms": { + "schema": { + "type": "object", + "properties": { "id": { "type": "string" } } + }, + "key": ["/id"] + } + }, + "captures": { + "cats/capture": { + "endpoint": { + "connector": { + "image": "source/test:test", + "config": {} + } + }, + "bindings": [ + { + "resource": { "id": "noms" }, + "target": "cats/noms" + } + ] + } + } + })), + ) + .await; + assert!( + result.status.is_success(), + "setup publication failed: {:?} {:?}", + result.status, + result.errors + ); + + harness.setup_tenant("dogs").await +} + +/// A publication must reschedule when its selected data plane is denied by a +/// Snapshot which predates the queued publication. This is the concrete race: +/// the grant is restored in Postgres before the publication is queued, but the +/// in-memory Snapshot still reflects the brief revocation. +#[tokio::test] +async fn test_publication_reschedules_on_stale_data_plane_authz() { + let mut harness = TestHarness::init("test_publication_stale_data_plane_authz").await; + let cats_user = harness.setup_tenant("cats").await; + + let deleted = sqlx::query( + "delete from role_grants + where subject_role = 'cats/' and object_role = 'ops/dp/public/'", + ) + .execute(&harness.pool) + .await + .expect("failed to remove the tenant's public-plane grant"); + assert_eq!(1, deleted.rows_affected()); + + // Snapshot A observes the revocation. Restore the grant without refreshing, + // then queue the publication so A is not authoritative for its denial. + harness.refresh_snapshot().await; + harness + .add_role_grant_unobserved("cats/", "ops/dp/public/", Capability::Read) + .await; + let pub_id = harness + .queue_publication( + cats_user, + "public-plane grant awaiting Snapshot refresh", + Either::L(draft_catalog(serde_json::json!({ + "collections": { + "cats/noms": { + "schema": { + "type": "object", + "properties": { "id": { "type": "string" } } + }, + "key": ["/id"] + } + }, + "captures": { + "cats/capture": { + "endpoint": { + "connector": { "image": "source/test:test", "config": {} } + }, + "bindings": [ + { "resource": { "id": "noms" }, "target": "cats/noms" } + ] + } + } + }))), + ) + .await; + + let first = harness.poll_publication_once(pub_id).await; + assert_eq!( + publications::StatusType::Queued, + first.status.r#type, + "publication should reschedule while the restored plane grant is unobserved, got: {:?}", + first.errors + ); + + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(pub_id).await; + + let second = harness.poll_publication_once(pub_id).await; + assert!( + second.status.is_success(), + "publication should succeed once the plane grant is observed, got: {:?}", + second.errors + ); +} + +/// After a stale-Snapshot denial, the executor persists the instant an +/// authoritative Snapshot must postdate (in `internal.tasks`, so whichever +/// agent instance dequeues the next poll applies the same criterion) and +/// defers re-polls without loading or building the draft. Once the local +/// Snapshot postdates that instant, the retry proceeds and succeeds. +#[tokio::test] +async fn test_publication_defers_polls_until_authoritative_snapshot() { + let mut harness = TestHarness::init("test_publication_defers_polls").await; + let dogs_user = setup_cross_tenant_publication(&mut harness).await; + + harness.refresh_snapshot_stale().await; + harness + .add_user_grant_unobserved(dogs_user, "cats/", Capability::Read) + .await; + harness + .add_role_grant_unobserved("dogs/", "cats/", Capability::Read) + .await; + + let pub_id = harness + .queue_publication( + dogs_user, + "deferred until authoritative", + Either::L(dogs_materialize_cats_draft()), + ) + .await; + + let first = harness.poll_publication_once(pub_id).await; + assert_eq!( + publications::StatusType::Queued, + first.status.r#type, + "publication should reschedule while the grants are unobserved, got: {:?}", + first.errors + ); + + let state: serde_json::Value = harness.get_task_state(pub_id).await; + assert!( + state + .get("awaiting_snapshot_after") + .is_some_and(|v| v.is_string()), + "the executor should record the instant a Snapshot must postdate, got: {state}" + ); + + // Because the anchor is persisted, the re-poll may be dequeued by a + // *different* agent instance whose own local Snapshot is stale — one whose + // revoke token the original attempt never cancelled. Model that handoff by + // replacing the watch with another stale Snapshot bearing a fresh token. + harness.refresh_snapshot_stale().await; + let handoff_revoke = harness + .snapshot_watch + .token() + .result() + .unwrap() + .revoke + .clone(); + assert!(!handoff_revoke.is_cancelled()); + + // A re-poll under the still-stale Snapshot defers, leaving the row queued. + harness.set_min_task_wake_at(pub_id).await; + let deferred = harness.poll_publication_once(pub_id).await; + assert_eq!( + publications::StatusType::Queued, + deferred.status.r#type, + "a re-poll under a still-stale Snapshot should defer, got: {:?}", + deferred.errors + ); + + // The deferring poll must request a refresh of the Snapshot it observed: + // no prior cancellation covers this instance's Snapshot, and without one + // the task would idle until the watch's ordinary refresh interval. + assert!( + handoff_revoke.is_cancelled(), + "a deferring poll should cancel the stale Snapshot it observed" + ); + + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(pub_id).await; + let resolved = harness.poll_publication_once(pub_id).await; + assert!( + resolved.status.is_success(), + "publication should succeed once the Snapshot postdates the anchor, got: {:?}", + resolved.errors + ); +} + +/// The variant of the late-grant race that the test above cannot catch: the +/// referenced spec is *old*. A Snapshot taken after the spec's publication but +/// before the new grants is inconclusive for a publication queued after those +/// grants — staleness is a property of the publication's queued time, not of +/// the referenced spec's age. The publication must remain queued under that +/// Snapshot and succeed once a refresh observes the grants. +#[tokio::test] +async fn test_old_spec_publication_succeeds_after_late_grant() { + let mut harness = + TestHarness::init("test_old_spec_publication_succeeds_after_late_grant").await; + let dogs_user = setup_cross_tenant_publication(&mut harness).await; + + // `cats/noms` was published long before any of the events below. + harness.age_live_spec("cats/noms").await; + + // Snapshot A: taken after the (old) spec but before the grants and the + // publication, so it holds the pre-grant world. + harness.refresh_snapshot_stale().await; + harness + .add_user_grant_unobserved(dogs_user, "cats/", Capability::Read) + .await; + harness + .add_role_grant_unobserved("dogs/", "cats/", Capability::Read) + .await; + + let pub_id = harness + .queue_publication( + dogs_user, + "late grant, old spec", + Either::L(dogs_materialize_cats_draft()), + ) + .await; + + let first = harness.poll_publication_once(pub_id).await; + assert_eq!( + publications::StatusType::Queued, + first.status.r#type, + "a publication evaluated against a Snapshot older than its queued time \ + must reschedule regardless of the referenced spec's age, got: {:?}", + first.errors + ); + + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(pub_id).await; + + let second = harness.poll_publication_once(pub_id).await; + assert!( + second.status.is_success(), + "publication should succeed once the grants are observed, got: {:?}", + second.errors + ); +} + +/// An `Initialize` stage which revokes the grants that authorize the test's +/// publication and pushes a refreshed, authoritative Snapshot into the watch — +/// exactly what a background refresh landing between draft initialization and +/// live-spec resolution does in production. Composed as the final Initialize +/// stage, it runs after `ExpandDraft` and before `build`, squarely on the +/// phase boundary that snapshot pinning exists to protect. +struct RevokeMidPublication<'h> { + harness: &'h TestHarness, + dogs_user: uuid::Uuid, +} + +impl publications::Initialize for RevokeMidPublication<'_> { + async fn initialize( + &self, + db: &sqlx::PgPool, + _user_id: uuid::Uuid, + _draft: &mut tables::DraftCatalog, + _snapshot: &control_plane_api::Snapshot, + ) -> anyhow::Result<()> { + sqlx::query( + "delete from role_grants where subject_role = 'dogs/' and object_role = 'cats/'", + ) + .execute(db) + .await?; + sqlx::query("delete from user_grants where user_id = $1 and object_role = 'cats/'") + .bind(self.dogs_user) + .execute(db) + .await?; + self.harness.refresh_snapshot_authoritative().await; + Ok(()) + } +} + +/// One publication must evaluate authorization against exactly one Snapshot: +/// `try_publish` resolves the watch once and threads that Snapshot through +/// both draft initialization and live-spec resolution. A refresh landing +/// between those phases must not swap the view mid-flight. +/// +/// `RevokeMidPublication` deletes the authorizing grants and refreshes the +/// watch after expansion. Resolution still authorizes under the pinned +/// pre-revocation Snapshot, so the publication succeeds; if it consulted the +/// watch anew it would see the revoked world and deny. The guard publication +/// then proves the refreshed watch really does deny the same draft, so the +/// first result is attributable to pinning alone. +#[tokio::test] +async fn test_publication_uses_one_snapshot_across_phases() { + let mut harness = TestHarness::init("test_publication_one_snapshot_across_phases").await; + let dogs_user = setup_cross_tenant_publication(&mut harness).await; + + // Snapshot A: grants written and observed, stamped authoritative. + harness + .add_user_grant(dogs_user, "cats/", Capability::Read) + .await; + harness + .add_role_grant("dogs/", "cats/", Capability::Read) + .await; + harness.refresh_snapshot_authoritative().await; + + // Pin the pre-revocation Snapshot which the whole publication evaluates + // against; the mid-publication refresh below must not displace it. + let refresh = harness.snapshot_watch.token(); + let publication = publications::DraftPublication { + user_id: dogs_user, + logs_token: uuid::Uuid::new_v4(), + dry_run: true, + detail: Some("one snapshot across phases".to_string()), + draft: dogs_materialize_cats_draft(), + started_at: Some(tokens::now()), + snapshot: refresh + .result() + .expect("authorization snapshot is not ready"), + verify_user_authz: true, + default_data_plane_name: Some("ops/dp/public/test".to_string()), + initialize: ( + publications::ExpandDraft { + filter_user_authz: true, + }, + RevokeMidPublication { + harness: &harness, + dogs_user, + }, + ), + finalize: publications::PruneUnboundCollections, + retry: publications::DoNotRetry, + with_commit: publications::NoopWithCommit, + }; + let result = harness + .publisher + .publish(publication) + .await + .expect("publish should not error"); + assert!( + result.status.is_success(), + "the pinned pre-revocation Snapshot should authorize both phases, got: {:?} draft: {:?} live: {:?} built: {:?}", + result.status, + result.draft.errors, + result.live.errors, + result.built.errors, + ); + + // Guard: the same draft judged against the refreshed watch is denied — + // the revocation above is real, and the success was due to pinning. The + // extra refresh stamps the Snapshot authoritative for this publication's + // `started_at`, making the denial terminal rather than a stale retry. + let started_at = tokens::now(); + harness.refresh_snapshot_authoritative().await; + let guard_refresh = harness.snapshot_watch.token(); + let guard = publications::DraftPublication { + user_id: dogs_user, + logs_token: uuid::Uuid::new_v4(), + dry_run: true, + detail: Some("post-revocation guard".to_string()), + draft: dogs_materialize_cats_draft(), + started_at: Some(started_at), + snapshot: guard_refresh + .result() + .expect("authorization snapshot is not ready"), + verify_user_authz: true, + default_data_plane_name: Some("ops/dp/public/test".to_string()), + initialize: publications::ExpandDraft { + filter_user_authz: true, + }, + finalize: publications::PruneUnboundCollections, + retry: publications::DoNotRetry, + with_commit: publications::NoopWithCommit, + }; + let denied = harness + .publisher + .publish(guard) + .await + .expect("guard publish should not error"); + assert!( + !denied.status.is_success(), + "the revoked, authoritative Snapshot must deny the same draft, got: {:?}", + denied.status, + ); +} + +/// The guard on the test above: a genuinely unauthorized publication must not be +/// hidden by the reschedule path. It reschedules only while the Snapshot is +/// inconclusive, then fails with the same authorization errors as before. +/// This also pins the anchor's other boundary: changes committed after the +/// queued publication carry no observation guarantee, so an authoritative +/// denial is terminal regardless of what commits later. +#[tokio::test] +async fn test_publication_stale_then_authoritative_denial() { + let mut harness = TestHarness::init("test_publication_stale_denial").await; + let dogs_user = setup_cross_tenant_publication(&mut harness).await; + + // No grants are ever added — only the Snapshot's age changes. + harness.refresh_snapshot_stale().await; + let pub_id = harness + .queue_publication( + dogs_user, + "never authorized", + Either::L(dogs_materialize_cats_draft()), + ) + .await; + + let first = harness.poll_publication_once(pub_id).await; + assert_eq!( + publications::StatusType::Queued, + first.status.r#type, + "an inconclusive denial should reschedule, got: {:?}", + first.errors + ); + + harness.refresh_snapshot_authoritative().await; + harness.set_min_task_wake_at(pub_id).await; + + let second = harness.poll_publication_once(pub_id).await; + assert!(!second.status.is_success()); + insta::assert_debug_snapshot!(second.errors, @r#" + [ + ( + "flow://unauthorized/cats/noms", + "User is not authorized to read this catalog name", + ), + ( + "flow://materialization/dogs/materialize", + "Specification 'dogs/materialize' is not read-authorized to 'cats/noms'.\nAvailable grants are: [\n {\n \"subject_role\": \"dogs/\",\n \"object_role\": \"dogs/\",\n \"capability\": \"write\",\n \"bundles\": []\n },\n {\n \"subject_role\": \"dogs/\",\n \"object_role\": \"ops/dp/public/\",\n \"capability\": \"read\",\n \"bundles\": []\n }\n]", + ), + ] + "#); +} + +/// Rescheduling alone isn't enough: the raising site must also cancel the +/// Snapshot's `revoke` token, which is what asks the background watch to refresh +/// ahead of its normal interval. Without it a stale publication would sleep +/// against an unchanged Snapshot until the next scheduled refresh. +#[tokio::test] +async fn test_publication_requests_snapshot_refresh() { + let mut harness = TestHarness::init("test_publication_requests_refresh").await; + let dogs_user = setup_cross_tenant_publication(&mut harness).await; + + harness.refresh_snapshot_stale().await; + let token = harness.snapshot_watch.token(); + let snapshot = token.result().expect("snapshot should be ready"); + assert!( + !snapshot.revoke.is_cancelled(), + "a freshly-published Snapshot should not already be revoked" + ); + + let pub_id = harness + .queue_publication( + dogs_user, + "requests refresh", + Either::L(dogs_materialize_cats_draft()), + ) + .await; + let result = harness.poll_publication_once(pub_id).await; + assert_eq!(publications::StatusType::Queued, result.status.r#type); + + assert!( + snapshot.revoke.is_cancelled(), + "a stale-snapshot publication must request an early Snapshot refresh" + ); +} + async fn assert_publication_included( publication_id: Id, catalog_names: &[&str], diff --git a/crates/agent/src/main.rs b/crates/agent/src/main.rs index dab23d93311..668d6f2eb8b 100644 --- a/crates/agent/src/main.rs +++ b/crates/agent/src/main.rs @@ -314,6 +314,20 @@ async fn async_main(args: Args) -> Result<(), anyhow::Error> { .context("failed to create builds-root directory")?; } + // Create the snapshot source and start the refresh loop. + // Snapshot fetches retry internally forever, so a persistent failure (a + // broken query, sops / KMS breakage) would otherwise hang here with the + // port unbound and nothing logged at error level. Bound the wait so that + // startup fails visibly, and fits within Cloud Run's 240s startup probe + // window even after the database retry budget above. + let snapshot_source = control_plane_api::snapshot::PgSnapshotSource::new(pg_pool.clone()); + let snapshot_watch = tokio::time::timeout( + std::time::Duration::from_secs(60), + tokens::watch(snapshot_source).ready_owned(), + ) + .await + .context("timed out fetching the initial authorization snapshot")?; + // Start a logs sink into which agent loops may stream logs. let (logs_tx, logs_rx) = tokio::sync::mpsc::channel(8192); let logs_sink = control_plane_api::logs::serve_sink(pg_pool.clone(), logs_rx); @@ -348,20 +362,6 @@ async fn async_main(args: Args) -> Result<(), anyhow::Error> { } .shared(); - // Create the snapshot source and start the refresh loop. - // Snapshot fetches retry internally forever, so a persistent failure (a - // broken query, sops / KMS breakage) would otherwise hang here with the - // port unbound and nothing logged at error level. Bound the wait so that - // startup fails visibly, and fits within Cloud Run's 240s startup probe - // window even after the database retry budget above. - let snapshot_source = control_plane_api::snapshot::PgSnapshotSource::new(pg_pool.clone()); - let snapshot_watch = tokio::time::timeout( - std::time::Duration::from_secs(60), - tokens::watch(snapshot_source).ready_owned(), - ) - .await - .context("timed out fetching the initial authorization snapshot")?; - let controller_publication_cooldown = chrono::Duration::from_std(args.controller_publication_cooldown)?; let alert_config_defaults = args.controller_config.alert_config_defaults(); @@ -390,7 +390,7 @@ async fn async_main(args: Args) -> Result<(), anyhow::Error> { jwt_secret.as_bytes(), pg_pool.clone(), publisher.clone(), - snapshot_watch, + snapshot_watch.clone(), args.stripe_webhook_secret, )); let api_router = control_plane_api::build_router( @@ -417,12 +417,14 @@ async fn async_main(args: Args) -> Result<(), anyhow::Error> { .register(agent::publications::PublicationsExecutor { publisher, pg_pool: pg_pool.clone(), + snapshot_watch: snapshot_watch.clone(), runtime_v2_new_captures: args.runtime_v2_new_captures, runtime_v2_new_materializations: args.runtime_v2_new_materializations, runtime_v2_new_derivations: args.runtime_v2_new_derivations, }) .register(agent::DiscoverExecutor { handler: discover_handler, + snapshot_watch, }) .register(directive_executor) .register(connector_tags_executor) diff --git a/crates/agent/src/publications.rs b/crates/agent/src/publications.rs index 326bfd20296..a77b7da94f1 100644 --- a/crates/agent/src/publications.rs +++ b/crates/agent/src/publications.rs @@ -1,4 +1,5 @@ use anyhow::Context; +use control_plane_api::Snapshot; use control_plane_api::publications::{Row, fetch_publication}; use models::draft_error; use tracing::info; @@ -15,6 +16,11 @@ use control_plane_api::{ pub struct PublicationsExecutor { pub publisher: Publisher, pub pg_pool: sqlx::PgPool, + /// Authorization Snapshot watch. Each poll pins one Snapshot from this + /// watch: first to cheaply defer while it remains stale for a queued + /// publication (see `Snapshot::taken_after`), and then to serve + /// every authorization decision of the publication itself. + pub snapshot_watch: std::sync::Arc>, /// When true, newly-created captures are published onto runtime v2; see [`RuntimeV2Rollout`]. pub runtime_v2_new_captures: bool, /// When true, newly-created materializations are published onto runtime v2; see [`RuntimeV2Rollout`]. @@ -25,16 +31,14 @@ pub struct PublicationsExecutor { /// Poll state persisted to `internal.tasks` between polls, and therefore /// shared with whichever agent instance dequeues the next poll. -/// -/// This deploy doesn't yet read or write this state: it's carried so that -/// state persisted by the upcoming stale-snapshot deferral changes remains -/// decodable by this version during a deploy or rollback. -#[allow(dead_code)] #[derive(Debug, Default, serde::Serialize, serde::Deserialize)] pub struct PublicationState { /// The instant a Snapshot must postdate (per `Snapshot::taken_after`) /// before this publication is retried: the queued time its prior attempt - /// anchored authorization staleness on. + /// anchored authorization staleness on. While set, polls defer — without + /// loading or building the draft — until the local Snapshot satisfies it. + /// Optional so that reschedules for other, future reasons aren't bound to + /// this check. #[serde(default)] pub awaiting_snapshot_after: Option, } @@ -56,23 +60,29 @@ impl automations::Executor for PublicationsExecutor { pool: &'s sqlx::PgPool, task_id: models::Id, _parent_id: Option, - _state: &'s mut Self::State, + state: &'s mut Self::State, inbox: &'s mut std::collections::VecDeque<(models::Id, Option)>, ) -> anyhow::Result { tracing::debug!(?inbox, "starting publication task"); let row = fetch_publication(task_id, pool).await?; - self.handle_task(row).await?; + let action = self.handle_task(row, state).await?; // Always clear inbox, or else we'll get re-polled. inbox.clear(); - // Publication tasks are always done at the end. We don't retry because there is likely - // a user waiting for the result, who could easily retry the operation themselves. - Ok(automations::Action::Done) + // A publication is normally `Done` at the end — we don't retry failures + // because a user is likely waiting and can retry themselves. The one + // exception is a stale authorization snapshot, where `handle_task` + // returns a `Sleep` and we re-poll until a fresher snapshot decides it. + Ok(action) } } impl PublicationsExecutor { - async fn handle_task(&self, row: Row) -> anyhow::Result<()> { + async fn handle_task( + &self, + row: Row, + state: &mut Option, + ) -> anyhow::Result { let id = row.id; // First ensure that the publication status is queued. Otherwise, @@ -81,7 +91,7 @@ impl PublicationsExecutor { Ok(status) if status.r#type == StatusType::Queued => { /* continue to publish */ } Ok(other) => { tracing::warn!(?other, "skipping publication which is no longer queued"); - return Ok(()); + return Ok(automations::Action::Done); } Err(error) => { // Weird edge case, but we don't update the status so that we @@ -89,16 +99,37 @@ impl PublicationsExecutor { // the task completed so that the user can update the status // back to queued if they want. tracing::error!(?error, "failed to parse publication job status"); - return Ok(()); + return Ok(automations::Action::Done); + } + } + + // Pin one Snapshot for this poll: the deferral decision and every + // authorization decision of the publication observe the same view. + let snapshot = self.snapshot_watch.token(); + let snapshot = snapshot.result().unwrap(); + + // A prior attempt was denied under a Snapshot that was not + // authoritative for this publication. Defer — without loading or + // building the draft — until this instance's Snapshot is, at which + // point the retry is guaranteed to classify deterministically. + if let Some(anchor) = state.as_ref().and_then(|s| s.awaiting_snapshot_after) { + if !snapshot.taken_after(anchor) { + snapshot.revoke.cancel(); + return Ok(automations::Action::Sleep( + Snapshot::STALE_RETRY_WAKE + .to_std() + .expect("wake interval is positive"), + )); } } let dry_run = row.dry_run; let draft_id = row.draft_id; + let queued_at = row.updated_at; let time_queued = chrono::Utc::now().signed_duration_since(row.updated_at); - let (status, draft_errors, final_pub_id) = match self.process(row).await { + let (status, draft_errors, final_pub_id) = match self.process(row, snapshot).await { Ok(result) => { if dry_run { specs::add_built_specs_to_draft_specs(draft_id, &result.built, &self.pg_pool) @@ -114,6 +145,24 @@ impl PublicationsExecutor { }; (result.status, errors, final_id) } + Err(error) if validation::is_authz_snapshot_stale(&error) => { + // An authorization denial was evaluated against a Snapshot that + // isn't authoritative for this publication. `Publisher::publish` + // already requested an early refresh; record the instant an + // authoritative Snapshot must postdate and reschedule, so that + // re-polls defer cheaply until one lands rather than reporting + // a failure. + tracing::info!( + pub_id = %id, %time_queued, + "publication authorization snapshot is stale; rescheduling" + ); + state.get_or_insert_default().awaiting_snapshot_after = Some(queued_at); + return Ok(automations::Action::Sleep( + Snapshot::STALE_RETRY_WAKE + .to_std() + .expect("wake interval is positive"), + )); + } Err(error) => { tracing::warn!(?error, pub_id = %id, "build finished with error"); let errors = vec![draft_error::Error { @@ -146,7 +195,7 @@ impl PublicationsExecutor { if status.is_success() && !dry_run { delete_draft(draft_id, &self.pg_pool).await?; } - Ok(()) + Ok(automations::Action::Done) } #[tracing::instrument(skip_all, fields( @@ -155,7 +204,7 @@ impl PublicationsExecutor { %row.dry_run, %row.user_id, ))] - async fn process(&self, row: Row) -> anyhow::Result { + async fn process(&self, row: Row, snapshot: &Snapshot) -> anyhow::Result { info!( %row.logs_token, %row.created_at, @@ -192,6 +241,12 @@ impl PublicationsExecutor { dry_run: row.dry_run, detail: row.detail.clone(), draft, + // `updated_at` is the instant this row entered `queued`, and is + // stable across our reschedules. Authorization denials evaluated + // against a snapshot older than it are treated as not-yet-observed + // and retried rather than reported. + started_at: Some(row.updated_at), + snapshot, verify_user_authz: true, default_data_plane_name: row.data_plane_name.clone().filter(|s| !s.is_empty()), initialize: ( @@ -201,7 +256,7 @@ impl PublicationsExecutor { new_derivations: self.runtime_v2_new_derivations, }, ExpandDraft { - filter_user_has_admin: true, + filter_user_authz: true, }, ), finalize: PruneUnboundCollections, diff --git a/crates/control-plane-api/src/discovers/mod.rs b/crates/control-plane-api/src/discovers/mod.rs index d5686059e49..f27ad368ab7 100644 --- a/crates/control-plane-api/src/discovers/mod.rs +++ b/crates/control-plane-api/src/discovers/mod.rs @@ -1,7 +1,7 @@ pub mod db; pub mod specs; -use crate::proxy_connectors::DiscoverConnectors; +use crate::{Snapshot, proxy_connectors::DiscoverConnectors}; use anyhow::Context; use models::discovers::{Changed, Changes}; @@ -14,7 +14,7 @@ pub use db::{Row, fetch_discover, resolve}; /// Represents the desire to discover an endpoint. The discovered bindings will be merged with /// those in the `base_model`. -pub struct Discover { +pub struct Discover<'a> { /// The name of the capture, which _must_ exist within the `draft`. pub capture_name: models::Capture, /// The data plane to use for the discover. For an existing capture, this @@ -41,6 +41,19 @@ pub struct Discover { /// from the live task's control-plane Id. Empty if the task doesn't exist /// yet: the connector assumes a current date for a new task's discover. pub created_at: String, + /// The authorization Snapshot pinned by the caller for this entire + /// operation: preflight checks, the connector RPC window, and the + /// post-RPC merge all consult this same instance, so one discover + /// observes exactly one authorization view regardless of refreshes + /// landing mid-flight. + pub snapshot: &'a crate::Snapshot, + /// Time at which the discover was queued, when the caller has a durable + /// one. Anchors authorization staleness of the merge's target collections: + /// a denial from a Snapshot older than this instant is provisional — + /// authority committed before queuing may be missing from it — and + /// reschedules the discover rather than silently dropping the live + /// collection. A Snapshot taken after this instant is authoritative. + pub started_at: Option, } #[derive(Debug)] @@ -161,7 +174,7 @@ impl DiscoverHandler { update_only = %req.update_only, image ))] - pub async fn discover(&self, db: &PgPool, req: Discover) -> anyhow::Result { + pub async fn discover(&self, db: &PgPool, req: Discover<'_>) -> anyhow::Result { let Discover { capture_name, data_plane, @@ -172,6 +185,8 @@ impl DiscoverHandler { reset_on_key_change, mut draft, created_at, + snapshot, + started_at, } = req; let Some(capture_def) = draft.captures.get_mut_by_key(&capture_name) else { @@ -232,6 +247,8 @@ impl DiscoverHandler { spec.resource_path_pointers, db, reset_on_key_change, + snapshot, + started_at, ) .await?; @@ -252,7 +269,7 @@ impl DiscoverHandler { async fn build_merged_catalog( capture_name: models::Capture, - user_id: uuid::Uuid, + user_id: Uuid, filter_user_authz: bool, update_only: bool, mut draft: tables::DraftCatalog, @@ -260,6 +277,8 @@ impl DiscoverHandler { resource_path_pointers: Vec, db: &PgPool, reset_on_key_change: bool, + snapshot: &Snapshot, + started_at: Option, ) -> anyhow::Result { let discovered_bindings = match specs::parse_response(discovered) .context("converting connector discovery response into specs") @@ -307,12 +326,13 @@ impl DiscoverHandler { .iter() .map(|b| b.target.to_string()) .collect::>(); - let live = crate::live_specs::get_live_specs( user_id, &collection_names, - filter_user_authz.then_some(models::Capability::Read), + filter_user_authz.then_some(models::authz::Capability::CatalogRead.into()), db, + snapshot, + started_at, ) .await?; diff --git a/crates/control-plane-api/src/envelope.rs b/crates/control-plane-api/src/envelope.rs index 3ca7d720771..ef6b28cc57f 100644 --- a/crates/control-plane-api/src/envelope.rs +++ b/crates/control-plane-api/src/envelope.rs @@ -272,7 +272,7 @@ impl axum::extract::FromRequestParts> for Envelope { Ok(Envelope { maybe_claims, retry_after: retry_after.unwrap_or(tokens::DateTime::UNIX_EPOCH), - refresh: state.snapshot.token(), + refresh: state.snapshot_watch.token(), started: started.unwrap_or_else(|| tokens::now()), pg_pool: state.pg_pool.clone(), original_uri, diff --git a/crates/control-plane-api/src/fixtures/attenuated_grants.sql b/crates/control-plane-api/src/fixtures/attenuated_grants.sql new file mode 100644 index 00000000000..432a5035251 --- /dev/null +++ b/crates/control-plane-api/src/fixtures/attenuated_grants.sql @@ -0,0 +1,36 @@ +-- Grant paths whose *raw* legacy capability reaches a data-plane prefix with +-- `admin`, but whose *effective* (attenuated) authority differs. Used to pin +-- that data-plane visibility is decided by effective authority — the exact +-- regression where a filter consults the raw legacy capability of the edge +-- which reached the prefix. +-- +-- Both users traverse the same 2-hop shape through `sharedCo/`: +-- +-- user_grant(user, 'sharedCo/', C, B) -> role_grant('sharedCo/' -> 'ops/dp/public/', 'admin') +-- +-- The role_grant node's effective bits are `admin`'s bits intersected with +-- what the parent may delegate: +-- +-- * erin: 'none' + '{editor}' delegates CatalogRead|JournalRead|SpecEdit|Delegate, +-- which misses ViewDataPlanePrivateNetworking — so she fails the Viewer +-- requirement of `Capability::Read` despite the raw `admin` edge. +-- * frank: 'read' + '{delegate}' delegates the full Viewer set, so the same +-- path *does* authorize him: the positive control proving the traversal +-- works and only attenuation blocks erin. +-- +-- Their own tenants deliberately hold no role_grants: a second path to +-- `ops/dp/public/` would union its bits into the plane node and mask the +-- attenuation under test. +insert into auth.users (id, email) values + ('55555555-5555-5555-5555-555555555555', 'erin@example.com'), + ('66666666-6666-6666-6666-666666666666', 'frank@example.com') +; +insert into public.user_grants (user_id, object_role, capability, bundles) values + ('55555555-5555-5555-5555-555555555555', 'erinCo/', 'admin', '{}'), + ('55555555-5555-5555-5555-555555555555', 'sharedCo/', 'none', '{editor}'), + ('66666666-6666-6666-6666-666666666666', 'frankCo/', 'admin', '{}'), + ('66666666-6666-6666-6666-666666666666', 'sharedCo/', 'read', '{delegate}') +; +insert into public.role_grants (subject_role, object_role, capability, bundles) values + ('sharedCo/', 'ops/dp/public/', 'admin', '{}') +; diff --git a/crates/control-plane-api/src/fixtures/authz_specs.sql b/crates/control-plane-api/src/fixtures/authz_specs.sql new file mode 100644 index 00000000000..d96a7ea9ed5 --- /dev/null +++ b/crates/control-plane-api/src/fixtures/authz_specs.sql @@ -0,0 +1,88 @@ +-- Live specs with *deserializable* models and built specs, for tests that +-- actually load them into a `tables::LiveCatalog` (rather than only reading +-- their names or authorization). `alice.sql` deliberately stores `'{}'` specs, +-- which is enough for name-and-authorization tests but fails to deserialize. +-- +-- `carol` is admin of `carolCo/`; `dan` exists with no grants at all and so +-- models an unauthorized caller. +do $$ +declare + data_plane_one_id flowid := '111111111111'; + + carol_uid uuid := '33333333-3333-3333-3333-333333333333'; + dan_uid uuid := '44444444-4444-4444-4444-444444444444'; + + -- A flowid's high 41 bits are milliseconds since the Estuary epoch, and + -- authorization staleness is decided against that embedded timestamp. These + -- ids are therefore chosen to sit a few days *after* the epoch, so that a + -- Snapshot taken shortly before them is still comfortably after the zero id + -- that a not-yet-published spec resolves to. Spell them out in full: a + -- 12-hex-digit literal is widened to macaddr8 by inserting FF:FE in the + -- middle, which would scramble the timestamp. + collection_id flowid := '00:08:00:00:00:00:04:01'; + capture_id flowid := '00:08:00:00:00:00:04:02'; + materialization_id flowid := '00:08:00:00:00:00:04:03'; + last_pub_id flowid := '00:08:00:00:00:00:00:00'; + +begin + + insert into auth.users (id, email) values + (carol_uid, 'carol@example.com'), + (dan_uid, 'dan@example.com') + ; + -- Dan administers his own tenant but is granted nothing else — not even the + -- shared data-plane — so he is unauthorized to everything under `carolCo/`. + insert into public.user_grants (user_id, object_role, capability) values + (carol_uid, 'carolCo/', 'admin'), + (dan_uid, 'danCo/', 'admin') + ; + -- `carolCo/in/` may write to `carolCo/data/`; `carolCo/out/` is deliberately + -- granted nothing, so a spec under it fails its own read authorization. + insert into public.role_grants (subject_role, object_role, capability) values + ('carolCo/in/', 'carolCo/data/', 'write'), + ('carolCo/', 'ops/dp/public/', 'read') + ; + + perform internal.create_task(collection_id, 1::smallint, '00:00:00:00:00:00:00:00'::flowid); + perform internal.create_task(capture_id, 1::smallint, '00:00:00:00:00:00:00:00'::flowid); + perform internal.create_task(materialization_id, 1::smallint, '00:00:00:00:00:00:00:00'::flowid); + + insert into public.live_specs ( + id, controller_task_id, catalog_name, last_pub_id, spec_type, spec, built_spec, data_plane_id + ) values ( + collection_id, + collection_id, + 'carolCo/data/foo', + last_pub_id, + 'collection', + '{"schema": {"type": "object", "properties": {"id": {"type": "string"}}, "required": ["id"]}, "key": ["/id"]}', + '{"name": "carolCo/data/foo", "writeSchemaJson": "{}", "key": ["/id"], "partitionTemplate": {"name": "carolCo/data/foo/gen1234"}}', + data_plane_one_id + ), ( + capture_id, + capture_id, + 'carolCo/in/capture-foo', + last_pub_id, + 'capture', + '{"endpoint": {"connector": {"image": "source/test:test", "config": {}}}, "bindings": []}', + '{"name": "carolCo/in/capture-foo", "shardTemplate": {"id": "capture/carolCo/in/capture-foo/gen5678"}}', + data_plane_one_id + ), ( + materialization_id, + materialization_id, + 'carolCo/out/materialize-bar', + last_pub_id, + 'materialization', + '{"endpoint": {"connector": {"image": "materialize/test:test", "config": {}}}, "bindings": []}', + '{"name": "carolCo/out/materialize-bar", "shardTemplate": {"id": "materialization/carolCo/out/materialize-bar/gen9012"}}', + data_plane_one_id + ); + + -- The capture writes to the collection, which is what makes it reachable + -- from it via `fetch_expanded_live_specs`. + insert into public.live_spec_flows (source_id, target_id, flow_type) values + (capture_id, collection_id, 'capture') + ; + +end +$$; diff --git a/crates/control-plane-api/src/lib.rs b/crates/control-plane-api/src/lib.rs index 856766a693e..ca16775c095 100644 --- a/crates/control-plane-api/src/lib.rs +++ b/crates/control-plane-api/src/lib.rs @@ -53,7 +53,7 @@ pub use envelope::{Envelope, Locale, MaybeControlClaims}; pub(crate) use server::evaluate_names_authorization; pub use server::{ ApiError, App, AuthZRetry, build_router, - snapshot::{self, Snapshot}, + snapshot::{self, Authorization, Snapshot}, }; // Re-export the GraphQL schema SDL function for flow-client build script diff --git a/crates/control-plane-api/src/live_specs/db.rs b/crates/control-plane-api/src/live_specs/db.rs index 3215710aafc..7da76ae3d50 100644 --- a/crates/control-plane-api/src/live_specs/db.rs +++ b/crates/control-plane-api/src/live_specs/db.rs @@ -1,8 +1,7 @@ use crate::TextJson; -use models::{Capability, CatalogType, Id}; +use models::{CatalogType, Id}; use serde_json::value::RawValue; -use sqlx::types::{Json, Uuid}; -use tables::RoleGrant; +use sqlx::types::Json; /// Deletes the given live spec row, along with the corresponding `controller_jobs` row. pub async fn hard_delete_live_spec(id: Id, txn: &mut sqlx::PgConnection) -> sqlx::Result<()> { @@ -33,30 +32,18 @@ pub struct LiveSpec { pub spec: Option>>, pub built_spec: Option>>, pub inferred_schema_md5: Option, - // User's capability to the specification `catalog_name`. - pub user_capability: Option, - // Capabilities of the specification with respect to other roles. - pub spec_capabilities: Json>, pub dependency_hash: Option, } /// Returns a `LiveSpec` row for each of the given `names`. This will always return a row for each /// name, even if no live spec exists in the database. pub async fn fetch_live_specs( - user_id: Uuid, names: &[String], - fetch_user_capabilities: bool, - fetch_spec_capabilities: bool, db: impl sqlx::Executor<'_, Database = sqlx::Postgres>, ) -> sqlx::Result> { - // The materialized CTE here ensures that `user_roles` is only invoked once, - // and the results used for the rest of the query. - sqlx::query_as!( + let live_spec = sqlx::query_as!( LiveSpec, r#" - with user_roles as materialized ( - select role_prefix, capability from internal.user_roles($1) - ) select coalesce(ls.id, '00:00:00:00:00:00:00:00'::flowid) as "id!: Id", coalesce(ls.last_pub_id, '00:00:00:00:00:00:00:00'::flowid) as "last_pub_id!: Id", @@ -67,31 +54,16 @@ pub async fn fetch_live_specs( ls.spec as "spec: TextJson>", ls.built_spec as "built_spec: TextJson>", ls.inferred_schema_md5, - case when $3 then ( - select max(capability) from user_roles - where starts_with(names, user_roles.role_prefix) - ) else - null - end as "user_capability: Capability", - case when $4 then coalesce( - (select json_agg(row_to_json(role_grants)) - from role_grants - where starts_with(names, subject_role)), - '[]' - ) else - '[]' - end as "spec_capabilities!: Json>", ls.dependency_hash - from unnest($2::text[]) names + from unnest($1::text[]) names left outer join live_specs ls on ls.catalog_name = names "#, - user_id, names, - fetch_user_capabilities, - fetch_spec_capabilities, ) .fetch_all(db) - .await + .await?; + + Ok(live_spec) } pub struct InferredSchemaRow { @@ -122,7 +94,6 @@ pub async fn fetch_inferred_schemas( /// Queries for all non-deleted `live_specs` that are connected to the given `collection_names` via /// `live_spec_flows`. pub async fn fetch_expanded_live_specs( - user_id: Uuid, collection_names: &[&str], exclude_names: &[&str], db: impl sqlx::Executor<'_, Database = sqlx::Postgres>, @@ -132,7 +103,7 @@ pub async fn fetch_expanded_live_specs( r#" with collections(id) as ( select ls.id - from unnest($2::text[]) as names(catalog_name) + from unnest($1::text[]) as names(catalog_name) join live_specs ls on ls.catalog_name = names.catalog_name ), exp(id) as ( @@ -154,22 +125,11 @@ pub async fn fetch_expanded_live_specs( ls.spec as "spec: TextJson>", ls.built_spec as "built_spec: TextJson>", ls.inferred_schema_md5, - ( - select max(capability) from internal.user_roles($1) r - where starts_with(ls.catalog_name, r.role_prefix) - ) as "user_capability: Capability", - coalesce( - (select json_agg(row_to_json(role_grants)) - from role_grants - where starts_with(ls.catalog_name, subject_role)), - '[]' - ) as "spec_capabilities!: Json>", ls.dependency_hash from exp join live_specs ls on ls.id = exp.id - where ls.spec is not null and not ls.catalog_name = any($3); + where ls.spec is not null and not ls.catalog_name = any($2); "#, - user_id, collection_names as &[&str], exclude_names as &[&str], ) diff --git a/crates/control-plane-api/src/live_specs/mod.rs b/crates/control-plane-api/src/live_specs/mod.rs index c2849eb3288..a7863e2d366 100644 --- a/crates/control-plane-api/src/live_specs/mod.rs +++ b/crates/control-plane-api/src/live_specs/mod.rs @@ -1,39 +1,43 @@ mod db; use anyhow::Context; -use models::Capability; -use std::ops::Deref; -use uuid::Uuid; - pub use db::{ InferredSchemaRow, LiveSpec, fetch_expanded_live_specs, fetch_inferred_schemas, fetch_live_spec_names_by_prefix, fetch_live_specs, hard_delete_live_spec, }; +use std::ops::Deref; +use uuid::Uuid; /// Fetches live specs, returning them as a `tables::LiveCatalog`. Optionally /// filters the specs based on user capability. If `filter_capability` is /// `None`, then no filtering will be done. +/// +/// `started_at` anchors the staleness check to the given time (request-relative). +/// When `None`, staleness is anchored to each spec's publication time (spec-relative). +/// A denial from a snapshot older than the anchor is provisional — authority +/// committed before the anchor may be missing from it — and surfaces as a +/// retryable error. A snapshot taken after the anchor is authoritative: it is +/// guaranteed to reflect everything committed before the anchor, but not +/// necessarily changes committed after it. pub async fn get_live_specs( - user_id: Uuid, + user_id: uuid::Uuid, names: &[String], - filter_capability: Option, + filter_capability: Option, db: &sqlx::PgPool, + snapshot: &crate::Snapshot, + started_at: Option, ) -> anyhow::Result { let mut live = tables::LiveCatalog::default(); - // The query that's used by `fetch_live_specs` can be pretty slow because of how - // it queries authZ capabilities for each name, even if it doesn't exist. - // Limit each individual query to 512 names to avoid statement timeouts when - // fetching a large number of specs when `filter_capability` is `Some`. + // Fetch in batches of 512 names. The recursive per-name authorization + // work which originally motivated batching (see #1895) has moved + // in-process, but each returned row still carries unbounded `spec` and + // `built_spec` JSON documents, and a large discover can request thousands + // of names at once. Batching bounds each statement's execution and + // transfer time — keeping every statement clear of `statement_timeout` + // regardless of catalog size — at the cost of a round trip per batch. for names_chunk in names.chunks(512) { - let rows = db::fetch_live_specs( - user_id, - names_chunk, - filter_capability.is_some(), // fetch user capabilities only if needed - false, // we never need spec_capabilities here - db, - ) - .await?; + let rows = db::fetch_live_specs(names_chunk, db).await?; for row in rows { // Spec type might be null because we used to set it to null when deleting specs. // For recently deleted specs, it will still be present. @@ -44,14 +48,19 @@ pub async fn get_live_specs( continue; }; if let Some(min_capability) = filter_capability { - if !row - .user_capability - .is_some_and(|actual_capability| actual_capability >= min_capability) + // For discovers, anchor to the discover request time (started_at). + // For other callers, anchor to the spec's publication time. + // An authoritative denial is today's silent drop; a provisional + // one surfaces as a retryable stale error. + let anchor = started_at.unwrap_or_else(|| row.last_pub_id.timestamp()); + if !snapshot + .user_authorization(user_id, &row.catalog_name, min_capability, Some(anchor)) + .ok_or_stale(&row.catalog_name)? { continue; } } - let built_spec_json = row.built_spec.as_ref().ok_or_else(|| { + let built_spec_json: &Box = row.built_spec.as_ref().ok_or_else(|| { tracing::warn!(catalog_name = %row.catalog_name, id = %row.id, "got row with spec but not built_spec"); anyhow::anyhow!("missing built_spec for {:?}, but spec is non-null", row.catalog_name) })?.deref(); @@ -74,22 +83,34 @@ pub async fn get_live_specs( Ok(live) } +/// Fetches the live specs connected to `collection_names` — tasks that read +/// from or write to them — excluding `exclude_names`. When `filter_capability` +/// is set, specs to which the user lacks that capability are silently omitted: +/// expansion is filtering, so a denial is final regardless of Snapshot +/// freshness and is never surfaced as a retryable stale error. pub async fn get_connected_live_specs( user_id: Uuid, collection_names: &[&str], exclude_names: &[&str], - filter_capability: Option, + filter_capability: Option, db: &sqlx::PgPool, + snapshot: &crate::Snapshot, ) -> anyhow::Result { - let expanded_rows = - db::fetch_expanded_live_specs(user_id, collection_names, exclude_names, db).await?; + let expanded_rows = db::fetch_expanded_live_specs(collection_names, exclude_names, db).await?; let mut live = tables::LiveCatalog::default(); + for exp in expanded_rows { if let Some(minimum_capability) = filter_capability { - if !exp - .user_capability - .map(|c| c >= minimum_capability) - .unwrap_or(false) + // Expansion widens validation with specs the caller never named, so + // a denial is a final omission rather than an error, and never + // consults Snapshot freshness (`None` anchor): the worst case of a + // not-yet-observed grant is only a narrower validation, while an + // anchored check would defer nearly every publication touching a + // connected spec its user can't edit, since the pinned Snapshot + // almost always predates the queued row. + if !snapshot + .user_authorization(user_id, &exp.catalog_name, minimum_capability, None) + .ok_or_stale(&exp.catalog_name)? { continue; } @@ -122,3 +143,209 @@ pub async fn get_connected_live_specs( } Ok(live) } + +/// Both fetchers apply authorization in-process against a `Snapshot` rather than +/// in SQL, but they trust a denial differently. `get_live_specs` fetches specs +/// the caller explicitly named, where a wrongly-dropped spec corrupts the +/// operation's output; because the Snapshot lags Postgres, a denial is only +/// trusted once the Snapshot is authoritative for the operation asking — its +/// `started_at` request time when the caller has a durable one, or the denied +/// spec's own last publication otherwise — and until then the caller gets a +/// retryable `AuthorizationSnapshotStale` rather than a silently-dropped spec. +/// `get_connected_live_specs` expands to specs the caller never named, purely to +/// widen validation, so a denial is always a final silent omission and Snapshot +/// freshness is never consulted. +/// These tests pin the three-way outcome — included / dropped / retryable — for +/// the former, the exact instant the last two swap over, and the two-way +/// outcome for the latter. +#[cfg(test)] +mod tests { + use super::*; + + // From `fixtures/authz_specs.sql`. Carol is admin of `carolCo/`; Dan holds no + // grants at all and so models an unauthorized caller. + const CAROL: uuid::Uuid = uuid::uuid!("33333333-3333-3333-3333-333333333333"); + const DAN: uuid::Uuid = uuid::uuid!("44444444-4444-4444-4444-444444444444"); + const COLLECTION: &str = "carolCo/data/foo"; + const CAPTURE: &str = "carolCo/in/capture-foo"; + + /// Staleness compares the Snapshot's `taken` against the timestamp embedded + /// in a spec's `last_pub_id`, so read that back rather than recomputing it — + /// `flowid` is `macaddr8`, which silently widens short literals. + async fn published_at(pool: &sqlx::PgPool) -> tokens::DateTime { + sqlx::query_scalar!( + r#"select last_pub_id as "last_pub_id: models::Id" + from live_specs where catalog_name = $1"#, + COLLECTION, + ) + .fetch_one(pool) + .await + .expect("fixture collection should exist") + .timestamp() + } + + /// A Snapshot holding the fixture's real grants, stamped `offset` away from + /// the instant the fixture's specs were published. + async fn snapshot_offset(pool: &sqlx::PgPool, offset: chrono::TimeDelta) -> crate::Snapshot { + let mut decrypted_hmac_keys = std::collections::HashMap::new(); + let data = crate::snapshot::try_fetch(pool, &mut decrypted_hmac_keys) + .await + .expect("failed to fetch snapshot"); + crate::Snapshot::new(published_at(pool).await + offset, data) + } + + /// Taken clear of the publication plus `TEMPORAL_SKEW`: denials are definitive. + async fn authoritative(pool: &sqlx::PgPool) -> crate::Snapshot { + snapshot_offset(pool, crate::Snapshot::TEMPORAL_SKEW * 4).await + } + + /// Taken before the publication it would judge: denials are retryable. + async fn stale(pool: &sqlx::PgPool) -> crate::Snapshot { + snapshot_offset(pool, -crate::Snapshot::TEMPORAL_SKEW * 4).await + } + + fn assert_stale_for(err: anyhow::Error, catalog_name: &str) { + assert!( + validation::is_authz_snapshot_stale(&err), + "expected a retryable stale-snapshot error, got: {err:#}" + ); + assert!( + err.to_string().contains(catalog_name), + "stale error should name the offending spec, got: {err:#}" + ); + } + + /// With no capability filter the Snapshot is never consulted, so even a + /// wholly unauthorized caller reading against a stale Snapshot gets the spec. + /// This is the path controllers and other system callers take. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_get_live_specs_unfiltered_never_stale(pool: sqlx::PgPool) { + let snapshot = stale(&pool).await; + let live = get_live_specs(DAN, &[COLLECTION.to_string()], None, &pool, &snapshot, None) + .await + .expect("an unfiltered fetch should not consult the Snapshot"); + + assert_eq!(1, live.collections.len()); + assert_eq!(COLLECTION, live.collections[0].collection.as_str()); + } + + /// An authorized caller gets the spec no matter how old the Snapshot is: + /// staleness only ever converts a *denial* into a retry. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_get_live_specs_authorized_is_included(pool: sqlx::PgPool) { + for snapshot in [stale(&pool).await, authoritative(&pool).await] { + let live = get_live_specs( + CAROL, + &[COLLECTION.to_string()], + Some(models::authz::Capability::CatalogRead.into()), + &pool, + &snapshot, + None, + ) + .await + .expect("carol is admin of carolCo/"); + + assert_eq!(1, live.collections.len()); + } + } + + /// An authoritative denial keeps the pre-existing behavior: the spec is + /// silently omitted rather than raising. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_get_live_specs_authoritative_denial_is_dropped(pool: sqlx::PgPool) { + let snapshot = authoritative(&pool).await; + let live = get_live_specs( + DAN, + &[COLLECTION.to_string()], + Some(models::authz::Capability::CatalogRead.into()), + &pool, + &snapshot, + None, + ) + .await + .expect("an authoritative denial is not an error"); + + assert!( + live.collections.is_empty(), + "an unauthorized spec should be omitted" + ); + } + + /// The new behavior: the same denial, judged by a Snapshot that predates the + /// spec, is retryable instead — the grant that would allow it may simply not + /// have propagated into this Snapshot yet. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_get_live_specs_stale_denial_is_retryable(pool: sqlx::PgPool) { + let snapshot = stale(&pool).await; + let err = get_live_specs( + DAN, + &[COLLECTION.to_string()], + Some(models::authz::Capability::CatalogRead.into()), + &pool, + &snapshot, + None, + ) + .await + .expect_err("a denial against a stale Snapshot should be retryable"); + + assert_stale_for(err, COLLECTION); + } + + /// `get_connected_live_specs` reaches specs by graph traversal rather than by + /// name, and it filters rather than authorizes: an unauthorized spec is + /// silently omitted, and the Snapshot's age never converts that omission + /// into a retryable error. The fixture's capture writes to the collection, + /// so it is reachable from it. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_get_connected_live_specs_filtering(pool: sqlx::PgPool) { + // Exclude the collection itself, leaving just the capture that writes it. + async fn connected( + pool: &sqlx::PgPool, + user: uuid::Uuid, + snapshot: &crate::Snapshot, + filter: Option, + ) -> anyhow::Result { + get_connected_live_specs(user, &[COLLECTION], &[COLLECTION], filter, pool, snapshot) + .await + } + let read_filter = Some(models::authz::CapabilitySet::from( + models::authz::Capability::CatalogRead, + )); + + let live = connected(&pool, CAROL, &authoritative(&pool).await, read_filter) + .await + .expect("carol is authorized"); + assert_eq!(1, live.captures.len()); + assert_eq!(CAPTURE, live.captures[0].capture.as_str()); + + let live = connected(&pool, DAN, &authoritative(&pool).await, read_filter) + .await + .expect("an authoritative denial is not an error"); + assert!(live.captures.is_empty()); + + let live = connected(&pool, DAN, &stale(&pool).await, read_filter) + .await + .expect("a denial filters silently even under a stale Snapshot"); + assert!(live.captures.is_empty()); + + let live = connected(&pool, DAN, &stale(&pool).await, None) + .await + .expect("an unfiltered traversal should not consult the Snapshot"); + assert_eq!(1, live.captures.len()); + } +} diff --git a/crates/control-plane-api/src/publications/initialize.rs b/crates/control-plane-api/src/publications/initialize.rs index b2648fd8180..8eb38a6b69e 100644 --- a/crates/control-plane-api/src/publications/initialize.rs +++ b/crates/control-plane-api/src/publications/initialize.rs @@ -1,16 +1,20 @@ use anyhow::Context; use itertools::Itertools; -use models::Capability; use std::future::Future; use uuid::Uuid; /// Initialize a draft prior to build/validation. This may add additional specs to the draft. +/// +/// `snapshot` is the publication's pinned authorization view; it must be the +/// same Snapshot the subsequent build uses, so that expansion and resolution +/// cannot disagree about one publication. pub trait Initialize: Send + Sync { fn initialize( &self, db: &sqlx::PgPool, user_id: Uuid, draft: &mut tables::DraftCatalog, + snapshot: &crate::Snapshot, ) -> impl Future> + Send; } @@ -22,6 +26,7 @@ impl Initialize for NoopInitialize { _db: &sqlx::PgPool, _user_id: Uuid, _draft: &mut tables::DraftCatalog, + _snapshot: &crate::Snapshot, ) -> anyhow::Result<()> { Ok(()) } @@ -37,20 +42,22 @@ where db: &sqlx::PgPool, user_id: Uuid, draft: &mut tables::DraftCatalog, + snapshot: &crate::Snapshot, ) -> anyhow::Result<()> { - self.0.initialize(db, user_id, draft).await?; - self.1.initialize(db, user_id, draft).await?; + self.0.initialize(db, user_id, draft, snapshot).await?; + self.1.initialize(db, user_id, draft, snapshot).await?; Ok(()) } } /// An `Initialize` that expands the draft to touch live specs that read from or write to /// any drafted collections. This may optionally filter the specs based on whether the user -/// has `admin` capability to them. +/// is authorized to edit them. pub struct ExpandDraft { - /// Whether to filter specs based on the user's capability. If true, then only specs for which - /// the user has `admin` capability will be added to the draft. - pub filter_user_has_admin: bool, + /// Whether to filter specs based on the user's capability. If true, then only specs for + /// which the user holds `SpecEdit` will be added to the draft — matching the capability + /// which publication requires of every drafted spec. + pub filter_user_authz: bool, } impl Initialize for ExpandDraft { @@ -58,13 +65,14 @@ impl Initialize for ExpandDraft { level = "debug", skip_all, err, - fields(filter_user_has_admin = self.filter_user_has_admin) + fields(filter_user_authz = self.filter_user_authz) )] async fn initialize( &self, db: &sqlx::PgPool, user_id: Uuid, draft: &mut tables::DraftCatalog, + snapshot: &crate::Snapshot, ) -> anyhow::Result<()> { // Expand the set of drafted specs to include any tasks that read from or write to any of // the published collections. We do this so that validation can catch any inconsistencies @@ -76,17 +84,16 @@ impl Initialize for ExpandDraft { .collect::>(); let all_drafted_specs = draft.all_spec_names().collect::>(); - let capability_filter = if self.filter_user_has_admin { - Some(Capability::Admin) - } else { - None - }; + let capability_filter = self + .filter_user_authz + .then_some(models::authz::Capability::SpecEdit.into()); let expanded_catalog = crate::live_specs::get_connected_live_specs( user_id, &drafted_collections, &all_drafted_specs, capability_filter, db, + snapshot, ) .await?; tracing::debug!( @@ -116,6 +123,7 @@ impl Initialize for RuntimeV2Rollout { db: &sqlx::PgPool, _user_id: Uuid, draft: &mut tables::DraftCatalog, + _snapshot: &crate::Snapshot, ) -> anyhow::Result<()> { let flag = models::Token::new(models::ENABLE_RUNTIME_V2); diff --git a/crates/control-plane-api/src/publications/mod.rs b/crates/control-plane-api/src/publications/mod.rs index a7bc7056eaf..05da666911e 100644 --- a/crates/control-plane-api/src/publications/mod.rs +++ b/crates/control-plane-api/src/publications/mod.rs @@ -1,11 +1,11 @@ -use std::u32; - use super::logs; +use crate::Snapshot; use anyhow::Context; use chrono::{DateTime, Utc}; use rand::Rng; use sqlx::Executor; use sqlx::types::Uuid; +use std::u32; use tables::BuiltRow; pub mod builds; @@ -29,7 +29,13 @@ use models::draft_error; /// Represents a desire to publish the given `draft`, along with associated metadata and behavior /// for handling draft initialization, build finalizing, and retrying failures. -pub struct DraftPublication { +pub struct DraftPublication< + 's, + Init: Initialize, + Fin: FinalizeBuild, + Ret: RetryPolicy, + C: WithCommit, +> { /// The id of the user that is publishing the draft. pub user_id: Uuid, /// Write logs to `internal.log_lines` using this token. @@ -42,8 +48,25 @@ pub struct DraftPublication, - /// Whether to check user permissions when publishing specs. If this is false, then all - /// permission checks will be skipped, and the publication may modify any specs. + /// The instant this publication was queued, which decides whether an + /// authorization denial is terminal or merely not-yet-observed by the + /// snapshot: a denial counts only once the snapshot was taken after it. + /// + /// This is distinct from `UncommittedBuild::started_at`, which is stamped + /// per build attempt. It must be durable across attempts for the retry to + /// converge, so it comes from the queued `publications` row (`updated_at`). + /// `None` means "no durable instant" — see [`specs::resolve_live_specs`]. + pub started_at: Option, + /// The authorization Snapshot to evaluate this publication against. One + /// pinned Snapshot serves every phase and internal retry of this + /// publication; its freshness relative to `started_at` decides whether a + /// denial is terminal or retryable (see [`specs::resolve_live_specs`]). + pub snapshot: &'s Snapshot, + /// Whether to verify that `user_id` is authorized to the drafted and + /// referenced catalog names, and to the selected data plane. Set `false` + /// by system-initiated publications (controllers, data-plane creation) + /// which are pre-authorized and may touch any spec. Spec-to-spec + /// `RoleGrant` checks are enforced regardless of this setting. pub verify_user_authz: bool, /// Default data plane to use for publishing new specs. This is optional only when the /// publication _only_ updates and/or deletes existing live specs. @@ -141,7 +164,7 @@ impl PublicationResult { } /// A PublishHandler is a Handler which publishes catalog specifications. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct Publisher { flowctl_go: std::path::PathBuf, builds_root: url::Url, @@ -270,16 +293,28 @@ impl Publisher { ))] pub async fn publish( &self, - publication: DraftPublication, + publication: DraftPublication<'_, Ini, Fin, Ret, C>, ) -> anyhow::Result { let mut retry_count = 0u32; loop { // Generate a new id on each attempt, so that we can retry `PublicationSuperseded` // errors with a greater id. let publication_id = self.next_id(); - let result = self + let result = match self .try_publish(publication_id, retry_count, &publication) - .await?; + .await + { + Ok(result) => result, + Err(err) if validation::is_authz_snapshot_stale(&err) => { + // The draft was denied by a Snapshot older than the denial + // freshness anchor, so the required grant may not be reflected + // yet. Request an early refresh and return the retryable error; + // task-based callers reschedule it against a newer Snapshot. + publication.snapshot.revoke.cancel(); + return Err(err); + } + Err(err) => return Err(err), + }; if result.status.is_success() || result.status.is_empty_draft() { return Ok(result); @@ -303,16 +338,19 @@ impl Publisher { draft: raw_draft, verify_user_authz, detail, + started_at, + snapshot, default_data_plane_name, initialize, finalize, retry: _, with_commit, - }: &DraftPublication, + }: &DraftPublication<'_, Ini, Fin, Ret, C>, ) -> anyhow::Result { let mut draft = raw_draft.clone_specs(); + let snapshot = *snapshot; initialize - .initialize(&self.db, *user_id, &mut draft) + .initialize(&self.db, *user_id, &mut draft, snapshot) .await .context("initializing draft")?; // It's important that we generate the pub id inside the retry loop so that we can @@ -327,6 +365,8 @@ impl Publisher { default_data_plane_name.as_deref(), *verify_user_authz, retry_count, + snapshot, + *started_at, ) .await?; finalize.finalize(&mut built).context("finalizing build")?; @@ -350,7 +390,7 @@ impl Publisher { /// Build and verify the given draft. This is `pub` only because we have existing tests that /// use it. If you want to publish something, use the `Publisher::publish` function instead. - #[tracing::instrument(level = "info", skip(self, draft))] + #[tracing::instrument(level = "info", skip(self, draft, snapshot))] pub async fn build( &self, user_id: Uuid, @@ -361,6 +401,8 @@ impl Publisher { explicit_plane_name: Option<&str>, verify_user_authz: bool, retry_count: u32, + snapshot: &crate::Snapshot, + started_at: Option, ) -> anyhow::Result { let start_time = tokens::now(); let build_id = self.id_gen.lock().unwrap().next(); @@ -406,6 +448,8 @@ impl Publisher { &self.db, verify_user_authz, explicit_plane_name, + snapshot, + started_at, ) .await?; diff --git a/crates/control-plane-api/src/publications/specs.rs b/crates/control-plane-api/src/publications/specs.rs index 74387fd44a0..8252cfecf2f 100644 --- a/crates/control-plane-api/src/publications/specs.rs +++ b/crates/control-plane-api/src/publications/specs.rs @@ -727,12 +727,35 @@ pub fn get_ops_collection_names() -> BTreeSet { names } +/// Builds the retryable `AuthorizationSnapshotStale` error returned when an +/// authorization denial was evaluated against a snapshot that isn't yet +/// authoritative for the operation being denied. +/// Resolves the live specs which a draft drafts or references, authorizing each +/// against `snapshot`. +/// +/// `started` is the instant the publication was queued, and decides whether an +/// authorization denial is terminal or merely not-yet-observed: a denial is +/// authoritative only once `snapshot` was taken after it. It must therefore be +/// durable across retries — a value re-stamped per attempt (`now()`) can never +/// be overtaken by a snapshot, so denials would retry forever. +/// +/// `None` is for callers with no such durable instant: controllers and ad-hoc +/// system publications, which construct a fresh publication per attempt and +/// carry their own retry/backoff. They fall back to anchoring on each denied +/// spec's own last publication, which bounds the window in which grants could +/// have been committed alongside the spec. Named data planes have no equivalent +/// fallback timestamp, so their denials remain terminal omissions. +/// +/// `verify_user_authz` skips only user-to-catalog authorization. Specification +/// `RoleGrant` checks remain mandatory. pub async fn resolve_live_specs( - user_id: Uuid, + user_id: uuid::Uuid, draft: &tables::DraftCatalog, db: &sqlx::PgPool, verify_user_authz: bool, explicit_plane_name: Option<&str>, + snapshot: &crate::Snapshot, + started: Option, ) -> anyhow::Result { // We're expecting to get a row for catalog name that's either drafted or referenced // by a drafted spec, even if the live spec does not exist. In that case, the row will @@ -759,15 +782,9 @@ pub async fn resolve_live_specs( } } - let rows = crate::live_specs::fetch_live_specs( - user_id, - &all_spec_names, - verify_user_authz, - true, // always fetch spec capabilities - db, - ) - .await - .context("fetching live specs")?; + let rows = crate::live_specs::fetch_live_specs(&all_spec_names, db) + .await + .context("fetching live specs")?; // Check the user and spec authorizations. // Start by making an easy way to lookup whether each row was drafted or not. @@ -782,14 +799,28 @@ pub async fn resolve_live_specs( let catalog_name = spec_row.catalog_name.as_str(); let n_errors = live.errors.len(); + // Use the queued publication time when available; callers without one + // fall back to the last publication time of the spec. + let freshness_anchor = Some(started.unwrap_or_else(|| spec_row.last_pub_id.timestamp())); + if drafted_names.contains(catalog_name) { // Get the metadata about the draft spec that matches this catalog name. // This must exist in `draft`, otherwise `spec_meta` will panic. let (catalog_type, reads_from, writes_to) = spec_meta(draft, catalog_name); let scope = tables::synthetic_scope(catalog_type, catalog_name); - // If the spec is included in the draft, then the user must have admin capability to it. - if verify_user_authz && !matches!(spec_row.user_capability, Some(Capability::Admin)) { + // If the spec is included in the draft, then the user must be + // authorized to edit it. + if verify_user_authz + && !snapshot + .user_authorization( + user_id, + &spec_row.catalog_name, + models::authz::Capability::SpecEdit, + freshness_anchor, + ) + .ok_or_stale(catalog_name)? + { live.errors.push(tables::Error { scope: scope.clone(), error: anyhow::anyhow!( @@ -800,30 +831,44 @@ pub async fn resolve_live_specs( // of referenced collections. continue; } - // Spec authz must always be checked, even if we're not checking user authz + // Spec authz must always be checked, even if we're not checking user authz. + // These spec-to-spec checks stay on legacy capabilities: they must agree + // with the runtime's task authorization, which enforces the same legacy + // roles when the task actually reads or writes collection journals. for source in reads_from { - if !spec_row.spec_capabilities.iter().any(|c| { - source.starts_with(c.object_role.as_str()) && c.capability >= Capability::Read - }) { + if !snapshot + .role_authorization( + &spec_row.catalog_name, + &source, + Capability::Read, + freshness_anchor, + ) + .ok_or_stale(catalog_name)? + { live.errors.push(tables::Error { scope: scope.clone(), error: anyhow::anyhow!( "Specification '{catalog_name}' is not read-authorized to '{source}'.\nAvailable grants are: {}", - serde_json::to_string_pretty(&spec_row.spec_capabilities.0).unwrap(), + serde_json::to_string_pretty(&snapshot.spec_capabilities(&spec_row.catalog_name)).unwrap(), ), }); } } for target in writes_to { - if !spec_row.spec_capabilities.iter().any(|c| { - target.starts_with(c.object_role.as_str()) - && matches!(c.capability, Capability::Write | Capability::Admin) - }) { + if !snapshot + .role_authorization( + &spec_row.catalog_name, + &target, + Capability::Write, + freshness_anchor, + ) + .ok_or_stale(catalog_name)? + { live.errors.push(tables::Error { scope: scope.clone(), error: anyhow::anyhow!( "Specification is not write-authorized to '{target}'.\nAvailable grants are: {}", - serde_json::to_string_pretty(&spec_row.spec_capabilities.0).unwrap(), + serde_json::to_string_pretty(&snapshot.spec_capabilities(&spec_row.catalog_name)).unwrap(), ), }); } @@ -832,15 +877,19 @@ pub async fn resolve_live_specs( // access capability to them as long as they are not drafted. } else if !ops_collection_names.contains(&spec_row.catalog_name) { // This is a live spec that is not included in the draft. - // The user needs read capability to it because it was referenced by one of the specs - // in their draft. Note that the _user_ does not need `Capability::Write` as long as + // The user needs `CatalogRead` to it because it was referenced by one of the specs + // in their draft. Note that the _user_ does not need any write capability as long as // the _spec_ is authorized to do what it needs. The user just needs to be allowed to // know it exists. if verify_user_authz - && !spec_row - .user_capability - .map(|c| c >= Capability::Read) - .unwrap_or(false) + && !snapshot + .user_authorization( + user_id, + &spec_row.catalog_name, + models::authz::Capability::CatalogRead, + freshness_anchor, + ) + .ok_or_stale(catalog_name)? { let scope = tables::synthetic_scope("unauthorized", &spec_row.catalog_name); live.errors.push(tables::Error { @@ -911,7 +960,7 @@ pub async fn resolve_live_specs( // Fetch data planes that are referenced by live specs (`data_plane_ids`), // or by storage mappings (`data_plane_names`), or by `explicit_plane_name`. - let data_plane_names: Vec<&str> = live + let candidate_data_plane_names: Vec<&str> = live .storage_mappings .iter() .flat_map(|m| m.data_planes.iter().map(String::as_str)) @@ -920,6 +969,21 @@ pub async fn resolve_live_specs( .dedup() .collect(); + let mut data_plane_names = Vec::with_capacity(candidate_data_plane_names.len()); + for name in candidate_data_plane_names { + // Legacy `read` is what conveys deploy-level trust in a data plane; + // no narrow capability bit expresses that trust yet. + if !verify_user_authz + || snapshot + .user_authorization(user_id, name, models::Capability::Read, started) + .ok_or_stale(name)? + { + data_plane_names.push(name); + } + } + + // IDs preserve the assignments of live specs already accepted above. They + // are not user-selected plane names and intentionally bypass this user check. data_plane_ids.sort(); data_plane_ids.dedup(); @@ -932,14 +996,10 @@ pub async fn resolve_live_specs( FROM UNNEST($1::flowid[]) AS t(id) ), data_plane_names AS ( + -- Names have already passed the caller's user-authorization policy, + -- so no in-SQL authorization check is needed here. SELECT name FROM UNNEST($2::text[]) AS t(name) - -- User must be read-authorized to data-plane. - WHERE EXISTS ( - SELECT 1 - FROM internal.user_roles($3, 'read') AS r - WHERE starts_with(t.name, r.role_prefix) - ) ) SELECT d.id AS "control_id: Id", @@ -961,7 +1021,6 @@ pub async fn resolve_live_specs( "#, &data_plane_ids as &[Id], &data_plane_names as &[&str], - user_id as Uuid, ) .fetch_all(db) .await? @@ -1151,3 +1210,685 @@ mod test { } } } + +/// `resolve_live_specs` makes four independent authorization decisions per row — +/// the drafter must hold `SpecEdit` to a drafted spec; a drafted spec must itself +/// be read-authorized to each source and write-authorized to each target; and the +/// user must hold `CatalogRead` to any *referenced* spec. Named data planes add +/// another user-authorization decision. Each denial is evaluated against a `Snapshot` +/// and short-circuits with retryable `AuthorizationSnapshotStale` when that +/// Snapshot is not authoritative for the operation. +/// +/// These tests pin both halves of every branch: what a stale Snapshot returns, +/// and the (unchanged) error text an authoritative one reports. +#[cfg(test)] +mod resolve_tests { + use super::*; + + // From `fixtures/authz_specs.sql`. + const CAROL: uuid::Uuid = uuid::uuid!("33333333-3333-3333-3333-333333333333"); + const DAN: uuid::Uuid = uuid::uuid!("44444444-4444-4444-4444-444444444444"); + // From `fixtures/attenuated_grants.sql`. + const ERIN: uuid::Uuid = uuid::uuid!("55555555-5555-5555-5555-555555555555"); + const FRANK: uuid::Uuid = uuid::uuid!("66666666-6666-6666-6666-666666666666"); + const COLLECTION: &str = "carolCo/data/foo"; + const CAPTURE: &str = "carolCo/in/capture-foo"; + const MATERIALIZATION: &str = "carolCo/out/materialize-bar"; + const PLANE: &str = "ops/dp/public/aws-us-west-2-c1"; + + fn draft_of(catalog_json: serde_json::Value) -> tables::DraftCatalog { + let catalog: models::Catalog = + serde_json::from_value(catalog_json).expect("failed to parse catalog"); + tables::DraftCatalog::from(catalog) + } + + /// A materialization drafted under `carolCo/out/`, which holds no grants and + /// so is not read-authorized to `sources`. + fn materialization_draft(sources: &[&str]) -> tables::DraftCatalog { + draft_of(serde_json::json!({ + "materializations": { + MATERIALIZATION: { + "endpoint": { "connector": { "image": "materialize/test:test", "config": {} } }, + "bindings": sources.iter().map(|source| serde_json::json!({ + "resource": { "table": "t" }, + "source": source, + })).collect::>(), + } + } + })) + } + + /// A capture drafted under `carolCo/in/`, which may write to `carolCo/data/` + /// but nowhere else. + fn capture_draft(targets: &[&str]) -> tables::DraftCatalog { + draft_of(serde_json::json!({ + "captures": { + CAPTURE: { + "endpoint": { "connector": { "image": "source/test:test", "config": {} } }, + "bindings": targets.iter().map(|target| serde_json::json!({ + "resource": { "id": "r" }, + "target": target, + })).collect::>(), + } + } + })) + } + + /// Staleness compares the Snapshot's `taken` against the timestamp embedded + /// in a spec's `last_pub_id`, so read that back rather than recomputing it — + /// `flowid` is `macaddr8`, which silently widens short literals. + async fn published_at(pool: &sqlx::PgPool) -> tokens::DateTime { + sqlx::query_scalar!( + r#"select last_pub_id as "last_pub_id: models::Id" + from live_specs where catalog_name = $1"#, + COLLECTION, + ) + .fetch_one(pool) + .await + .expect("fixture collection should exist") + .timestamp() + } + + async fn snapshot_offset(pool: &sqlx::PgPool, offset: chrono::TimeDelta) -> crate::Snapshot { + let mut decrypted_hmac_keys = std::collections::HashMap::new(); + let data = crate::snapshot::try_fetch(pool, &mut decrypted_hmac_keys) + .await + .expect("failed to fetch snapshot"); + crate::Snapshot::new(published_at(pool).await + offset, data) + } + + /// Taken clear of the publication plus `TEMPORAL_SKEW`: denials are definitive. + async fn authoritative(pool: &sqlx::PgPool) -> crate::Snapshot { + snapshot_offset(pool, crate::Snapshot::TEMPORAL_SKEW * 4).await + } + + /// Taken before the publication it would judge: denials are retryable. + async fn stale(pool: &sqlx::PgPool) -> crate::Snapshot { + snapshot_offset(pool, -crate::Snapshot::TEMPORAL_SKEW * 4).await + } + + /// Renders `live.errors` as `(scope, message)` pairs for snapshot assertions. + fn error_pairs(live: &tables::LiveCatalog) -> Vec<(String, String)> { + live.errors + .iter() + .map(|e| (e.scope.to_string(), format!("{:#}", e.error))) + .collect() + } + + fn assert_stale_for(err: anyhow::Error, catalog_name: &str) { + assert!( + validation::is_authz_snapshot_stale(&err), + "expected a retryable stale-snapshot error, got: {err:#}" + ); + assert!( + err.to_string().contains(catalog_name), + "stale error should name the offending spec, got: {err:#}" + ); + } + + /// Branch 1: a user drafting an existing spec must hold `SpecEdit` to it. + /// Dan does not, but the denial is only definitive once the Snapshot + /// outlives the spec. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_drafted_spec_requires_spec_edit(pool: sqlx::PgPool) { + let draft = draft_of(serde_json::json!({ + "collections": { + COLLECTION: { + "schema": { "type": "object", "properties": { "id": { "type": "string" } } }, + "key": ["/id"] + } + } + })); + + let err = resolve_live_specs(DAN, &draft, &pool, true, None, &stale(&pool).await, None) + .await + .expect_err("a denial against a stale Snapshot should be retryable"); + assert_stale_for(err, COLLECTION); + + let live = resolve_live_specs( + DAN, + &draft, + &pool, + true, + None, + &authoritative(&pool).await, + None, + ) + .await + .expect("an authoritative denial is reported, not raised"); + insta::assert_debug_snapshot!(error_pairs(&live), @r#" + [ + ( + "flow://collection/carolCo/data/foo", + "User is not authorized to create or change this catalog name", + ), + ] + "#); + } + + /// Branch 2: a drafted spec must itself be read-authorized to each source. + /// Carol admins the whole tenant, so the user check passes and only the + /// *spec's* own role grants are at issue. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_drafted_spec_reads_from_authz(pool: sqlx::PgPool) { + let draft = materialization_draft(&[COLLECTION]); + + let err = resolve_live_specs(CAROL, &draft, &pool, true, None, &stale(&pool).await, None) + .await + .expect_err("a denial against a stale Snapshot should be retryable"); + assert_stale_for(err, MATERIALIZATION); + + let live = resolve_live_specs( + CAROL, + &draft, + &pool, + true, + None, + &authoritative(&pool).await, + None, + ) + .await + .expect("an authoritative denial is reported, not raised"); + // The rendered grant list comes from `Snapshot::spec_capabilities`, which + // replaced a SQL-computed column; pin it so the two can't drift. + insta::assert_debug_snapshot!(error_pairs(&live), @r#" + [ + ( + "flow://materialization/carolCo/out/materialize-bar", + "Specification 'carolCo/out/materialize-bar' is not read-authorized to 'carolCo/data/foo'.\nAvailable grants are: [\n {\n \"subject_role\": \"carolCo/\",\n \"object_role\": \"ops/dp/public/\",\n \"capability\": \"read\",\n \"bundles\": []\n }\n]", + ), + ] + "#); + } + + /// Branch 3: a drafted spec must be write-authorized to each target. + /// `carolCo/in/` may write to `carolCo/data/` but nowhere else. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_drafted_spec_writes_to_authz(pool: sqlx::PgPool) { + let draft = capture_draft(&["carolCo/elsewhere/thing"]); + + let err = resolve_live_specs(CAROL, &draft, &pool, true, None, &stale(&pool).await, None) + .await + .expect_err("a denial against a stale Snapshot should be retryable"); + assert_stale_for(err, CAPTURE); + + let live = resolve_live_specs( + CAROL, + &draft, + &pool, + true, + None, + &authoritative(&pool).await, + None, + ) + .await + .expect("an authoritative denial is reported, not raised"); + insta::assert_debug_snapshot!(error_pairs(&live), @r#" + [ + ( + "flow://capture/carolCo/in/capture-foo", + "Specification is not write-authorized to 'carolCo/elsewhere/thing'.\nAvailable grants are: [\n {\n \"subject_role\": \"carolCo/\",\n \"object_role\": \"ops/dp/public/\",\n \"capability\": \"read\",\n \"bundles\": []\n },\n {\n \"subject_role\": \"carolCo/in/\",\n \"object_role\": \"carolCo/data/\",\n \"capability\": \"write\",\n \"bundles\": []\n }\n]", + ), + ] + "#); + } + + /// The write-authorized target resolves cleanly, confirming the branch above + /// fails for the reason claimed rather than incidentally. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_authorized_draft_resolves_without_errors(pool: sqlx::PgPool) { + let draft = capture_draft(&[COLLECTION]); + + for snapshot in [stale(&pool).await, authoritative(&pool).await] { + let live = resolve_live_specs(CAROL, &draft, &pool, true, Some(PLANE), &snapshot, None) + .await + .expect("an authorized draft resolves"); + + assert!( + live.errors.is_empty(), + "unexpected errors: {:?}", + error_pairs(&live) + ); + assert_eq!(1, live.captures.len()); + assert_eq!(1, live.collections.len()); + assert_eq!( + vec![PLANE], + live.data_planes + .iter() + .map(|d| d.data_plane_name.as_str()) + .collect::>(), + ); + } + } + + /// Branch 4: a *referenced* (non-drafted) spec only requires `CatalogRead`. + /// Dan admins `danCo/`, so his own drafted spec passes, and the denial lands + /// on `carolCo/data/foo` — which, being an existing spec, can be stale. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_referenced_spec_requires_catalog_read(pool: sqlx::PgPool) { + let draft = draft_of(serde_json::json!({ + "materializations": { + "danCo/materialize-x": { + "endpoint": { "connector": { "image": "materialize/test:test", "config": {} } }, + "bindings": [ { "resource": { "table": "t" }, "source": COLLECTION } ], + } + } + })); + + let err = resolve_live_specs(DAN, &draft, &pool, true, None, &stale(&pool).await, None) + .await + .expect_err("a denial against a stale Snapshot should be retryable"); + assert_stale_for(err, COLLECTION); + + let live = resolve_live_specs( + DAN, + &draft, + &pool, + true, + None, + &authoritative(&pool).await, + None, + ) + .await + .expect("an authoritative denial is reported, not raised"); + insta::assert_debug_snapshot!(error_pairs(&live), @r#" + [ + ( + "flow://unauthorized/carolCo/data/foo", + "User is not authorized to read this catalog name", + ), + ( + "flow://materialization/danCo/materialize-x", + "Specification 'danCo/materialize-x' is not read-authorized to 'carolCo/data/foo'.\nAvailable grants are: []", + ), + ] + "#); + } + + /// Without a durable request timestamp (`started: None`), a brand-new spec + /// falls back to its zero `last_pub_id` as the freshness anchor, so its + /// denial is terminal against any Snapshot — this keeps such a first + /// publication from looping instead of reporting its error. This holds + /// only for the `None` fallback: a queued publication supplies `started`, + /// which replaces the anchor and can make the same denial retryable. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_new_spec_denial_without_started_is_terminal(pool: sqlx::PgPool) { + let draft = draft_of(serde_json::json!({ + "collections": { + "carolCo/data/brand-new": { + "schema": { "type": "object", "properties": { "id": { "type": "string" } } }, + "key": ["/id"] + } + } + })); + + let live = resolve_live_specs(DAN, &draft, &pool, true, None, &stale(&pool).await, None) + .await + .expect("without a request anchor, a spec with no publication history cannot be stale"); + insta::assert_debug_snapshot!(error_pairs(&live), @r#" + [ + ( + "flow://collection/carolCo/data/brand-new", + "User is not authorized to create or change this catalog name", + ), + ] + "#); + } + + /// Spec-level (`reads_from` / `writes_to`) checks remain active when user + /// authorization is skipped. This test pins that stale denials from those + /// checks still propagate as retryable errors. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_spec_authz_staleness_applies_without_user_authz(pool: sqlx::PgPool) { + let draft = capture_draft(&["carolCo/elsewhere/thing"]); + + let err = resolve_live_specs( + uuid::Uuid::nil(), + &draft, + &pool, + false, // verify_user_authz + None, + &stale(&pool).await, + None, + ) + .await + .expect_err("spec authorization is checked regardless of verify_user_authz"); + assert_stale_for(err, CAPTURE); + } + + /// Named data planes use the publication's durable `started` timestamp as + /// their freshness anchor. Grants win regardless of Snapshot age, while a + /// denial is retryable only until the Snapshot becomes authoritative. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_data_plane_name_authorization_freshness(pool: sqlx::PgPool) { + let dan_draft = draft_of(serde_json::json!({ + "collections": { + "danCo/thing": { + "schema": { "type": "object", "properties": { "id": { "type": "string" } } }, + "key": ["/id"] + } + } + })); + let started = published_at(&pool).await; + let stale_snapshot = stale(&pool).await; + + // Dan admins `danCo/` but was granted nothing on `ops/dp/public/`. + // Because this Snapshot is not authoritative for `started`, its denial + // is provisional and names the plane which triggered it. + let err = resolve_live_specs( + DAN, + &dan_draft, + &pool, + true, + Some(PLANE), + &stale_snapshot, + Some(started), + ) + .await + .expect_err("a stale data-plane denial should be retryable"); + assert_stale_for(err, PLANE); + + // Once the Snapshot is authoritative, the same denial preserves the + // existing non-disclosure behavior and silently omits the plane. + let live = resolve_live_specs( + DAN, + &dan_draft, + &pool, + true, + Some(PLANE), + &authoritative(&pool).await, + Some(started), + ) + .await + .expect("an authoritative data-plane denial is terminal omission"); + assert!( + live.errors.is_empty(), + "unexpected errors: {:?}", + error_pairs(&live) + ); + assert!( + live.data_planes.is_empty(), + "an authoritatively denied data-plane should be omitted" + ); + + // Carol holds `carolCo/ -> ops/dp/public/ read`, so the same plane is + // included even though the Snapshot is too old to make denials final. + let carol_draft = draft_of(serde_json::json!({ + "collections": { + "carolCo/thing": { + "schema": { "type": "object", "properties": { "id": { "type": "string" } } }, + "key": ["/id"] + } + } + })); + let live = resolve_live_specs( + CAROL, + &carol_draft, + &pool, + true, + Some(PLANE), + &stale_snapshot, + Some(started), + ) + .await + .expect("an observed grant wins regardless of Snapshot age"); + assert_eq!(1, live.data_planes.len()); + + // System publications skip user authorization for named planes as well + // as catalog specs. Spec-to-spec RoleGrant checks remain mandatory. + let live = resolve_live_specs( + DAN, + &dan_draft, + &pool, + false, + Some(PLANE), + &stale_snapshot, + Some(started), + ) + .await + .expect("verify_user_authz=false should include the named plane"); + assert_eq!(1, live.data_planes.len()); + + // Callers without a durable operation timestamp must not invent one: + // their denials preserve the prior terminal omission behavior. + let live = resolve_live_specs( + DAN, + &dan_draft, + &pool, + true, + Some(PLANE), + &stale_snapshot, + None, + ) + .await + .expect("a plane denial without a freshness anchor is terminal"); + assert!(live.data_planes.is_empty()); + } + + /// Storage-mapping plane names follow the same freshness policy even when + /// there is no explicit/default plane name in the publication. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("data_planes", "authz_specs")) + )] + async fn test_storage_mapping_data_plane_authorization_freshness(pool: sqlx::PgPool) { + let mapping = crate::TextJson(models::StorageDef { + data_planes: vec![PLANE.to_string()], + stores: vec![models::Store::example()], + }); + sqlx::query("insert into storage_mappings (catalog_prefix, spec) values ($1, $2)") + .bind("danCo/") + .bind(&mapping) + .execute(&pool) + .await + .expect("failed to insert test storage mapping"); + + let draft = draft_of(serde_json::json!({ + "collections": { + "danCo/thing": { + "schema": { "type": "object", "properties": { "id": { "type": "string" } } }, + "key": ["/id"] + } + } + })); + let err = resolve_live_specs( + DAN, + &draft, + &pool, + true, + None, + &stale(&pool).await, + Some(published_at(&pool).await), + ) + .await + .expect_err("a stale storage-mapping plane denial should be retryable"); + assert_stale_for(err, PLANE); + } + + /// The data-plane name filter must be decided by *effective* (attenuated) + /// authority, not the raw legacy capability of the edge which reached the + /// prefix. Erin and frank traverse the identical 2-hop path through + /// `sharedCo/` to a raw-`admin` grant on `ops/dp/public/`; only frank's + /// root grant delegates the Viewer bits, so only frank sees the plane. A + /// regression to raw-capability filtering makes the plane visible to erin. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures( + path = "../fixtures", + scripts("data_planes", "authz_specs", "attenuated_grants") + ) + )] + async fn test_attenuated_data_plane_grant_is_not_visible(pool: sqlx::PgPool) { + let snapshot = authoritative(&pool).await; + + // The premise that makes this attenuation rather than simple absence: + // erin's raw reachable capability at the plane is Admin, and yet her + // effective authority does not satisfy Read. + assert_eq!( + Some(models::Capability::Admin), + tables::UserGrant::get_user_capability( + &snapshot.role_grants, + &snapshot.user_grants, + ERIN, + PLANE, + ), + ); + assert!(!tables::UserGrant::is_authorized( + &snapshot.role_grants, + &snapshot.user_grants, + ERIN, + PLANE, + models::Capability::Read, + )); + + let erin_draft = draft_of(serde_json::json!({ + "collections": { + "erinCo/thing": { + "schema": { "type": "object", "properties": { "id": { "type": "string" } } }, + "key": ["/id"] + } + } + })); + let live = resolve_live_specs(ERIN, &erin_draft, &pool, true, Some(PLANE), &snapshot, None) + .await + .expect("an unauthorized data-plane name is not an error"); + assert!( + live.errors.is_empty(), + "unexpected errors: {:?}", + error_pairs(&live) + ); + assert!( + live.data_planes.is_empty(), + "a plane reached with raw admin but attenuated effective authority must not be visible" + ); + + let frank_draft = draft_of(serde_json::json!({ + "collections": { + "frankCo/thing": { + "schema": { "type": "object", "properties": { "id": { "type": "string" } } }, + "key": ["/id"] + } + } + })); + let live = resolve_live_specs( + FRANK, + &frank_draft, + &pool, + true, + Some(PLANE), + &snapshot, + None, + ) + .await + .expect("frank is authorized to the plane"); + assert!( + live.errors.is_empty(), + "unexpected errors: {:?}", + error_pairs(&live) + ); + assert_eq!(1, live.data_planes.len()); + } + + /// Scenario 2: Request-relative staleness anchoring allows retries when the + /// snapshot predates the request, even if the spec is old. This is the + /// "old-spec late-grant" case: a grant might exist but arrive in the system + /// after the snapshot was taken but before the request was queued. + /// + /// This test shows that with request-relative anchoring, a denial is: + /// - Retried if snapshot.taken_before(request_start) (grant might exist but not in snapshot) + /// - Terminal if snapshot.taken_after(request_start) (grant would be in snapshot if it existed) + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("authz_specs")) + )] + async fn test_old_spec_stale_snapshot_relative_to_request(pool: sqlx::PgPool) { + let draft = capture_draft(&[CAPTURE]); + + // A snapshot taken well before "now" is stale relative to any request + // queued around "now". This should trigger a retry even though the spec + // itself is old. + let stale_snapshot = stale(&pool).await; + let now = published_at(&pool).await + chrono::TimeDelta::seconds(3600); + + let err = resolve_live_specs( + uuid::Uuid::nil(), + &draft, + &pool, + false, + None, + &stale_snapshot, + // Request was queued at `now`, well after the stale snapshot. + Some(now), + ) + .await + .expect_err("spec authorization required even without user authz"); + + // The denial should be stale relative to the request time, so retryable. + assert_stale_for(err, CAPTURE); + } + + /// When the snapshot is authoritative relative to the request start time, + /// an authorization denial is terminal (not retried), even for an old spec. + /// This shows the request-relative anchor is properly applied. + #[sqlx::test( + migrations = "../../supabase/migrations", + fixtures(path = "../fixtures", scripts("authz_specs")) + )] + async fn test_old_spec_authoritative_snapshot_relative_to_request(pool: sqlx::PgPool) { + let draft = capture_draft(&[CAPTURE]); + + // An authoritative snapshot is taken at published_at + TEMPORAL_SKEW * 4. + let authoritative_snapshot = authoritative(&pool).await; + let pub_time = published_at(&pool).await; + // Request queued just before the snapshot. Since snapshot is at pub_time + 1s, + // queuing at pub_time means snapshot.taken_after(now) is true (snapshot is authoritative). + let now = pub_time; + + let live = resolve_live_specs( + uuid::Uuid::nil(), + &draft, + &pool, + false, + None, + &authoritative_snapshot, + // Request was queued at `now`, before the authoritative snapshot. + Some(now), + ) + .await + .expect("resolve should not error with authoritative snapshot"); + + // The denial should be terminal (not stale) because the snapshot is + // authoritative relative to the request start time. The capture spec + // lacks authorization, so we get a hard error, not a retry. + assert!(!live.errors.is_empty(), "expected authorization denial"); + let error = &live.errors.iter().next().unwrap().error; + assert!( + !validation::is_authz_snapshot_stale(error), + "error should not be stale-snapshot error" + ); + } +} diff --git a/crates/control-plane-api/src/server/create_data_plane.rs b/crates/control-plane-api/src/server/create_data_plane.rs index 15ca92f9802..cacdd507887 100644 --- a/crates/control-plane-api/src/server/create_data_plane.rs +++ b/crates/control-plane-api/src/server/create_data_plane.rs @@ -232,12 +232,18 @@ pub async fn create_data_plane( .unwrap() .into(); + let snapshot = app.snapshot_watch.token(); let publication = DraftPublication { user_id: *user_id, logs_token: insert.logs_token, draft, dry_run: false, detail: Some(format!("publication for data-plane {base_name}")), + // A one-shot handler invocation, with no queued row to anchor on. + started_at: None, + snapshot: snapshot + .result() + .expect("authorization snapshot is not ready"), // We've already validated that the user can admin `ops/`, // so further authZ checks are unnecessary. verify_user_authz: false, diff --git a/crates/control-plane-api/src/server/mod.rs b/crates/control-plane-api/src/server/mod.rs index e3eaef0fe71..406393688d2 100644 --- a/crates/control-plane-api/src/server/mod.rs +++ b/crates/control-plane-api/src/server/mod.rs @@ -39,7 +39,7 @@ pub struct App { pub control_plane_jwt_encode_key: tokens::jwt::EncodingKey, pub pg_pool: sqlx::PgPool, pub publisher: crate::publications::Publisher, - pub snapshot: Arc>, + pub snapshot_watch: Arc>, /// Signing secret for verifying inbound Stripe webhook deliveries. `None` /// when unconfigured, in which case the webhook endpoint fails closed rather /// than trusting any request. See `server::public::stripe_webhooks`. @@ -53,7 +53,7 @@ impl App { jwt_secret: &[u8], pg_pool: sqlx::PgPool, publisher: crate::publications::Publisher, - snapshot: Arc>, + snapshot_watch: Arc>, stripe_webhook_secret: Option, ) -> Self { Self { @@ -63,7 +63,7 @@ impl App { control_plane_jwt_encode_key: tokens::jwt::EncodingKey::from_secret(jwt_secret), pg_pool, publisher, - snapshot, + snapshot_watch, stripe_webhook_secret, } } diff --git a/crates/control-plane-api/src/server/snapshot.rs b/crates/control-plane-api/src/server/snapshot.rs index 9c81400bfc0..2281a60ae3e 100644 --- a/crates/control-plane-api/src/server/snapshot.rs +++ b/crates/control-plane-api/src/server/snapshot.rs @@ -73,6 +73,37 @@ pub struct SnapshotTask { pub data_plane_id: models::Id, } +/// Outcome of an authorization check evaluated against a Snapshot, +/// classified by `Snapshot::resolve_authorization`. +#[must_use] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Authorization { + /// The required grant exists in the Snapshot. + Authorized, + /// The grant is absent and the Snapshot is authoritative for the + /// operation's anchor: the denial is final. + Denied, + /// The grant is absent but the Snapshot predates the anchor: a grant + /// committed before the anchor may not be reflected yet, so the denial is + /// provisional and the operation should retry under a fresher Snapshot. + Stale, +} + +impl Authorization { + /// Collapse to "is authorized?", surfacing a provisional denial as the + /// retryable `AuthorizationSnapshotStale` error which callers + /// (see `validation::is_authz_snapshot_stale`) convert into a retry. + pub fn ok_or_stale(self, catalog_name: &str) -> Result { + match self { + Authorization::Authorized => Ok(true), + Authorization::Denied => Ok(false), + Authorization::Stale => Err(validation::Error::AuthorizationSnapshotStale { + catalog_name: catalog_name.to_string(), + }), + } + } +} + // SnapshotMigration is the state of an underway data-plane migration. #[derive(Clone, Debug, serde::Serialize, serde::Deserialize)] pub struct SnapshotMigration { @@ -181,6 +212,68 @@ impl Snapshot { self.taken > (started + Self::TEMPORAL_SKEW) } + /// Classify an already-evaluated authorization check against this + /// Snapshot's freshness: the single three-way policy — authorized / + /// authoritative denial / provisional denial — applied at every snapshot + /// authorization enforcement point. + /// + /// A denial is `Denied` only when this Snapshot was taken after `anchor`, + /// the instant the asking operation started: any grant committed before + /// the anchor is then necessarily reflected. Otherwise it is `Stale` — + /// possibly just unobserved. `None` means the caller has no instant to + /// anchor a staleness claim on, so denials are final. + pub fn resolve_authorization( + &self, + authorized: bool, + anchor: Option, + ) -> Authorization { + if authorized { + Authorization::Authorized + } else if anchor.is_none_or(|anchor| self.taken_after(anchor)) { + Authorization::Denied + } else { + Authorization::Stale + } + } + + /// Evaluate whether `user_id` holds `capability` to `name` under this + /// Snapshot's grants, classified against `anchor` freshness + /// (see `resolve_authorization`). + pub fn user_authorization( + &self, + user_id: uuid::Uuid, + name: &str, + capability: impl Into, + anchor: Option, + ) -> Authorization { + self.resolve_authorization( + tables::UserGrant::is_authorized( + &self.role_grants, + &self.user_grants, + user_id, + name, + capability, + ), + anchor, + ) + } + + /// Evaluate whether `subject` (a catalog spec acting as a role) holds + /// `capability` to `object` under this Snapshot's role grants, classified + /// against `anchor` freshness (see `resolve_authorization`). + pub fn role_authorization( + &self, + subject: &str, + object: &str, + capability: impl Into, + anchor: Option, + ) -> Authorization { + self.resolve_authorization( + tables::RoleGrant::is_authorized(&self.role_grants, subject, object, capability), + anchor, + ) + } + // Retrieve all tasks whose names start with the given `prefix`. pub fn tasks_by_prefix<'s>( &'s self, @@ -341,9 +434,26 @@ impl Snapshot { }) } + /// Returns the "spec capabilities" of a spec named `catalog_name`: the role + /// grants whose `subject_role` is a prefix of the name — the capabilities the + /// spec holds by virtue of its own name/role. This is only to be used for error + /// reporting to improve error messages. + pub fn spec_capabilities(&self, catalog_name: &str) -> Vec { + self.role_grants + .iter() + .filter(|grant| catalog_name.starts_with(grant.subject_role.as_str())) + .cloned() + .collect() + } + // Minimal interval between Snapshot refreshes. // We will postpone a requested refresh prior to this interval. pub const MIN_REFRESH_INTERVAL: chrono::TimeDelta = chrono::TimeDelta::seconds(20); + /// Re-poll cadence for a queued task which is deferring until its + /// Snapshot is authoritative (see `taken_after`). This equals + /// `MIN_REFRESH_INTERVAL` because that's the soonest a refresh can land: + /// waking sooner burns polls, waking later delays the task. + pub const STALE_RETRY_WAKE: chrono::TimeDelta = Self::MIN_REFRESH_INTERVAL; // Maximum interval between Snapshot refreshes. // We will refresh an older Snapshot in the background. pub const MAX_REFRESH_INTERVAL: chrono::TimeDelta = chrono::TimeDelta::minutes(5); @@ -837,4 +947,187 @@ mod tests { chrono::DateTime::from_timestamp(300_000, 0).unwrap() ); } + + /// `taken_after` is the single definition of "this Snapshot is authoritative + /// for that instant", and every authorization-staleness decision routes + /// through it. The `TEMPORAL_SKEW` allowance and the strictness of the + /// comparison are therefore load-bearing, so pin both. + #[test] + fn test_taken_after_allows_for_temporal_skew() { + let started = chrono::DateTime::from_timestamp(1_000_000, 0).unwrap(); + let at = |offset: chrono::TimeDelta| Snapshot { + taken: started + offset, + ..Snapshot::empty() + }; + + assert!( + !at(chrono::TimeDelta::zero()).taken_after(started), + "a Snapshot taken at the same instant is not authoritative" + ); + assert!( + !at(-Snapshot::TEMPORAL_SKEW).taken_after(started), + "a Snapshot taken before the event is not authoritative" + ); + assert!( + !at(Snapshot::TEMPORAL_SKEW).taken_after(started), + "the skew allowance is exclusive: exactly TEMPORAL_SKEW later is still not authoritative" + ); + assert!( + at(Snapshot::TEMPORAL_SKEW + chrono::TimeDelta::milliseconds(1)).taken_after(started), + "one millisecond past the skew allowance is authoritative" + ); + } + + /// `resolve_authorization` is the shared three-way classifier behind every + /// snapshot authorization enforcement point. Pin its anchor semantics — + /// a denial is authoritative only under a Snapshot postdating the anchor, + /// and a `None` anchor makes denials final — and `ok_or_stale`'s collapse + /// into authorized / dropped / retryable. + #[test] + fn test_resolve_authorization() { + let anchor = chrono::DateTime::from_timestamp(1_000_000, 0).unwrap(); + let stale = Snapshot { + taken: anchor, + ..Snapshot::empty() + }; + let fresh = Snapshot { + taken: anchor + Snapshot::TEMPORAL_SKEW * 2, + ..Snapshot::empty() + }; + + // A held grant is Authorized regardless of freshness. + assert_eq!( + Authorization::Authorized, + stale.resolve_authorization(true, Some(anchor)) + ); + assert_eq!( + Authorization::Authorized, + stale.resolve_authorization(true, None) + ); + + // A denial is authoritative only under a Snapshot postdating the anchor. + assert_eq!( + Authorization::Denied, + fresh.resolve_authorization(false, Some(anchor)) + ); + assert_eq!( + Authorization::Stale, + stale.resolve_authorization(false, Some(anchor)) + ); + + // Without an anchor there is no basis for a staleness claim. + assert_eq!( + Authorization::Denied, + stale.resolve_authorization(false, None) + ); + + assert!(matches!( + Authorization::Authorized.ok_or_stale("acmeCo/task"), + Ok(true) + )); + assert!(matches!( + Authorization::Denied.ok_or_stale("acmeCo/task"), + Ok(false) + )); + assert!(matches!( + Authorization::Stale.ok_or_stale("acmeCo/task"), + Err(validation::Error::AuthorizationSnapshotStale { catalog_name }) + if catalog_name == "acmeCo/task" + )); + } + + /// `spec_capabilities` replaced a SQL-computed `spec_capabilities` column and + /// now renders the "Available grants are:" list in publication authorization + /// errors. It answers "what may a spec named X do, by virtue of its own + /// name?", which is a prefix match on `subject_role` — not on `object_role`, + /// and not scoped to any user. + #[test] + fn test_spec_capabilities() { + let snapshot = Snapshot::build_fixture(None); + let subjects = |name: &str| { + snapshot + .spec_capabilities(name) + .into_iter() + .map(|g| { + ( + g.subject_role.to_string(), + g.object_role.to_string(), + g.capability, + ) + }) + .collect::>() + }; + + // A name under a granted prefix picks up every grant whose subject_role + // is a prefix of it — here both the tenant-wide grants and the more + // specific `bobCo/tires/` one. + insta::assert_debug_snapshot!(subjects("bobCo/tires/source-tread"), @r#" + [ + ( + "bobCo/", + "bobCo/", + Write, + ), + ( + "bobCo/", + "ops/dp/public/", + Read, + ), + ( + "bobCo/tires/", + "acmeCo/shared/", + Read, + ), + ] + "#); + + // The narrower `bobCo/tires/` grant must not leak to a sibling prefix: + // subject matching is by prefix of the *name*, not by shared tenancy. + insta::assert_debug_snapshot!(subjects("bobCo/widgets/source-squash"), @r#" + [ + ( + "bobCo/", + "bobCo/", + Write, + ), + ( + "bobCo/", + "ops/dp/public/", + Read, + ), + ] + "#); + + // `subject_role` is matched as a prefix of the name, so the role itself + // qualifies. + assert_eq!( + vec![( + "bobCo/tires/".to_string(), + "acmeCo/shared/".to_string(), + models::Capability::Read + )], + subjects("bobCo/tires/") + .into_iter() + .filter(|(s, _, _)| s == "bobCo/tires/") + .collect::>(), + ); + + // Grants are not matched by their object_role: `acmeCo/shared/` is + // reachable *from* `bobCo/tires/`, but a spec named `acmeCo/shared/x` + // holds only `acmeCo/`'s own grants. + insta::assert_debug_snapshot!(subjects("acmeCo/shared/thing"), @r#" + [ + ( + "acmeCo/", + "acmeCo/", + Write, + ), + ] + "#); + + assert!( + subjects("unknownCo/thing").is_empty(), + "a name under no granted prefix holds nothing" + ); + } } diff --git a/crates/control-plane-api/src/server/update_l2_reporting.rs b/crates/control-plane-api/src/server/update_l2_reporting.rs index 9bf0e42d082..f7fc33ec5a0 100644 --- a/crates/control-plane-api/src/server/update_l2_reporting.rs +++ b/crates/control-plane-api/src/server/update_l2_reporting.rs @@ -295,12 +295,18 @@ export class Derivation extends Types.IDerivation {"# }; let logs_token = uuid::Uuid::new_v4(); + let snapshot = app.snapshot_watch.token(); let publication = DraftPublication { user_id: *user_id, logs_token, draft, dry_run, detail: Some(format!("publication for updating L2 reporting")), + // A one-shot handler invocation, with no queued row to anchor on. + started_at: None, + snapshot: snapshot + .result() + .expect("authorization snapshot is not ready"), default_data_plane_name: if default_data_plane.trim().is_empty() { None } else { diff --git a/crates/validation/src/errors.rs b/crates/validation/src/errors.rs index 63fa9b2198f..187a5d17df6 100644 --- a/crates/validation/src/errors.rs +++ b/crates/validation/src/errors.rs @@ -299,6 +299,10 @@ pub enum Error { build_id: models::Id, larger_id: models::Id, }, + #[error( + "authorization for {catalog_name} was evaluated against a control-plane snapshot that is not authoritative for this operation; please retry the operation" + )] + AuthorizationSnapshotStale { catalog_name: String }, #[error( "This spec was updated while you were editing — please refresh and re-apply your changes.\nThis may have been an automated system update. (expected publication ID {expect_id}, actual {actual_id})" )] @@ -413,3 +417,15 @@ impl Error { errors.insert_row(scope.flatten(), anyhow::anyhow!(self)); } } + +/// Returns true if `err` is (or wraps) an [`Error::AuthorizationSnapshotStale`]. +/// This classifies a *retryable* authorization failure: the decision was made +/// against a control-plane snapshot that is not authoritative for the operation, +/// so it should be retried against a fresher snapshot rather than surfaced as a +/// terminal error. +pub fn is_authz_snapshot_stale(err: &anyhow::Error) -> bool { + matches!( + err.downcast_ref::(), + Some(Error::AuthorizationSnapshotStale { .. }) + ) +} diff --git a/crates/validation/src/lib.rs b/crates/validation/src/lib.rs index 7c154e44081..4c5bafe6843 100644 --- a/crates/validation/src/lib.rs +++ b/crates/validation/src/lib.rs @@ -17,7 +17,7 @@ mod schema; mod storage_mapping; mod test_step; -pub use errors::Error; +pub use errors::{Error, is_authz_snapshot_stale}; pub use noop::NoOpConnectors; /// Combiner slots reserved for runtime-internal bindings (today: the capture