fix(datatables): no entry without roles may newly reach a database under roles

The previous guard only caught a new name replacing an entry under roles.
A whole-map save could also repoint an existing entry without roles at
that database, or another workspace could point one there, and every
caller of that entry would connect as admin. The rule is now stated on
the saved entries: one that carries no roles and newly points at an
instance database any entry under roles uses, in this workspace or
another, is refused. A declared rename carries its roles and passes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BjfMkJyKzodxkobqGZ6Lqb

backend/windmill-api-integration-tests/tests/datatable_roles.rs

@@ -936,30 +936,58 @@ async fn a_settings_save_dropping_a_governing_entry_names_the_forks_it_strands(
 }
 
 #[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))]
-async fn a_save_replacing_an_entry_under_roles_on_its_database_is_refused(
+async fn an_entry_without_roles_cannot_newly_reach_a_database_under_roles(
     db: Pool<Postgres>,
 ) -> anyhow::Result<()> {
     initialize_tracing().await;
-    let server = ApiServer::start(db.clone()).await?;
-    // A rename as a settings sync sends it: the whole map, no `renames`.
-    let resp = authed(
-        client().post(format!(
-            "http://localhost:{}/api/w/test-workspace/workspaces/edit_datatable_config",
-            server.addr.port()
-        )),
-        "SECRET_TOKEN",
+    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'"#,
     )
-    .json(&json!({ "settings": { "datatables": {
-        "main_renamed": { "database": { "resource_type": "instance", "resource_path": "dt_main" } }
-    } } }))
-    .send()
+    .execute(&db)
     .await?;
-    let status = resp.status();
-    let body = resp.text().await?;
-    assert_eq!(
-        status, 400,
-        "a save dropped the roles of the database it kept: {body}"
-    );
+    let server = ApiServer::start(db.clone()).await?;
+    let port = server.addr.port();
+
+    // Whole-map saves with no `renames`, as a settings sync sends them.
+    let dt_main =
+        json!({ "database": { "resource_type": "instance", "resource_path": "dt_main" } });
+    let dt_other =
+        json!({ "database": { "resource_type": "instance", "resource_path": "dt_other" } });
+    for (case, w_id, datatables) in [
+        (
+            "a rename to a new name",
+            "test-workspace",
+            json!({ "main_renamed": dt_main, "other": dt_other }),
+        ),
+        (
+            "an existing name repointed",
+            "test-workspace",
+            json!({ "other": dt_main }),
+        ),
+        (
+            "another workspace's entry",
+            "wm-fork-dt",
+            json!({ "direct": dt_main }),
+        ),
+    ] {
+        let resp = authed(
+            client().post(format!(
+                "http://localhost:{port}/api/w/{w_id}/workspaces/edit_datatable_config"
+            )),
+            "SECRET_TOKEN",
+        )
+        .json(&json!({ "settings": { "datatables": datatables } }))
+        .send()
+        .await?;
+        let status = resp.status();
+        let body = resp.text().await?;
+        assert!(
+            status == 400 && body.contains("which a data table under roles uses"),
+            "{case} reached the database under roles without them ({status}): {body}"
+        );
+    }
 
     let still_governed: bool = sqlx::query_scalar(
         "SELECT (datatable->'datatables'->'main') ? 'permissions' FROM workspace_settings

backend/windmill-api-workspaces/src/workspaces.rs

@@ -3883,36 +3883,54 @@ async fn edit_datatable_config(
         .cloned()
         .collect();
 
-    // Roles follow an entry only through a declared rename. A save that drops an entry under roles
-    // and adds another on the same database without one — which is how a settings sync sends a
-    // rename — would leave that database answering everyone as `admin`.
-    for name in &removed {
-        let Some(old_db) = old_datatables
-            .get(name)
-            .filter(|old| old.permissions.is_some())
-            .and_then(|old| old.database.as_ref())
+    // A database under roles is reached only through an entry that carries them. Roles follow an
+    // 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<String> = 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, 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 added_on_same_database =
-            new_config
-                .settings
-                .datatables
-                .iter()
-                .find_map(|(added, dt)| {
-                    let db = dt.database.as_ref()?;
-                    (!old_datatables.contains_key(added)
-                        && !new_config.renames.iter().any(|r| &r.to == added)
-                        && db.resource_type == old_db.resource_type
-                        && db.resource_path == old_db.resource_path)
-                        .then_some(added)
-                });
-        if let Some(added) = added_on_same_database {
+        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_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
+                })
+        });
+        if repointed && (governed_here || governed_elsewhere.contains(&db.resource_path)) {
             return Err(Error::BadRequest(format!(
-                "Data table '{name}' is under roles, and this save removes it while adding '{added}' \
-                 on the same database. Its roles would not carry over, leaving that database open to \
-                 everyone as `admin`. Rename it from the data table settings, which carries its \
-                 roles, or turn its roles off first."
+                "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
             )));
         }
     }