diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 862676f204..e6df7f7752 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -3fe285af8293aafd4d4ba31002bfa84a30544586 +4c6cbb234019db102c44a91ada36c4a197721aa4 diff --git a/backend/windmill-api-groups/src/folders.rs b/backend/windmill-api-groups/src/folders.rs index 434335e034..4bfb6d10f4 100644 --- a/backend/windmill-api-groups/src/folders.rs +++ b/backend/windmill-api-groups/src/folders.rs @@ -800,6 +800,16 @@ async fn delete_folder( not_found_if_none(get_folderopt(&mut tx, &w_id, &name).await?, "Folder", &name)?; + // 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_unchecked( + &w_id, + &format!("f/{name}"), + &mut tx, + ) + .await?; + let del = sqlx::query_scalar!( "DELETE FROM folder WHERE name = $1 AND workspace_id = $2 RETURNING 1", name, @@ -816,16 +826,6 @@ 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_unchecked( - &w_id, - &format!("f/{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 55aeb56219..1b82897622 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -1656,18 +1656,10 @@ async fn delete_user( forbid_superadmin_job_token(&db, &authed.email, job_id).await?; let mut tx = db.begin().await?; - sqlx::query!("DELETE FROM token WHERE email = $1", &email_to_delete) - .execute(&mut *tx) - .await?; - sqlx::query!("DELETE FROM password WHERE email = $1", &email_to_delete) - .execute(&mut *tx) - .await?; - windmill_common::user_drafts::delete_drafts_of_email(&mut *tx, &email_to_delete).await?; - - // Read before deleting, so each membership's tenant can go first. A username - // is scoped to one workspace, and so are the tenants naming it. - // Ordered, so two deletions that share workspaces lock their settings rows - // in the same sequence rather than head-on. + // A username is scoped to one workspace, and so are the tenants naming it, so + // the memberships are read before anything is deleted. Ordered, and before + // every other write of this transaction — see + // `lock_workspace_settings_unchecked`. let memberships = sqlx::query!( "SELECT workspace_id, username FROM usr WHERE email = $1 ORDER BY workspace_id", &email_to_delete @@ -1676,7 +1668,7 @@ async fn delete_user( .await?; for row in &memberships { - // The username is free in that workspace once the row below is gone, so + // The username is free in that workspace once the rows below are gone, so // a role still naming it would hand itself to whoever takes it next. windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( &row.workspace_id, @@ -1686,6 +1678,14 @@ async fn delete_user( .await?; } + sqlx::query!("DELETE FROM token WHERE email = $1", &email_to_delete) + .execute(&mut *tx) + .await?; + sqlx::query!("DELETE FROM password WHERE email = $1", &email_to_delete) + .execute(&mut *tx) + .await?; + windmill_common::user_drafts::delete_drafts_of_email(&mut *tx, &email_to_delete).await?; + sqlx::query!("DELETE FROM usr WHERE email = $1", &email_to_delete) .execute(&mut *tx) .await?; @@ -2416,6 +2416,18 @@ pub async fn delete_workspace_user_internal( tx: &mut Transaction<'_, Postgres>, authed: Option<&ApiAuthed>, // None for system operations ) -> Result<()> { + // ---- Clean up data table role tenants ---- + + // The username is free once this user's rows are gone, so a tenant left + // behind would hand every role it names to whoever is invited into it next. + // First in the transaction — see `lock_workspace_settings_unchecked`. + windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( + w_id, + &format!("u/{username_to_delete}"), + tx, + ) + .await?; + // ---- Clean up extra_perms referencing this user ---- let extra_perms_tables = [ "script", @@ -2458,17 +2470,6 @@ 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_unchecked( - 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_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 2932fc52ca..aa54913777 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -116,13 +116,14 @@ pub struct DatatablePermissionsPreview { pub(crate) struct PlannedStatement { pub(crate) sql: String, pub(crate) display: String, - /// Whether this one waits for the config it belongs to to commit. + /// The Postgres role this statement is part of destroying, if any. /// /// Creating a role before the config names it leaves at worst a role the /// next save adopts, so those run inside the request. Dropping one is not - /// reversible — `DROP OWNED` discards every grant it accumulated — so a save - /// that fails after the drop would have destroyed what it then rolls back. - pub(crate) deferred: bool, + /// reversible — `DROP OWNED` discards every grant it accumulated — so these + /// wait for the config that stops naming the role to commit, and are then + /// checked against it one role at a time. + pub(crate) drops_role: Option, } impl PlannedStatement { @@ -132,7 +133,7 @@ impl PlannedStatement { allow(dead_code) )] pub(crate) fn plain(sql: String) -> Self { - Self { display: sql.clone(), sql, deferred: false } + Self { display: sql.clone(), sql, drops_role: None } } } @@ -516,25 +517,54 @@ pub(crate) async fn plan_drop_of_deleted_datatable( } } -/// Run statements the config had to commit before, against the data table's -/// database, once it has. +/// The Postgres logins the workspace's config currently names. +fn pg_rolenames_in_use(settings: Option<&serde_json::Value>) -> HashSet { + settings + .and_then(|s| s.get("datatables")) + .and_then(|d| d.as_object()) + .map(|datatables| { + datatables + .values() + .filter_map(|dt| dt.pointer("/permissions/roles")?.as_object()) + .flat_map(|roles| roles.values()) + .filter_map(|role| role.get("pg_rolename")?.as_str()) + .map(str::to_string) + .collect() + }) + .unwrap_or_default() +} + +/// Destroy the roles a committed config stopped naming. /// -/// `still_wanted` is asked under the settings row, and that row is held through -/// the statements — which is the lock every save of a data table in this -/// workspace takes first, so nothing can adopt or depend on a role between the -/// question and the answer. Returns whether they ran. -async fn run_after_commit( +/// Asked under the settings row, one role at a time: a role the config names +/// again — a data table recreated under the same name, or a save that put the +/// role back — is left alone, and one it does not name is dropped whatever else +/// has changed in the meantime. Nobody else can be planning against these +/// between the question and the answer, since that row is what every save takes +/// first. +/// +/// Best-effort: a role outliving its config is recoverable, dropping one a live +/// data table depends on is not. +async fn drop_roles_the_config_no_longer_names( db: &DB, w_id: &str, client: &mut tokio_postgres::Client, statements: &[&PlannedStatement], - still_wanted: impl FnOnce(Option<&serde_json::Value>) -> bool, -) -> Result { +) -> Result<()> { let mut tx = db.begin().await?; let settings = windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, w_id).await?; - let wanted = still_wanted(settings.as_ref()); - if wanted { + let in_use = pg_rolenames_in_use(settings.as_ref()); + let to_run: Vec<&PlannedStatement> = statements + .iter() + .filter(|s| { + s.drops_role + .as_ref() + .is_none_or(|role| !in_use.contains(role)) + }) + .copied() + .collect(); + if !to_run.is_empty() { // The settings row is held for as long as these run, and they run on a // database this workspace does not control — a lock held there, or a role // with a great deal to reassign, would otherwise stall every save of every @@ -548,28 +578,14 @@ async fn run_after_commit( pg_error_message(&e) )) })?; - run_statements(client, statements).await?; + run_statements(client, &to_run).await?; } tx.commit().await?; - Ok(wanted) + Ok(()) } -fn datatable_is_configured(settings: Option<&serde_json::Value>, datatable_name: &str) -> bool { - settings - .and_then(|s| s.get("datatables")) - .and_then(|d| d.get(datatable_name)) - .is_some_and(|v| !v.is_null()) -} - -/// Run a planned drop, unless the data table came back. -/// -/// A role name is generated from `(workspace, data table, role)`, so a data -/// table recreated under the same name generates the same names — and its save -/// adopts the very roles this plan plans to drop, since they are still granted -/// to the same admin login. -/// -/// Best-effort: roles outliving their data table are recoverable, dropping the -/// ones a live data table depends on is not. +/// Drop the roles of a data table that was deleted, once the config saying so +/// has committed. pub(crate) async fn run_planned_drop( db: &DB, w_id: &str, @@ -577,19 +593,11 @@ pub(crate) async fn run_planned_drop( (mut client, plan): PlannedRoleDrop, ) { let statements: Vec<&PlannedStatement> = plan.statements.iter().collect(); - match run_after_commit(db, w_id, &mut client, &statements, |settings| { - !datatable_is_configured(settings, datatable_name) - }) - .await + if let Err(e) = drop_roles_the_config_no_longer_names(db, w_id, &mut client, &statements).await { - Ok(false) => tracing::info!( - "Data table {datatable_name} in {w_id} was recreated before its old Postgres roles \ - were dropped; leaving them to the data table that now holds them" - ), - Ok(true) => {} - Err(e) => tracing::error!( + tracing::error!( "Could not drop the Postgres roles of deleted data table {datatable_name} in {w_id}: {e:#}" - ), + ); } } @@ -753,9 +761,21 @@ async fn set_datatable_permissions( // failure after this point leaves roles the config does not know about, which // the next plan adopts (it reads `pg_roles`), whereas the reverse order would // leave the config naming roles that were never created. Dropping one has no - // such way back, so those wait below. - let (deferred, immediate): (Vec<&PlannedStatement>, Vec<&PlannedStatement>) = - plan.statements.iter().partition(|s| s.deferred); + // such way back, so those wait below — except where this save gives the freed + // name to another role, which only works in one order. + let keeps_the_name = |statement: &PlannedStatement| { + statement.drops_role.as_ref().is_some_and(|dropped| { + plan.permissions + .roles + .values() + .filter_map(|r| r.pg_rolename.as_ref()) + .any(|kept| kept == dropped) + }) + }; + let (deferred, immediate): (Vec<&PlannedStatement>, Vec<&PlannedStatement>) = plan + .statements + .iter() + .partition(|s| s.drops_role.is_some() && !keeps_the_name(s)); run_statements(&mut client, &immediate).await?; let permissions = serde_json::to_value(&plan.permissions) @@ -799,23 +819,16 @@ async fn set_datatable_permissions( tx.commit().await?; - // What the config no longer names, now that it says so. Skipped when another - // save has landed since: its own plan was built against `pg_roles` as they - // are, so these roles are its to keep or drop. + // What the config no longer names, now that it says so. if !deferred.is_empty() { - let ours = permissions.clone(); - run_after_commit(&db, &w_id, &mut client, &deferred, |settings| { - settings - .and_then(|s| s.pointer(&format!("/datatables/{datatable_name}/permissions"))) - .is_some_and(|current| *current == ours) - }) - .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." - )) - })?; + 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." + )) + })?; } Ok(format!( diff --git a/backend/windmill-api/src/users.rs b/backend/windmill-api/src/users.rs index 7e78219461..aeb1423ce5 100644 --- a/backend/windmill-api/src/users.rs +++ b/backend/windmill-api/src/users.rs @@ -265,6 +265,47 @@ async fn update_username_in_workpsace<'c>( new_username: &str, w_id: &str, ) -> error::Result<()> { + // Before anything else in the transaction — see + // `lock_workspace_settings_unchecked`. `rename_user` walks memberships in + // `workspace_id` order, so a rename spanning workspaces takes their rows in + // that order too. + let datatable_settings = + windmill_common::workspaces::lock_workspace_settings_unchecked(tx, w_id).await?; + + // ---- instance and workspace users ---- + + // Scoped to this workspace, like everything else here: `usr` and + // `usr_to_group` rows of another workspace answer to that workspace's own + // settings row. + sqlx::query!( + "UPDATE usr SET username = $1 WHERE email = $2 AND workspace_id = $3", + new_username, + email, + w_id + ) + .execute(&mut **tx) + .await?; + + sqlx::query!( + "UPDATE usr_to_group SET usr = $1 WHERE usr = $2 AND workspace_id = $3", + new_username, + old_username, + w_id + ) + .execute(&mut **tx) + .await?; + + // ---- group_ ---- + + sqlx::query!( + "UPDATE group_ SET extra_perms = extra_perms - ('u/' || $2) || jsonb_build_object(('u/' || $1), extra_perms->('u/' || $2)) WHERE extra_perms ? ('u/' || $2) AND workspace_id = $3", + new_username, + old_username, + w_id + ) + .execute(&mut **tx) + .await?; + // ---- v2_job ---- sqlx::query!( r#"UPDATE v2_job SET runnable_path = REGEXP_REPLACE(runnable_path,'u/' || $2 || '/(.*)','u/' || $1 || '/\1') WHERE runnable_path LIKE ('u/' || $2 || '/%') AND workspace_id = $3"#, @@ -848,13 +889,6 @@ async fn update_username_in_workpsace<'c>( // executor compares it against the caller's name. Left behind, the rename // takes the role away from the user it followed and hands it to whoever // takes the old name next. - 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 { let mut renamed = windmill_common::workspaces::rename_datatable_tenant( &mut settings, @@ -880,40 +914,6 @@ async fn update_username_in_workpsace<'c>( } } - // ---- group_ and workspace users ---- - - // Last, after the settings row above, and scoped to this workspace like - // everything else here: every path that frees or renames a principal takes - // `workspace_settings` before `group_`, `usr` and `usr_to_group`, and - // reaching into another workspace's rows would hold this one's settings row - // while waiting on a principal that workspace's own settings row guards. - sqlx::query!( - "UPDATE group_ SET extra_perms = extra_perms - ('u/' || $2) || jsonb_build_object(('u/' || $1), extra_perms->('u/' || $2)) WHERE extra_perms ? ('u/' || $2) AND workspace_id = $3", - new_username, - old_username, - w_id - ) - .execute(&mut **tx) - .await?; - - sqlx::query!( - "UPDATE usr SET username = $1 WHERE email = $2 AND workspace_id = $3", - new_username, - email, - w_id - ) - .execute(&mut **tx) - .await?; - - sqlx::query!( - "UPDATE usr_to_group SET usr = $1 WHERE usr = $2 AND workspace_id = $3", - new_username, - old_username, - w_id - ) - .execute(&mut **tx) - .await?; - Ok(()) } diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 58453b05d2..fdce49840a 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1152,11 +1152,20 @@ pub fn remove_datatable_tenant(datatable: &mut serde_json::Value, tenant: &str) /// Take the workspace's settings row, and read the data table config under it, /// for the length of the caller's transaction. /// -/// Everything that reads that config, decides something from it and writes it -/// back holds this first: a role save, an ACL change, the settings form, and the -/// principal cleanups below. Without it each of them can persist a block it -/// computed before another one committed — a save that planned with `g/devs` -/// puts the tenant back after the group's deletion took it away. +/// This row is the one lock over everything a data table's permissions depend +/// on: the config itself, and the users, groups and folders its roles name as +/// tenants. Every path that touches either — a role save, an ACL change, the +/// settings form, a rename, a deletion — takes it, which is what stops one of +/// them persisting a block it computed before another committed: a save that +/// planned with `g/devs` would otherwise put the tenant back after the group's +/// deletion took it away. +/// +/// **Take it before the transaction locks anything else.** One lock, always +/// acquired first, cannot deadlock; a caller that writes `usr` or `group_` and +/// then reaches for this one holds two in an order some other path holds the +/// other way round. A transaction spanning workspaces takes them in +/// `workspace_id` order, for the same reason. That is the whole ordering rule: +/// what a handler writes after taking it, and in what order, does not matter. /// /// Authorization: performs none, for any workspace it is handed. What it returns /// is the config as stored, generated role passwords included, so callers MUST @@ -1181,10 +1190,7 @@ pub async fn lock_workspace_settings_unchecked( /// /// 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. Call this *before* deleting the principal's own row: this -/// takes the workspace settings row, so a caller that deleted first would hold -/// two rows in the opposite order to every other caller, and two of them at once -/// would deadlock. +/// inherit the role. /// /// Authorization: performs none. Callers MUST have already authorized the /// removal of the principal itself — the rules differ per caller (a workspace