refactor(datatables): put the role catalog in its own table, not in global_settings

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ti5HyeTikPMYyW8YSdiHR
This commit is contained in:
Diego Imbert
2026-09-08 16:29:51 +02:00
co-authored by Claude Opus 5
parent 30803e4e5a
commit bc78383c74
15 changed files with 260 additions and 113 deletions
@@ -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"
}
@@ -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"
}
@@ -0,0 +1,14 @@
{
"db_name": "PostgreSQL",
"query": "DELETE FROM datatable_role WHERE id = $1",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Text"
]
},
"nullable": []
},
"hash": "c85d362fe2e652d4ac01a35bf470e80b993020a2ff5dcb5849dc570d52798587"
}
@@ -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"
}
@@ -0,0 +1 @@
DROP TABLE IF EXISTS datatable_role;
@@ -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;
@@ -341,9 +341,9 @@ async fn concurrent_role_creations_both_survive(db: Pool<Postgres>) -> 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!(
@@ -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": {
+13 -24
View File
@@ -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.
@@ -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
+77 -62
View File
@@ -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<String>,
}
@@ -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<DatatableRoleCatalog> {
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<DatatableRoleCatalog> {
pub async fn read_role_catalog_tx(
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
) -> Result<DatatableRoleCatalog> {
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<serde_json::Value>) -> 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
@@ -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, `{ "<id>": { 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),
@@ -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.
@@ -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)
)
@@ -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)