From 0fc9ba39a7bab8f1187e150008fd1e192eef0c19 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 2 Sep 2026 10:27:28 +0200 Subject: [PATCH] fix(datatables): a tenant and a resource path follow the principal they name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A data table role names its tenants — `u/alice`, `g/devs`, `f/team` — and a data table names its database by resource path. Both are strings that outlive what they point at, and the flows that free those names left them behind: - Deleting a workspace user, a group or a folder left its tenant on every role it could run as. The name is free afterwards, so whoever takes it next inherits the role without an admin granting it. All three now drop it in the same transaction that removes the principal. - Offboarding reassigns a user's resources to a folder without the data table following, which both stops it resolving and frees the path it named — a new resource there points it at another database with its roles intact. The username rename had the same gap. Both move the config with the resource. The walk both need is one function each now, with the table test that pins which tenant forms it touches. Saving permissions also takes a lock per data table, held from the read that plans to the write that persists: two saves interleaved each planned against what the other was leaving, and the one that persisted last stored roles the other had already dropped. The ACL apply takes the same lock, since it reads the same config and catalog. --- ...70f9b6a37855341e85ce3091e0ab4409d5def.json | 23 ++ backend/windmill-api-groups/src/folders.rs | 10 + backend/windmill-api-groups/src/groups.rs | 10 + backend/windmill-api-users/src/users.rs | 11 + .../src/datatable_acl.rs | 10 +- .../src/datatable_permissions.rs | 30 ++- backend/windmill-api/src/offboarding.rs | 26 ++ backend/windmill-api/src/users.rs | 43 ++-- backend/windmill-common/src/workspaces.rs | 228 +++++++++++++++++- 9 files changed, 359 insertions(+), 32 deletions(-) create mode 100644 backend/.sqlx/query-546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def.json diff --git a/backend/.sqlx/query-546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def.json b/backend/.sqlx/query-546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def.json new file mode 100644 index 0000000000..261e1ca101 --- /dev/null +++ b/backend/.sqlx/query-546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT pg_advisory_xact_lock(hashtext('datatable_permissions:' || $1), hashtext($2))", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "pg_advisory_xact_lock", + "type_info": "Void" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def" +} diff --git a/backend/windmill-api-groups/src/folders.rs b/backend/windmill-api-groups/src/folders.rs index 646f764ec6..c10c02feb5 100644 --- a/backend/windmill-api-groups/src/folders.rs +++ b/backend/windmill-api-groups/src/folders.rs @@ -816,6 +816,16 @@ async fn delete_folder( ))); } + // A data table role names its tenants by principal, so the name is free + // after this — and recreating a folder with it would inherit every role the + // old one could run as. + windmill_common::workspaces::remove_datatable_tenant_in_workspace( + &w_id, + &format!("f/{name}"), + &mut tx, + ) + .await?; + audit_log( &mut *tx, &authed, diff --git a/backend/windmill-api-groups/src/groups.rs b/backend/windmill-api-groups/src/groups.rs index a15b9823d3..d7178641c0 100644 --- a/backend/windmill-api-groups/src/groups.rs +++ b/backend/windmill-api-groups/src/groups.rs @@ -809,6 +809,16 @@ async fn delete_group( .execute(&mut *tx) .await?; + // A data table role names its tenants by principal, so the name is free + // after this — and recreating a group with it would inherit every role the + // old one could run as. + windmill_common::workspaces::remove_datatable_tenant_in_workspace( + &w_id, + &format!("g/{name}"), + &mut tx, + ) + .await?; + audit_log( &mut *tx, &authed, diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 8daa414b46..40f2374659 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2435,6 +2435,17 @@ pub async fn delete_workspace_user_internal( username_to_delete, w_id ).execute(&mut **tx).await?; + // ---- Clean up data table role tenants ---- + + // The username is free once the row below is gone, so a tenant left behind + // would hand every role it names to whoever is invited into it next. + windmill_common::workspaces::remove_datatable_tenant_in_workspace( + w_id, + &format!("u/{username_to_delete}"), + tx, + ) + .await?; + // ---- Delete personal data ---- sqlx::query!( "DELETE FROM draft WHERE path LIKE ('u/' || $1 || '/%') AND workspace_id = $2", diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 3e9510ecb1..61c15d870f 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -1041,6 +1041,13 @@ async fn apply_datatable_acl( Path((w_id, datatable_name)): Path<(String, String)>, Json(req): Json, ) -> Result { + // Taken for the same reason the role save takes it: the plan below is read + // off the catalog and the config, and a role save running at the same time + // is what changes both under it. Held to the end of this handler. + let mut lock_tx = db.begin().await?; + crate::datatable_permissions::lock_datatable_permissions(&mut lock_tx, &w_id, &datatable_name) + .await?; + // Authorization first: the repair below opens a connection as the instance's // own Postgres user, which is not something a request that is about to be // refused gets to reach. @@ -1076,7 +1083,7 @@ async fn apply_datatable_acl( })?; audit_log( - &db, + &mut *lock_tx, &authed, "datatables.acl", ActionKind::Update, @@ -1085,6 +1092,7 @@ async fn apply_datatable_acl( Some([("target", format!("{:?}", req.target).as_str())].into()), ) .await?; + lock_tx.commit().await?; Ok(format!("Updated access on {}", req.target.label(&dbname))) } diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 1df699c833..0ca324e317 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -137,6 +137,27 @@ pub(crate) struct RolePlan { pub(crate) warnings: Vec, } +/// Serialize everything that changes one data table's roles or their access. +/// +/// Both the role save and an ACL apply read the config, plan against it and run +/// the result on the data table's own database; two of them at once plan against +/// a state the other is leaving. Keyed per data table, and released when the +/// caller's transaction ends. +pub(crate) async fn lock_datatable_permissions( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + w_id: &str, + datatable_name: &str, +) -> Result<()> { + sqlx::query!( + "SELECT pg_advisory_xact_lock(hashtext('datatable_permissions:' || $1), hashtext($2))", + w_id, + datatable_name, + ) + .execute(&mut **tx) + .await?; + Ok(()) +} + /// Refuse to plan a change on an enterprise binary whose plan does not cover it. /// A build that is not enterprise has no planner at all — see /// [`crate::datatable_permissions_oss`] — so this only has the licensed @@ -571,6 +592,14 @@ async fn set_datatable_permissions( ) -> Result { require_admin(authed.is_admin, &authed.username)?; + // Reading the config, planning against it, running the plan and persisting + // it are one operation: two saves of the same data table interleaved would + // each plan against the state the other is leaving, and the one that + // persists last would store roles the other already dropped. Held to commit, + // so the whole sequence below is inside it. + let mut tx = db.begin().await?; + lock_datatable_permissions(&mut tx, &w_id, &datatable_name).await?; + // The roles about to be created are handed privileges by this connection, // which cannot pass on what it holds without the grant option. ensure_instance_db_can_delegate(&db, &w_id, &datatable_name).await; @@ -588,7 +617,6 @@ async fn set_datatable_permissions( let permissions = serde_json::to_value(&plan.permissions) .map_err(|e| Error::internal_err(format!("Failed to serialize permissions: {e}")))?; - let mut tx = db.begin().await?; // Written at the permissions path only, so a concurrent edit of the data // table's own settings is not clobbered. let updated = sqlx::query_scalar!( diff --git a/backend/windmill-api/src/offboarding.rs b/backend/windmill-api/src/offboarding.rs index a69441ea75..6ae384250b 100644 --- a/backend/windmill-api/src/offboarding.rs +++ b/backend/windmill-api/src/offboarding.rs @@ -985,6 +985,32 @@ async fn offboard_user_from_workspace<'c>( .await? .unwrap_or(0); + // A data table names its database by resource path, so one just moved has to + // move in the config too: left behind it stops resolving, and the path it + // named is free for a resource pointing somewhere else entirely. + let datatable_settings = sqlx::query_scalar!( + "SELECT datatable FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + w_id + ) + .fetch_optional(&mut **tx) + .await? + .flatten(); + if let Some(mut settings) = datatable_settings { + if windmill_common::workspaces::move_datatable_resource_paths( + &mut settings, + &format!("u/{username}/"), + &format!("{new_prefix}/"), + ) { + sqlx::query!( + "UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = $2", + settings, + w_id + ) + .execute(&mut **tx) + .await?; + } + } + // ---- eval datasets ---- // The foreign keys cascade the rename onto cases and experiments; the paths held inside JSONB // (an experiment's subject, a dataset's scorers) are rewritten separately since the cascade diff --git a/backend/windmill-api/src/users.rs b/backend/windmill-api/src/users.rs index e384ac8103..83fc8eb75a 100644 --- a/backend/windmill-api/src/users.rs +++ b/backend/windmill-api/src/users.rs @@ -868,7 +868,7 @@ async fn update_username_in_workpsace<'c>( .execute(&mut **tx) .await?; - // ---- data table role tenants ---- + // ---- data table role tenants and backing resources ---- // Who may run as a data table role is stored as `u/`, and the // executor compares it against the caller's name. Left behind, the rename @@ -882,34 +882,19 @@ async fn update_username_in_workpsace<'c>( .await? .flatten(); if let Some(mut settings) = datatable_settings { - let old_tenant = format!("u/{old_username}"); - let new_tenant = serde_json::Value::String(format!("u/{new_username}")); - let mut renamed = false; - for dt in settings - .get_mut("datatables") - .and_then(|d| d.as_object_mut()) - .into_iter() - .flat_map(|datatables| datatables.values_mut()) - { - let Some(roles) = dt - .get_mut("permissions") - .and_then(|p| p.get_mut("roles")) - .and_then(|r| r.as_object_mut()) - else { - continue; - }; - for tenant in roles - .values_mut() - .filter_map(|role| role.get_mut("tenants")) - .filter_map(|t| t.as_array_mut()) - .flatten() - { - if tenant.as_str() == Some(old_tenant.as_str()) { - *tenant = new_tenant.clone(); - renamed = true; - } - } - } + let mut renamed = windmill_common::workspaces::rename_datatable_tenant( + &mut settings, + &format!("u/{old_username}"), + &format!("u/{new_username}"), + ); + // The resource rewrite above moves a data table's own postgres resource + // with everything else the user owns; the config names it by path, so it + // has to travel too. + renamed |= windmill_common::workspaces::move_datatable_resource_paths( + &mut settings, + &format!("u/{old_username}/"), + &format!("u/{new_username}/"), + ); if renamed { sqlx::query!( "UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = $2", diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 62a3edae70..0627d0ced5 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1076,6 +1076,140 @@ pub struct DataTableRole { pub tenants: Vec, } +/// Every role's tenant list in a `workspace_settings.datatable` value. +fn datatable_tenant_lists( + datatable: &mut serde_json::Value, +) -> impl Iterator> { + datatable + .get_mut("datatables") + .and_then(|d| d.as_object_mut()) + .into_iter() + .flat_map(|datatables| datatables.values_mut()) + .filter_map(|dt| dt.get_mut("permissions")) + .filter_map(|p| p.get_mut("roles")) + .filter_map(|r| r.as_object_mut()) + .flat_map(|roles| roles.values_mut()) + .filter_map(|role| role.get_mut("tenants")) + .filter_map(|t| t.as_array_mut()) +} + +/// Follow one tenant across a rename, or drop it where the principal is gone. +/// +/// A tenant is matched whole (`u/alice`, `g/devs`, `f/team`), so the `*` +/// wildcard and every other principal are left alone. Returns whether anything +/// changed, so a caller can skip the write. +/// +/// Which of the two a flow needs follows the principal: a rename keeps the role +/// with the same person, while a deletion has to take it away — the name is free +/// afterwards, and whoever takes it next would otherwise inherit every role the +/// old one was a tenant of. +fn update_datatable_tenant( + datatable: &mut serde_json::Value, + tenant: &str, + replacement: Option<&str>, +) -> bool { + let mut changed = false; + for tenants in datatable_tenant_lists(datatable) { + match replacement { + Some(new) => { + for entry in tenants.iter_mut() { + if entry.as_str() == Some(tenant) { + *entry = serde_json::Value::String(new.to_string()); + changed = true; + } + } + } + None => { + let before = tenants.len(); + tenants.retain(|entry| entry.as_str() != Some(tenant)); + changed |= tenants.len() != before; + } + } + } + changed +} + +/// `u/` becomes `u/` on every role that named it. +pub fn rename_datatable_tenant( + datatable: &mut serde_json::Value, + old_tenant: &str, + new_tenant: &str, +) -> bool { + update_datatable_tenant(datatable, old_tenant, Some(new_tenant)) +} + +/// Take a deleted user, group or folder off every role it was a tenant of. +pub fn remove_datatable_tenant(datatable: &mut serde_json::Value, tenant: &str) -> bool { + update_datatable_tenant(datatable, tenant, None) +} + +/// Take a principal off every data table role of one workspace, in the caller's +/// own transaction. +/// +/// The tenant and the principal have to go in the same transaction: between the +/// two the name is free while a role still names it, and taking it is enough to +/// inherit the role. +pub async fn remove_datatable_tenant_in_workspace( + w_id: &str, + tenant: &str, + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, +) -> Result<()> { + let settings = sqlx::query_scalar!( + "SELECT datatable FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + w_id + ) + .fetch_optional(&mut **tx) + .await? + .flatten(); + let Some(mut settings) = settings else { + return Ok(()); + }; + if remove_datatable_tenant(&mut settings, tenant) { + sqlx::query!( + "UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = $2", + settings, + w_id + ) + .execute(&mut **tx) + .await?; + } + Ok(()) +} + +/// Follow the backing postgres resource of every data table under a path that +/// moves — a username rename, an offboarding reassignment. +/// +/// The config stores the resource path plain, so a move that leaves it behind +/// both stops the data table resolving and frees the old path: recreating a +/// resource there would point it at another database with this one's roles +/// still configured. +pub fn move_datatable_resource_paths( + datatable: &mut serde_json::Value, + old_prefix: &str, + new_prefix: &str, +) -> bool { + let mut changed = false; + for dt in datatable + .get_mut("datatables") + .and_then(|d| d.as_object_mut()) + .into_iter() + .flat_map(|datatables| datatables.values_mut()) + { + let Some(path) = dt + .get_mut("database") + .and_then(|d| d.get_mut("resource_path")) + else { + continue; + }; + let Some(rest) = path.as_str().and_then(|p| p.strip_prefix(old_prefix)) else { + continue; + }; + *path = serde_json::Value::String(format!("{new_prefix}{rest}")); + changed = true; + } + changed +} + /// Strip the generated role passwords out of a `workspace_settings.datatable` /// value. /// @@ -1113,7 +1247,11 @@ pub fn redact_datatable_settings_for_export( /// The data tables with permissions enabled that reach their database through /// this resource. -async fn datatables_permissioned_on_resource(db: &DB, w_id: &str, path: &str) -> Result> { +async fn datatables_permissioned_on_resource( + db: &DB, + w_id: &str, + path: &str, +) -> Result> { let datatables: std::collections::HashMap = sqlx::query_scalar!( "SELECT ws.datatable->'datatables' FROM workspace_settings ws WHERE ws.workspace_id = $1", w_id @@ -2963,6 +3101,94 @@ mod tests { ); } + /// The tenant walk is what every flow that renames or deletes a principal + /// has to run, so it answers for the shapes those flows produce. + #[test] + fn a_tenant_follows_a_rename_and_leaves_with_a_deletion() { + let sample = || { + serde_json::json!({ + "datatables": { + "main": { "permissions": { "roles": { + "admin": { "tenants": [] }, + "analyst": { "tenants": ["u/alice", "g/devs", "*"] } + }}}, + "other": { "permissions": { "enabled": false, "roles": { + "reader": { "tenants": ["f/team", "u/alice"] } + }}} + } + }) + }; + + // A rename matches the whole tenant, so a group or folder of the same + // name — and the wildcard — are not touched. + let mut renamed = sample(); + assert!(rename_datatable_tenant(&mut renamed, "u/alice", "u/bob")); + assert_eq!( + renamed["datatables"]["main"]["permissions"]["roles"]["analyst"]["tenants"], + serde_json::json!(["u/bob", "g/devs", "*"]) + ); + // Every data table, including one whose permissions are off: a later + // re-enable would otherwise bring the stale name back. + assert_eq!( + renamed["datatables"]["other"]["permissions"]["roles"]["reader"]["tenants"], + serde_json::json!(["f/team", "u/bob"]) + ); + + // A deletion takes the tenant away instead: the name is free afterwards. + let mut removed = sample(); + assert!(remove_datatable_tenant(&mut removed, "g/devs")); + assert_eq!( + removed["datatables"]["main"]["permissions"]["roles"]["analyst"]["tenants"], + serde_json::json!(["u/alice", "*"]) + ); + + // Nothing to do reports nothing to write. + let mut untouched = sample(); + assert!(!rename_datatable_tenant( + &mut untouched, + "u/carol", + "u/dave" + )); + assert!(!remove_datatable_tenant(&mut untouched, "f/nope")); + assert_eq!(untouched, sample()); + } + + /// A data table names its database by resource path, so a flow that moves + /// the resource has to move the name with it. + #[test] + fn a_data_table_follows_its_resource_across_a_path_move() { + let mut settings = serde_json::json!({ + "datatables": { + "main": { "database": { "resource_type": "postgresql", "resource_path": "u/alice/mypg" } }, + "managed": { "database": { "resource_type": "instance", "resource_path": "dt_main" } }, + "elsewhere": { "database": { "resource_type": "postgresql", "resource_path": "f/team/pg" } } + } + }); + assert!(move_datatable_resource_paths( + &mut settings, + "u/alice/", + "f/team/" + )); + assert_eq!( + settings["datatables"]["main"]["database"]["resource_path"], + "f/team/mypg" + ); + // A path that does not start with what moved is left as it is. + assert_eq!( + settings["datatables"]["managed"]["database"]["resource_path"], + "dt_main" + ); + assert_eq!( + settings["datatables"]["elsewhere"]["database"]["resource_path"], + "f/team/pg" + ); + assert!(!move_datatable_resource_paths( + &mut settings, + "u/nobody/", + "u/somebody/" + )); + } + #[test] fn datatable_settings_export_drops_role_passwords() { let settings = serde_json::json!({