From 189bc3297b59c5dc3d644a825730d39df559c787 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 8 Sep 2026 21:04:18 +0200 Subject: [PATCH] fix(datatables): cascade on the leave route that is used, gate migrations before the admin connection, and drop a role atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tenant cascade on leaving went onto `/users/leave`. The UI and the generated client call `/workspaces/leave` — a different handler in a different crate with the same name — which deleted the membership and left `u/` in the tenant lists. Leaving and rejoining therefore restored the access the leave was supposed to end, and a later account taking the username would have inherited it. The regression test drives the route the client actually calls; without the fix it fails with "leaving kept the tenant". The migration endpoints authorized too late. `run_datatable_migrations` opened the data table's admin connection, created `_wm_migrations` and read it before reaching the per-migration role check — so with nothing pending, nothing was checked at all. Rollback returned before its check when nothing was applied, and the status endpoint had none. All three now ask, before any connection is opened, whether the caller can reach the data table as any role at all; which role a given migration runs as is still decided per migration, and by the executor after that. Deleting a role committed the cluster drop and the catalog row, then swept the tenant lists in separate transactions. A sweep failing part-way left workspaces naming a role nothing can connect as, while the retry answered `NotFound` because the catalog entry was already gone. The sweep now runs in the same transaction, so the drop, the row and every tenant list commit together. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR --- .../tests/datatable_roles.rs | 23 +++++++++- backend/windmill-api-settings/src/lib.rs | 4 +- .../src/datatable_migrations.rs | 17 +++++++- .../src/datatable_permissions.rs | 42 +++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 8 ++++ backend/windmill-common/src/workspaces.rs | 18 +++++--- 6 files changed, 101 insertions(+), 11 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index feb9aa1b3e..c7621e9996 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -63,9 +63,30 @@ async fn freeing_a_principal_takes_its_datatable_tenant(db: Pool) -> a .await?; assert_eq!(resp.status(), 200, "delete user: {}", resp.text().await?); + // Leaving is the other way a membership ends, and there are two `/leave` routes — the one the + // UI and the generated client call is this one. A tenant left behind here comes back with the + // person on rejoin, or attaches to whoever takes the username next. + sqlx::query( + r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, + '{datatables,main,permissions,roles,role1,tenants}', '["u/test-user-3"]'::jsonb) + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + let resp = authed( + client().post(format!("{base}/workspaces/leave")), + "SECRET_TOKEN_3", + ) + .send() + .await?; + assert_eq!(resp.status(), 200, "leave: {}", resp.text().await?); // Nothing left naming a principal that no longer exists: a later group or account reusing one // of those names must not inherit the access this one had. - assert!(tenants(&db, "test-workspace").await.is_empty()); + assert!( + tenants(&db, "test-workspace").await.is_empty(), + "leaving kept the tenant: {:?}", + tenants(&db, "test-workspace").await + ); Ok(()) } diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 768cdaa249..fc3dddde2a 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -2774,10 +2774,8 @@ async fn delete_datatable_role( 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?; + windmill_common::workspaces::forget_datatable_role_everywhere(&mut tx, &id).await?; tx.commit().await?; - // After the drop commits: a tenant naming a role that still exists is harmless, one naming a - // role that is gone is not, so this only ever runs once the cluster agrees it is gone. - windmill_common::workspaces::forget_datatable_role_everywhere(&db, &id).await?; audit_log( &db, diff --git a/backend/windmill-api-workspaces/src/datatable_migrations.rs b/backend/windmill-api-workspaces/src/datatable_migrations.rs index 266d5fb0cc..7e60d8ff89 100644 --- a/backend/windmill-api-workspaces/src/datatable_migrations.rs +++ b/backend/windmill-api-workspaces/src/datatable_migrations.rs @@ -432,6 +432,11 @@ async fn run_datatable_migrations( Path((w_id, datatable_name)): Path<(String, String)>, Query(query): Query, ) -> JsonResult { + // Before the admin connection is opened at all: the bookkeeping below is created and read + // through it, so a caller no role covers must be refused here rather than after the fact. + crate::datatable_permissions::ensure_reaches_datatable(&db, &w_id, &datatable_name, &authed) + .await?; + audit_log( &db, &authed, @@ -564,6 +569,11 @@ async fn rollback_datatable_migrations( Path((w_id, datatable_name)): Path<(String, String)>, Query(query): Query, ) -> JsonResult { + // Before the admin connection is opened at all: the bookkeeping below is created and read + // through it, so a caller no role covers must be refused here rather than after the fact. + crate::datatable_permissions::ensure_reaches_datatable(&db, &w_id, &datatable_name, &authed) + .await?; + audit_log( &db, &authed, @@ -817,10 +827,15 @@ async fn read_applied_datatable_versions( /// List a data table's migrations annotated with whether each has been applied. async fn datatable_migrations_status( - _authed: ApiAuthed, + authed: ApiAuthed, Extension(db): Extension, Path((w_id, datatable_name)): Path<(String, String)>, ) -> JsonResult { + // Reads `_wm_migrations` through the data table's admin connection, so it answers to the same + // question as running one: may you reach this data table at all. + crate::datatable_permissions::ensure_reaches_datatable(&db, &w_id, &datatable_name, &authed) + .await?; + let enabled = datatable_migrations_enabled(&db, &w_id, &datatable_name).await?; if !enabled { return Ok(Json(DatatableMigrationsStatusResult { diff --git a/backend/windmill-api-workspaces/src/datatable_permissions.rs b/backend/windmill-api-workspaces/src/datatable_permissions.rs index 861df58f53..a1493c4b98 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions.rs @@ -135,6 +135,48 @@ pub(crate) async fn ensure_governs_datatable( ))) } +/// Refuse a caller that no tenant of this data table covers. +/// +/// The bookkeeping endpoints below open the data table's `admin` connection to read or create +/// `_wm_migrations` before they know which migration will run — so without this, someone covered +/// by no role at all can still force admin-backed reads and writes on a database they may not +/// touch. It asks only "may you reach this data table as anything"; which role a given migration +/// runs as is still decided per migration, and by the executor after that. +pub(crate) async fn ensure_reaches_datatable( + db: &DB, + w_id: &str, + datatable_name: &str, + authed: &ApiAuthed, +) -> Result<()> { + let governing = resolve_governing_datatable(db, w_id, datatable_name).await?; + let Some(permissions) = governing.datatable.permissions.as_ref() else { + return Ok(()); + }; + let catalog = read_role_catalog(db).await?; + let access = DatatableAccess::Authed(authed.to_authed_ref()); + for (id, tenants) in &permissions.roles { + // A role the instance no longer defines, or has disabled, cannot be connected as, so being + // tenanted into it is not reach. + if id != ADMIN_DATATABLE_ROLE && !catalog.get(id).is_some_and(|r| r.enabled) { + continue; + } + if can_use_datatable_role_in_governing_workspace( + db, + &governing.workspace_id, + w_id, + tenants, + &access, + ) + .await? + { + return Ok(()); + } + } + Err(Error::NotAuthorized(format!( + "Not allowed to use data table '{datatable_name}': no role of it covers you" + ))) +} + fn validate_tenant(tenant: &str) -> Result<()> { if tenant == DATATABLE_TENANT_WILDCARD { return Ok(()); diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 302c8666af..ef7c832e1c 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -9233,6 +9233,14 @@ async fn leave_workspace( ) -> Result { windmill_api_auth::forbid_job_token_account_destruction(&authed)?; let mut tx = db.begin().await?; + // The membership is what made `u/` mean this person. Leaving it behind in a tenant + // list would hand their data table access back on rejoin, or to whoever takes the name next. + windmill_common::workspaces::remove_datatable_tenant_in_workspace( + &mut tx, + &w_id, + &format!("u/{}", authed.username), + ) + .await?; sqlx::query!( "DELETE FROM usr WHERE workspace_id = $1 AND email = $2", &w_id, diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 20fe602a41..6201e367ed 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -2045,19 +2045,26 @@ pub async fn rename_datatable_tenant_in_workspace( /// naming a role that no longer exists. /// /// Authorization: reaches every workspace on the instance. Callers MUST be the superadmin path -/// that just dropped the role from the cluster — it exists to follow that, not to edit tenants. A data table whose default role was the deleted one falls +/// dropping the role from the cluster — it exists to follow that, not to edit tenants. +/// +/// Takes that path's transaction rather than opening its own: run afterwards, a failure part-way +/// leaves the catalog row already gone, so the retry answers `NotFound` while some workspaces +/// still name a role nothing can connect as. In the transaction, the cluster drop, the catalog row +/// and every tenant list commit together or not at all. A data table whose default role was the deleted one falls /// back to `admin` — the one role that is always present. -pub async fn forget_datatable_role_everywhere(db: &DB, role_id: &str) -> Result<()> { +pub async fn forget_datatable_role_everywhere( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + role_id: &str, +) -> Result<()> { let workspaces = sqlx::query_scalar!( "SELECT workspace_id FROM workspace_settings WHERE datatable::text LIKE $1", format!("%{}%", role_id) ) - .fetch_all(db) + .fetch_all(&mut **tx) .await?; for w_id in workspaces { - let mut tx = db.begin().await?; - update_datatable_permissions_in_workspace(&mut tx, &w_id, |permissions| { + update_datatable_permissions_in_workspace(tx, &w_id, |permissions| { let mut touched = permissions.roles.remove(role_id).is_some(); if permissions.default_role.as_deref() == Some(role_id) { permissions.default_role = Some(ADMIN_DATATABLE_ROLE.to_string()); @@ -2066,7 +2073,6 @@ pub async fn forget_datatable_role_everywhere(db: &DB, role_id: &str) -> Result< touched }) .await?; - tx.commit().await?; } Ok(()) }