From e6ca0de9c3c06475318009c09b8d50e8f1d5869e Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Fri, 4 Sep 2026 13:14:51 +0200 Subject: [PATCH] fix(datatables): a fork does not carry a copy of who may run as what MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A data table the fork shares rather than forks still points at the parent's database, and its cloned permissions block froze the tenants at the moment of the fork: the parent revoking one never reached the copy, so a member who forked while authorized kept running as that role afterwards. Dropping the block instead would hand the fork the data table's own connection, which owns everything — so a permissioned data table is not shared into a fork at all. A deferred drop that fails is the end of the line for those logins, since the config that named them has committed and no later plan diffs against them. The error says so, and names them. The new-app picker resets its role when the data table changes rather than only when the name is gone: two data tables can both define an `analyst`, and the name surviving is not the role surviving. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S5arH3G2Sa1Qqm32veJQ1n --- ...583f43c8157abf675358cfd84ff056925fc4.json} | 4 +-- .../src/datatable_permissions.rs | 29 +++++++++++++------ .../windmill-api-workspaces/src/workspaces.rs | 14 +++++++-- .../raw_apps/RawAppTemplatePicker.svelte | 10 +++++-- 4 files changed, 41 insertions(+), 16 deletions(-) rename backend/.sqlx/{query-6020173a1ed1f946766e7865892ed2010bfd7271fda90166e768be924a854456.json => query-7637d311391ab08865b10f69d048583f43c8157abf675358cfd84ff056925fc4.json} (59%) diff --git a/backend/.sqlx/query-6020173a1ed1f946766e7865892ed2010bfd7271fda90166e768be924a854456.json b/backend/.sqlx/query-7637d311391ab08865b10f69d048583f43c8157abf675358cfd84ff056925fc4.json similarity index 59% rename from backend/.sqlx/query-6020173a1ed1f946766e7865892ed2010bfd7271fda90166e768be924a854456.json rename to backend/.sqlx/query-7637d311391ab08865b10f69d048583f43c8157abf675358cfd84ff056925fc4.json index 4b94d1efd5..f9cd9c9577 100644 --- a/backend/.sqlx/query-6020173a1ed1f946766e7865892ed2010bfd7271fda90166e768be924a854456.json +++ b/backend/.sqlx/query-7637d311391ab08865b10f69d048583f43c8157abf675358cfd84ff056925fc4.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(datatable, '{datatables}', (\n SELECT COALESCE(jsonb_object_agg(\n key,\n CASE WHEN key = ANY($2) THEN value - 'permissions' ELSE value END\n ), '{}'::jsonb)\n FROM jsonb_each(datatable->'datatables')\n ))\n WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", + "query": "UPDATE workspace_settings\n SET datatable = jsonb_set(datatable, '{datatables}', (\n SELECT COALESCE(jsonb_object_agg(\n key,\n CASE WHEN key = ANY($2) THEN value - 'permissions' ELSE value END\n ), '{}'::jsonb)\n FROM jsonb_each(datatable->'datatables')\n WHERE key = ANY($2)\n OR COALESCE((value->'permissions'->>'enabled')::boolean, false) = false\n ))\n WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'", "describe": { "columns": [], "parameters": { @@ -11,5 +11,5 @@ }, "nullable": [] }, - "hash": "6020173a1ed1f946766e7865892ed2010bfd7271fda90166e768be924a854456" + "hash": "7637d311391ab08865b10f69d048583f43c8157abf675358cfd84ff056925fc4" } diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 6555a3e102..d3d6d0f1b1 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -836,16 +836,27 @@ async fn set_datatable_permissions( tx.commit().await?; - // What the config no longer names, now that it says so. + // What the config no longer names, now that it says so. A failure here is the + // end of the line for these logins: the config that named them has committed, + // so no later plan diffs against them and nothing will try again. Say which + // ones, since dropping them is now a database administrator's job. if !deferred.is_empty() { - drop_roles_the_config_no_longer_names(&db, &w_id, &mut client, &deferred) - .await - .map_err(|e| { - Error::ExecutionErr(format!( - "Permissions of data table {datatable_name} were saved, but removing the roles \ - they no longer name failed: {e}. Save them again to retry." - )) - })?; + if let Err(e) = + drop_roles_the_config_no_longer_names(&db, &w_id, &mut client, &deferred).await + { + let mut orphans: Vec<&str> = deferred + .iter() + .filter_map(|s| s.drops_role.as_deref()) + .collect(); + orphans.sort(); + orphans.dedup(); + return Err(Error::ExecutionErr(format!( + "Permissions of data table {datatable_name} were saved, but the Postgres logins \ + they no longer name could not be removed: {e}. Saving again will not retry \ + them — the config no longer names them. Drop {} by hand.", + orphans.join(", ") + ))); + } } Ok(format!( diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 4b4a593a64..c274f632d8 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -8121,9 +8121,15 @@ async fn create_workspace_fork( // A forked data table now points at a fresh database where the parent's roles hold // nothing, so its cloned `permissions` block is meaningless and is dropped — the fork - // opts in on its own. A data table that was NOT forked still points at the parent's - // database, where those roles do hold grants: keeping its block is what stops a fork - // (which any member may create) from reaching the parent's data as root. + // opts in on its own. + // + // A data table that was NOT forked still points at the parent's database, where those + // roles do hold grants, and neither answer is safe: dropping the block would let a + // fork (which any member may create) reach the parent's data as root, while keeping it + // freezes who may run as what at the moment of the fork — the parent revoking a tenant + // would never reach the copy, and the fork would keep running as the role it named. So + // a permissioned data table is not shared into a fork at all; the fork can fork it, or + // go without it. let forked_datatable_names: Vec = nw .forked_datatables .iter() @@ -8137,6 +8143,8 @@ async fn create_workspace_fork( CASE WHEN key = ANY($2) THEN value - 'permissions' ELSE value END ), '{}'::jsonb) FROM jsonb_each(datatable->'datatables') + WHERE key = ANY($2) + OR COALESCE((value->'permissions'->>'enabled')::boolean, false) = false )) WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'"#, &forked_id, diff --git a/frontend/src/lib/components/raw_apps/RawAppTemplatePicker.svelte b/frontend/src/lib/components/raw_apps/RawAppTemplatePicker.svelte index 12ea1557b0..b0cc89fe2c 100644 --- a/frontend/src/lib/components/raw_apps/RawAppTemplatePicker.svelte +++ b/frontend/src/lib/components/raw_apps/RawAppTemplatePicker.svelte @@ -1,4 +1,5 @@