From 8a7f364cfe0c3309c08131497655e800d1b9abca Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 17 Sep 2026 13:29:35 +0200 Subject: [PATCH] fix(datatables): hold the parent's settings while a fork points at its data tables Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/datatable_roles.rs | 54 +++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 8 +++ .../windmill-common/src/datatable_roles.rs | 3 ++ backend/windmill-common/src/workspaces.rs | 4 ++ backend/windmill-trigger-postgres/src/lib.rs | 5 +- 5 files changed, 73 insertions(+), 1 deletion(-) diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index f12daf1953..57b45088bd 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -1071,6 +1071,60 @@ async fn an_alias_saved_elsewhere_waits_for_roles_going_on_for_its_database( Ok(()) } +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_fork_waits_for_a_rename_of_the_data_table_it_keeps( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + // A rename in flight holds the parent's settings row and moves only the pointers it can see; a + // fork being created is invisible to it, so the fork has to read the name the rename commits. + let mut renaming = db.begin().await?; + sqlx::query( + "SELECT 1 FROM workspace_settings WHERE workspace_id = 'test-workspace' FOR UPDATE", + ) + .execute(&mut *renaming) + .await?; + sqlx::query( + "UPDATE workspace_settings SET datatable = jsonb_set(datatable #- '{datatables,main}', + '{datatables,renamed}', datatable->'datatables'->'main') + WHERE workspace_id = 'test-workspace'", + ) + .execute(&mut *renaming) + .await?; + + let server = ApiServer::start(db.clone()).await?; + let url = format!( + "http://localhost:{}/api/w/test-workspace/workspaces/create_fork", + server.addr.port() + ); + let fork = tokio::spawn( + authed(client().post(&url), "SECRET_TOKEN") + .json(&json!({ "id": "wm-fork-race", "name": "race", "color": "#0000ff" })) + .send(), + ); + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + assert!( + !fork.is_finished(), + "the fork copied the parent's data tables while a rename held them" + ); + renaming.commit().await?; + + let resp = fork.await??; + assert!(resp.status().is_success(), "{}", resp.text().await?); + let datatables: Option = sqlx::query_scalar( + "SELECT datatable->'datatables' FROM workspace_settings WHERE workspace_id = 'wm-fork-race'", + ) + .fetch_one(&db) + .await?; + let datatables = datatables.unwrap(); + assert_eq!( + datatables["renamed"]["reference"], + json!({ "workspace_id": "test-workspace", "datatable": "renamed" }), + "{datatables}" + ); + 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 6bd7d7ea7e..97ab07c0a5 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -8549,6 +8549,14 @@ async fn create_workspace_fork( .execute(&mut *tx) .await?; + // The pointers this fork writes to the parent's data tables stay invisible until it commits, so + // a rename of one of them cannot carry them. Holding the parent's settings row makes such a + // rename wait for this commit, and makes the copy below read one that committed first. + sqlx::query("SELECT 1 FROM workspace_settings WHERE workspace_id = $1 FOR SHARE") + .bind(&parent_workspace_id) + .execute(&mut *tx) + .await?; + // Clone all data from the parent workspace using Rust implementation if let Err(e) = clone_workspace_data(&mut tx, &db, &parent_workspace_id, &forked_id, &authed).await diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 4dd9b06fde..a9195055ac 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -233,6 +233,9 @@ pub fn role_id_by_name<'a>(catalog: &'a DatatableRoleCatalog, name: &str) -> Res /// Every instance database the registry knows about. Role provisioning has to reach all of them: /// a role that cannot `CONNECT` to a database is refused by Postgres before any grant matters. +/// +/// Authorization: checks nothing, and names every instance database across all workspaces. Callers +/// MUST be superadmin-gated or keep the names server-side; never return them to a workspace caller. pub async fn registered_instance_databases(db: &DB) -> Result> { crate::datatable_roles_oss::registered_instance_databases(db).await } diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 1bf6314e5e..711b4dac29 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1967,6 +1967,10 @@ pub fn strip_datatable_permissions( /// As [`parse_datatable_ref`], except that an entry whose stored name itself contains `?` — which /// names could before they were restricted — resolves by that exact name, without a role. It is /// looked up first, so `sales?role=x` never reaches a different entry than the one stored so. +/// +/// Authorization: checks nothing, and its answer reveals whether `w_id` stores that exact name. +/// Callers MUST already act for `w_id` — a job of it, or a caller authenticated into it — and +/// MUST still pass the name to [`get_datatable_resource_from_db`] or an admin-access check. pub async fn parse_datatable_ref_for( db: &DB, w_id: &str, diff --git a/backend/windmill-trigger-postgres/src/lib.rs b/backend/windmill-trigger-postgres/src/lib.rs index 25b19e6c1b..b92860662d 100644 --- a/backend/windmill-trigger-postgres/src/lib.rs +++ b/backend/windmill-trigger-postgres/src/lib.rs @@ -377,7 +377,10 @@ pub async fn get_raw_postgres_connection( /// A replication stream reads every row of every table whatever the data table's roles grant, so /// the two don't mix: a data table under roles takes no triggers or captures, and roles cannot be /// turned on while one is enabled on it. -pub async fn ensure_not_under_roles( +/// +/// Authorization: checks nothing, and its refusal says whether `w_id`'s data table is under roles. +/// Callers MUST have established that the caller may manage triggers in `w_id` first. +pub(crate) async fn ensure_not_under_roles( db: &DB, w_id: &str, postgres_resource_path: &str,