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 f30bf9ff51..b7b60612cf 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -9317,6 +9317,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(()) }