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!({