fix(datatables): one lock for everything that reads the config and writes it back

The role save took an advisory lock of its own while principal cleanup took the
settings row, so the two never excluded each other: a save could persist a
permissions block it had computed before a group's deletion took that group off
it. The settings form had the same shape with no lock at all — it carries the
old permissions forward by construction.

They all take the settings row now, before reading and until they have written,
which is the lock the cleanups already used. One mechanism, so there is no
ordering to get wrong, and the advisory lock goes away with its cached query.

The tenant removal says what it does not do: it authorizes nothing, and the
rules differ per caller — a workspace admin for a user, the owner for a group
or folder, no identity for the system paths — so the name carries `_unchecked`
the way the resolution helpers next to it do.

backend/.sqlx/query-546ea8f10c2b5f6493f347370f870f9b6a37855341e85ce3091e0ab4409d5def.json

@@ -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"
-}

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,

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,

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,

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

backend/windmill-api-workspaces/src/datatable_permissions.rs

@@ -137,27 +137,6 @@ pub(crate) struct RolePlan {
     pub(crate) warnings: Vec<String>,
 }
 
-/// 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.

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<String, DataTable> = 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();
 

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<Option<serde_json::Value>> {
+    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) {