fix(datatables): disable a data table role before deleting it

Deleting a role reassigns and drops what it owns in each registered
database on its own connection, and each of those passes commits as it
goes. A database failing part-way left the role enabled in the catalog and
able to log in, but already stripped in the databases reached before it.
The role is now disabled in its own commit first, so a failed delete
leaves a disabled role to retry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/windmill-api-integration-tests/tests/datatable_roles.rs

@@ -407,6 +407,76 @@ async fn concurrent_role_creations_both_survive(db: Pool<Postgres>) -> anyhow::R
     outcome
 }
 
+#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))]
+async fn a_role_delete_that_fails_part_way_leaves_the_role_disabled(
+    db: Pool<Postgres>,
+) -> anyhow::Result<()> {
+    initialize_tracing().await;
+    let server = ApiServer::start(db.clone()).await?;
+    let port = server.addr.port();
+    let suffix: String = uuid::Uuid::new_v4().simple().to_string()[..8].to_string();
+    let name = format!("wmtest_del_{suffix}");
+
+    let outcome = async {
+        let created: Value = authed(
+            client().post(format!(
+                "http://localhost:{port}/api/settings/datatable_roles"
+            )),
+            "SECRET_TOKEN",
+        )
+        .json(&json!({ "name": name }))
+        .send()
+        .await?
+        .error_for_status()?
+        .json()
+        .await?;
+        let id = created["id"].as_str().unwrap().to_string();
+
+        // Each database's pass commits on its own, so one that cannot be reached fails the delete
+        // after the others may already have stripped the role.
+        sqlx::query(
+            "UPDATE global_settings SET value = jsonb_set(value, '{databases,wm_unreachable}', '{}')
+             WHERE name = 'custom_instance_pg_databases'",
+        )
+        .execute(&db)
+        .await?;
+
+        let resp = authed(
+            client().delete(format!(
+                "http://localhost:{port}/api/settings/datatable_roles/{id}"
+            )),
+            "SECRET_TOKEN",
+        )
+        .send()
+        .await?;
+        let status = resp.status();
+        let body = resp.text().await?;
+        assert_eq!(status, 400, "{body}");
+
+        let catalog = windmill_common::datatable_roles::read_role_catalog(&db).await?;
+        let role = catalog
+            .get(&id)
+            .expect("a failed delete keeps the entry to retry");
+        assert!(
+            !role.enabled,
+            "a half-deleted role is still enabled in the catalog"
+        );
+        let can_login: bool =
+            sqlx::query_scalar("SELECT rolcanlogin FROM pg_roles WHERE rolname = $1")
+                .bind(&name)
+                .fetch_one(&db)
+                .await?;
+        assert!(!can_login, "a half-deleted role can still log in");
+        Ok::<_, anyhow::Error>(())
+    }
+    .await;
+
+    let _ = sqlx::query(&format!("DROP ROLE IF EXISTS \"{name}\""))
+        .execute(&db)
+        .await;
+    outcome
+}
+
 #[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))]
 async fn renaming_a_governing_data_table_carries_its_forks(
     db: Pool<Postgres>,

backend/windmill-api-settings/src/lib.rs

@@ -150,7 +150,10 @@ pub fn global_service() -> Router {
             "/list_custom_instance_pg_databases",
             post(list_custom_instance_pg_databases),
         )
-        .route("/datatable_roles", get(list_datatable_roles).post(create_datatable_role))
+        .route(
+            "/datatable_roles",
+            get(list_datatable_roles).post(create_datatable_role),
+        )
         .route(
             "/datatable_roles/{id}",
             post(update_datatable_role).delete(delete_datatable_role),
@@ -2651,7 +2654,11 @@ async fn create_datatable_role(
     )
     .await?;
 
-    Ok(Json(DatatableRoleInfo { id, name: req.name, enabled: true }))
+    Ok(Json(DatatableRoleInfo {
+        id,
+        name: req.name,
+        enabled: true,
+    }))
 }
 
 async fn update_datatable_role(
@@ -2714,24 +2721,45 @@ async fn update_datatable_role(
     }))
 }
 
-/// Drop the Postgres role, then forget it, then strip it from every workspace that tenanted it.
+/// Disable the role in its own commit, then drop the Postgres role, then forget it, then strip it
+/// from every workspace that tenanted it.
 ///
-/// Dropping first is what makes the catalog trustworthy: the drop refuses while any instance
-/// database is unreachable, so a failure leaves the entry in place to retry rather than a live
-/// Postgres login nothing names.
+/// Dropping before forgetting is what makes the catalog trustworthy: the drop refuses while any
+/// instance database is unreachable, so a failure leaves the entry in place to retry rather than a
+/// live Postgres login nothing names.
 async fn delete_datatable_role(
     authed: ApiAuthed,
     Extension(db): Extension<DB>,
     Path(id): Path<String>,
 ) -> JsonResult<()> {
     require_super_admin(&db, &authed).await?;
+    let find = |catalog: &windmill_common::datatable_roles::DatatableRoleCatalog| {
+        catalog
+            .get(&id)
+            .cloned()
+            .ok_or_else(|| error::Error::NotFound(format!("No data table role with id '{id}'")))
+    };
+
     let mut tx = db.begin().await?;
     windmill_common::datatable_roles::lock_role_catalog(&mut tx).await?;
-    let catalog = windmill_common::datatable_roles::read_role_catalog_tx(&mut tx).await?;
-    let role = catalog
-        .get(&id)
-        .cloned()
-        .ok_or_else(|| error::Error::NotFound(format!("No data table role with id '{id}'")))?;
+    let mut role = find(&windmill_common::datatable_roles::read_role_catalog_tx(&mut tx).await?)?;
+    if role.enabled {
+        windmill_common::datatable_roles::set_instance_role_login(&mut tx, &role.name, false)
+            .await?;
+        role.enabled = false;
+        windmill_common::datatable_roles::update_role_catalog_entry(&mut tx, &id, &role).await?;
+    }
+    tx.commit().await?;
+
+    let mut tx = db.begin().await?;
+    windmill_common::datatable_roles::lock_role_catalog(&mut tx).await?;
+    let role = find(&windmill_common::datatable_roles::read_role_catalog_tx(&mut tx).await?)?;
+    if role.enabled {
+        return Err(error::Error::BadRequest(format!(
+            "Data table role '{}' was re-enabled while being deleted",
+            role.name
+        )));
+    }
 
     windmill_common::datatable_roles::drop_instance_role(&db, &mut tx, &role.name).await?;
     windmill_common::datatable_roles::delete_role_catalog_entry(&mut tx, &id).await?;
@@ -2771,7 +2799,9 @@ async fn converge_connect_grants_everywhere(
             windmill_common::datatable_roles::converge_connect_grants_with(db, &dbname, catalog)
                 .await
         {
-            tracing::warn!("Could not converge CONNECT grants on instance database '{dbname}': {e}");
+            tracing::warn!(
+                "Could not converge CONNECT grants on instance database '{dbname}': {e}"
+            );
         }
     }
 }

backend/windmill-common/src/datatable_roles.rs

@@ -403,7 +403,9 @@ pub async fn rename_instance_role(
 ///
 /// The per-database passes open their own connections and cannot join `tx`; the lock is what keeps
 /// a concurrent mutation out while they run. Only the final `DROP ROLE` is on `tx`, so it commits
-/// or rolls back with the catalog write that forgets the role.
+/// or rolls back with the catalog write that forgets the role. Those passes commit as they go, so
+/// callers MUST have disabled the role in an earlier committed transaction: a failure part-way
+/// then leaves a disabled role to retry, not an enabled one already stripped in some databases.
 pub async fn drop_instance_role(
     db: &DB,
     tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,