diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 719b8e42db..d8a44d7a55 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -904,3 +904,33 @@ async fn a_stored_name_containing_a_question_mark_resolves_as_itself( ); Ok(()) } + +#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))] +async fn a_settings_save_dropping_a_governing_entry_names_the_forks_it_strands( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + // The whole map and no `deleted_datatables`, as a settings sync sends it. + let resp = authed( + client().post(format!( + "http://localhost:{}/api/w/test-workspace/workspaces/edit_datatable_config", + server.addr.port() + )), + "SECRET_TOKEN", + ) + .json(&json!({ "settings": { "datatables": {} } })) + .send() + .await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!(status, 200, "{body}"); + let result: Value = serde_json::from_str(&body)?; + assert!( + result["stranded_references"] + .as_array() + .is_some_and(|refs| refs.iter().any(|r| r["workspace_id"] == "wm-fork-dt")), + "the fork left pointing at nothing was not named: {body}" + ); + Ok(()) +} diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 9636134eae..29b12d292e 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3820,6 +3820,18 @@ async fn edit_datatable_config( } } + // Worked out from the locked entries rather than taken from `deleted_datatables`: a settings + // sync sends the whole map without that list, and dropping a governing entry strands every + // fork pointing at it all the same. + let removed: Vec = old_datatables + .keys() + .filter(|name| { + !new_config.settings.datatables.contains_key(*name) + && !new_config.renames.iter().any(|r| &r.from == *name) + }) + .cloned() + .collect(); + let config: serde_json::Value = serde_json::to_value(new_config.settings) .map_err(|err| Error::internal_err(err.to_string()))?; @@ -3859,7 +3871,7 @@ async fn edit_datatable_config( // A deletion cannot be followed the same way — there is nothing to point at any more. Read who // is left stranded so the caller is told, the way deleting a workspace does. let mut stranded: Vec = Vec::new(); - for name in &new_config.deleted_datatables { + for name in &removed { let rows = sqlx::query!( r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!" FROM workspace_settings ws diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 05ec5f9800..f1f551d2da 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1575,8 +1575,9 @@ pub async fn get_datatable_resource_from_db_unchecked( /// datatables resolve to the user's own resource unchanged; configuring it for /// replication there is the user's responsibility. /// -/// Authorization: a replication connection reads every row whatever the roles grant, so callers -/// must gate it with [`ensure_datatable_admin_access`] rather than a role check. +/// Authorization: a replication connection reads every row whatever the roles grant, so no role or +/// admin check makes it safe. Callers MUST refuse a data table under roles outright — the Postgres +/// trigger crate's `ensure_not_under_roles` — and turning roles on is refused while one streams. pub async fn get_datatable_replication_resource_from_db_unchecked( db: &DB, w_id: &str, @@ -1880,8 +1881,9 @@ pub async fn ensure_can_use_datatable_role( ))) } -/// Gate the operations that see the whole database whatever the roles grant: replication streams, -/// a migration that declares no role, exports, and editing the permissions themselves. Passing +/// Gate the operations that see the whole database whatever the roles grant: a migration that +/// declares no role, exports, and editing the permissions themselves. Not replication, which a +/// data table under roles refuses whoever asks (see `ensure_not_under_roles`). Passing /// means the caller could have connected as `admin` anyway. pub async fn ensure_datatable_admin_access( db: &DB, diff --git a/cli/src/core/settings.ts b/cli/src/core/settings.ts index 66fc77d230..33687ceb31 100644 --- a/cli/src/core/settings.ts +++ b/cli/src/core/settings.ts @@ -386,10 +386,17 @@ export async function pushWorkspaceSettings( if (!deepEqual(localSettings.datatable, settings.datatable)) { log.debug(`Updating datatable config...`); - await wmill.editDataTableConfig({ + const { stranded_references } = await wmill.editDataTableConfig({ workspace, requestBody: { settings: localSettings.datatable ?? { datatables: {} } }, }); + if (stranded_references?.length) { + log.warn( + `Removed data tables governed data tables in other workspaces, which no longer resolve: ${stranded_references + .map((r) => `${r.workspace_id}/${r.datatable}`) + .join(", ")}. A superadmin can point them somewhere else.`, + ); + } } if (localSettings.slack_command_script != settings.slack_command_script) {