From dd2ca5513ea6557a1611a73998dd7e5d950754ef Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Fri, 11 Sep 2026 11:34:51 +0200 Subject: [PATCH] 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) --- .../tests/datatable_roles.rs | 70 +++++++++++++++++++ backend/windmill-api-settings/src/lib.rs | 54 ++++++++++---- .../windmill-common/src/datatable_roles.rs | 4 +- 3 files changed, 115 insertions(+), 13 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index e619e73f23..f709637750 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -407,6 +407,76 @@ async fn concurrent_role_creations_both_survive(db: Pool) -> 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, +) -> 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, diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index fc3dddde2a..d8bcf0174f 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -151,7 +151,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), @@ -2690,7 +2693,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( @@ -2753,24 +2760,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, Path(id): Path, ) -> 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?; @@ -2810,7 +2838,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}" + ); } } } diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index ab5e519c3a..07e3e16114 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/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>,