starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-21 08:02:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(datatables): give the role catalog its own row, out of reach of the config machinery

Browse Source 
Putting it inside `custom_instance_pg_databases` was the wrong call, and it cost two ways.
The catalog serializes a generated Postgres password per role, and that row is the
operator-facing instance config, so the passwords reached `get_instance_config` and its YAML
editor — a live cluster credential in a response body, a UI field and any log of either.
Worse in the other direction: `to_settings_map` strips the catalog, so a full-row upsert of
that key writes the row back without it and the catalog is gone, while the cluster keeps every
login it described.

`custom_instance_replication_pwd` is the precedent and says exactly why — a generated secret,
written only by the server, never operator-authored, hidden so the config machinery cannot
read, rewrite or drop it. The catalog is the same thing, so it now has the same shape:
`datatable_roles`, in `HIDDEN_SETTINGS`, `PROTECTED_SETTINGS` and the agent-worker denylist.
No redaction to keep in step with three code paths, and no way for a neighbouring write to
take it out.

Two races on the same shared documents. `edit_datatable_config` read the stored data tables
outside its transaction and then wrote the whole `datatable` document, so a permissions save
committing in between was silently rolled back; it now reads under `FOR UPDATE`. And
`set_datatable_permissions` validated role ids against the catalog before opening its
transaction, so a deletion in between let it write a deleted role back — including as the
default, which every later job then fails on; it now holds the catalog lock and the settings
row across validation and write.

Completes the authorization contracts the previous commit claimed but did not finish:
`read_datatable_entry` (which it named and missed), `resolve_governing_datatable`, whose whole
job is to answer for a workspace the caller may not belong to, and
`converge_connect_grants_with`, which had not inherited its wrapper's.

Also the generic Python SDK reference: `_format_py_params` learned the bare `*` last time, but
`extract_py_functions` is a second formatter and still rendered `datatable(name, role)`, so
code written from that page passed a keyword-only argument positionally.

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-17 10:01:15 +02:00
committed by GitHub
co-authored by Claude Opus 5
parent a7bff8de97
commit a74f73ccb1
18 changed files with 215 additions and 133 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/windmill-api-workspaces/src/datatable_permissions.rs
+40 -25
View File
@@ -274,8 +274,21 @@ async fn set_datatable_permissions(
)));
}
// One transaction for the whole save, holding both locks the decision depends on: the role
// catalog, so a role cannot be deleted between validating an id and writing it back, and the
// workspace settings row, so a concurrent settings save cannot carry a stale copy of this
// block forward over what is written here.
let mut tx = db.begin().await?;
windmill_common::datatable_roles::lock_role_catalog(&mut tx).await?;
sqlx::query!(
"SELECT 1 AS one FROM workspace_settings WHERE workspace_id = $1 FOR UPDATE",
&governing.workspace_id
)
.fetch_optional(&mut *tx)
.await?;
let permissions = if req.permissioned {
let catalog = read_role_catalog(&db).await?;
let catalog = windmill_common::datatable_roles::read_role_catalog_tx(&mut tx).await?;
let mut roles: BTreeMap<String, DataTableRoleTenants> = BTreeMap::new();
for role in req.roles {
if role.id != ADMIN_DATATABLE_ROLE && !catalog.contains_key(&role.id) {
@@ -306,30 +319,6 @@ async fn set_datatable_permissions(
None
};
// An instance database provisioned before data table roles existed has neither the grant
// options the admin connection needs to delegate privileges, nor a CONNECT grant for any role
// — so a role would be refused at login however its tenants read. Repair it here, at the one
// moment someone is deciding this data table's roles. Best-effort: neither is worth failing a
// tenant edit over, and both converge again on the next save.
if permissions.is_some() {
if let Some(database) = governing.datatable.database.as_ref() {
if database.resource_type == DataTableCatalogResourceType::Instance {
let dbname = &database.resource_path;
if let Err(e) =
windmill_common::ensure_instance_db_grant_options_unchecked(&db, dbname).await
{
tracing::warn!("Could not refresh grant options on '{dbname}': {e}");
}
if let Err(e) =
windmill_common::datatable_roles::converge_connect_grants(&db, dbname).await
{
tracing::warn!("Could not refresh CONNECT grants on '{dbname}': {e}");
}
}
}
}
let mut tx = db.begin().await?;
let value = match &permissions {
Some(p) => serde_json::to_value(p).map_err(|e| Error::internal_err(e.to_string()))?,
None => serde_json::Value::Null,
@@ -362,6 +351,32 @@ async fn set_datatable_permissions(
.await?;
tx.commit().await?;
// An instance database provisioned before data table roles existed has neither the grant
// options the admin connection needs to delegate privileges, nor a CONNECT grant for any role
// — so a role would be refused at login however its tenants read. Repair it here, at the one
// moment someone is deciding this data table's roles. Best-effort: neither is worth failing a
// tenant edit over, and both converge again on the next save.
//
// Runs after the commit: it opens its own connections to other databases, which has no place
// inside a transaction holding two locks.
if permissions.is_some() {
if let Some(database) = governing.datatable.database.as_ref() {
if database.resource_type == DataTableCatalogResourceType::Instance {
let dbname = &database.resource_path;
if let Err(e) =
windmill_common::ensure_instance_db_grant_options_unchecked(&db, dbname).await
{
tracing::warn!("Could not refresh grant options on '{dbname}': {e}");
}
if let Err(e) =
windmill_common::datatable_roles::converge_connect_grants(&db, dbname).await
{
tracing::warn!("Could not refresh CONNECT grants on '{dbname}': {e}");
}
}
}
}
// A live replication stream holds a connection it opened under the old decision. Bouncing the
// rows makes every listener reconnect and re-authorize.
restart_streams_reaching(&db, &governing).await?;
backend/windmill-api-workspaces/src/workspaces.rs
+7 -2
View File
@@ -3595,12 +3595,17 @@ async fn edit_datatable_config(
let mut tx = db.begin().await?;
// Read under the row lock this transaction will write with. `permissions`, `reference` and
// `forked_from` are carried across from what this read returns, so a permissions save
// committing between the read and the whole-document write below would be silently rolled back
// by it.
let old_datatables: HashMap<String, DataTable> = serde_json::from_value(
sqlx::query_scalar!(
"SELECT ws.datatable->'datatables' FROM workspace_settings ws WHERE ws.workspace_id = $1",
"SELECT ws.datatable->'datatables' FROM workspace_settings ws
WHERE ws.workspace_id = $1 FOR UPDATE",
&w_id
)
.fetch_one(&db)
.fetch_one(&mut *tx)
.await?
.unwrap_or(serde_json::Value::Null),
)