mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
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:
co-authored by
Claude Opus 5
parent
7f3c7a19af
commit
e400daabb8
+38
@@ -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"
|
||||
}
|
||||
+17
@@ -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"
|
||||
}
|
||||
+14
@@ -0,0 +1,14 @@
|
||||
{
|
||||
"db_name": "PostgreSQL",
|
||||
"query": "DELETE FROM datatable_role WHERE id = $1",
|
||||
"describe": {
|
||||
"columns": [],
|
||||
"parameters": {
|
||||
"Left": [
|
||||
"Text"
|
||||
]
|
||||
},
|
||||
"nullable": []
|
||||
},
|
||||
"hash": "c85d362fe2e652d4ac01a35bf470e80b993020a2ff5dcb5849dc570d52798587"
|
||||
}
|
||||
+17
@@ -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": {
|
||||
|
||||
@@ -1356,7 +1356,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)))
|
||||
}
|
||||
|
||||
@@ -1394,13 +1393,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))
|
||||
}
|
||||
@@ -2659,9 +2652,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?;
|
||||
@@ -2676,15 +2668,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", "");
|
||||
@@ -2740,8 +2730,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;
|
||||
|
||||
@@ -2776,15 +2766,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.
|
||||
|
||||
@@ -3630,6 +3630,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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -355,18 +355,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 {
|
||||
@@ -998,7 +988,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),
|
||||
|
||||
@@ -969,7 +969,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",
|
||||
@@ -995,9 +994,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)
|
||||
|
||||
Reference in New Issue
Block a user