From e5ec5217d41885b4f9f098202e3a7a7d124af7b8 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 16 Sep 2026 18:09:07 +0200 Subject: [PATCH] fix(datatables): serialize roles going on with aliases saved from other workspaces Co-Authored-By: Claude Opus 5 (1M context) --- backend/ee-repo-ref.txt | 2 +- .../tests/datatable_roles.rs | 62 ++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 84 +++++++++++-------- .../windmill-common/src/datatable_roles.rs | 19 +++++ 4 files changed, 131 insertions(+), 36 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 1d1f24af71..9717f3f96f 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -53614778ef0eec12ef22c6001c8905cd46f5ee17 +38d6fcf2aeb39cfdac21814bbdbbcc02911e566a diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 08790386a4..f12daf1953 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -1009,6 +1009,68 @@ async fn an_entry_without_roles_cannot_newly_reach_a_database_under_roles( Ok(()) } +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn an_alias_saved_elsewhere_waits_for_roles_going_on_for_its_database( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + sqlx::query( + r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, '{datatables,other}', + '{"database": {"resource_type": "instance", "resource_path": "dt_other"}}') + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&db) + .await?; + + // Roles going on for `dt_other`, not committed yet: it holds only its own workspace's settings + // row, so an alias saved from another workspace that looked for roles now would miss them. + let enabling = { + let mut tx = db.begin().await?; + windmill_common::datatable_roles::lock_instance_databases_governance( + &mut *tx, + ["dt_other"], + ) + .await?; + sqlx::query( + r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, + '{datatables,other,permissions}', + '{"default_role": "admin", "roles": {"admin": {"tenants": ["*"]}}}') + WHERE workspace_id = 'test-workspace'"#, + ) + .execute(&mut *tx) + .await?; + tx + }; + + let server = ApiServer::start(db.clone()).await?; + let url = format!( + "http://localhost:{}/api/w/wm-fork-dt/workspaces/edit_datatable_config", + server.addr.port() + ); + let save = tokio::spawn( + authed(client().post(&url), "SECRET_TOKEN") + .json(&json!({ "settings": { "datatables": { + "direct": { "database": { "resource_type": "instance", "resource_path": "dt_other" } } + } } })) + .send(), + ); + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + assert!( + !save.is_finished(), + "an alias was saved while roles were going on for its database" + ); + enabling.commit().await?; + + let resp = save.await??; + let status = resp.status(); + let body = resp.text().await?; + assert!( + status == 400 && body.contains("which a data table under roles uses"), + "the alias reached the database whose roles went on while it waited ({status}): {body}" + ); + Ok(()) +} + #[cfg(not(all(feature = "private", feature = "enterprise")))] const ENTERPRISE_REFUSAL: &str = "Data table roles are a Windmill Enterprise Edition feature"; diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 38b3797518..ed01dede99 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3891,50 +3891,64 @@ async fn edit_datatable_config( // entry through a declared rename alone, and a settings sync never declares one, so an entry // without roles that newly points at such a database — a name added, or an existing one // repointed — would answer everyone there as `admin`. That holds whichever workspace governs it. - let governed_elsewhere: Vec = sqlx::query_scalar( - "SELECT DISTINCT dt.value->'database'->>'resource_path' FROM workspace_settings ws - CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt - WHERE ws.workspace_id <> $1 AND dt.value ? 'permissions' - AND dt.value->'database'->>'resource_type' = 'instance'", + let newly_pointed: Vec<(&String, &str)> = new_config + .settings + .datatables + .iter() + .filter(|(_, dt)| dt.permissions.is_none()) + .filter_map(|(name, dt)| { + let db = dt + .database + .as_ref() + .filter(|d| d.resource_type == DataTableCatalogResourceType::Instance)?; + let lookup = rename_src + .get(name.as_str()) + .copied() + .unwrap_or(name.as_str()); + let repointed = old_datatables + .get(lookup) + .and_then(|old| old.database.as_ref()) + .is_none_or(|old_db| { + old_db.resource_type != db.resource_type + || old_db.resource_path != db.resource_path + }); + repointed.then_some((name, db.resource_path.as_str())) + }) + .collect(); + // Another workspace turning roles on for the same database holds only its own settings row, so + // without this the scan below could read past its uncommitted write. + windmill_common::datatable_roles::lock_instance_databases_governance( + &mut *tx, + newly_pointed.iter().map(|(_, dbname)| *dbname), ) - .bind(&w_id) - .fetch_all(&mut *tx) .await?; - for (name, dt) in new_config.settings.datatables.iter() { - if dt.permissions.is_some() { - continue; - } - let Some(db) = dt - .database - .as_ref() - .filter(|d| d.resource_type == DataTableCatalogResourceType::Instance) - else { - continue; - }; - let lookup = rename_src - .get(name.as_str()) - .copied() - .unwrap_or(name.as_str()); - let repointed = old_datatables - .get(lookup) - .and_then(|old| old.database.as_ref()) - .is_none_or(|old_db| { - old_db.resource_type != db.resource_type || old_db.resource_path != db.resource_path - }); + let governed_elsewhere: Vec = if newly_pointed.is_empty() { + vec![] + } else { + sqlx::query_scalar( + "SELECT DISTINCT dt.value->'database'->>'resource_path' FROM workspace_settings ws + CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt + WHERE ws.workspace_id <> $1 AND dt.value ? 'permissions' + AND dt.value->'database'->>'resource_type' = 'instance'", + ) + .bind(&w_id) + .fetch_all(&mut *tx) + .await? + }; + for (name, dbname) in newly_pointed { let governed_here = old_datatables.values().any(|old| { old.permissions.is_some() && old.database.as_ref().is_some_and(|d| { d.resource_type == DataTableCatalogResourceType::Instance - && d.resource_path == db.resource_path + && d.resource_path == dbname }) }); - if repointed && (governed_here || governed_elsewhere.contains(&db.resource_path)) { + if governed_here || governed_elsewhere.iter().any(|g| g == dbname) { return Err(Error::BadRequest(format!( - "Data table '{name}' would point at database '{}', which a data table under roles \ - uses, without carrying those roles: everyone reaching '{name}' would connect there \ - as `admin`. Rename the data table under roles from the data table settings, which \ - carries its roles, or turn its roles off first.", - db.resource_path + "Data table '{name}' would point at database '{dbname}', which a data table under \ + roles uses, without carrying those roles: everyone reaching '{name}' would connect \ + there as `admin`. Rename the data table under roles from the data table settings, \ + which carries its roles, or turn its roles off first." ))); } } diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 3c269d1ada..4dd9b06fde 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -140,6 +140,25 @@ pub async fn lock_datatable_streams(conn: &mut sqlx::PgConnection, exclusive: bo Ok(()) } +/// Whether an instance database is reached only through entries under roles is decided by two +/// writes that lock different workspaces' settings rows: turning roles on for one entry, and a +/// settings save pointing an entry without roles at the database. Each holds this for every +/// database it decides on, so neither reads past the other's uncommitted write. Held for the +/// transaction; the names are locked in sorted order so two holders cannot deadlock. +pub async fn lock_instance_databases_governance<'a>( + conn: &mut sqlx::PgConnection, + dbnames: impl IntoIterator, +) -> Result<()> { + let dbnames: std::collections::BTreeSet<&str> = dbnames.into_iter().collect(); + for dbname in dbnames { + sqlx::query("SELECT pg_advisory_xact_lock(hashtext('datatable_instance_database:' || $1))") + .bind(dbname) + .execute(&mut *conn) + .await?; + } + Ok(()) +} + /// Disclosure: returns every role's stored Postgres password in plaintext. Any server path that /// has to resolve or name a role may call it — including handlers open to a workspace member, who /// need the names — but callers MUST NOT let `pwd` reach a response, a log line, an audit record