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