fix(datatables): only the opt-in is gated, and a rename leaves no permissioned copy behind

backend/.sqlx/query-3c76304619fba1222eec7fcfa7057e488a4ba601be7062d3c79097513cb93723.json

@@ -0,0 +1,22 @@
+{
+  "db_name": "PostgreSQL",
+  "query": "\n            WITH RECURSIVE tree AS (\n                SELECT id, deleted, 0 AS depth FROM workspace WHERE id = $1\n                UNION ALL\n                SELECT w.id, w.deleted, tree.depth + 1 FROM workspace w\n                JOIN tree ON w.parent_workspace_id = tree.id\n                WHERE tree.depth < 20\n            )\n            SELECT id AS \"id!\" FROM tree WHERE id != $1 AND NOT deleted ORDER BY id\n        ",
+  "describe": {
+    "columns": [
+      {
+        "ordinal": 0,
+        "name": "id!",
+        "type_info": "Varchar"
+      }
+    ],
+    "parameters": {
+      "Left": [
+        "Text"
+      ]
+    },
+    "nullable": [
+      null
+    ]
+  },
+  "hash": "3c76304619fba1222eec7fcfa7057e488a4ba601be7062d3c79097513cb93723"
+}

backend/.sqlx/query-f117e4fd909fe5f2fe821e176011e838bd797c023123735ed52eb0507455165a.json

@@ -0,0 +1,14 @@
+{
+  "db_name": "PostgreSQL",
+  "query": "UPDATE workspace_settings\n         SET datatable = jsonb_set(datatable, '{datatables}', COALESCE((\n             SELECT jsonb_object_agg(key, value - 'permissions')\n             FROM jsonb_each(datatable->'datatables')\n             WHERE COALESCE((value->'permissions'->>'enabled')::boolean, false) = false\n         ), '{}'::jsonb))\n         WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'",
+  "describe": {
+    "columns": [],
+    "parameters": {
+      "Left": [
+        "Text"
+      ]
+    },
+    "nullable": []
+  },
+  "hash": "f117e4fd909fe5f2fe821e176011e838bd797c023123735ed52eb0507455165a"
+}

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

@@ -152,6 +152,56 @@ async fn enabling_permissions_is_refused_while_a_fork_exists(
         assert!(text.contains("wm-fork-t"), "{endpoint}: {text}");
     }
 
+    // Only the opt-in is gated: once permissions are on, forks made afterwards
+    // never receive the data table, and editing the roles — revoking a tenant
+    // above all — has to keep working while they exist. The preview then gets as
+    // far as the database, which this test does not have.
+    sqlx::query(
+        r#"UPDATE workspace_settings
+           SET datatable = jsonb_set(datatable, '{datatables,main,permissions}',
+               '{"enabled": true, "roles": {"admin": {"tenants": []}}}')
+           WHERE workspace_id = 'test-workspace'"#,
+    )
+    .execute(&db)
+    .await?;
+    let edit = json!({ "enabled": true, "roles": [
+        { "name": "admin", "tenants": [] }, { "name": "analyst", "tenants": ["u/test-user"] }
+    ]});
+    let resp = authed(
+        client().post(format!(
+            "{ws}/workspaces/datatable_permissions/main/preview"
+        )),
+        "SECRET_TOKEN",
+    )
+    .json(&edit)
+    .send()
+    .await?;
+    let text = resp.text().await?;
+    assert!(!text.contains("cannot be enabled"), "{text}");
+    sqlx::query(
+        r#"UPDATE workspace_settings
+           SET datatable = datatable #- '{datatables,main,permissions}'
+           WHERE workspace_id = 'test-workspace'"#,
+    )
+    .execute(&db)
+    .await?;
+
+    // An archived fork has no members left to reach anything.
+    sqlx::query("UPDATE workspace SET deleted = true WHERE id = 'wm-fork-t'")
+        .execute(&db)
+        .await?;
+    let resp = authed(
+        client().post(format!(
+            "{ws}/workspaces/datatable_permissions/main/preview"
+        )),
+        "SECRET_TOKEN",
+    )
+    .json(&body)
+    .send()
+    .await?;
+    let text = resp.text().await?;
+    assert!(!text.contains("has forks"), "{text}");
+
     // A workspace that is no longer a fork — a detached dev workspace — keeps its
     // copy of the data table, pointing at the same instance database.
     sqlx::query("DELETE FROM workspace WHERE id = 'wm-fork-t'")
@@ -202,3 +252,72 @@ async fn enabling_permissions_is_refused_while_a_fork_exists(
 
     Ok(())
 }
+
+/// The rename keeps a copy of the settings under the archived id, and commits it
+/// before the old id is archived. A permissioned data table must not be in that
+/// copy: without its `permissions` block it would resolve, for anyone still
+/// using the old id, to the owner connection.
+#[sqlx::test(migrations = "../migrations", fixtures("base"))]
+async fn a_rename_leaves_no_permissioned_datatable_under_the_old_id(
+    db: Pool<Postgres>,
+) -> anyhow::Result<()> {
+    initialize_tracing().await;
+    let server = ApiServer::start(db.clone()).await?;
+    let port = server.addr.port();
+
+    sqlx::query(
+        r#"UPDATE workspace_settings SET datatable = $1 WHERE workspace_id = 'test-workspace'"#,
+    )
+    .bind(json!({
+        "datatables": {
+            "open": { "database": { "resource_type": "instance", "resource_path": "dt_open" } },
+            "main": {
+                "database": { "resource_type": "instance", "resource_path": "dt_main" },
+                "permissions": { "enabled": true, "roles": {
+                    "admin": { "tenants": [] },
+                    "analyst": { "tenants": ["*"], "pg_rolename": "wm_x", "pg_password": "s3cret" }
+                }}
+            }
+        }
+    }))
+    .execute(&db)
+    .await?;
+
+    let resp = authed(
+        client().post(format!(
+            "http://localhost:{port}/api/w/test-workspace/workspaces/change_workspace_id"
+        )),
+        "SECRET_TOKEN",
+    )
+    .json(&json!({ "new_id": "renamed-ws", "new_name": "Renamed" }))
+    .send()
+    .await?;
+    assert_eq!(resp.status(), 200, "{}", resp.text().await?);
+
+    let names = |w: &'static str| {
+        let db = db.clone();
+        async move {
+            let value: serde_json::Value = sqlx::query_scalar(
+                "SELECT datatable->'datatables' FROM workspace_settings WHERE workspace_id = $1",
+            )
+            .bind(w)
+            .fetch_one(&db)
+            .await
+            .unwrap();
+            let mut keys: Vec<String> = value.as_object().unwrap().keys().cloned().collect();
+            keys.sort();
+            (keys, value)
+        }
+    };
+    let (old_names, _) = names("test-workspace").await;
+    assert_eq!(old_names, vec!["open".to_string()]);
+    let (new_names, new_value) = names("renamed-ws").await;
+    assert_eq!(new_names, vec!["main".to_string(), "open".to_string()]);
+    assert_eq!(new_value["main"]["permissions"]["enabled"], json!(true));
+    assert_eq!(
+        new_value["main"]["permissions"]["roles"]["analyst"]["pg_password"],
+        json!("s3cret")
+    );
+
+    Ok(())
+}

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

@@ -734,17 +734,17 @@ pub(crate) async fn ensure_can_use_datatable_role(
 /// the data table was unpermissioned carries a verbatim copy of it, and every
 /// member of that fork — including members this workspace does not have — would
 /// keep reaching the database through the copy's own connection, which owns
-/// everything in it. Forks made after the opt-in never receive a permissioned
-/// data table (see the strip in the fork creation), so refusing while any exist
-/// is what closes the gap. A dev workspace detached from this one keeps such a
-/// copy without being a fork any more, which is what the last check is for; it
-/// is exact for an instance database, whose only credential holders are the
-/// data tables naming it, and meaningless for a resource-backed one, where
-/// whoever holds the resource's credentials reaches the database regardless.
+/// everything in it. A dev workspace detached from this one keeps such a copy
+/// without being a fork any more, which is what the last check is for; it is
+/// exact for an instance database, whose only credential holders are the data
+/// tables naming it, and meaningless for a resource-backed one, where whoever
+/// holds the resource's credentials reaches the database regardless.
 ///
-/// Turning them off is always allowed, or a workspace carrying permissions from
-/// before this rule could never be rid of them, and the roles behind them never
-/// dropped.
+/// All three are properties of the opt-in, so only the save that turns
+/// permissions on is checked. Forks made afterwards never receive a permissioned
+/// data table (see the strip in the fork creation), and a save that edits the
+/// roles of a live config must keep working while they exist — revoking a tenant
+/// above all. Turning permissions off is never refused.
 ///
 /// The save calls this under the settings row lock, which fork creation takes on
 /// the parent before copying its settings: a fork mid-creation has either
@@ -758,6 +758,10 @@ async fn refuse_enabling_permissions_over_shared_access(
     if !enabled {
         return Ok(());
     }
+    let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?;
+    if datatable.permissions.as_ref().is_some_and(|p| p.enabled) {
+        return Ok(());
+    }
     if crate::workspaces_extra::workspace_is_fork(db, w_id).await? {
         return Err(Error::BadRequest(
             "Data table permissions cannot be enabled from a fork workspace: a fork's data \
@@ -768,7 +772,7 @@ async fn refuse_enabling_permissions_over_shared_access(
                 .to_string(),
         ));
     }
-    let forks = windmill_common::workspaces::list_fork_descendants(db, w_id).await?;
+    let forks = windmill_common::workspaces::list_live_fork_descendants(db, w_id).await?;
     if !forks.is_empty() {
         return Err(Error::BadRequest(format!(
             "Data table permissions cannot be enabled while this workspace has forks ({}): a \
@@ -778,7 +782,6 @@ async fn refuse_enabling_permissions_over_shared_access(
             forks.join(", ")
         )));
     }
-    let datatable = read_datatable_unchecked(db, w_id, datatable_name).await?;
     if datatable.database.resource_type == DataTableCatalogResourceType::Instance {
         let others = sqlx::query!(
             r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "name!"

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

@@ -3126,15 +3126,14 @@ pub(crate) async fn is_instance_datatable(db: &DB, w_id: &str, name: &str) -> Re
         .unwrap_or(false))
 }
 
-/// Same, for the `datatable://<name>` / `$res:<path>` form the import endpoints take.
 /// Refuse to clone a data table whose role permissions are enabled.
 ///
 /// A clone lands in a brand-new database where none of the roles exist, and the
 /// fork's copy of the config is stripped of its permissions — so every member of
 /// the fork resolves to the copy's own owner connection and reads, in full, the
 /// data the roles existed to divide. Reproducing the roles in the copy is a
-/// separate piece of work; until it exists, a fork shares the original, which
-/// keeps the parent's restrictions, or goes without.
+/// separate piece of work; until it exists, a fork goes without the data table
+/// (the fork creation leaves a permissioned one out of the fork's config).
 pub(crate) async fn refuse_clone_of_permissioned_datatable(
     db: &DB,
     w_id: &str,
@@ -3157,12 +3156,14 @@ pub(crate) async fn refuse_clone_of_permissioned_datatable(
         return Err(Error::BadRequest(format!(
             "Data table '{name}' has role permissions enabled and cannot be cloned into a fork: \
              the copy cannot carry its roles, so it would be readable in full by every member of \
-             the fork. Keep the original instead — the fork shares it with the same restrictions."
+             the fork. The fork goes without it; disable its permissions first to clone it."
         )));
     }
     Ok(())
 }
 
+/// Same as [`is_instance_datatable`], for the `datatable://<name>` / `$res:<path>` form the
+/// import endpoints take.
 async fn is_instance_datatable_source(db: &DB, w_id: &str, source: &str) -> Result<bool> {
     match source.strip_prefix("datatable://") {
         Some(name) => is_instance_datatable(db, w_id, name).await,

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

@@ -62,6 +62,12 @@ pub(crate) async fn change_workspace_id(
     // rename moves the chain from one to the other.
     crate::workspaces::lock_dev_pairing(&mut tx, &[&old_id, &rw.new_id]).await?;
 
+    // The settings are copied below, and a permissions save holds this row while it changes
+    // the roles in the database and then the config: copied without it, the new workspace
+    // could carry the config from before that save while the database has the roles from
+    // after it. Same order as fork creation: pairing lock, then the settings row.
+    windmill_common::workspaces::lock_workspace_settings_unchecked(&mut tx, &old_id).await?;
+
     check_w_id_conflict(&mut tx, &rw.new_id).await?;
 
     info!(
@@ -120,21 +126,25 @@ pub(crate) async fn change_workspace_id(
     .execute(&mut *tx)
     .await?;
 
-    // Two configs now name the same Postgres logins, and only one of them owns
-    // them: deleting the archived id would plan drops for logins the renamed
-    // workspace is still using. The archived copy stops naming them — it is kept
-    // for reference, and a reference does not need credentials.
+    // Two configs now name the same Postgres logins, and only one of them owns them:
+    // deleting the archived id would plan drops for logins the renamed workspace is still
+    // using. A permissioned data table leaves the archived copy entirely rather than losing
+    // its `permissions` block: this transaction commits before the old id is archived, and
+    // a copy that still named the database without the block would hand every caller of the
+    // old id the owner connection in between — and again if the shell were ever unarchived.
+    // A missing data table fails closed. The unpermissioned ones stay, for reference.
     //
-    // The renamed workspace keeps them and keeps working: a role's `pg_rolename`
-    // is what resolution uses, and the generated name only decides what a *new*
-    // role is called. Its next permissions save finds the stored name no longer
-    // matches the one this workspace id generates and renames the login to match,
-    // under the ownership proof that rename already carries.
+    // The renamed workspace keeps the roles and keeps working: a role's `pg_rolename` is
+    // what resolution uses, and the generated name only decides what a *new* role is called.
+    // Its next permissions save finds the stored name no longer matches the one this
+    // workspace id generates and renames the login to match, under the ownership proof that
+    // rename already carries.
     sqlx::query!(
         "UPDATE workspace_settings
          SET datatable = jsonb_set(datatable, '{datatables}', COALESCE((
              SELECT jsonb_object_agg(key, value - 'permissions')
              FROM jsonb_each(datatable->'datatables')
+             WHERE COALESCE((value->'permissions'->>'enabled')::boolean, false) = false
          ), '{}'::jsonb))
          WHERE workspace_id = $1 AND jsonb_typeof(datatable->'datatables') = 'object'",
         &old_id,

backend/windmill-common/src/workspaces.rs

@@ -737,6 +737,31 @@ pub async fn list_fork_descendants(db: &crate::DB, w_id: &str) -> Result<Vec<Str
     Ok(ids)
 }
 
+/// Same, without the soft-deleted ones: the descendants that still have members. An archived
+/// fork, or the shell a fork's rename archives, keeps its `parent_workspace_id` and would
+/// otherwise count as a workspace that can still reach what the parent owns.
+///
+/// Unauthenticated helper, like [`list_fork_descendants`].
+pub async fn list_live_fork_descendants(db: &crate::DB, w_id: &str) -> Result<Vec<String>> {
+    let ids = sqlx::query_scalar!(
+        r#"
+            WITH RECURSIVE tree AS (
+                SELECT id, deleted, 0 AS depth FROM workspace WHERE id = $1
+                UNION ALL
+                SELECT w.id, w.deleted, tree.depth + 1 FROM workspace w
+                JOIN tree ON w.parent_workspace_id = tree.id
+                WHERE tree.depth < 20
+            )
+            SELECT id AS "id!" FROM tree WHERE id != $1 AND NOT deleted ORDER BY id
+        "#,
+        w_id
+    )
+    .fetch_all(db)
+    .await
+    .map_err(|e| Error::internal_err(format!("listing live fork descendants of {w_id}: {e:#}")))?;
+    Ok(ids)
+}
+
 /// Count non-deleted fork/dev workspaces anywhere under `root` (excludes `root` itself).
 ///
 /// Unauthenticated metering helper: it reads workspace hierarchy for any `root` id, so callers must
@@ -3476,7 +3501,8 @@ mod tests {
         // Repointed — however the resource got there, including through a `$var:`
         // no guard on the resource itself would see.
         assert!(
-            ensure_datatable_database_unchanged("main", &dt, &resolved("elsewhere", "one")).is_err()
+            ensure_datatable_database_unchanged("main", &dt, &resolved("elsewhere", "one"))
+                .is_err()
         );
         // A config that never recorded one cannot claim to match: the roles it
         // names were created against a database nobody wrote down.