diff --git a/backend/windmill-api-workspaces/src/datatable_permissions_api.rs b/backend/windmill-api-workspaces/src/datatable_permissions_api.rs index d3f92293ce..71b59d7984 100644 --- a/backend/windmill-api-workspaces/src/datatable_permissions_api.rs +++ b/backend/windmill-api-workspaces/src/datatable_permissions_api.rs @@ -221,13 +221,12 @@ async fn set_datatable_permissions( validate_permissions_config(&perms)?; let mut tx = db.begin().await?; - if perms.enabled { - // Serialize the exclusivity scan with every other writer of datatable - // configs (same lock in `edit_datatable_config`): two concurrent - // enables/creations targeting the same physical database must not both - // pass the pre-write check. Held until commit. - sqlx::query(SHARED_DB_CHECK_LOCK).execute(&mut *tx).await?; - } + // Serialize with every other writer of datatable configs (same lock in + // `edit_datatable_config`, which rewrites the whole document): two + // concurrent enables targeting the same physical database must not both + // pass the exclusivity scan, and a disable must not be silently restored + // by a concurrent config save's stale snapshot. Held until commit. + sqlx::query(SHARED_DB_CHECK_LOCK).execute(&mut *tx).await?; // Load the config only under the lock: a concurrent config edit could // otherwise re-point the data table between validation and the write, // making the exclusivity/CREATEROLE checks judge a stale database. diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index f13913a288..2dd7b75284 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -2975,6 +2975,15 @@ async fn edit_datatable_config( let mut tx = db.begin().await?; + // Serialized with every permissions/config writer (held until commit) + // BEFORE reading the old config: this save writes the whole datatable + // document back, so an unserialized concurrent permissions edit would be + // silently overwritten by the stale snapshot read below. The same lock + // also makes the shared-database exclusivity scan below race-free. + sqlx::query(crate::datatable_permissions_api::SHARED_DB_CHECK_LOCK) + .execute(&mut *tx) + .await?; + let old_datatables: HashMap = serde_json::from_value( sqlx::query_scalar!( "SELECT ws.datatable->'datatables' FROM workspace_settings ws WHERE ws.workspace_id = $1", @@ -3034,12 +3043,8 @@ async fn edit_datatable_config( // A permissions-enabled data table must be the only config pointing at its // physical database (enforcement is per database, config is per data // table). Check every data table whose database is new or changed, against - // both the stored global state and the other entries of this request. - // Serialized with every other config writer (held until commit) so two - // concurrent saves can't both pass the exclusivity scan. - sqlx::query(crate::datatable_permissions_api::SHARED_DB_CHECK_LOCK) - .execute(&mut *tx) - .await?; + // both the stored global state and the other entries of this request + // (race-free under the lock taken above). let mut default_disabled: Vec = vec![]; let mut database_changed_names: Vec = vec![]; for (name, dt) in new_config.settings.datatables.iter() { @@ -3120,6 +3125,23 @@ async fn edit_datatable_config( // creation of a second config on a shared // (non-permissioned) database. default_disabled.push(name.clone()); + self_enabled = false; + } else { + return Err(e); + } + } + } + // An external target must be able to create the enforcement roles. + // Instance targets are skipped: their shared role's CREATEROLE is + // cluster-wide (granted at provisioning, re-checked at grant-save), + // and the instance database may legitimately not be set up yet when + // the config is saved. + if self_enabled && dt.database.resource_type == DataTableCatalogResourceType::Postgresql { + if let Err(e) = + crate::datatable_permissions_api::check_owner_can_create_roles(&db, &w_id, dt).await + { + if self_defaulted { + default_disabled.push(name.clone()); } else { return Err(e); } diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index 2a120d08f1..9047fe6dc6 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -924,13 +924,20 @@ pub(crate) async fn delete_workspace( vec![] }); - // Drop the workspace's data table ephemeral roles while its datatable - // config (needed to reach their databases) still exists; the bookkeeping - // rows themselves cascade with the workspace row. - windmill_common::datatable_permissions::drop_datatable_ephemeral_roles_best_effort( - &db, &w_id, None, - ) - .await; + // Revoke the workspace's data table ephemeral roles while the bookkeeping + // rows and the workspace key (which encrypts their recorded targets) still + // exist — both cascade with the workspace row, after which an unrevoked + // role could never be reached again. Failure (e.g. an unreachable external + // cluster) aborts the deletion; retry once the database is reachable. + windmill_common::datatable_permissions::teardown_datatable_roles_strict(&db, &w_id) + .await + .map_err(|e| { + Error::internal_err(format!( + "cannot delete workspace {w_id}: {e}. Retry when the data table database(s) \ + are reachable, or clear the datatable_ephemeral_role entries manually if a \ + database is permanently gone." + )) + })?; sqlx::query!("DELETE FROM ai_agent_memory WHERE workspace_id = $1", &w_id) .execute(&mut *tx) diff --git a/backend/windmill-common/src/datatable_permissions.rs b/backend/windmill-common/src/datatable_permissions.rs index c1b9194d8c..b50a3f96d2 100644 --- a/backend/windmill-common/src/datatable_permissions.rs +++ b/backend/windmill-common/src/datatable_permissions.rs @@ -222,6 +222,10 @@ fn folder_access_satisfies( /// `g/`). Returns the matched statements plus the group memberships /// they were resolved through (folded into the perms hash so membership /// changes invalidate the role). +/// +/// Authorization: does not authenticate the supplied identity — callers MUST +/// pass a `permissioned_as` they have verified belongs to the caller (a job's +/// `permissioned_as`, or the authed user's own username). pub async fn compute_effective_grants( db: &DB, w_id: &str, @@ -686,21 +690,24 @@ async fn ensure_ephemeral_role( // If the data table's database moved since the role was created (resource // edit, config re-point), the role still exists on the PREVIOUS cluster - // with its old grants — revoke it there first, using the stored owner - // credentials (best-effort: the old cluster may be unreachable, in which - // case the role at least keeps only its old-cluster grants and any active - // holder is handled by the NOLOGIN path when reachable again). + // with its old grants — revoke it there first. Failure is fatal, not + // logged: continuing would overwrite the stored target with the new + // cluster's and lose the only pointer to the unrevoked role. Failing + // keeps the row (old target intact) so the next access or sweep retries. let stored_creds = existing.and_then(|r| r.owner_creds); if let Some(stored) = decode_stored_target(db, w_id, stored_creds).await { if !same_target(&stored.pg, owner) { - if let Err(e) = - drop_or_disable_on_target(db, &stored.pg, stored.is_instance, &role).await - { - tracing::warn!( - "revoking ephemeral role {role} on its previous cluster {}: {e:#}", - stored.pg.host - ); - } + drop_or_disable_on_target(db, &stored.pg, stored.is_instance, &role) + .await + .map_err(|e| { + Error::internal_err(format!( + "cannot revoke this data table's previous credentials on its former \ + database ({}/{}): {e:#}. Access stays blocked until that database is \ + reachable; if it is permanently gone, ask an admin to clear the \ + datatable_ephemeral_role entry.", + stored.pg.host, stored.pg.dbname + )) + })?; } } @@ -904,38 +911,57 @@ async fn cleanup_one_expired_role(db: &DB, role: &str, w_id: &str, datatable: &s } /// Best-effort teardown of every ephemeral role of a data table (or a whole -/// workspace when `datatable` is `None`), used on data table deletion and -/// workspace deletion. Roles with active sessions are skipped; their -/// bookkeeping rows are removed regardless (workspace deletion cascades them -/// anyway) and the roles become inert stragglers. +/// workspace when `datatable` is `None`), used on data table deletion, +/// database re-points and permission edits. Roles that cannot be dropped keep +/// their bookkeeping row (with the recorded target), so the expiry sweep +/// retries them — failure here loses nothing. +/// +/// Authorization: mutates cluster roles; call only from admin-gated flows. pub async fn drop_datatable_ephemeral_roles_best_effort( db: &DB, w_id: &str, datatable: Option<&str>, ) { - let rows = match sqlx::query!( + if let Err(e) = teardown_datatable_roles(db, w_id, datatable).await { + tracing::warn!("tearing down datatable ephemeral roles: {e:#}"); + } +} + +/// Strict variant for workspace deletion: the bookkeeping rows (and the +/// workspace key their targets are encrypted with) are about to cascade away, +/// so every role must be revoked NOW — dropped, or at least stripped of LOGIN +/// when sessions are still active. Any failure (e.g. an unreachable external +/// cluster) must abort the deletion; proceeding would orphan a live LOGIN +/// role with no remaining way to ever revoke it. +/// +/// Authorization: mutates cluster roles; call only from admin-gated flows. +pub async fn teardown_datatable_roles_strict(db: &DB, w_id: &str) -> Result<()> { + teardown_datatable_roles(db, w_id, None).await +} + +async fn teardown_datatable_roles(db: &DB, w_id: &str, datatable: Option<&str>) -> Result<()> { + let rows = sqlx::query!( "SELECT role_name, datatable FROM datatable_ephemeral_role WHERE workspace_id = $1 AND ($2::text IS NULL OR datatable = $2)", w_id, datatable ) .fetch_all(db) - .await - { - Ok(rows) => rows, - Err(e) => { - tracing::warn!("listing datatable ephemeral roles for teardown: {e:#}"); - return; - } - }; + .await?; + let mut failures: Vec = vec![]; for row in rows { if let Err(e) = teardown_role(db, w_id, &row.datatable, &row.role_name).await { - tracing::warn!( - "tearing down datatable ephemeral role {}: {e:#}", - row.role_name - ); + failures.push(format!("{} ({}): {e:#}", row.role_name, row.datatable)); } } + if failures.is_empty() { + Ok(()) + } else { + Err(Error::internal_err(format!( + "could not revoke data table role(s): {}", + failures.join("; ") + ))) + } } /// Strip a live role's ability to open new connections. An ordinary role can @@ -961,21 +987,40 @@ async fn disable_role_login(client: &tokio_postgres::Client, role: &str) { /// Where a role must be revoked: the stored owner target from its bookkeeping /// row when present (survives resource/config re-points), else the currently -/// configured database. +/// configured database. When the stored and configured targets are the same +/// physical database, the configured credentials win — a password-only +/// rotation of the resource must not leave revocation retrying obsolete +/// credentials forever. async fn resolve_role_target( db: &DB, w_id: &str, datatable: &str, owner_creds: Option, ) -> Option<(PgDatabase, bool)> { - if let Some(stored) = decode_stored_target(db, w_id, owner_creds).await { - return Some((stored.pg, stored.is_instance)); + let stored = decode_stored_target(db, w_id, owner_creds).await; + let current = match get_datatable_config(db, w_id, datatable).await { + Ok(config) => { + let is_instance = + config.database.resource_type == DataTableCatalogResourceType::Instance; + datatable_shared_resource(db, w_id, &config) + .await + .ok() + .and_then(|v| serde_json::from_value::(v).ok()) + .map(|owner| (owner, is_instance)) + } + Err(_) => None, + }; + match (stored, current) { + (Some(stored), Some((current, current_is_instance))) => { + if same_target(&stored.pg, ¤t) { + Some((current, current_is_instance)) + } else { + Some((stored.pg, stored.is_instance)) + } + } + (Some(stored), None) => Some((stored.pg, stored.is_instance)), + (None, current) => current, } - let config = get_datatable_config(db, w_id, datatable).await.ok()?; - let owner: PgDatabase = - serde_json::from_value(datatable_shared_resource(db, w_id, &config).await.ok()?).ok()?; - let is_instance = config.database.resource_type == DataTableCatalogResourceType::Instance; - Some((owner, is_instance)) } async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Result<()> { diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 4a0c6cb09e..4820749dac 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1091,6 +1091,10 @@ fn datatable_not_found_error(name: &str, datatables: Option<&serde_json::Value>) } /// Load a data table's config from workspace settings. +/// +/// Authorization: performs none — the returned config includes the grant +/// statements and database coordinates (no secrets). Callers exposing it to a +/// user must gate on workspace membership/admin themselves. pub async fn get_datatable_config(db: &DB, w_id: &str, name: &str) -> Result { let datatables = sqlx::query_scalar!( r#"