fix(datatables): validate a rename against the save it describes, and re-check under the locks

Three from the round, all about deciding on state that could already have moved.

A permission save resolved the data table and checked it was instance-backed before taking any
lock, then wrote under one. A config save committing in between could move the table onto a
PostgreSQL resource — recreating exactly what the transition guard refuses — or rename it, in
which case the write targeted a key that no longer existed and reported success having changed
nothing. It now re-resolves and re-checks on the locked state.

Rename validation checked that the source existed before and the target existed after, which
still accepts `main -> decoy` against a save that keeps both: every fork of `main` then follows
onto a different data table, silently, because it keeps resolving. The rule is now the actual
old-to-new key transition — a source may only survive if another rename took its name, and a
target may only pre-exist if another rename freed it. That also stops two sources sharing one
target, and it admits a swap, which the previous guard refused: `datatables` is keyed by name, so
a swap cannot be done one save at a time, and refusing it was a regression against main. The
pointer cascade now runs in two passes through a temporary name, the way the migration cascade
one layer down already handles the same shape, so `A -> B` with `B -> C` moves each pointer once
from what it named before the save.

The tenant mutators say what they are for: they write an access decision for any workspace named,
with an arbitrary mutation, and exist for the transaction that frees or renames a principal.
Editing a decision on purpose belongs in the permissions endpoint.

Carried in the same change: the stranded-fork list is a field rather than a phrase to grep out of
a success string; the pointer cascade matches with `EXISTS` instead of a `LIKE` over the whole
document, so a workspace whose pointers name something else is not rewritten to a byte-identical
value under an exclusive lock; and `InstanceDatatableRole` drops the serde derives left over from
the JSON document, one of which would emit `pwd`.

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 17:40:19 +02:00
co-authored by Claude Opus 5
parent bc78383c74
commit 67091eec69
9 changed files with 274 additions and 102 deletions
@@ -0,0 +1,16 @@
{
"db_name": "PostgreSQL",
"query": "UPDATE workspace_settings ws\n SET datatable = (\n SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg(\n dt.key,\n CASE WHEN dt.value->'reference'->>'workspace_id' = $1\n AND dt.value->'reference'->>'datatable' = $2\n THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text))\n ELSE dt.value END\n ))\n FROM jsonb_each(ws.datatable->'datatables') dt\n )\n WHERE EXISTS (\n SELECT 1 FROM jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) d\n WHERE d.value->'reference'->>'workspace_id' = $1\n AND d.value->'reference'->>'datatable' = $2\n )",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Text",
"Text",
"Text"
]
},
"nullable": []
},
"hash": "06ce02cd7ce2f5a57355153edb573c242f9ba758db66e9a5e16f30e3e1494201"
}
@@ -1,14 +0,0 @@
{
"db_name": "PostgreSQL",
"query": "UPDATE global_settings SET value = jsonb_set(COALESCE(value, '{}'::jsonb), '{roles}', $1)\n WHERE name = 'custom_instance_pg_databases'",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Jsonb"
]
},
"nullable": []
},
"hash": "3fd36fa26a61be923ca23316482ed1ce17184271668622c53e94de7224da38ca"
}
@@ -433,3 +433,82 @@ async fn renaming_a_governing_data_table_carries_its_forks(db: Pool<Postgres>) -
assert_eq!(resp.status(), 200, "{}", resp.text().await?);
Ok(())
}
#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))]
async fn a_rename_has_to_match_the_save_it_claims_to_describe(
db: Pool<Postgres>,
) -> anyhow::Result<()> {
initialize_tracing().await;
let server = ApiServer::start(db.clone()).await?;
let port = server.addr.port();
let url = format!("http://localhost:{port}/api/w/test-workspace/workspaces/edit_datatable_config");
let instance = |path: &str| {
json!({"database": {"resource_type": "instance", "resource_path": path}})
};
// Fork pointers are rewritten from the rename list, so a rename nobody performed moves every
// fork of one data table onto another. `main` survives this save, so it was not renamed.
let resp = authed(client().post(&url), "SECRET_TOKEN")
.json(&json!({
"settings": {"datatables": {"main": instance("dt_main"), "decoy": instance("dt_two")}},
"renames": [{"from": "main", "to": "decoy"}],
"deleted_datatables": []
}))
.send()
.await?;
assert_eq!(resp.status(), 400, "a forged rename was accepted");
let entry: Option<Value> = sqlx::query_scalar(
"SELECT datatable->'datatables'->'main'->'reference' FROM workspace_settings
WHERE workspace_id = $1",
)
.bind("wm-fork-dt")
.fetch_one(&db)
.await?;
assert_eq!(entry.unwrap()["datatable"], "main", "the fork was repointed anyway");
// A swap is two renames whose sources and targets cross. It cannot be done one at a time —
// `datatables` is keyed by name — so refusing it would be a regression, and applying the two
// in order without a temporary name would carry `main`'s pointers back to `main`.
let resp = authed(client().post(&url), "SECRET_TOKEN")
.json(&json!({
"settings": {"datatables": {"main": instance("dt_two"), "other": instance("dt_main")}},
"renames": [{"from": "main", "to": "other"}, {"from": "other", "to": "main"}],
"deleted_datatables": []
}))
.send()
.await?;
// `other` does not exist yet, so this particular pair is still refused — the swap shape is
// covered by the pair below, which starts from two real data tables.
assert_eq!(resp.status(), 400, "{}", resp.text().await?);
sqlx::query(
r#"UPDATE workspace_settings SET datatable = jsonb_set(datatable, '{datatables,other}',
'{"database": {"resource_type": "instance", "resource_path": "dt_two"}}'::jsonb)
WHERE workspace_id = 'test-workspace'"#,
)
.execute(&db)
.await?;
let resp = authed(client().post(&url), "SECRET_TOKEN")
.json(&json!({
"settings": {"datatables": {"main": instance("dt_two"), "other": instance("dt_main")}},
"renames": [{"from": "main", "to": "other"}, {"from": "other", "to": "main"}],
"deleted_datatables": []
}))
.send()
.await?;
assert_eq!(resp.status(), 200, "a swap was refused: {}", resp.text().await?);
// The fork named `main`, which is now called `other`.
let entry: Option<Value> = sqlx::query_scalar(
"SELECT datatable->'datatables'->'main'->'reference' FROM workspace_settings
WHERE workspace_id = $1",
)
.bind("wm-fork-dt")
.fetch_one(&db)
.await?;
assert_eq!(entry.unwrap()["datatable"], "other", "the swap did not carry the pointer");
Ok(())
}
@@ -287,6 +287,23 @@ async fn set_datatable_permissions(
.fetch_optional(&mut *tx)
.await?;
// Everything above was decided on a read taken before the locks. A settings save committing in
// between could have moved this data table onto a PostgreSQL resource — recreating the exact
// state the transition guard refuses — or renamed it, in which case the write below would
// target a key that no longer exists and report success having changed nothing. Re-resolve and
// re-check on the locked state; the earlier pass stays because it is what refuses without
// taking locks at all.
let governing = resolve_governing_datatable(&db, &w_id, &datatable_name).await?;
ensure_governs_datatable(&db, &authed, &w_id, &governing).await?;
if req.permissioned && !governing.is_instance() {
return Err(Error::BadRequest(format!(
"Data table '{}' is backed by a Postgres resource. Data table roles are logins on the \
Windmill instance's own Postgres, so only a data table on the instance database can \
use them.",
governing.name
)));
}
let permissions = if req.permissioned {
let catalog = windmill_common::datatable_roles::read_role_catalog_tx(&mut tx).await?;
let mut roles: BTreeMap<String, DataTableRoleTenants> = BTreeMap::new();
+111 -69
View File
@@ -3532,13 +3532,28 @@ async fn edit_ducklake_config(
Ok(format!("Edit ducklake config for workspace {}", &w_id))
}
/// What a save left behind. `stranded_references` names the data tables in other workspaces that
/// were governed by one this save deleted — a field rather than a sentence in a success string,
/// so the UI decides whether to warn on the data rather than on the server's prose.
#[derive(Serialize)]
pub struct EditDataTableConfigResult {
#[serde(skip_serializing_if = "Vec::is_empty")]
stranded_references: Vec<StrandedReference>,
}
#[derive(Serialize)]
pub struct StrandedReference {
workspace_id: String,
datatable: String,
}
async fn edit_datatable_config(
authed: ApiAuthed,
Extension(db): Extension<DB>,
Path(w_id): Path<String>,
ApiAuthed { is_admin, username, .. }: ApiAuthed,
Json(mut new_config): Json<EditDataTableConfig>,
) -> Result<String> {
) -> JsonResult<EditDataTableConfigResult> {
require_admin(is_admin, &username)?;
let is_superadmin = require_super_admin(&db, &authed).await.is_ok();
@@ -3575,40 +3590,55 @@ 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();
// A rename is a claim about what this save is doing, and other workspaces' pointers are
// rewritten from it — so the claim has to match the configuration it describes, or a caller
// can move every fork of one data table onto another by asserting a rename that did not
// happen. The shape below is what "these old keys became those new keys" actually means.
{
let old_keys = &old_datatables;
let new_keys = &new_config.settings.datatables;
let froms: std::collections::HashSet<&str> =
new_config.renames.iter().map(|r| r.from.as_str()).collect();
let tos: std::collections::HashSet<&str> =
new_config.renames.iter().map(|r| r.to.as_str()).collect();
if froms.len() != new_config.renames.len() {
return Err(Error::BadRequest(
"A data table is renamed twice in one save".to_string(),
));
}
if tos.len() != new_config.renames.len() {
return Err(Error::BadRequest(
"Two data tables are renamed to the same name in one save".to_string(),
));
}
for r in &new_config.renames {
if !seen.insert(r.from.as_str()) {
if !old_keys.contains_key(&r.from) {
return Err(Error::BadRequest(format!(
"Data table '{}' is renamed twice in one save",
"Cannot rename data table '{}': this workspace has no such data table",
r.from
)));
}
}
for r in &new_config.renames {
if seen.contains(r.to.as_str()) && r.to != r.from {
if !new_keys.contains_key(&r.to) {
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
"Cannot rename data table '{}' to '{}': the save does not contain '{}'",
r.from, r.to, r.to
)));
}
// The source has to be gone, or gone-and-reoccupied by another rename — which is what
// a swap is. Without this, `main -> decoy` passes against a save that keeps both, and
// every fork of `main` silently follows onto a different data table.
if new_keys.contains_key(&r.from) && !tos.contains(r.from.as_str()) {
return Err(Error::BadRequest(format!(
"Data table '{}' is renamed to '{}' but the save still contains '{}'",
r.from, r.to, r.from
)));
}
// And the target has to be free, or freed by another rename.
if old_keys.contains_key(&r.to) && !froms.contains(r.to.as_str()) {
return Err(Error::BadRequest(format!(
"Cannot rename data table '{}' to '{}': '{}' already exists",
r.from, r.to, r.to
)));
}
}
@@ -3738,34 +3768,21 @@ async fn edit_datatable_config(
.await?;
// A fork points at a data table by name, so a rename here has to follow or every fork's entry
// resolves to nothing. Inside the transaction: the rename and the pointers that name it are one
// change, and half of it is a fork whose jobs stop.
for r in &new_config.renames {
sqlx::query!(
r#"UPDATE workspace_settings ws
SET datatable = (
SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg(
dt.key,
CASE WHEN dt.value->'reference'->>'workspace_id' = $1
AND dt.value->'reference'->>'datatable' = $2
THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text))
ELSE dt.value END
))
FROM jsonb_each(ws.datatable->'datatables') dt
)
WHERE jsonb_typeof(ws.datatable->'datatables') = 'object'
AND ws.datatable::text LIKE '%"reference"%'"#,
&w_id,
&r.from,
&r.to,
)
.execute(&mut *tx)
.await?;
// resolves to nothing. In two passes through a temporary name, like the migration cascade one
// layer down: applied in order, `sa -> sb` then `sb -> sa` would move what pointed at `sa` all
// the way back to `sa`, and `A -> B`, `B -> C` would carry `A`'s pointers to `C`. Each pointer
// moves once, from what it named before this save. Inside the transaction: the rename and the
// pointers that name it are one change, and half of it is a fork whose jobs stop.
for (i, r) in new_config.renames.iter().enumerate() {
repoint_datatable_references(&mut tx, &w_id, &r.from, &format!("__wm_rename_tmp/{i}")).await?;
}
for (i, r) in new_config.renames.iter().enumerate() {
repoint_datatable_references(&mut tx, &w_id, &format!("__wm_rename_tmp/{i}"), &r.to).await?;
}
// 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<String> = Vec::new();
let mut stranded: Vec<StrandedReference> = Vec::new();
for name in &new_config.deleted_datatables {
let rows = sqlx::query!(
r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!"
@@ -3778,10 +3795,10 @@ async fn edit_datatable_config(
)
.fetch_all(&mut *tx)
.await?;
stranded.extend(
rows.into_iter()
.map(|r| format!("{}/{}", r.workspace_id, r.datatable)),
);
stranded.extend(rows.into_iter().map(|r| StrandedReference {
workspace_id: r.workspace_id,
datatable: r.datatable,
}));
}
tx.commit().await?;
@@ -3798,19 +3815,7 @@ async fn edit_datatable_config(
)
.await?;
if stranded.is_empty() {
Ok(format!("Edit datatable config for workspace {}", &w_id))
} else {
Ok(format!(
concat!(
"Edit datatable config for workspace {}. These data tables were governed by one ",
"you deleted and no longer resolve: {}. Their databases still exist; a superadmin ",
"can point them at another workspace's data table."
),
&w_id,
stranded.join(", ")
))
}
Ok(Json(EditDataTableConfigResult { stranded_references: stranded }))
}
#[derive(Deserialize)]
@@ -7771,6 +7776,43 @@ async fn point_kept_datatables_at_parent(
Ok(())
}
/// Move every pointer in any workspace that names `(w_id, from)` to `(w_id, to)`.
///
/// `EXISTS` rather than a `LIKE` over the whole document: the update rewrites the row, so matching
/// every workspace that holds any pointer would rewrite rows to a byte-identical value and hold an
/// exclusive lock on them until commit.
async fn repoint_datatable_references(
tx: &mut Transaction<'_, Postgres>,
w_id: &str,
from: &str,
to: &str,
) -> Result<()> {
sqlx::query!(
r#"UPDATE workspace_settings ws
SET datatable = (
SELECT jsonb_set(ws.datatable, '{datatables}', jsonb_object_agg(
dt.key,
CASE WHEN dt.value->'reference'->>'workspace_id' = $1
AND dt.value->'reference'->>'datatable' = $2
THEN jsonb_set(dt.value, '{reference,datatable}', to_jsonb($3::text))
ELSE dt.value END
))
FROM jsonb_each(ws.datatable->'datatables') dt
)
WHERE EXISTS (
SELECT 1 FROM jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) d
WHERE d.value->'reference'->>'workspace_id' = $1
AND d.value->'reference'->>'datatable' = $2
)"#,
w_id,
from,
to,
)
.execute(&mut **tx)
.await?;
Ok(())
}
async fn apply_forked_datatable(
db: &DB,
tx: &mut Transaction<'_, Postgres>,
+16 -1
View File
@@ -5345,7 +5345,22 @@ paths:
description: status
content:
application/json:
schema: {}
schema:
type: object
properties:
stranded_references:
description: >-
Data tables in other workspaces that were governed by one this save deleted
and no longer resolve.
type: array
items:
type: object
required: [workspace_id, datatable]
properties:
workspace_id:
type: string
datatable:
type: string
/w/{workspace}/workspaces/run_datatable_migrations/{datatable_name}:
post:
@@ -17,8 +17,6 @@
use std::collections::BTreeMap;
use serde::{Deserialize, Serialize};
use crate::{
error::{Error, Result},
DB,
@@ -35,11 +33,13 @@ pub const CUSTOM_INSTANCE_USER: &str = "custom_instance_user";
/// 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)]
/// No `Serialize`/`Deserialize`: the catalog is rows now, and a derived `Serialize` would emit
/// `pwd` — the same way out for a credential that the hand-written `Debug` below closes on the log
/// side.
#[derive(Clone)]
pub struct InstanceDatatableRole {
/// The Postgres role name, verbatim.
pub name: String,
#[serde(default = "crate::more_serde::default_true")]
pub enabled: bool,
/// Absent only for a role whose provisioning did not finish; resolving as it then errors
/// rather than falling back to admin.
@@ -49,7 +49,6 @@ pub struct InstanceDatatableRole {
/// 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>,
}
+14 -3
View File
@@ -1882,6 +1882,12 @@ pub async fn ensure_datatable_admin_access(
/// transaction. `change` reports whether it touched anything; the row is only written when
/// something did.
///
/// Authorization: writes an access decision for any workspace named, with an arbitrary mutation,
/// and checks nothing. It exists for the cascades below — the transaction that frees or renames a
/// principal — so callers MUST be the operation that made the principal change, and MUST run in
/// its transaction. Anything editing a decision on purpose belongs in the permissions endpoint,
/// which is gated on the workspace that governs the data table.
///
/// The tenant lists name principals of this workspace, so anything that frees or renames one has
/// to come through here in the same transaction that frees it — otherwise a `u/alice` reused by a
/// later account silently inherits her access.
@@ -1940,7 +1946,8 @@ where
}
/// Drop a freed principal (`u/alice`, `g/analysts`, `f/finance`) from every tenant list of one
/// workspace.
/// workspace. Same contract as [`update_datatable_permissions_in_workspace`]: for the transaction
/// that frees the principal, not for editing a decision.
pub async fn remove_datatable_tenant_in_workspace(
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
w_id: &str,
@@ -1958,7 +1965,8 @@ pub async fn remove_datatable_tenant_in_workspace(
.await
}
/// Follow a renamed principal through every tenant list of one workspace.
/// Follow a renamed principal through every tenant list of one workspace. Same contract as
/// [`update_datatable_permissions_in_workspace`].
pub async fn rename_datatable_tenant_in_workspace(
tx: &mut sqlx::Transaction<'_, sqlx::Postgres>,
w_id: &str,
@@ -1989,7 +1997,10 @@ pub async fn rename_datatable_tenant_in_workspace(
}
/// Strip a deleted instance role from every workspace that had tenanted it, so nothing is left
/// naming a role that no longer exists. A data table whose default role was the deleted one falls
/// naming a role that no longer exists.
///
/// Authorization: reaches every workspace on the instance. Callers MUST be the superadmin path
/// that just dropped the role from the cluster — it exists to follow that, not to edit tenants. A data table whose default role was the deleted one falls
/// back to `admin` — the one role that is always present.
pub async fn forget_datatable_role_everywhere(db: &DB, role_id: &str) -> Result<()> {
let workspaces = sqlx::query_scalar!(
@@ -228,16 +228,23 @@
requestBody: { settings, renames, deleted_datatables }
})
dataTableSettings = clone(tempSettings)
// 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
)
// A delete can leave another workspace's data table governed by nothing. Swallowing
// that is what made it silent for the person who caused it.
const stranded = result?.stranded_references ?? []
if (stranded.length > 0) {
sendUserToast(
`These data tables were governed by one you deleted and no longer resolve: ${stranded
.map((s) => `${s.workspace_id}/${s.datatable}`)
.join(', ')}. Their databases still exist; a superadmin can point them at another ` +
`workspace's data table.`,
'warning',
[],
undefined,
20000
)
} else {
sendUserToast('Data table settings saved successfully')
}
} catch (e) {
sendUserToast(e, true)
console.error('Error saving data table settings', e)