fix(datatables): a tenant and a resource path follow the principal they name

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.

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

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,

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,

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",

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<AclChangeRequest>,
 ) -> Result<String> {
+    // 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)))
 }

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

@@ -137,6 +137,27 @@ 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
@@ -571,6 +592,14 @@ async fn set_datatable_permissions(
 ) -> Result<String> {
     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!(

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

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/<username>`, 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",

backend/windmill-common/src/workspaces.rs

@@ -1076,6 +1076,140 @@ pub struct DataTableRole {
     pub tenants: Vec<String>,
 }
 
+/// Every role's tenant list in a `workspace_settings.datatable` value.
+fn datatable_tenant_lists(
+    datatable: &mut serde_json::Value,
+) -> impl Iterator<Item = &mut Vec<serde_json::Value>> {
+    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/<old>` becomes `u/<new>` 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<Vec<String>> {
+async fn datatables_permissioned_on_resource(
+    db: &DB,
+    w_id: &str,
+    path: &str,
+) -> Result<Vec<String>> {
     let datatables: std::collections::HashMap<String, DataTable> = 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!({