fix(datatables): confine roles to the instance database, and stop a fork reaching the parent's bookkeeping

A data table role is a login on Windmill's own Postgres. Nothing stopped a workspace admin
putting a *resource-backed* data table under roles, at which point the executor dialled the
host that resource names — one the admin chose — with the role's real cluster password, and
`CONNECT` is granted to every registered instance database. Both ends now refuse: the
permissions endpoint rejects the save, and the chokepoint refuses to substitute credentials
on a non-instance entry rather than trusting the record it read.

Two more places reached the governing database without answering to it. The initial-migration
generator returned a `pg_dump` of the whole schema to any member. And the migration
rename/delete cascade followed a fork's pointer into the parent, so a fork admin renaming or
removing their own local entry relabelled or wiped the parent's `_wm_migrations` — after
which the parent re-runs every migration from zero. The remote half is now skipped when the
entry resolves into another workspace, which is also just correct: a fork renaming what it
calls a data table changes nothing about the data table.

Also: revoking a tenant now bounces the replication streams of every workspace holding an
entry that resolves here, not only the governing one, so a fork's trigger stops rather than
living on inside its open connection; the instance role catalog and the governing workspace's
tenant lists are no longer returned to someone who cannot edit them; and the tenant rename
dedup collapses non-adjacent duplicates, per role rather than once any role changed.

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 13:22:20 +02:00
co-authored by Claude Opus 5
parent 0ec1cb2bdd
commit 6a97f67713
10 changed files with 309 additions and 44 deletions
@@ -0,0 +1,16 @@
{
"db_name": "PostgreSQL",
"query": "UPDATE capture_config SET server_id = NULL, last_server_ping = NULL\n WHERE workspace_id = $1 AND trigger_kind = 'postgres'\n AND (trigger_config->>'postgres_resource_path' = $2\n OR trigger_config->>'postgres_resource_path' LIKE $3)",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Text",
"Text",
"Text"
]
},
"nullable": []
},
"hash": "2a391cc1bfcd2f75b46144a394c01237e09c3060da88170f1f6e06468309d213"
}
@@ -0,0 +1,29 @@
{
"db_name": "PostgreSQL",
"query": "SELECT ws.workspace_id AS \"workspace_id!\", dt.key AS \"datatable!\"\n FROM workspace_settings ws\n CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt\n WHERE dt.value->'reference'->>'workspace_id' = $1\n AND dt.value->'reference'->>'datatable' = $2",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "workspace_id!",
"type_info": "Varchar"
},
{
"ordinal": 1,
"name": "datatable!",
"type_info": "Text"
}
],
"parameters": {
"Left": [
"Text",
"Text"
]
},
"nullable": [
false,
null
]
},
"hash": "5048e21546f9710697100100e1255ab103979433bc386d7c89d0e30db12bfd57"
}
@@ -0,0 +1,16 @@
{
"db_name": "PostgreSQL",
"query": "UPDATE postgres_trigger SET server_id = NULL, last_server_ping = NULL\n WHERE workspace_id = $1\n AND (postgres_resource_path = $2 OR postgres_resource_path LIKE $3)",
"describe": {
"columns": [],
"parameters": {
"Left": [
"Text",
"Text",
"Text"
]
},
"nullable": []
},
"hash": "e159b2ff15633f85e839ee4fe1ec2ecd11caf228ea8d0f52ad66def595644250"
}
@@ -180,3 +180,83 @@ async fn a_second_entry_on_the_same_database_is_reported_rather_than_governed(
);
Ok(())
}
#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))]
async fn a_resource_backed_data_table_cannot_be_put_under_roles(
db: Pool<Postgres>,
) -> anyhow::Result<()> {
initialize_tracing().await;
// A role is a login on Windmill's own cluster. A resource-backed data table dials a host the
// workspace admin chose, so accepting one here would hand that host a real cluster credential.
sqlx::query(
r#"UPDATE workspace_settings SET datatable = '{"datatables": {"byo": {
"database": {"resource_type": "postgresql", "resource_path": "u/test-user/pg"}}}}'::jsonb
WHERE workspace_id = 'test-workspace'"#,
)
.execute(&db)
.await?;
let server = ApiServer::start(db.clone()).await?;
let port = server.addr.port();
let base = format!("http://localhost:{port}/api/w/test-workspace/workspaces");
let resp = authed(
client().get(format!("{base}/datatable_permissions/byo")),
"SECRET_TOKEN",
)
.send()
.await?;
let body: Value = resp.json().await?;
assert_eq!(body["supported"], false, "{body}");
let resp = authed(
client().post(format!("{base}/datatable_permissions/byo")),
"SECRET_TOKEN",
)
.json(&json!({"permissioned": true, "default_role": "role1",
"roles": [{"id": "role1", "tenants": ["*"]}]}))
.send()
.await?;
assert_eq!(resp.status(), 400, "{}", resp.text().await?);
Ok(())
}
#[sqlx::test(migrations = "../migrations", fixtures("base", "datatable_roles"))]
async fn a_fork_renaming_its_own_entry_leaves_the_governing_bookkeeping_alone(
db: Pool<Postgres>,
) -> anyhow::Result<()> {
initialize_tracing().await;
// The parent's migration definitions. A rename or delete through the fork's settings form
// resolves through the pointer, so without a guard it would relabel or wipe these.
sqlx::query(
"INSERT INTO datatable_migrations (workspace_id, datatable, timestamp, name, code_up)
VALUES ('test-workspace', 'main', 1, 'init', 'SELECT 1')",
)
.execute(&db)
.await?;
let server = ApiServer::start(db.clone()).await?;
let port = server.addr.port();
let resp = authed(
client().post(format!(
"http://localhost:{port}/api/w/wm-fork-dt/workspaces/edit_datatable_config"
)),
"SECRET_TOKEN_2",
)
.json(&json!({
"settings": {"datatables": {}},
"renames": [],
"deleted_datatables": ["main"]
}))
.send()
.await?;
assert_eq!(resp.status(), 200, "{}", resp.text().await?);
let left: i64 = sqlx::query_scalar(
"SELECT COUNT(*) FROM datatable_migrations WHERE workspace_id = 'test-workspace'",
)
.fetch_one(&db)
.await?;
assert_eq!(left, 1, "the fork's delete reached the parent's migrations");
Ok(())
}
@@ -42,7 +42,7 @@ use windmill_common::datatable_roles::ADMIN_DATATABLE_ROLE;
use windmill_common::worker::SqlAnnotations;
use windmill_common::workspaces::{
ensure_can_use_datatable_role, ensure_datatable_admin_access,
get_datatable_resource_from_db_unchecked, DatatableAccess,
get_datatable_resource_from_db_unchecked, resolve_governing_datatable, DatatableAccess,
};
use windmill_common::{PgDatabase, DB};
use windmill_git_sync::{
@@ -1500,6 +1500,15 @@ async fn generate_initial_datatable_migration(
Extension(db): Extension<DB>,
Path((w_id, datatable_name)): Path<(String, String)>,
) -> JsonResult<DatatableMigration> {
// Returns a `pg_dump` of the whole schema and writes into the data table's own bookkeeping, so
// it answers to the workspace that governs it rather than to whoever is asking.
ensure_datatable_admin_access(
&db,
&w_id,
&datatable_name,
&DatatableAccess::Authed(authed.to_authed_ref()),
)
.await?;
validate_datatable_path_segment(&datatable_name)?;
ensure_datatable_migrations_enabled(&db, &w_id, &datatable_name).await?;
@@ -1670,9 +1679,21 @@ pub(crate) struct DatatableRename {
pub(crate) to: String,
}
async fn resolve_datatable_pg(db: &DB, w_id: &str, datatable: &str) -> Result<PgDatabase> {
/// The database whose `_wm_migrations` a rename or delete of `datatable` in `w_id` should touch —
/// `None` when that is somebody else's.
///
/// A fork's entry points at the workspace that governs the data table, so renaming or removing it
/// changes what the fork calls the data table and nothing more. Following the pointer here would
/// let a fork admin relabel or wipe the *governing* workspace's migration bookkeeping through
/// their own settings form, and the parent would then re-run every migration from zero.
async fn resolve_datatable_pg(db: &DB, w_id: &str, datatable: &str) -> Result<Option<PgDatabase>> {
let governing = resolve_governing_datatable(db, w_id, datatable).await?;
if governing.workspace_id != w_id {
return Ok(None);
}
let db_resource = get_datatable_resource_from_db_unchecked(db, w_id, datatable).await?;
serde_json::from_value(db_resource)
.map(Some)
.map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e)))
}
@@ -1690,7 +1711,9 @@ fn ignore_missing_wm_migrations(e: tokio_postgres::Error) -> Result<()> {
/// Drop a data table's rows from its own database's `_wm_migrations`.
async fn remote_forget_datatable_migrations(db: &DB, w_id: &str, datatable: &str) -> Result<()> {
let pg_db = resolve_datatable_pg(db, w_id, datatable).await?;
let Some(pg_db) = resolve_datatable_pg(db, w_id, datatable).await? else {
return Ok(());
};
let (client, connection) = pg_db.connect(Some(db)).await?;
tokio::spawn(async move {
let _ = connection.await;
@@ -1715,7 +1738,9 @@ async fn remote_rename_datatable_migrations(
from: &str,
to: &str,
) -> Result<()> {
let pg_db = resolve_datatable_pg(db, w_id, resolve_by).await?;
let Some(pg_db) = resolve_datatable_pg(db, w_id, resolve_by).await? else {
return Ok(());
};
let (client, connection) = pg_db.connect(Some(db)).await?;
tokio::spawn(async move {
let _ = connection.await;
@@ -62,6 +62,9 @@ pub struct DatatableRoleTenantsInfo {
#[derive(Serialize)]
struct DatatablePermissionsInfo {
/// Whether this data table can be put under roles at all — only one on the instance database
/// can, since a role is a login on that cluster.
supported: bool,
/// Whether the data table is under roles at all.
permissioned: bool,
default_role: String,
@@ -205,13 +208,22 @@ async fn get_datatable_permissions(
} else {
catalog.get(id).map(|r| r.name.clone())
},
tenants: tenants.tenants.clone(),
// Tenants name users, groups and folders of the governing workspace, so they
// are for the people who set them. Someone reading from a fork gets the shape
// of the decision, not the parent's membership; what they may use themselves
// is what `datatable_usable_roles` answers.
tenants: if editable {
tenants.tenants.clone()
} else {
vec![]
},
})
.collect()
})
.unwrap_or_default();
Ok(Json(DatatablePermissionsInfo {
supported: governing.is_instance(),
permissioned: permissions.is_some(),
default_role: permissions
.map(|p| p.default_role().to_string())
@@ -220,14 +232,20 @@ async fn get_datatable_permissions(
governing_workspace_id: (governing.workspace_id != w_id)
.then(|| governing.workspace_id.clone()),
editable,
available_roles: catalog
.iter()
.map(|(id, role)| AvailableRole {
id: id.clone(),
name: role.name.clone(),
enabled: role.enabled,
})
.collect(),
// The instance's role names are only of use to someone who can pick from them, and
// enumerating them is the first step of anything that wants to name one it shouldn't.
available_roles: if editable {
catalog
.iter()
.map(|(id, role)| AvailableRole {
id: id.clone(),
name: role.name.clone(),
enabled: role.enabled,
})
.collect()
} else {
vec![]
},
ungoverned_reachers: if editable {
ungoverned_reachers(&db, &governing).await?
} else {
@@ -245,6 +263,17 @@ async fn set_datatable_permissions(
let governing = resolve_governing_datatable(&db, &w_id, &datatable_name).await?;
ensure_governs_datatable(&db, &authed, &w_id, &governing).await?;
// A data table role is a login on Windmill's own cluster; a resource-backed data table dials a
// host the workspace admin chose, so it has no business naming one.
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 = read_role_catalog(&db).await?;
let mut roles: BTreeMap<String, DataTableRoleTenants> = BTreeMap::new();
@@ -356,33 +385,54 @@ pub(crate) async fn restart_streams_reaching(
db: &DB,
governing: &GoverningDatatable,
) -> Result<()> {
let reference = format!("datatable://{}", governing.name);
let prefix = format!("{reference}?%");
sqlx::query!(
"UPDATE postgres_trigger SET server_id = NULL, last_server_ping = NULL
WHERE workspace_id = $1
AND (postgres_resource_path = $2 OR postgres_resource_path LIKE $3)",
// Every workspace holding an entry that resolves here, under the name it calls it: the
// governing one, plus each fork pointing at it. A fork's trigger names its own local entry, so
// filtering on the governing workspace alone would leave its stream running on the connection
// it already opened under the old decision — which is the one window this function exists to
// close.
let mut reached = vec![(governing.workspace_id.clone(), governing.name.clone())];
let pointers = sqlx::query!(
r#"SELECT ws.workspace_id AS "workspace_id!", dt.key AS "datatable!"
FROM workspace_settings ws
CROSS JOIN LATERAL jsonb_each(COALESCE(ws.datatable->'datatables', '{}'::jsonb)) dt
WHERE dt.value->'reference'->>'workspace_id' = $1
AND dt.value->'reference'->>'datatable' = $2"#,
&governing.workspace_id,
&reference,
&prefix,
&governing.name,
)
.execute(db)
.fetch_all(db)
.await?;
reached.extend(pointers.into_iter().map(|r| (r.workspace_id, r.datatable)));
// A capture keeps the reference inside its `trigger_config` blob rather than in a column of
// its own, and only a postgres capture has one there at all.
sqlx::query!(
"UPDATE capture_config SET server_id = NULL, last_server_ping = NULL
WHERE workspace_id = $1 AND trigger_kind = 'postgres'
AND (trigger_config->>'postgres_resource_path' = $2
OR trigger_config->>'postgres_resource_path' LIKE $3)",
&governing.workspace_id,
&reference,
&prefix,
)
.execute(db)
.await?;
for (w_id, name) in reached {
let reference = format!("datatable://{name}");
let prefix = format!("{reference}?%");
sqlx::query!(
"UPDATE postgres_trigger SET server_id = NULL, last_server_ping = NULL
WHERE workspace_id = $1
AND (postgres_resource_path = $2 OR postgres_resource_path LIKE $3)",
&w_id,
&reference,
&prefix,
)
.execute(db)
.await?;
// A capture keeps the reference inside its `trigger_config` blob rather than in a column
// of its own, and only a postgres capture has one there at all.
sqlx::query!(
"UPDATE capture_config SET server_id = NULL, last_server_ping = NULL
WHERE workspace_id = $1 AND trigger_kind = 'postgres'
AND (trigger_config->>'postgres_resource_path' = $2
OR trigger_config->>'postgres_resource_path' LIKE $3)",
&w_id,
&reference,
&prefix,
)
.execute(db)
.await?;
}
Ok(())
}
+6 -1
View File
@@ -32142,8 +32142,13 @@ components:
DatatablePermissions:
type: object
required: [permissioned, default_role, roles, editable, available_roles]
required: [supported, permissioned, default_role, roles, editable, available_roles]
properties:
supported:
type: boolean
description: >-
Whether this data table can be put under roles at all. Only one backed by the
instance database can: a role is a login on that cluster.
permissioned:
type: boolean
default_role:
+3 -2
View File
@@ -8145,8 +8145,9 @@ pub async fn run_wait_result_flow_by_version(
/// job lives, in particular DuckDB, which runs in-process in the worker.
///
/// What it does permit is any statement against the workspace's data tables, writes and DDL
/// included: the helper's body is an unrestricted SQL template and data tables carry no
/// per-user ACL. Narrowing that is a separate decision from this exemption.
/// included: the helper's body is an unrestricted SQL template. What that reaches is the
/// operator's own data table role — the preview job is permissioned as them, so the executor
/// resolves it under their tenancy like any other job.
///
/// The database argument is only half the target: the executor honors a `-- database`
/// directive in the SQL over it, and `-- s3` redirects the result set, so both are refused.
+40 -3
View File
@@ -1371,6 +1371,17 @@ pub struct GoverningDatatable {
pub datatable: DataTable,
}
impl GoverningDatatable {
/// Backed by the Windmill instance's own Postgres, which is the only substrate data table
/// roles apply to.
pub fn is_instance(&self) -> bool {
self.datatable
.database
.as_ref()
.is_some_and(|d| d.resource_type == DataTableCatalogResourceType::Instance)
}
}
pub async fn resolve_governing_datatable(
db: &DB,
w_id: &str,
@@ -1635,6 +1646,21 @@ pub async fn get_datatable_resource_from_db(
let governing = resolve_governing_datatable(db, w_id, name).await?;
let mut db_resource = resolve_datatable_connection_unchecked(db, &governing, false).await?;
// A data table role is a login on Windmill's own cluster, so it is only meaningful against an
// instance database. Substituting its password into a resource-backed connection would hand a
// real cluster credential to whatever host that resource names — which a workspace admin
// chooses. Refused rather than ignored: an entry that reached this state was never a shape the
// permissions endpoint accepts, so silently resolving it as admin would hide a broken record.
if !governing.is_instance() {
return match (governing.datatable.permissions.as_ref(), role) {
(None, None | Some(ADMIN_DATATABLE_ROLE)) => Ok(db_resource),
_ => Err(Error::BadRequest(format!(
"Data table '{name}' is backed by a Postgres resource, which cannot be put under \
data table roles"
))),
};
}
let catalog = crate::datatable_roles::read_role_catalog(db).await?;
let Some((role_id, tenants)) = datatable_role_entry(
governing.datatable.permissions.as_ref(),
@@ -1712,6 +1738,9 @@ pub async fn ensure_can_use_datatable_role(
context: &str,
) -> Result<()> {
let governing = resolve_governing_datatable(db, w_id, name).await?;
if !governing.is_instance() {
return Ok(());
}
let catalog = crate::datatable_roles::read_role_catalog(db).await?;
let Some((role_id, tenants)) =
datatable_role_entry(governing.datatable.permissions.as_ref(), &catalog, name, role)?
@@ -1748,6 +1777,9 @@ pub async fn ensure_datatable_admin_access(
access: &DatatableAccess<'_>,
) -> Result<()> {
let governing = resolve_governing_datatable(db, w_id, name).await?;
if !governing.is_instance() {
return Ok(());
}
let Some(permissions) = governing.datatable.permissions.as_ref() else {
return Ok(());
};
@@ -1865,14 +1897,19 @@ pub async fn rename_datatable_tenant_in_workspace(
update_datatable_permissions_in_workspace(tx, w_id, |permissions| {
let mut touched = false;
for role in permissions.roles.values_mut() {
let mut role_touched = false;
for tenant in role.tenants.iter_mut() {
if tenant == old {
*tenant = new.to_string();
touched = true;
role_touched = true;
}
}
if touched {
role.tenants.dedup();
if role_touched {
// The rename can collide with a name already in the list, and the two need not be
// adjacent — `Vec::dedup` only collapses neighbours, so it would leave the pair.
let mut seen = std::collections::HashSet::new();
role.tenants.retain(|t| seen.insert(t.clone()));
touched = true;
}
}
touched
@@ -151,7 +151,13 @@
<Alert type="error" title="Could not load roles" size="xs">{loadError}</Alert>
{:else}
<div class="flex flex-col gap-4">
{#if governing}
{#if !info?.supported}
<Alert type="info" title="Not available on this data table" size="xs">
A data table role is a Postgres login on the Windmill instance's own database, so only a
data table backed by that database can use one. This one is backed by a PostgreSQL
resource — grant access on that server directly.
</Alert>
{:else if governing}
<Alert type="info" title="Governed by {governing}" size="xs">
This data table points at the one in workspace <span class="font-mono">{governing}</span
>, so its roles are decided there. You are evaluated as a member of that workspace.
@@ -176,7 +182,7 @@
<Toggle
bind:checked={permissioned}
disabled={!editable}
disabled={!editable || !info?.supported}
options={{
right: 'Put this data table under roles',
rightTooltip: