From bc78383c743cfc170e69cd0d6934b51e314ee806 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 8 Sep 2026 16:29:51 +0200 Subject: [PATCH] refactor(datatables): put the role catalog in its own table, not in global_settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings across three rounds were all the same choice. A set of live Postgres credentials was living in `global_settings`, which has generic read, list, write, config-export and CLI round-trip paths that know nothing about what they carry: the passwords reached the instance config and its YAML editor, a full-row upsert of a neighbouring key erased the catalog, `GET /settings/global/{key}` and the settings listing returned them raw, and this round the redaction that fixed the last two turned `wmill instance push` into something that wipes every password — a fix breaking the assumption the previous fix made. `POST /settings/global/datatable_roles` could also empty it outside the lock. The approved plan offered a table or `global_settings`, so this is the other option it already allowed rather than a new design. `datatable_role` is a table: no generic settings path can read it, list it, export it, write it or round-trip it, so none of the five needs a guard. The redaction, the hidden/protected/agent-denylist entries and the JSON document all go with it. One row per role also removes the read-modify-write the concurrency work was about: two concurrent creates are two inserts, and the unique index on `name` is what settles a collision. The advisory lock stays for the one window rows do not cover — `CREATE ROLE` is invisible to another transaction until commit, so without it both creates pass their `pg_roles` check. Also from this round: rename mappings are checked against the configuration they claim to describe, since fork pointers are rewritten from them — a caller could otherwise submit `main -> missing` against an unchanged config and repoint every fork of `main` at a name nothing has, and `A -> B` plus `B -> C` moved what pointed at `A` all the way to `C`. And the warning naming forks a delete stranded reached the response but not the screen: both the data table settings save and the workspace delete now show it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR --- ...d368347c59f36fd87df6dc7996152ccb84af0.json | 38 +++++ ...df2a63695f8cbf8af648da5a0a77a5b9d02ba.json | 17 +++ ...0e80b993020a2ff5dcb5849dc570d52798587.json | 14 ++ ...94ab31ac856252aaa76c75ea27a447009c363.json | 17 +++ ...0908142148_datatable_role_catalog.down.sql | 1 + ...260908142148_datatable_role_catalog.up.sql | 21 +++ .../tests/datatable_roles.rs | 6 +- .../tests/fixtures/datatable_roles.sql | 11 +- backend/windmill-api-settings/src/lib.rs | 37 ++--- .../windmill-api-workspaces/src/workspaces.rs | 37 +++++ .../windmill-common/src/datatable_roles.rs | 139 ++++++++++-------- .../windmill-common/src/global_settings.rs | 11 -- .../windmill-common/src/instance_config.rs | 4 - .../sidebar/DeleteForkedWorkspaceModal.svelte | 7 +- .../DataTableSettings.svelte | 13 +- 15 files changed, 260 insertions(+), 113 deletions(-) create mode 100644 backend/.sqlx/query-71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0.json create mode 100644 backend/.sqlx/query-86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba.json create mode 100644 backend/.sqlx/query-c85d362fe2e652d4ac01a35bf470e80b993020a2ff5dcb5849dc570d52798587.json create mode 100644 backend/.sqlx/query-fcb34e643b888122766e115a01394ab31ac856252aaa76c75ea27a447009c363.json create mode 100644 backend/migrations/20260908142148_datatable_role_catalog.down.sql create mode 100644 backend/migrations/20260908142148_datatable_role_catalog.up.sql diff --git a/backend/.sqlx/query-71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0.json b/backend/.sqlx/query-71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0.json new file mode 100644 index 0000000000..a03fab7081 --- /dev/null +++ b/backend/.sqlx/query-71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0.json @@ -0,0 +1,38 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT id, name, enabled, pwd FROM datatable_role", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "id", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "name", + "type_info": "Varchar" + }, + { + "ordinal": 2, + "name": "enabled", + "type_info": "Bool" + }, + { + "ordinal": 3, + "name": "pwd", + "type_info": "Text" + } + ], + "parameters": { + "Left": [] + }, + "nullable": [ + false, + false, + false, + true + ] + }, + "hash": "71ee2cb6661cca1fa4d8874a7f6d368347c59f36fd87df6dc7996152ccb84af0" +} diff --git a/backend/.sqlx/query-86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba.json b/backend/.sqlx/query-86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba.json new file mode 100644 index 0000000000..7627d9828d --- /dev/null +++ b/backend/.sqlx/query-86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO datatable_role (id, name, enabled, pwd) VALUES ($1, $2, $3, $4)", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Varchar", + "Bool", + "Text" + ] + }, + "nullable": [] + }, + "hash": "86af9d51a158ea5cb6161461ecddf2a63695f8cbf8af648da5a0a77a5b9d02ba" +} diff --git a/backend/.sqlx/query-c85d362fe2e652d4ac01a35bf470e80b993020a2ff5dcb5849dc570d52798587.json b/backend/.sqlx/query-c85d362fe2e652d4ac01a35bf470e80b993020a2ff5dcb5849dc570d52798587.json new file mode 100644 index 0000000000..7a7da4308d --- /dev/null +++ b/backend/.sqlx/query-c85d362fe2e652d4ac01a35bf470e80b993020a2ff5dcb5849dc570d52798587.json @@ -0,0 +1,14 @@ +{ + "db_name": "PostgreSQL", + "query": "DELETE FROM datatable_role WHERE id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [] + }, + "hash": "c85d362fe2e652d4ac01a35bf470e80b993020a2ff5dcb5849dc570d52798587" +} diff --git a/backend/.sqlx/query-fcb34e643b888122766e115a01394ab31ac856252aaa76c75ea27a447009c363.json b/backend/.sqlx/query-fcb34e643b888122766e115a01394ab31ac856252aaa76c75ea27a447009c363.json new file mode 100644 index 0000000000..26920a2943 --- /dev/null +++ b/backend/.sqlx/query-fcb34e643b888122766e115a01394ab31ac856252aaa76c75ea27a447009c363.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE datatable_role SET name = $2, enabled = $3, pwd = $4 WHERE id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Varchar", + "Bool", + "Text" + ] + }, + "nullable": [] + }, + "hash": "fcb34e643b888122766e115a01394ab31ac856252aaa76c75ea27a447009c363" +} diff --git a/backend/migrations/20260908142148_datatable_role_catalog.down.sql b/backend/migrations/20260908142148_datatable_role_catalog.down.sql new file mode 100644 index 0000000000..0bbf50bc89 --- /dev/null +++ b/backend/migrations/20260908142148_datatable_role_catalog.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS datatable_role; diff --git a/backend/migrations/20260908142148_datatable_role_catalog.up.sql b/backend/migrations/20260908142148_datatable_role_catalog.up.sql new file mode 100644 index 0000000000..59cce4cc08 --- /dev/null +++ b/backend/migrations/20260908142148_datatable_role_catalog.up.sql @@ -0,0 +1,21 @@ +-- The instance's data table role catalog: one row per Postgres login Windmill created for data +-- table access. +-- +-- A table rather than a `global_settings` key, because the value is a set of live cluster +-- credentials and that table has generic read, list, write and CLI round-trip paths that know +-- nothing about what they are carrying. Every one of them is a way to leak the passwords or to +-- overwrite the catalog with a copy that has none, and a row nothing generic touches has none of +-- those. One row per role also makes two concurrent creates two inserts rather than a +-- read-modify-write over one document. +CREATE TABLE datatable_role ( + id VARCHAR(50) PRIMARY KEY, + -- The Postgres role name, verbatim. Unique because it is the cluster's own key. + name VARCHAR(63) NOT NULL UNIQUE, + enabled BOOLEAN NOT NULL DEFAULT true, + -- Generated by Windmill, never entered by anyone, and never leaves the server. + pwd TEXT, + created_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +GRANT ALL ON datatable_role TO windmill_user; +GRANT ALL ON datatable_role TO windmill_admin; diff --git a/backend/windmill-api-integration-tests/tests/datatable_roles.rs b/backend/windmill-api-integration-tests/tests/datatable_roles.rs index 08cc43d0d4..dff2b59cb3 100644 --- a/backend/windmill-api-integration-tests/tests/datatable_roles.rs +++ b/backend/windmill-api-integration-tests/tests/datatable_roles.rs @@ -341,9 +341,9 @@ async fn concurrent_role_creations_both_survive(db: Pool) -> anyhow::R 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. + // The cluster DDL is not visible to another transaction until commit, so without the lock both + // of these pass their `pg_roles` existence check and one loses — leaving a live cluster login + // the catalog never recorded. let create = |name: String| async move { let resp = authed( client().post(format!( diff --git a/backend/windmill-api-integration-tests/tests/fixtures/datatable_roles.sql b/backend/windmill-api-integration-tests/tests/fixtures/datatable_roles.sql index 8cf9aaf2cf..55d50d212c 100644 --- a/backend/windmill-api-integration-tests/tests/fixtures/datatable_roles.sql +++ b/backend/windmill-api-integration-tests/tests/fixtures/datatable_roles.sql @@ -2,15 +2,14 @@ -- carrying a copy. `test-user-2` is a non-admin of the parent and an admin of the fork: the shape -- the pointer exists for. +-- 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. INSERT INTO global_settings (name, value) VALUES - -- 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) + ('custom_instance_pg_databases', '{"user_pwd": "pw", "databases": {}}'::jsonb) ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value; +INSERT INTO datatable_role (id, name, enabled, pwd) VALUES ('role1', 'analytics', true, 'pw'); + UPDATE workspace_settings SET datatable = '{ "datatables": { "main": { diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index 118f1fe59e..13dbcd7b9b 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -1325,7 +1325,6 @@ pub async fn get_global_setting( .await? .map(|x| x.value); - let value = value.map(|v| windmill_common::datatable_roles::redact_role_catalog_setting(&key, v)); Ok(Json(value.unwrap_or_else(|| serde_json::Value::Null))) } @@ -1363,13 +1362,7 @@ async fn list_global_settings( require_super_admin(&db, &authed).await?; let settings = sqlx::query_as!(GlobalSetting, "SELECT name, value FROM global_settings") .fetch_all(&db) - .await? - .into_iter() - .map(|s| GlobalSetting { - value: windmill_common::datatable_roles::redact_role_catalog_setting(&s.name, s.value), - name: s.name, - }) - .collect(); + .await?; Ok(Json(settings)) } @@ -2604,9 +2597,8 @@ async fn create_datatable_role( require_super_admin(&db, &authed).await?; windmill_common::datatable_roles::validate_role_name(&req.name)?; - // The catalog is one JSON document, so read, DDL and write are one critical section: two - // concurrent creates would otherwise both land in the cluster and the second write would drop - // the first, leaving a live login nobody recorded. + // The cluster DDL and the row that records it are one transaction under one lock, so a + // half-done create cannot leave a live login the catalog does not know about. let mut tx = db.begin().await?; windmill_common::datatable_roles::lock_role_catalog(&mut tx).await?; let mut catalog = windmill_common::datatable_roles::read_role_catalog_tx(&mut tx).await?; @@ -2621,15 +2613,13 @@ async fn create_datatable_role( let pwd = uuid::Uuid::new_v4().to_string(); windmill_common::datatable_roles::create_instance_role(&mut tx, &req.name, &pwd).await?; - catalog.insert( - id.clone(), - windmill_common::datatable_roles::InstanceDatatableRole { - name: req.name.clone(), - enabled: true, - pwd: Some(pwd), - }, - ); - windmill_common::datatable_roles::write_role_catalog(&mut tx, &catalog).await?; + let entry = windmill_common::datatable_roles::InstanceDatatableRole { + name: req.name.clone(), + enabled: true, + pwd: Some(pwd), + }; + windmill_common::datatable_roles::insert_role_catalog_entry(&mut tx, &id, &entry).await?; + catalog.insert(id.clone(), entry); tx.commit().await?; converge_connect_grants_everywhere(&db, &catalog).await; windmill_common::feature_usage::log_feature_usage("datatable", "role_created", ""); @@ -2685,8 +2675,8 @@ async fn update_datatable_role( updated.enabled = enabled; } + windmill_common::datatable_roles::update_role_catalog_entry(&mut tx, &id, &updated).await?; catalog.insert(id.clone(), updated.clone()); - windmill_common::datatable_roles::write_role_catalog(&mut tx, &catalog).await?; tx.commit().await?; converge_connect_grants_everywhere(&db, &catalog).await; @@ -2721,15 +2711,14 @@ async fn delete_datatable_role( require_super_admin(&db, &authed).await?; let mut tx = db.begin().await?; windmill_common::datatable_roles::lock_role_catalog(&mut tx).await?; - let mut catalog = windmill_common::datatable_roles::read_role_catalog_tx(&mut tx).await?; + let catalog = windmill_common::datatable_roles::read_role_catalog_tx(&mut tx).await?; let role = catalog .get(&id) .cloned() .ok_or_else(|| error::Error::NotFound(format!("No data table role with id '{id}'")))?; windmill_common::datatable_roles::drop_instance_role(&db, &mut tx, &role.name).await?; - catalog.remove(&id); - windmill_common::datatable_roles::write_role_catalog(&mut tx, &catalog).await?; + windmill_common::datatable_roles::delete_role_catalog_entry(&mut tx, &id).await?; tx.commit().await?; // After the drop commits: a tenant naming a role that still exists is harmless, one naming a // role that is gone is not, so this only ever runs once the cluster agrees it is gone. diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 5847f2d4c6..0f0381db14 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3575,6 +3575,43 @@ async fn edit_datatable_config( for r in &new_config.renames { crate::datatable_migrations::validate_datatable_path_segment(&r.from)?; crate::datatable_migrations::validate_new_datatable_name(&r.to)?; + // A rename is a claim about what this save is doing, and other workspaces' pointers are + // rewritten from it. Unchecked, a caller could submit an unchanged configuration with + // `main -> missing` and repoint every fork of `main` at a name nothing has. + if !old_datatables.contains_key(&r.from) { + return Err(Error::BadRequest(format!( + "Cannot rename data table '{}': this workspace has no such data table", + r.from + ))); + } + if !new_config.settings.datatables.contains_key(&r.to) { + return Err(Error::BadRequest(format!( + "Cannot rename data table '{}' to '{}': the save does not contain '{}'", + r.from, r.to, r.to + ))); + } + } + // `A -> B` and `B -> C` applied one after another would move what pointed at `A` all the way + // to `C`. Each pointer moves once, from what it named before this save. + if new_config.renames.len() > 1 { + let mut seen = std::collections::HashSet::new(); + for r in &new_config.renames { + if !seen.insert(r.from.as_str()) { + return Err(Error::BadRequest(format!( + "Data table '{}' is renamed twice in one save", + r.from + ))); + } + } + for r in &new_config.renames { + if seen.contains(r.to.as_str()) && r.to != r.from { + return Err(Error::BadRequest(format!( + "Data table '{}' is both renamed and the target of another rename in one \ + save; do them one at a time", + r.to + ))); + } + } } // Map new name -> old name so a renamed data table inherits the previous diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 4090baee75..c2f49cd9b5 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -21,7 +21,6 @@ use serde::{Deserialize, Serialize}; use crate::{ error::{Error, Result}, - global_settings::DATATABLE_ROLES_SETTING, DB, }; @@ -34,8 +33,8 @@ pub const ADMIN_DATATABLE_ROLE: &str = "admin"; /// membership is what later lets it `ALTER ... OWNER TO` a role and drop it. pub const CUSTOM_INSTANCE_USER: &str = "custom_instance_user"; -/// One catalog entry. The password is per role and instance-wide, and lives in the instance's own -/// [`DATATABLE_ROLES_SETTING`] row rather than in any workspace's settings. +/// One catalog entry, as stored in `datatable_role`. The password is per role and instance-wide; +/// it belongs to the instance, not to any workspace's settings. #[derive(Deserialize, Serialize, Clone)] pub struct InstanceDatatableRole { /// The Postgres role name, verbatim. @@ -45,11 +44,11 @@ pub struct InstanceDatatableRole { /// Absent only for a role whose provisioning did not finish; resolving as it then errors /// rather than falling back to admin. /// - /// A plain string rather than a `StringOrSecretRef` like the instance user's password beside - /// it: that one is a secret ref because an operator supplies it and may want it to come from - /// their own backend, while this one is minted here and never entered by anyone, so there is - /// nothing for a ref to point at. Encrypting generated secrets at rest is a separate change - /// that would take the replication password with it. + /// A plain string rather than a `StringOrSecretRef` like the instance user's password: that + /// one is a secret ref because an operator supplies it and may want it to come from their own + /// backend, while this one is minted here and never entered by anyone, so there is nothing for + /// a ref to point at. Encrypting generated secrets at rest is a separate change that would + /// take the replication password with it. #[serde(default, skip_serializing_if = "Option::is_none")] pub pwd: Option, } @@ -114,29 +113,14 @@ fn quote_literal(value: &str) -> String { format!("'{}'", value.replace('\'', "''")) } -/// The catalog as a settings reader may see it: every entry, no password. +/// Serialize the mutations that are not already serialized by the row itself. /// -/// `GET /settings/global/{key}` and the settings listing hand back whatever is in the row, so the -/// one key whose value is a set of live cluster credentials has to be filtered on the way out. -/// Applied by name, since those endpoints do not know what they are returning. -pub fn redact_role_catalog_setting(name: &str, value: serde_json::Value) -> serde_json::Value { - if name != DATATABLE_ROLES_SETTING { - return value; - } - let mut catalog = parse_role_catalog(Some(value)); - for role in catalog.values_mut() { - role.pwd = None; - } - serde_json::to_value(&catalog).unwrap_or(serde_json::Value::Null) -} - -/// Serialize every mutation of the catalog, from the read through the cluster DDL to the write. -/// -/// The catalog is one JSON document, so create/rename/enable/delete are all read-modify-write. -/// Without this, two concurrent creates both read the same snapshot, both succeed in the cluster, -/// and the second write drops the first — leaving a live Postgres login with a password nobody -/// recorded, which is exactly the state the whole delete path exists to avoid. Held for the -/// transaction, so the DDL has to run on that same transaction to be covered. +/// A create is an insert and a delete is a delete, which Postgres orders for us — the unique index +/// on `name` is what makes two concurrent creates of the same name one winner and one error. What +/// still needs it is the window between the cluster DDL and the row: `CREATE ROLE` is not visible +/// to another transaction's `pg_roles` check until commit, so without this two creates of the same +/// name both pass their existence check and one fails on the index having already made the login. +/// Held for the transaction, so the DDL has to run on that same transaction to be covered. pub async fn lock_role_catalog(tx: &mut sqlx::Transaction<'_, sqlx::Postgres>) -> Result<()> { sqlx::query!("SELECT pg_advisory_xact_lock(hashtext('datatable_role_catalog'))") .execute(&mut **tx) @@ -150,13 +134,18 @@ pub async fn lock_role_catalog(tx: &mut sqlx::Transaction<'_, sqlx::Postgres>) - /// 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 { - let value = sqlx::query_scalar!( - "SELECT value FROM global_settings WHERE name = $1", - DATATABLE_ROLES_SETTING - ) - .fetch_optional(db) - .await?; - Ok(parse_role_catalog(value)) + let rows = sqlx::query!("SELECT id, name, enabled, pwd FROM datatable_role") + .fetch_all(db) + .await?; + Ok(rows + .into_iter() + .map(|r| { + ( + r.id, + InstanceDatatableRole { name: r.name, enabled: r.enabled, pwd: r.pwd }, + ) + }) + .collect()) } /// As [`read_role_catalog`], reading inside the caller's transaction so the value is the one @@ -164,44 +153,70 @@ pub async fn read_role_catalog(db: &DB) -> Result { pub async fn read_role_catalog_tx( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, ) -> Result { - let value = sqlx::query_scalar!( - "SELECT value FROM global_settings WHERE name = $1", - DATATABLE_ROLES_SETTING - ) - .fetch_optional(&mut **tx) - .await?; - Ok(parse_role_catalog(value)) + let rows = sqlx::query!("SELECT id, name, enabled, pwd FROM datatable_role") + .fetch_all(&mut **tx) + .await?; + Ok(rows + .into_iter() + .map(|r| { + ( + r.id, + InstanceDatatableRole { name: r.name, enabled: r.enabled, pwd: r.pwd }, + ) + }) + .collect()) } -/// Persist the catalog, in the caller's transaction so it commits with the cluster DDL it -/// describes. Upserts: the row does not exist until the first role is created. +/// Record a role, in the caller's transaction so it commits with the `CREATE ROLE` it describes. /// -/// Authorization: writes generated Postgres credentials. Callers MUST restrict this to superadmin +/// Authorization: writes a generated Postgres credential. Callers MUST restrict this to superadmin /// paths and MUST hold [`lock_role_catalog`] on `tx`. -pub async fn write_role_catalog( +pub async fn insert_role_catalog_entry( tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, - catalog: &DatatableRoleCatalog, + id: &str, + role: &InstanceDatatableRole, ) -> Result<()> { - let value = serde_json::to_value(catalog) - .map_err(|e| Error::internal_err(format!("serializing the role catalog: {e}")))?; sqlx::query!( - "INSERT INTO global_settings (name, value) VALUES ($1, $2) - ON CONFLICT (name) DO UPDATE SET value = $2, updated_at = now()", - DATATABLE_ROLES_SETTING, - value + "INSERT INTO datatable_role (id, name, enabled, pwd) VALUES ($1, $2, $3, $4)", + id, + role.name, + role.enabled, + role.pwd, ) .execute(&mut **tx) .await?; Ok(()) } -/// A catalog that will not deserialize is an empty one, which fails closed: tenants are keyed by -/// id independently of it, so every role then resolves to "no longer exists on this instance" -/// rather than to admin. -fn parse_role_catalog(value: Option) -> DatatableRoleCatalog { - value - .map(|v| serde_json::from_value(v).unwrap_or_default()) - .unwrap_or_default() +/// Update a role's recorded name, login flag and password. Same contract as +/// [`insert_role_catalog_entry`]. +pub async fn update_role_catalog_entry( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + id: &str, + role: &InstanceDatatableRole, +) -> Result<()> { + sqlx::query!( + "UPDATE datatable_role SET name = $2, enabled = $3, pwd = $4 WHERE id = $1", + id, + role.name, + role.enabled, + role.pwd, + ) + .execute(&mut **tx) + .await?; + Ok(()) +} + +/// Forget a role. Same contract as [`insert_role_catalog_entry`]; run it in the transaction that +/// drops the cluster login, so the two cannot disagree. +pub async fn delete_role_catalog_entry( + tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, + id: &str, +) -> Result<()> { + sqlx::query!("DELETE FROM datatable_role WHERE id = $1", id) + .execute(&mut **tx) + .await?; + Ok(()) } /// Resolve the role a caller named to its catalog id. A disabled role is an error rather than a diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index 31c4a4ab7b..8180e820f2 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -243,18 +243,8 @@ pub const AGENT_WORKER_BLOCKED_SETTINGS: &[&str] = &[ // resolve datatable connections through the dedicated datatable endpoints, never these. "custom_instance_pg_databases", "custom_instance_replication_pwd", - // The data table role catalog: one generated Postgres password per role. - DATATABLE_ROLES_SETTING, ]; -/// The instance's data table role catalog, `{ "": { name, enabled, pwd } }`. -/// -/// Its own row rather than a field of `custom_instance_pg_databases`, for the same reason -/// `custom_instance_replication_pwd` is: it holds generated credentials and is written only by the -/// server, so the config machinery must not be able to read it into an export, rewrite it, or drop -/// it on a full-row upsert of a neighbour. -pub const DATATABLE_ROLES_SETTING: &str = "datatable_roles"; - /// Whether an agent worker may read the given global setting over HTTP. /// Deny-by-exception: everything is readable except [`AGENT_WORKER_BLOCKED_SETTINGS`]. pub fn is_setting_readable_by_agent_worker(name: &str) -> bool { @@ -687,7 +677,6 @@ mod tests { OTEL_TRACING_PROXY_SETTING, "custom_instance_pg_databases", "custom_instance_replication_pwd", - DATATABLE_ROLES_SETTING, ] { assert!( !is_setting_readable_by_agent_worker(key), diff --git a/backend/windmill-common/src/instance_config.rs b/backend/windmill-common/src/instance_config.rs index 0d7f51da41..7ea6dbe5ae 100644 --- a/backend/windmill-common/src/instance_config.rs +++ b/backend/windmill-common/src/instance_config.rs @@ -959,7 +959,6 @@ pub const PROTECTED_SETTINGS: &[&str] = &[ "ducklake_settings", "custom_instance_pg_databases", "custom_instance_replication_pwd", - crate::global_settings::DATATABLE_ROLES_SETTING, "uid", "rsa_keys", "jwt_secret", @@ -985,9 +984,6 @@ pub const HIDDEN_SETTINGS: &[&str] = &[ // Server-only (written by setup/refresh via direct SQL), never operator-authored — // hidden so the config machinery can't read, rewrite, or drop it. "custom_instance_replication_pwd", - // The data table role catalog, one generated Postgres password per role. Same reasoning as - // the line above: server-written, never operator-authored, and it must not reach an export. - crate::global_settings::DATATABLE_ROLES_SETTING, ]; /// Top-level settings whose entire value is sensitive and must be fully redacted in logs. diff --git a/frontend/src/lib/components/sidebar/DeleteForkedWorkspaceModal.svelte b/frontend/src/lib/components/sidebar/DeleteForkedWorkspaceModal.svelte index 676f5b6af1..b81347bbae 100644 --- a/frontend/src/lib/components/sidebar/DeleteForkedWorkspaceModal.svelte +++ b/frontend/src/lib/components/sidebar/DeleteForkedWorkspaceModal.svelte @@ -122,7 +122,12 @@ } } - await WorkspaceService.deleteWorkspace({ workspace }) + const result = await WorkspaceService.deleteWorkspace({ workspace }) + // The server names any data table in another workspace that this delete left governed by + // nothing. Only surfaced when there is something to say. + if (typeof result === 'string' && result.includes('no longer resolve')) { + sendUserToast(result, 'warning', [], undefined, 20000) + } await deleteSessionsForWorkspace(workspace).catch((e) => console.error('Session cleanup after workspace delete failed', e) ) diff --git a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte index 50f6904751..19e7499044 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte @@ -223,12 +223,21 @@ const deleted_datatables = dataTableSettings.dataTables .filter((d) => !tempIds.has(d.id)) .map((d) => d.name) - await WorkspaceService.editDataTableConfig({ + const result = await WorkspaceService.editDataTableConfig({ workspace: $workspaceStore!, requestBody: { settings, renames, deleted_datatables } }) dataTableSettings = clone(tempSettings) - sendUserToast('Data table settings saved successfully') + // The server says here when a delete left another workspace's data table pointing at + // nothing. Swallowing it is what made that failure silent for the person who caused it. + const stranded = typeof result === 'string' && result.includes('no longer resolve') + sendUserToast( + stranded ? result : 'Data table settings saved successfully', + stranded ? 'warning' : 'success', + [], + undefined, + stranded ? 20000 : 5000 + ) } catch (e) { sendUserToast(e, true) console.error('Error saving data table settings', e)