From c63bfa86c20b498eeee34368b98d541f42c36a10 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Sat, 5 Sep 2026 00:07:12 +0200 Subject: [PATCH] fix(datatables): opting in reaches the forks made before it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fork made while a data table was unpermissioned carries a verbatim copy of the config, still pointing at this workspace's database. Opting in never reached that copy, so the fork went on resolving through the connection that owns everything there — for every member of the fork, including members this workspace does not have. The fork strip only covers forks made after the opt-in. Enabling permissions now converts those copies into pointers, over the whole descendant tree. Conservative by construction: a copy is taken only when it still names this workspace's database, was not cloned into one of its own, and the fork has not opted in on it itself — the other three are data tables the fork owns. It runs after the save commits, one fork at a time under that fork's own settings lock, so it cannot deadlock against a save there; it is idempotent, and the next save retries whatever failed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S5arH3G2Sa1Qqm32veJQ1n --- ...fd7187d6a678cbdb6dee545b468504285f50d.json | 16 ++ ...7dab50857f2a8b2f4339043c396dda0f53d14.json | 22 +++ .../src/datatable_permissions.rs | 145 ++++++++++++++++++ 3 files changed, 183 insertions(+) create mode 100644 backend/.sqlx/query-be9524e1876ccd3dcf5dab5dfc5fd7187d6a678cbdb6dee545b468504285f50d.json create mode 100644 backend/.sqlx/query-d4605c688b499dafa266075701b7dab50857f2a8b2f4339043c396dda0f53d14.json diff --git a/backend/.sqlx/query-be9524e1876ccd3dcf5dab5dfc5fd7187d6a678cbdb6dee545b468504285f50d.json b/backend/.sqlx/query-be9524e1876ccd3dcf5dab5dfc5fd7187d6a678cbdb6dee545b468504285f50d.json new file mode 100644 index 0000000000..3c77739d74 --- /dev/null +++ b/backend/.sqlx/query-be9524e1876ccd3dcf5dab5dfc5fd7187d6a678cbdb6dee545b468504285f50d.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(\n datatable #- ARRAY['datatables', $2],\n '{shared_datatables}',\n COALESCE(CASE WHEN jsonb_typeof(datatable->'shared_datatables') = 'object'\n THEN datatable->'shared_datatables' ELSE '{}'::jsonb END, '{}'::jsonb)\n || jsonb_build_object($2::text, jsonb_build_object('from', $3::text)))\n WHERE workspace_id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "be9524e1876ccd3dcf5dab5dfc5fd7187d6a678cbdb6dee545b468504285f50d" +} diff --git a/backend/.sqlx/query-d4605c688b499dafa266075701b7dab50857f2a8b2f4339043c396dda0f53d14.json b/backend/.sqlx/query-d4605c688b499dafa266075701b7dab50857f2a8b2f4339043c396dda0f53d14.json new file mode 100644 index 0000000000..fd641445c5 --- /dev/null +++ b/backend/.sqlx/query-d4605c688b499dafa266075701b7dab50857f2a8b2f4339043c396dda0f53d14.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "\n WITH RECURSIVE descendants AS (\n SELECT id, 0 AS depth FROM workspace WHERE id = $1\n UNION ALL\n SELECT w.id, descendants.depth + 1\n FROM workspace w JOIN descendants ON w.parent_workspace_id = descendants.id\n WHERE descendants.depth < 20\n )\n SELECT id AS \"id!\" FROM descendants WHERE depth > 0\n ", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id!", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "d4605c688b499dafa266075701b7dab50857f2a8b2f4339043c396dda0f53d14" +} diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 6587cf90e0..2e0ab06947 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -737,6 +737,117 @@ pub(crate) async fn ensure_can_use_datatable_role( /// List the roles `authed` may run this data table as. An unpermissioned data /// table reports `enabled: false` and no roles, so a picker can hide itself. +/// Whether a fork's entry is a copy of this workspace's data table and nothing +/// more — the only shape [`point_forks_at_this_datatable`] may take away. +/// +/// A clone points at a database of its own (`forked_from`), a repointed copy at +/// someone else's, and a copy the fork has opted in on has roles in a database +/// this workspace does not own. None of those are this workspace's to convert. +fn is_inherited_copy(entry: &serde_json::Value, database: &serde_json::Value) -> bool { + entry.get("forked_from").is_none() + && entry.get("permissions").is_none() + && entry.get("database") == Some(database) +} + +/// Turn every fork's inherited copy of a data table into a pointer at this one. +/// +/// A fork made while the data table was unpermissioned carries a verbatim copy +/// of the config, pointing at this workspace's database. Opting in never reached +/// that copy, so the fork went on resolving the database through its own +/// connection — the one that owns everything in it — for every member of the +/// fork, including members this workspace does not have. The fork strip covers +/// forks made after the opt-in; this covers the ones made before it. +/// +/// Deliberately conservative. A copy is converted only when it still names this +/// workspace's database and was not cloned into one of its own (`forked_from`), +/// and never when the fork has opted in on it itself: those roles live in a +/// database this does not own, and a pointer would orphan them. +/// +/// Runs after the save has committed, one fork at a time under that fork's own +/// settings lock, so it can neither deadlock against a save there nor hold this +/// workspace's row while it waits. Idempotent, and the next save retries what +/// failed — until then a fork that was not converted keeps the access it already +/// had. +async fn point_forks_at_this_datatable(db: &DB, w_id: &str, datatable_name: &str) { + let database = match read_datatable_unchecked(db, w_id, datatable_name).await { + Ok(datatable) => match serde_json::to_value(&datatable.database) { + Ok(database) => database, + Err(e) => { + tracing::error!("reading the database of {datatable_name} in {w_id}: {e:#}"); + return; + } + }, + Err(e) => { + tracing::error!("reading {datatable_name} of {w_id}: {e:#}"); + return; + } + }; + let forks = sqlx::query_scalar!( + r#" + WITH RECURSIVE descendants AS ( + SELECT id, 0 AS depth FROM workspace WHERE id = $1 + UNION ALL + SELECT w.id, descendants.depth + 1 + FROM workspace w JOIN descendants ON w.parent_workspace_id = descendants.id + WHERE descendants.depth < 20 + ) + SELECT id AS "id!" FROM descendants WHERE depth > 0 + "#, + w_id, + ) + .fetch_all(db) + .await; + let forks = match forks { + Ok(forks) => forks, + Err(e) => { + tracing::error!("listing the forks of {w_id}: {e:#}"); + return; + } + }; + for fork in forks { + let converted = async { + let mut tx = db.begin().await?; + let inherited = windmill_common::workspaces::lock_workspace_settings_unchecked( + &mut tx, &fork, + ) + .await? + .and_then(|s| s.get("datatables")?.get(datatable_name).cloned()) + .filter(|entry| is_inherited_copy(entry, &database)); + if inherited.is_none() { + return Ok::<_, Error>(false); + } + sqlx::query!( + "UPDATE workspace_settings + SET datatable = jsonb_set( + datatable #- ARRAY['datatables', $2], + '{shared_datatables}', + COALESCE(CASE WHEN jsonb_typeof(datatable->'shared_datatables') = 'object' + THEN datatable->'shared_datatables' ELSE '{}'::jsonb END, '{}'::jsonb) + || jsonb_build_object($2::text, jsonb_build_object('from', $3::text))) + WHERE workspace_id = $1", + &fork, + datatable_name, + w_id, + ) + .execute(&mut *tx) + .await?; + tx.commit().await?; + Ok(true) + } + .await; + match converted { + Ok(true) => tracing::info!( + "fork {fork} now points at data table {datatable_name} of {w_id} \ + instead of holding a copy of its config" + ), + Ok(false) => {} + Err(e) => tracing::error!( + "pointing fork {fork} at data table {datatable_name} of {w_id}: {e:#}" + ), + } + } +} + async fn list_usable_datatable_roles( authed: ApiAuthed, Extension(db): Extension, @@ -886,7 +997,41 @@ async fn set_datatable_permissions( })?; } + // A fork that predates this opt-in still holds a copy of the config, and a + // copy says nothing about roles. + if req.enabled { + point_forks_at_this_datatable(&db, &w_id, &datatable_name).await; + } + Ok(format!( "Updated permissions of data table {datatable_name}" )) } + +#[cfg(test)] +mod fork_sweep_tests { + use super::is_inherited_copy; + use serde_json::json; + + /// Opting in takes a fork's copy of the config away, so it must recognise + /// only a copy. Anything else it takes is a data table the fork owns, and + /// the fork loses it. + #[test] + fn only_a_plain_copy_of_this_database_is_taken() { + let database = json!({ "resource_type": "postgresql", "resource_path": "u/a/db" }); + assert!(is_inherited_copy( + &json!({ "database": database, "migrations_enabled": true }), + &database + )); + for kept in [ + // cloned into a database of its own + json!({ "database": database, "forked_from": { "schema": {} } }), + // repointed at another database + json!({ "database": { "resource_type": "postgresql", "resource_path": "u/a/other" } }), + // the fork opted in itself: those roles are not in this database + json!({ "database": database, "permissions": { "enabled": true } }), + ] { + assert!(!is_inherited_copy(&kept, &database), "{kept}"); + } + } +}