fix(datatables): make the concurrency test pin the handlers, and the contracts describe what is enforced

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR

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<Postgres>) -> anyhow::Result<()> {
-    use windmill_common::datatable_roles::{
-        lock_role_catalog, read_role_catalog_tx, InstanceDatatableRole,
-    };
-
+async fn concurrent_role_creations_both_survive(db: Pool<Postgres>) -> 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(())
 }

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)

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<DatatableRoleCatalog> {
     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<DatatableRoleCatalog> {
 }
 
 /// 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<DatatableRoleCatalog> {
@@ -228,14 +230,14 @@ pub async fn registered_instance_databases(db: &DB) -> Result<Vec<String>> {
 /// 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,

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<DataTable> {
     let datatables = sqlx::query_scalar!(
         r#"