fix(datatables): durable revocation across re-points and deletion, race-free config saves

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013fBWmrMs2DbpU1iYdCvGX6

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

@@ -221,13 +221,12 @@ async fn set_datatable_permissions(
     validate_permissions_config(&perms)?;
 
     let mut tx = db.begin().await?;
-    if perms.enabled {
-        // Serialize the exclusivity scan with every other writer of datatable
-        // configs (same lock in `edit_datatable_config`): two concurrent
-        // enables/creations targeting the same physical database must not both
-        // pass the pre-write check. Held until commit.
-        sqlx::query(SHARED_DB_CHECK_LOCK).execute(&mut *tx).await?;
-    }
+    // Serialize with every other writer of datatable configs (same lock in
+    // `edit_datatable_config`, which rewrites the whole document): two
+    // concurrent enables targeting the same physical database must not both
+    // pass the exclusivity scan, and a disable must not be silently restored
+    // by a concurrent config save's stale snapshot. Held until commit.
+    sqlx::query(SHARED_DB_CHECK_LOCK).execute(&mut *tx).await?;
     // Load the config only under the lock: a concurrent config edit could
     // otherwise re-point the data table between validation and the write,
     // making the exclusivity/CREATEROLE checks judge a stale database.

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

@@ -2975,6 +2975,15 @@ async fn edit_datatable_config(
 
     let mut tx = db.begin().await?;
 
+    // Serialized with every permissions/config writer (held until commit)
+    // BEFORE reading the old config: this save writes the whole datatable
+    // document back, so an unserialized concurrent permissions edit would be
+    // silently overwritten by the stale snapshot read below. The same lock
+    // also makes the shared-database exclusivity scan below race-free.
+    sqlx::query(crate::datatable_permissions_api::SHARED_DB_CHECK_LOCK)
+        .execute(&mut *tx)
+        .await?;
+
     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",
@@ -3034,12 +3043,8 @@ async fn edit_datatable_config(
     // A permissions-enabled data table must be the only config pointing at its
     // physical database (enforcement is per database, config is per data
     // table). Check every data table whose database is new or changed, against
-    // both the stored global state and the other entries of this request.
-    // Serialized with every other config writer (held until commit) so two
-    // concurrent saves can't both pass the exclusivity scan.
-    sqlx::query(crate::datatable_permissions_api::SHARED_DB_CHECK_LOCK)
-        .execute(&mut *tx)
-        .await?;
+    // both the stored global state and the other entries of this request
+    // (race-free under the lock taken above).
     let mut default_disabled: Vec<String> = vec![];
     let mut database_changed_names: Vec<String> = vec![];
     for (name, dt) in new_config.settings.datatables.iter() {
@@ -3120,6 +3125,23 @@ async fn edit_datatable_config(
                     // creation of a second config on a shared
                     // (non-permissioned) database.
                     default_disabled.push(name.clone());
+                    self_enabled = false;
+                } else {
+                    return Err(e);
+                }
+            }
+        }
+        // An external target must be able to create the enforcement roles.
+        // Instance targets are skipped: their shared role's CREATEROLE is
+        // cluster-wide (granted at provisioning, re-checked at grant-save),
+        // and the instance database may legitimately not be set up yet when
+        // the config is saved.
+        if self_enabled && dt.database.resource_type == DataTableCatalogResourceType::Postgresql {
+            if let Err(e) =
+                crate::datatable_permissions_api::check_owner_can_create_roles(&db, &w_id, dt).await
+            {
+                if self_defaulted {
+                    default_disabled.push(name.clone());
                 } else {
                     return Err(e);
                 }

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

@@ -924,13 +924,20 @@ pub(crate) async fn delete_workspace(
             vec![]
         });
 
-    // Drop the workspace's data table ephemeral roles while its datatable
-    // config (needed to reach their databases) still exists; the bookkeeping
-    // rows themselves cascade with the workspace row.
-    windmill_common::datatable_permissions::drop_datatable_ephemeral_roles_best_effort(
-        &db, &w_id, None,
-    )
-    .await;
+    // Revoke the workspace's data table ephemeral roles while the bookkeeping
+    // rows and the workspace key (which encrypts their recorded targets) still
+    // exist — both cascade with the workspace row, after which an unrevoked
+    // role could never be reached again. Failure (e.g. an unreachable external
+    // cluster) aborts the deletion; retry once the database is reachable.
+    windmill_common::datatable_permissions::teardown_datatable_roles_strict(&db, &w_id)
+        .await
+        .map_err(|e| {
+            Error::internal_err(format!(
+                "cannot delete workspace {w_id}: {e}. Retry when the data table database(s) \
+                 are reachable, or clear the datatable_ephemeral_role entries manually if a \
+                 database is permanently gone."
+            ))
+        })?;
 
     sqlx::query!("DELETE FROM ai_agent_memory WHERE workspace_id = $1", &w_id)
         .execute(&mut *tx)

backend/windmill-common/src/datatable_permissions.rs

@@ -222,6 +222,10 @@ fn folder_access_satisfies(
 /// `g/<group>`). Returns the matched statements plus the group memberships
 /// they were resolved through (folded into the perms hash so membership
 /// changes invalidate the role).
+///
+/// Authorization: does not authenticate the supplied identity — callers MUST
+/// pass a `permissioned_as` they have verified belongs to the caller (a job's
+/// `permissioned_as`, or the authed user's own username).
 pub async fn compute_effective_grants(
     db: &DB,
     w_id: &str,
@@ -686,21 +690,24 @@ async fn ensure_ephemeral_role(
 
     // If the data table's database moved since the role was created (resource
     // edit, config re-point), the role still exists on the PREVIOUS cluster
-    // with its old grants — revoke it there first, using the stored owner
-    // credentials (best-effort: the old cluster may be unreachable, in which
-    // case the role at least keeps only its old-cluster grants and any active
-    // holder is handled by the NOLOGIN path when reachable again).
+    // with its old grants — revoke it there first. Failure is fatal, not
+    // logged: continuing would overwrite the stored target with the new
+    // cluster's and lose the only pointer to the unrevoked role. Failing
+    // keeps the row (old target intact) so the next access or sweep retries.
     let stored_creds = existing.and_then(|r| r.owner_creds);
     if let Some(stored) = decode_stored_target(db, w_id, stored_creds).await {
         if !same_target(&stored.pg, owner) {
-            if let Err(e) =
-                drop_or_disable_on_target(db, &stored.pg, stored.is_instance, &role).await
-            {
-                tracing::warn!(
-                    "revoking ephemeral role {role} on its previous cluster {}: {e:#}",
-                    stored.pg.host
-                );
-            }
+            drop_or_disable_on_target(db, &stored.pg, stored.is_instance, &role)
+                .await
+                .map_err(|e| {
+                    Error::internal_err(format!(
+                        "cannot revoke this data table's previous credentials on its former \
+                         database ({}/{}): {e:#}. Access stays blocked until that database is \
+                         reachable; if it is permanently gone, ask an admin to clear the \
+                         datatable_ephemeral_role entry.",
+                        stored.pg.host, stored.pg.dbname
+                    ))
+                })?;
         }
     }
 
@@ -904,38 +911,57 @@ async fn cleanup_one_expired_role(db: &DB, role: &str, w_id: &str, datatable: &s
 }
 
 /// Best-effort teardown of every ephemeral role of a data table (or a whole
-/// workspace when `datatable` is `None`), used on data table deletion and
-/// workspace deletion. Roles with active sessions are skipped; their
-/// bookkeeping rows are removed regardless (workspace deletion cascades them
-/// anyway) and the roles become inert stragglers.
+/// workspace when `datatable` is `None`), used on data table deletion,
+/// database re-points and permission edits. Roles that cannot be dropped keep
+/// their bookkeeping row (with the recorded target), so the expiry sweep
+/// retries them — failure here loses nothing.
+///
+/// Authorization: mutates cluster roles; call only from admin-gated flows.
 pub async fn drop_datatable_ephemeral_roles_best_effort(
     db: &DB,
     w_id: &str,
     datatable: Option<&str>,
 ) {
-    let rows = match sqlx::query!(
+    if let Err(e) = teardown_datatable_roles(db, w_id, datatable).await {
+        tracing::warn!("tearing down datatable ephemeral roles: {e:#}");
+    }
+}
+
+/// Strict variant for workspace deletion: the bookkeeping rows (and the
+/// workspace key their targets are encrypted with) are about to cascade away,
+/// so every role must be revoked NOW — dropped, or at least stripped of LOGIN
+/// when sessions are still active. Any failure (e.g. an unreachable external
+/// cluster) must abort the deletion; proceeding would orphan a live LOGIN
+/// role with no remaining way to ever revoke it.
+///
+/// Authorization: mutates cluster roles; call only from admin-gated flows.
+pub async fn teardown_datatable_roles_strict(db: &DB, w_id: &str) -> Result<()> {
+    teardown_datatable_roles(db, w_id, None).await
+}
+
+async fn teardown_datatable_roles(db: &DB, w_id: &str, datatable: Option<&str>) -> Result<()> {
+    let rows = sqlx::query!(
         "SELECT role_name, datatable FROM datatable_ephemeral_role
          WHERE workspace_id = $1 AND ($2::text IS NULL OR datatable = $2)",
         w_id,
         datatable
     )
     .fetch_all(db)
-    .await
-    {
-        Ok(rows) => rows,
-        Err(e) => {
-            tracing::warn!("listing datatable ephemeral roles for teardown: {e:#}");
-            return;
-        }
-    };
+    .await?;
+    let mut failures: Vec<String> = vec![];
     for row in rows {
         if let Err(e) = teardown_role(db, w_id, &row.datatable, &row.role_name).await {
-            tracing::warn!(
-                "tearing down datatable ephemeral role {}: {e:#}",
-                row.role_name
-            );
+            failures.push(format!("{} ({}): {e:#}", row.role_name, row.datatable));
         }
     }
+    if failures.is_empty() {
+        Ok(())
+    } else {
+        Err(Error::internal_err(format!(
+            "could not revoke data table role(s): {}",
+            failures.join("; ")
+        )))
+    }
 }
 
 /// Strip a live role's ability to open new connections. An ordinary role can
@@ -961,21 +987,40 @@ async fn disable_role_login(client: &tokio_postgres::Client, role: &str) {
 
 /// Where a role must be revoked: the stored owner target from its bookkeeping
 /// row when present (survives resource/config re-points), else the currently
-/// configured database.
+/// configured database. When the stored and configured targets are the same
+/// physical database, the configured credentials win — a password-only
+/// rotation of the resource must not leave revocation retrying obsolete
+/// credentials forever.
 async fn resolve_role_target(
     db: &DB,
     w_id: &str,
     datatable: &str,
     owner_creds: Option<String>,
 ) -> Option<(PgDatabase, bool)> {
-    if let Some(stored) = decode_stored_target(db, w_id, owner_creds).await {
-        return Some((stored.pg, stored.is_instance));
+    let stored = decode_stored_target(db, w_id, owner_creds).await;
+    let current = match get_datatable_config(db, w_id, datatable).await {
+        Ok(config) => {
+            let is_instance =
+                config.database.resource_type == DataTableCatalogResourceType::Instance;
+            datatable_shared_resource(db, w_id, &config)
+                .await
+                .ok()
+                .and_then(|v| serde_json::from_value::<PgDatabase>(v).ok())
+                .map(|owner| (owner, is_instance))
+        }
+        Err(_) => None,
+    };
+    match (stored, current) {
+        (Some(stored), Some((current, current_is_instance))) => {
+            if same_target(&stored.pg, &current) {
+                Some((current, current_is_instance))
+            } else {
+                Some((stored.pg, stored.is_instance))
+            }
+        }
+        (Some(stored), None) => Some((stored.pg, stored.is_instance)),
+        (None, current) => current,
     }
-    let config = get_datatable_config(db, w_id, datatable).await.ok()?;
-    let owner: PgDatabase =
-        serde_json::from_value(datatable_shared_resource(db, w_id, &config).await.ok()?).ok()?;
-    let is_instance = config.database.resource_type == DataTableCatalogResourceType::Instance;
-    Some((owner, is_instance))
 }
 
 async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Result<()> {

backend/windmill-common/src/workspaces.rs

@@ -1091,6 +1091,10 @@ fn datatable_not_found_error(name: &str, datatables: Option<&serde_json::Value>)
 }
 
 /// Load a data table's config from workspace settings.
+///
+/// Authorization: performs none — the returned config includes the grant
+/// statements and database coordinates (no secrets). Callers exposing it to a
+/// user must gate on workspace membership/admin themselves.
 pub async fn get_datatable_config(db: &DB, w_id: &str, name: &str) -> Result<DataTable> {
     let datatables = sqlx::query_scalar!(
         r#"