From 1d9ee09b31625ef821fe46f117a313f538b7f15e Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 8 Sep 2026 15:30:21 +0200 Subject: [PATCH] fix(datatables): make the concurrency test pin the handlers, and the contracts describe what is enforced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The concurrency test reimplemented the read-modify-write inline, so deleting the lock from all three handlers left it green — it pinned Postgres, not the code it was written for. It now drives `create_datatable_role` twice concurrently and asserts the catalog kept both names. Checked the way the last one should have been: removing the lock from the handler makes it fail with "wmtest_a_… is a live cluster login the catalog forgot". The contracts added last commit were stricter than this PR's own callers, which is worse than none — the next reader sees a rule already broken and learns to ignore it. `read_role_catalog` said superadmin-only while two of its four callers are open to any workspace member, and `converge_connect_grants` said superadmin while `set_datatable_permissions` reaches it as a workspace admin. Both were fine on substance: the rule that actually holds is about the credential never reaching a response, log, audit record or export, not about who may call. They now say that. `read_datatable_entry` gets the same treatment rather than the one the earlier message claimed for it: it is the primitive every resolution goes through, so it is deliberately open, and what must not escape is `permissions` — it names the governing workspace's users, groups and folders. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR --- .../tests/datatable_roles.rs | 74 +++++++++++-------- .../tests/fixtures/datatable_roles.sql | 4 +- .../windmill-common/src/datatable_roles.rs | 16 ++-- backend/windmill-common/src/workspaces.rs | 8 +- 4 files changed, 58 insertions(+), 44 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 2f6d7ca37b..3101739f29 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -331,41 +331,51 @@ async fn a_caller_with_no_identity_reaches_a_permissioned_data_table_not_at_all( } #[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] -async fn concurrent_role_catalog_writes_do_not_lose_an_entry(db: Pool) -> anyhow::Result<()> { - use windmill_common::datatable_roles::{ - lock_role_catalog, read_role_catalog_tx, InstanceDatatableRole, - }; - +async fn concurrent_role_creations_both_survive(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; - // The catalog is one JSON document, so every mutation is read-modify-write. Without the lock - // two concurrent creates read the same snapshot and the second write drops the first — leaving - // the role it dropped as a live cluster login nobody recorded. No DDL here: the losable step is - // the catalog write, and that is what this pins. - let insert = |id: &'static str| { - let db = db.clone(); - async move { - let mut tx = db.begin().await?; - lock_role_catalog(&mut tx).await?; - let mut catalog = read_role_catalog_tx(&mut tx).await?; - // Widen the window the lock has to cover, so an unlocked version fails reliably rather - // than occasionally. - tokio::time::sleep(std::time::Duration::from_millis(150)).await; - catalog.insert( - id.to_string(), - InstanceDatatableRole { name: id.to_string(), enabled: true, pwd: None }, - ); - windmill_common::datatable_roles::write_role_catalog(&mut tx, &catalog).await?; - tx.commit().await?; - Ok::<_, anyhow::Error>(()) - } - }; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); - let (a, b) = tokio::join!(insert("first"), insert("second")); - a?; - b?; + // Postgres roles are cluster-wide and this cluster is shared with every other test database, + // so the names have to be unique to this run. + let suffix: String = uuid::Uuid::new_v4().simple().to_string()[..8].to_string(); + let names = [format!("wmtest_a_{suffix}"), format!("wmtest_b_{suffix}")]; + + // The catalog is one JSON document, so create is read-modify-write. Unserialized, both of + // these read the same snapshot, both `CREATE ROLE` succeeds, and the second write drops the + // first entry — leaving a live cluster login nobody recorded. + let create = |name: String| async move { + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/settings/datatable_roles" + )), + "SECRET_TOKEN", + ) + .json(&json!({ "name": name })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + Ok::<_, anyhow::Error>((status, body)) + }; + let (a, b) = tokio::join!(create(names[0].clone()), create(names[1].clone())); + let (a, b) = (a?, b?); + assert_eq!(a.0, 200, "{}", a.1); + assert_eq!(b.0, 200, "{}", b.1); let catalog = windmill_common::datatable_roles::read_role_catalog(&db).await?; - assert!(catalog.contains_key("first"), "lost 'first': {catalog:?}"); - assert!(catalog.contains_key("second"), "lost 'second': {catalog:?}"); + let recorded: Vec<&str> = catalog.values().map(|r| r.name.as_str()).collect(); + for name in &names { + assert!( + recorded.contains(&name.as_str()), + "{name} is a live cluster login the catalog forgot: {recorded:?}" + ); + } + + for name in &names { + sqlx::query(&format!("DROP ROLE IF EXISTS \"{name}\"")) + .execute(&db) + .await?; + } Ok(()) } diff --git a/backend/windmill-api-integration-tests/tests/fixtures/datatable_roles.sql b/backend/windmill-api-integration-tests/tests/fixtures/datatable_roles.sql index 42f88e3f13..8cf9aaf2cf 100644 --- a/backend/windmill-api-integration-tests/tests/fixtures/datatable_roles.sql +++ b/backend/windmill-api-integration-tests/tests/fixtures/datatable_roles.sql @@ -3,7 +3,9 @@ -- the pointer exists for. INSERT INTO global_settings (name, value) VALUES - ('custom_instance_pg_databases', '{"user_pwd": "pw", "databases": {"dt_main": {}}}'::jsonb), + -- Empty registry: role provisioning grants CONNECT on every database named here, and the + -- data table's `dt_main` is a name in workspace settings, not a database that exists. + ('custom_instance_pg_databases', '{"user_pwd": "pw", "databases": {}}'::jsonb), -- The role catalog has its own row: it holds generated credentials and must stay out of the -- operator-facing config the neighbouring row belongs to. ('datatable_roles', '{"role1": {"name": "analytics", "enabled": true, "pwd": "pw"}}'::jsonb) diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 522f37410e..3b0b012bb8 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -129,9 +129,11 @@ pub async fn lock_role_catalog(tx: &mut sqlx::Transaction<'_, sqlx::Postgres>) - Ok(()) } -/// Authorization: returns every role's stored Postgres password in plaintext. Callers MUST -/// restrict this to superadmin or internal server paths, and MUST NOT put what it returns into a -/// response, a log line or an audit record. +/// 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 +/// or an export. Nothing about who may call it: the credential is the whole risk, and `Debug` is +/// hand-written to redact it for the same reason. pub async fn read_role_catalog(db: &DB) -> Result { let value = sqlx::query_scalar!( "SELECT value FROM global_settings WHERE name = $1", @@ -143,7 +145,7 @@ pub async fn read_role_catalog(db: &DB) -> Result { } /// As [`read_role_catalog`], reading inside the caller's transaction so the value is the one -/// [`lock_role_catalog`] is protecting. Same authorization contract. +/// [`lock_role_catalog`] is protecting. Same disclosure contract. pub async fn read_role_catalog_tx( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, ) -> Result { @@ -228,14 +230,14 @@ pub async fn registered_instance_databases(db: &DB) -> Result> { /// provisioned before a role existed is repaired rather than left silently unreachable. /// /// Authorization: rewrites a database's ACL with the server's own credentials and checks nothing. -/// Callers MUST restrict this to superadmin or internal server paths. +/// Callers MUST have authorized administration of `dbname` — superadmin, or an admin of the +/// workspace governing a data table on it. pub async fn converge_connect_grants(db: &DB, dbname: &str) -> Result<()> { let catalog = read_role_catalog(db).await?; converge_connect_grants_with(db, dbname, &catalog).await } -/// As [`converge_connect_grants`], with a catalog the caller already read. Same authorization -/// contract: it rewrites a database's ACL with the server's own credentials and checks nothing. +/// As [`converge_connect_grants`], with a catalog the caller already read. Same contract. pub async fn converge_connect_grants_with( db: &DB, dbname: &str, diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 63ba4aaf50..ec3f834537 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1427,10 +1427,10 @@ fn datatable_not_found_error(name: &str, datatables: Option<&serde_json::Value>) /// Read one workspace's data table entry, without following a pointer. /// -/// Authorization: reads a workspace's stored configuration by id and checks nothing — not that the -/// caller belongs to that workspace, nor that they may see the data table. Callers MUST have -/// authorized access to `w_id` already, and MUST NOT return the entry to a caller from another -/// workspace: it names the database and, on a governing entry, who may reach it as what. +/// Disclosure: this is the primitive [`resolve_governing_datatable`] calls on every path, so it is +/// deliberately open to anything that has to resolve a data table, including for a workspace the +/// caller does not belong to. What it returns is not: callers MUST NOT put `permissions` into a +/// response, an export or a log — it names the governing workspace's users, groups and folders. pub async fn read_datatable_entry(db: &DB, w_id: &str, name: &str) -> Result { let datatables = sqlx::query_scalar!( r#"