diff --git a/backend/.sqlx/query-c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba.json b/backend/.sqlx/query-c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba.json new file mode 100644 index 0000000000..5d8ca8dc00 --- /dev/null +++ b/backend/.sqlx/query-c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba.json @@ -0,0 +1,28 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"datatable!\"\n FROM workspace_settings ws\n CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt\n WHERE dt.value->'reference'->>'workspace_id' = $1\n ORDER BY ws.workspace_id, dt.key", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id!", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "datatable!", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false, + null + ] + }, + "hash": "c5451ea9d9fa5146af242d1ee8c19ebd65b80e7ed9f29b9fb2e03767c2aa94ba" +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 0b0ca1f71c..7ec93dd159 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -260,3 +260,72 @@ async fn a_fork_renaming_its_own_entry_leaves_the_governing_bookkeeping_alone( assert_eq!(left, 1, "the fork's delete reached the parent's migrations"); Ok(()) } + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_caller_who_is_not_a_member_of_the_governing_workspace_reaches_nothing( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + // A fork member who was never added to the parent. Their fork membership says nothing there, + // and the email lookup that would evaluate them as a member of it finds no row. + sqlx::query( + "INSERT INTO usr (workspace_id, email, username, is_admin, role) + VALUES ('wm-fork-dt', 'test3@windmill.dev', 'test-user-3', false, 'User')", + ) + .execute(&db) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/wm-fork-dt/workspaces/datatable_usable_roles/main" + )), + "SECRET_TOKEN_3", + ) + .send() + .await?; + assert_eq!(resp.status(), 200); + let body: Value = resp.json().await?; + assert_eq!(body["roles"], json!([]), "{body}"); + Ok(()) +} + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_caller_with_no_identity_reaches_a_permissioned_data_table_not_at_all( + db: Pool, +) -> anyhow::Result<()> { + use windmill_common::workspaces::{get_datatable_resource_from_db, DatatableAccess}; + + initialize_tracing().await; + // The compatibility story for an agent worker that predates data table roles and sends no job + // id: it keeps resolving an unpermissioned data table, and is refused on a permissioned one + // rather than handed an unattributed admin connection. + let refused = get_datatable_resource_from_db( + &db, + "test-workspace", + "main", + None, + DatatableAccess::NoIdentity, + ) + .await; + assert!(refused.is_err(), "an unidentified caller was let in"); + + sqlx::query( + "UPDATE workspace_settings + SET datatable = datatable #- '{datatables,main,permissions}' + WHERE workspace_id = 'test-workspace'", + ) + .execute(&db) + .await?; + let resolved = get_datatable_resource_from_db( + &db, + "test-workspace", + "main", + None, + DatatableAccess::NoIdentity, + ) + .await?; + assert_eq!(resolved["dbname"], "dt_main", "{resolved}"); + Ok(()) +} diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index a4a0ec11c3..6a87e2f9e1 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -2635,18 +2635,31 @@ fn datatable_role_infos( /// Persist the catalog next to the instance Postgres password, in the same `global_settings` row /// the instance database registry lives in. +/// +/// Errors when it matches nothing. The cluster is written first, so a silent no-op here would +/// leave a live Postgres login with a password nobody recorded: invisible to the catalog, +/// un-recreatable (the name is taken) and un-deletable (there is no entry to delete). The row is +/// normally planted by the boot converge, but that swallows its own failures, so this is a check +/// rather than an assumption. async fn write_role_catalog( db: &DB, catalog: &windmill_common::datatable_roles::DatatableRoleCatalog, ) -> error::Result<()> { let value = serde_json::to_value(catalog).map_err(to_anyhow)?; - sqlx::query!( + let written = sqlx::query!( "UPDATE global_settings SET value = jsonb_set(COALESCE(value, '{}'::jsonb), '{roles}', $1) WHERE name = 'custom_instance_pg_databases'", value ) .execute(db) - .await?; + .await? + .rows_affected(); + if written == 0 { + return Err(error::Error::internal_err( + "The instance Postgres settings row is missing, so the data table role catalog could not be recorded. Refresh the custom instance user password in instance settings to recreate it, then try again." + .to_string(), + )); + } Ok(()) } diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 4da6d24624..61b4cc39be 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -995,6 +995,22 @@ pub(crate) async fn delete_workspace( // but the destructive cleanup itself runs only after the commit below: a delete that // fails mid-way must never leave a live workspace with its fork data destroyed and no // registry row to retry from. Read-only: nothing is dropped here. + // Read before the delete: another workspace's data table entry can point at one of this + // workspace's, and deleting the workspace it names leaves that pointer resolving to nothing. + // Nothing sweeps them — turning them back into copies would hand each fork the database + // outright — so the deleter is told which data tables they just stranded. + let stranded_pointers = sqlx::query!( + r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!" + FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt + WHERE dt.value->'reference'->>'workspace_id' = $1 + ORDER BY ws.workspace_id, dt.key"#, + &w_id, + ) + .fetch_all(&db) + .await + .unwrap_or_default(); + let fork_ducklake_cleanups = prepare_fork_ducklake_cleanups(&db, &w_id, None) .await .unwrap_or_else(|e| { @@ -1313,7 +1329,19 @@ pub(crate) async fn delete_workspace( tracing::warn!("failed to broadcast fork lineage change: {e:#}"); } - Ok(format!("Deleted workspace {}", &w_id)) + if stranded_pointers.is_empty() { + Ok(format!("Deleted workspace {}", &w_id)) + } else { + let stranded = stranded_pointers + .iter() + .map(|r| format!("{}/{}", r.workspace_id, r.datatable)) + .collect::>() + .join(", "); + Ok(format!( + "Deleted workspace {}. These data tables were governed by it and no longer resolve: {}. Their databases still exist; a superadmin can point them at another workspace's data table.", + &w_id, stranded + )) + } } #[derive(Deserialize)] diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 9efa05eca9..1a2f0df96b 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -36,7 +36,7 @@ pub const CUSTOM_INSTANCE_USER: &str = "custom_instance_user"; /// One catalog entry. The password is per role and instance-wide; it lives here rather than in any /// workspace's settings, next to the `custom_instance_user` password in the same /// `custom_instance_pg_databases` row. -#[derive(Deserialize, Serialize, Clone, Debug)] +#[derive(Deserialize, Serialize, Clone)] #[cfg_attr(feature = "instance_config_schema", derive(schemars::JsonSchema))] pub struct InstanceDatatableRole { /// The Postgres role name, verbatim. @@ -49,6 +49,18 @@ pub struct InstanceDatatableRole { pub pwd: Option, } +/// Hand-written so `{:?}` on a catalog cannot put a live Postgres password in a log line or an +/// audit record. Everything else about the entry is safe to print. +impl std::fmt::Debug for InstanceDatatableRole { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("InstanceDatatableRole") + .field("name", &self.name) + .field("enabled", &self.enabled) + .field("pwd", &self.pwd.as_ref().map(|_| "")) + .finish() + } +} + pub type DatatableRoleCatalog = BTreeMap; /// Names Postgres or Windmill already owns. `admin` is excluded because it never reaches the diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 0dcddf5600..2d5c6a4901 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1477,8 +1477,24 @@ pub async fn resolve_governing_datatable( ) -> Result { let mut workspace_id = w_id.to_string(); let mut name = name.to_string(); + let mut hops = 0; for _ in 0..DATATABLE_REFERENCE_MAX_DEPTH { - let datatable = read_datatable_entry(db, &workspace_id, &name).await?; + let datatable = read_datatable_entry(db, &workspace_id, &name) + .await + .map_err(|e| { + if hops == 0 { + e + } else { + // A pointer outlives the workspace it names: deleting one only nulls the fork + // lineage, it does not sweep the entries that pointed at it. Say which one is + // gone rather than reporting a data table this workspace never had. + Error::NotFound(format!( + "Data table '{name}' of workspace '{workspace_id}' governs this one and no \ + longer exists. A superadmin can point this data table somewhere else." + )) + } + })?; + hops += 1; validate_datatable_shape(&name, &datatable)?; match &datatable.reference { None => return Ok(GoverningDatatable { workspace_id, name, datatable }),