From d0ea1ce598f63aebc3198b3e5b2bbc21b254cb83 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 8 Sep 2026 15:56:03 +0200 Subject: [PATCH] fix(datatables): close the last ways a role or a pointer can be left pointing at nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The raw settings readers hand back whatever is in the row, so moving the catalog into its own `global_settings` key protected the config machinery and left `GET /settings/global/datatable_roles` and the settings listing returning every live password. Both now filter that one key. The neighbouring `custom_instance_replication_pwd` has the same shape and is not touched here: it predates this and widening the fix to it is a decision about an operator workflow, not a consequence of this change. Three ways a save could leave something resolving to nothing: A permissioned data table could be moved to a PostgreSQL resource. The block was carried across as a server-owned field, the runtime refuses roles on a resource-backed table, so the save succeeded and every job afterwards failed. Refused instead — turning roles off first is one step, and it keeps discarding an access decision something somebody chose. Renaming a governing data table left every fork pointing at the old name: the data table disappears from their pickers and their jobs stop, with nothing in the renaming workspace to suggest why. The rename now follows into the pointers in the same transaction. Deleting one cannot be followed the same way, so it is reported instead — the response names what it stranded, the way deleting a workspace does, and the fork's own error already says which workspace is gone. Also: `ensure_instance_db_grant_options_unchecked` claimed superadmin while the permissions handler reaches it as a workspace admin (the same class fixed last commit, one instance missed); the role entry kept an `instance_config_schema` derive it no longer needs; `write_role_catalog` was the one writer of that table not stamping `updated_at`; and the concurrency test dropped its roles only on success — a failing run is exactly the one that creates them without recording them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR --- ...0239c625f85e25b3364bf2edf566f518c8ee2.json | 29 +++++++ ...de094c7ab23330fdbc74d6e4ad472ddd3c820.json | 15 ++++ ...025ca9181a0c396405984d87e422e129521c1.json | 16 ++++ .../tests/datatable_roles.rs | 80 ++++++++++++++++--- backend/windmill-api-settings/src/lib.rs | 9 ++- .../windmill-api-workspaces/src/workspaces.rs | 76 +++++++++++++++++- .../windmill-common/src/datatable_roles.rs | 19 ++++- backend/windmill-common/src/lib.rs | 3 +- 8 files changed, 229 insertions(+), 18 deletions(-) create mode 100644 backend/.sqlx/query-538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2.json create mode 100644 backend/.sqlx/query-975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820.json create mode 100644 backend/.sqlx/query-da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1.json diff --git a/backend/.sqlx/query-538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2.json b/backend/.sqlx/query-538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2.json new file mode 100644 index 0000000000..03613519ad --- /dev/null +++ b/backend/.sqlx/query-538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2.json @@ -0,0 +1,29 @@ +{ + "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 AND dt.value->'reference'->>'datatable' = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "workspace_id!", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "datatable!", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + null + ] + }, + "hash": "538dd1779874e4003932d7f17750239c625f85e25b3364bf2edf566f518c8ee2" +} diff --git a/backend/.sqlx/query-975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820.json b/backend/.sqlx/query-975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820.json new file mode 100644 index 0000000000..87666986a3 --- /dev/null +++ b/backend/.sqlx/query-975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO global_settings (name, value) VALUES ($1, $2)\n ON CONFLICT (name) DO UPDATE SET value = $2, updated_at = now()", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Jsonb" + ] + }, + "nullable": [] + }, + "hash": "975e404ac3a6258bb8220e122e3de094c7ab23330fdbc74d6e4ad472ddd3c820" +} diff --git a/backend/.sqlx/query-da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1.json b/backend/.sqlx/query-da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1.json new file mode 100644 index 0000000000..4e63a2afa6 --- /dev/null +++ b/backend/.sqlx/query-da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings ws\n SET datatable = (\n SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg(\n dt.key,\n CASE WHEN dt.value->'reference'->>'workspace_id' = $1\n AND dt.value->'reference'->>'datatable' = $2\n THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text))\n ELSE dt.value END\n ))\n FROM jsonb_each(ws.datatable->'datatables') dt\n )\n WHERE jsonb_typeof(ws.datatable->'datatables') = 'object'\n AND ws.datatable::text LIKE '%\"reference\"%'", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "da5057c86607327bafc2942c218025ca9181a0c396405984d87e422e129521c1" +} diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 3101739f29..08cc43d0d4 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -358,24 +358,78 @@ async fn concurrent_role_creations_both_survive(db: Pool) -> anyhow::R let body = resp.text().await?; Ok::<_, anyhow::Error>((status, body)) }; - let (a, b) = tokio::join!(create(names[0].clone()), create(names[1].clone())); - let (a, b) = (a?, b?); - assert_eq!(a.0, 200, "{}", a.1); - assert_eq!(b.0, 200, "{}", b.1); + let outcome = async { + let (a, b) = tokio::join!(create(names[0].clone()), create(names[1].clone())); + let (a, b) = (a?, b?); + assert_eq!(a.0, 200, "{}", a.1); + assert_eq!(b.0, 200, "{}", b.1); - let catalog = windmill_common::datatable_roles::read_role_catalog(&db).await?; - let recorded: Vec<&str> = catalog.values().map(|r| r.name.as_str()).collect(); - for name in &names { - assert!( - recorded.contains(&name.as_str()), - "{name} is a live cluster login the catalog forgot: {recorded:?}" - ); + let catalog = windmill_common::datatable_roles::read_role_catalog(&db).await?; + let recorded: Vec<&str> = catalog.values().map(|r| r.name.as_str()).collect(); + for name in &names { + assert!( + recorded.contains(&name.as_str()), + "{name} is a live cluster login the catalog forgot: {recorded:?}" + ); + } + Ok::<_, anyhow::Error>(()) } + .await; + // Roles are cluster-wide, so they outlive this test's throwaway database. Dropped whatever + // happened above — a failing run is exactly the one that created them and did not record them. for name in &names { - sqlx::query(&format!("DROP ROLE IF EXISTS \"{name}\"")) + let _ = sqlx::query(&format!("DROP ROLE IF EXISTS \"{name}\"")) .execute(&db) - .await?; + .await; } + outcome +} + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn renaming_a_governing_data_table_carries_its_forks(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + // A pointer names the governing data table by name, so a rename that does not follow leaves + // every fork resolving to nothing — the data table vanishes from their pickers and their jobs + // stop, with nothing in the renaming workspace to suggest why. + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/test-workspace/workspaces/edit_datatable_config" + )), + "SECRET_TOKEN", + ) + .json(&json!({ + "settings": {"datatables": {"renamed": { + "database": {"resource_type": "instance", "resource_path": "dt_main"} + }}}, + "renames": [{"from": "main", "to": "renamed"}], + "deleted_datatables": [] + })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + + let entry: Option = sqlx::query_scalar( + "SELECT datatable->'datatables'->'main' FROM workspace_settings WHERE workspace_id = $1", + ) + .bind("wm-fork-dt") + .fetch_one(&db) + .await?; + let entry = entry.unwrap(); + assert_eq!(entry["reference"]["datatable"], "renamed", "{entry}"); + + // And it still resolves, which is the thing the fork actually cares about. + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/wm-fork-dt/workspaces/datatable_usable_roles/main" + )), + "SECRET_TOKEN_2", + ) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); Ok(()) } diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index d5d584bd9b..92dad9a4be 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -1356,6 +1356,7 @@ pub async fn get_global_setting( .await? .map(|x| x.value); + let value = value.map(|v| windmill_common::datatable_roles::redact_role_catalog_setting(&key, v)); Ok(Json(value.unwrap_or_else(|| serde_json::Value::Null))) } @@ -1393,7 +1394,13 @@ async fn list_global_settings( require_super_admin(&db, &authed).await?; let settings = sqlx::query_as!(GlobalSetting, "SELECT name, value FROM global_settings") .fetch_all(&db) - .await?; + .await? + .into_iter() + .map(|s| GlobalSetting { + value: windmill_common::datatable_roles::redact_role_catalog_setting(&s.name, s.value), + name: s.name, + }) + .collect(); Ok(Json(settings)) } diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index f8dcc5c579..11ab0c7f9b 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3670,6 +3670,21 @@ async fn edit_datatable_config( dt.permissions = old.and_then(|old| old.permissions.clone()); dt.reference = old.and_then(|old| old.reference.clone()); dt.forked_from = old.and_then(|old| old.forked_from.clone()); + // Carrying the block onto a resource-backed entry would produce a data table the chokepoint + // refuses on every job — a save that succeeds and breaks everything afterwards. Refuse it + // instead: turning roles off first is one step, and it keeps discarding an access decision + // something somebody chose rather than a side effect of moving a database. + if dt.permissions.is_some() + && dt + .database + .as_ref() + .is_some_and(|d| d.resource_type != DataTableCatalogResourceType::Instance) + { + return Err(Error::BadRequest(format!( + "Data table '{name}' is under roles, which only a data table on the instance \ + database can be. Turn its roles off before moving it to a PostgreSQL resource." + ))); + } // A pointer names no database of its own, so the form's empty `database` is correct there. if dt.reference.is_some() { dt.database = None; @@ -3736,6 +3751,53 @@ async fn edit_datatable_config( ) .await?; + // A fork points at a data table by name, so a rename here has to follow or every fork's entry + // resolves to nothing. Inside the transaction: the rename and the pointers that name it are one + // change, and half of it is a fork whose jobs stop. + for r in &new_config.renames { + sqlx::query!( + r#"UPDATE workspace_settings ws + SET datatable = ( + SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg( + dt.key, + CASE WHEN dt.value->'reference'->>'workspace_id' = $1 + AND dt.value->'reference'->>'datatable' = $2 + THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text)) + ELSE dt.value END + )) + FROM jsonb_each(ws.datatable->'datatables') dt + ) + WHERE jsonb_typeof(ws.datatable->'datatables') = 'object' + AND ws.datatable::text LIKE '%"reference"%'"#, + &w_id, + &r.from, + &r.to, + ) + .execute(&mut *tx) + .await?; + } + + // A deletion cannot be followed the same way — there is nothing to point at any more. Read who + // is left stranded so the caller is told, the way deleting a workspace does. + let mut stranded: Vec = Vec::new(); + for name in &new_config.deleted_datatables { + let rows = 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 + AND dt.value->'reference'->>'datatable' = $2"#, + &w_id, + name, + ) + .fetch_all(&mut *tx) + .await?; + stranded.extend( + rows.into_iter() + .map(|r| format!("{}/{}", r.workspace_id, r.datatable)), + ); + } + tx.commit().await?; for substrate in created_substrates { @@ -3750,7 +3812,19 @@ async fn edit_datatable_config( ) .await?; - Ok(format!("Edit datatable config for workspace {}", &w_id)) + if stranded.is_empty() { + Ok(format!("Edit datatable config for workspace {}", &w_id)) + } else { + Ok(format!( + concat!( + "Edit datatable config for workspace {}. These data tables were governed by one ", + "you deleted and no longer resolve: {}. Their databases still exist; a superadmin ", + "can point them at another workspace's data table." + ), + &w_id, + stranded.join(", ") + )) + } } #[derive(Deserialize)] diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 3b0b012bb8..4090baee75 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -37,7 +37,6 @@ pub const CUSTOM_INSTANCE_USER: &str = "custom_instance_user"; /// One catalog entry. The password is per role and instance-wide, and lives in the instance's own /// [`DATATABLE_ROLES_SETTING`] row rather than in any workspace's settings. #[derive(Deserialize, Serialize, Clone)] -#[cfg_attr(feature = "instance_config_schema", derive(schemars::JsonSchema))] pub struct InstanceDatatableRole { /// The Postgres role name, verbatim. pub name: String, @@ -115,6 +114,22 @@ fn quote_literal(value: &str) -> String { format!("'{}'", value.replace('\'', "''")) } +/// The catalog as a settings reader may see it: every entry, no password. +/// +/// `GET /settings/global/{key}` and the settings listing hand back whatever is in the row, so the +/// one key whose value is a set of live cluster credentials has to be filtered on the way out. +/// Applied by name, since those endpoints do not know what they are returning. +pub fn redact_role_catalog_setting(name: &str, value: serde_json::Value) -> serde_json::Value { + if name != DATATABLE_ROLES_SETTING { + return value; + } + let mut catalog = parse_role_catalog(Some(value)); + for role in catalog.values_mut() { + role.pwd = None; + } + serde_json::to_value(&catalog).unwrap_or(serde_json::Value::Null) +} + /// Serialize every mutation of the catalog, from the read through the cluster DDL to the write. /// /// The catalog is one JSON document, so create/rename/enable/delete are all read-modify-write. @@ -171,7 +186,7 @@ pub async fn write_role_catalog( .map_err(|e| Error::internal_err(format!("serializing the role catalog: {e}")))?; sqlx::query!( "INSERT INTO global_settings (name, value) VALUES ($1, $2) - ON CONFLICT (name) DO UPDATE SET value = $2", + ON CONFLICT (name) DO UPDATE SET value = $2, updated_at = now()", DATATABLE_ROLES_SETTING, value ) diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index 774da9ecdc..a7e84c55d7 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -1541,7 +1541,8 @@ fn instance_db_grants(dbname: &str) -> String { /// the database and `public` schema owner — since only it can hand out an option it holds. /// /// Authorization: reaches an instance database with the server's own credentials and checks -/// nothing. Callers MUST restrict this to superadmin or internal server paths. +/// nothing. Callers MUST have authorized administration of `dbname` — superadmin, or an admin of +/// the workspace governing a data table on it. pub async fn ensure_instance_db_grant_options_unchecked(db: &DB, dbname: &str) -> error::Result<()> { let dbname = dbname.trim(); validate_dbname(dbname)?;