diff --git a/backend/.sqlx/query-546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def.json b/backend/.sqlx/query-546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def.json deleted file mode 100644 index 261e1ca101..0000000000 --- a/backend/.sqlx/query-546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "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 c10c02feb5..434335e034 100644 --- a/backend/windmill-api-groups/src/folders.rs +++ b/backend/windmill-api-groups/src/folders.rs @@ -819,7 +819,7 @@ 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( + windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( &w_id, &format!("f/{name}"), &mut tx, diff --git a/backend/windmill-api-groups/src/groups.rs b/backend/windmill-api-groups/src/groups.rs index d7178641c0..89cb2da572 100644 --- a/backend/windmill-api-groups/src/groups.rs +++ b/backend/windmill-api-groups/src/groups.rs @@ -812,7 +812,7 @@ async fn delete_group( // 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( + windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( &w_id, &format!("g/{name}"), &mut tx, diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 40f2374659..d1b0697b02 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2439,7 +2439,7 @@ pub async fn delete_workspace_user_internal( // 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( + windmill_common::workspaces::remove_datatable_tenant_in_workspace_unchecked( w_id, &format!("u/{username_to_delete}"), tx, diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 61c15d870f..439d35b2b9 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -1045,8 +1045,7 @@ async fn apply_datatable_acl( // 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?; + windmill_common::workspaces::lock_workspace_settings(&mut lock_tx, &w_id).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 diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 0ca324e317..f02f668016 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -137,27 +137,6 @@ 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 @@ -593,12 +572,12 @@ async fn set_datatable_permissions( 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. + // it are one operation: interleaved with another save, or with the removal + // of a principal some role names as a tenant, this would store a block it + // computed before the other committed. The settings row is what everything + // touching that config takes, so taking it here is what serializes them. let mut tx = db.begin().await?; - lock_datatable_permissions(&mut tx, &w_id, &datatable_name).await?; + windmill_common::workspaces::lock_workspace_settings(&mut tx, &w_id).await?; // The roles about to be created are handed privileges by this connection, // which cannot pass on what it holds without the grant option. diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 4705a203fe..3e58d27e6b 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3705,15 +3705,15 @@ async fn edit_datatable_config( let is_superadmin = require_super_admin(&db, &authed).await.is_ok(); let mut tx = db.begin().await?; - + // This form carries the whole config forward — permissions restored from the + // old value included — so it reads and writes the settings under the same + // lock as the role save and the principal cleanups, or it puts back what one + // of them just took away. let old_datatables: HashMap = serde_json::from_value( - sqlx::query_scalar!( - "SELECT ws.datatable->'datatables' FROM workspace_settings ws WHERE ws.workspace_id = $1", - &w_id - ) - .fetch_one(&db) - .await? - .unwrap_or(serde_json::Value::Null), + windmill_common::workspaces::lock_workspace_settings(&mut tx, &w_id) + .await? + .and_then(|d| d.get("datatables").cloned()) + .unwrap_or(serde_json::Value::Null), ) .unwrap_or_default(); diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 0627d0ced5..c560c01382 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1143,25 +1143,46 @@ pub fn remove_datatable_tenant(datatable: &mut serde_json::Value, tenant: &str) update_datatable_tenant(datatable, tenant, None) } +/// 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. +pub async fn lock_workspace_settings( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + w_id: &str, +) -> Result> { + Ok(sqlx::query_scalar!( + "SELECT datatable FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE", + w_id + ) + .fetch_optional(&mut **tx) + .await? + .flatten()) +} + /// 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( +/// +/// Authorization: performs none. Callers MUST have already authorized the +/// removal of the principal itself — the rules differ per caller (a workspace +/// admin for a user, the group's or folder's owner for those, no identity at all +/// for the system paths), which is why the check stays with them. It can only +/// ever narrow access: a tenant leaves, none is added, so misuse costs a +/// revocation rather than an escalation. +pub async fn remove_datatable_tenant_in_workspace_unchecked( 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 { + let Some(mut settings) = lock_workspace_settings(tx, w_id).await? else { return Ok(()); }; if remove_datatable_tenant(&mut settings, tenant) {