fix(datatables): the database a permissioned data table points at cannot move

The path in the config staying the same said nothing: a postgres resource is
editable in place, so its host, database or user could change underneath roles
whose logins and grants live in the database it used to name. The identity a
connection resolves to is what has to hold still while those roles exist; a
password rotation is not an identity change and stays allowed.

Also generalizes the admin guard: what 'admin' holds is what every role here
connects through, on the database and on schema public alike, so a revoke
naming it is refused wherever it is aimed.
This commit is contained in:
Diego Imbert
2026-09-01 23:37:05 +02:00
parent 98adfd247b
commit 68e7f72560
5 changed files with 115 additions and 13 deletions
+1 -1
View File
@@ -1 +1 @@
2f0450cf3d5c79575bbb47f1a2dd7564e42a8b69
923564cebc131396ea47d9af76d02d15bd61ca43
@@ -980,16 +980,16 @@ async fn build_acl_plan(
AclChange::Grant { role, .. } | AclChange::Revoke { role, .. } => role,
};
let pg_role = pg_role_of(&roles, role_name)?;
// `admin` is the login the data table itself reaches Postgres through, so a
// revoke on the database would take away what every role here connects with
// — including the one that would have to grant it back.
if matches!(req.change, AclChange::Revoke { .. })
&& matches!(req.target, AclTarget::Database)
&& role_name == ADMIN_DATATABLE_ROLE
{
// `admin` is the login the data table itself reaches Postgres through — the
// one migrations, schema browsing and every other role's creation run as. Its
// own access is not the drawer's to take away, on any target: the grants an
// instance database is provisioned with cover the database and schema
// `public` alike, and a revoke that lands leaves nothing able to grant it
// back.
if matches!(req.change, AclChange::Revoke { .. }) && role_name == ADMIN_DATATABLE_ROLE {
return Err(Error::BadRequest(format!(
"'{ADMIN_DATATABLE_ROLE}' is how this data table reaches its database; \
revoking on the database itself would lock every role out of it"
its own access is not revocable from here"
)));
}
// The roles a plan may also write about: what a schema's new owner is kept
+60
View File
@@ -1111,6 +1111,66 @@ pub fn redact_datatable_settings_for_export(
Some(datatable)
}
/// Refuse a resource edit that would move a permissioned data table onto another
/// database.
///
/// The roles of such a data table live in the database it points at: their logins
/// were created there and every grant they hold is recorded there. The path in
/// the config staying the same says nothing — the resource behind it can be
/// edited — so the identity the connection resolves to is what has to hold
/// still. A password rotation is not an identity change and stays allowed.
pub async fn ensure_resource_identity_change_allowed(
db: &DB,
w_id: &str,
path: &str,
old_value: Option<&serde_json::Value>,
new_value: Option<&serde_json::Value>,
) -> Result<()> {
let (Some(old_value), Some(new_value)) = (old_value, new_value) else {
return Ok(());
};
let identity = |v: &serde_json::Value| {
(
v.get("host").cloned(),
v.get("port").cloned(),
v.get("dbname").cloned(),
v.get("user").cloned(),
)
};
if identity(old_value) == identity(new_value) {
return Ok(());
}
let datatables: std::collections::HashMap<String, DataTable> = sqlx::query_scalar!(
"SELECT ws.datatable->'datatables' FROM workspace_settings ws WHERE ws.workspace_id = $1",
w_id
)
.fetch_optional(db)
.await?
.flatten()
.and_then(|v| serde_json::from_value(v).ok())
.unwrap_or_default();
let permissioned: Vec<&str> = datatables
.iter()
.filter(|(_, dt)| {
dt.database.resource_type == DataTableCatalogResourceType::Postgresql
&& dt.database.resource_path == path
&& dt.permissions.as_ref().is_some_and(|p| p.enabled)
})
.map(|(name, _)| name.as_str())
.collect();
if permissioned.is_empty() {
return Ok(());
}
Err(Error::BadRequest(format!(
"This resource backs data table {} with permissions enabled, so the database it points \
at cannot be changed: disable them first, which drops the roles from the database they \
were created in.",
permissioned.join(", ")
)))
}
/// The data table settings as one audit parameter, which is stored and traced
/// in the clear — so it goes through the same redaction as any other export.
pub fn datatable_settings_for_audit(settings: &impl Serialize) -> String {
+42
View File
@@ -1811,6 +1811,29 @@ async fn update_resource(
return Err(Error::PermissionDenied(msg));
}
// Same as `set_resource_value`: the identity a permissioned data table's
// roles were created against is not free to move underneath them.
if let Some(nvalue) = ns.value.as_ref() {
let previous = sqlx::query_scalar!(
"SELECT value FROM resource WHERE path = $1 AND workspace_id = $2",
path,
&w_id
)
.fetch_optional(&db)
.await?
.flatten();
let nvalue: serde_json::Value = serde_json::from_str(nvalue.get())
.map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?;
windmill_common::workspaces::ensure_resource_identity_change_allowed(
&db,
&w_id,
path,
previous.as_ref(),
Some(&nvalue),
)
.await?;
}
let mut sqlb = SqlBuilder::update_table("resource");
sqlb.and_where_eq("path", "?".bind(&path));
sqlb.and_where_eq("workspace_id", "?".bind(&w_id));
@@ -2123,6 +2146,25 @@ async fn set_resource_value(
}
authorize_azure_devops_reference(authed, db, user_db, w_id, value.as_ref()).await?;
// A data table's roles live in the database its resource points at, so the
// identity behind that path is not free to move while they exist.
let previous = sqlx::query_scalar!(
"SELECT value FROM resource WHERE path = $1 AND workspace_id = $2",
path,
w_id
)
.fetch_optional(db)
.await?
.flatten();
windmill_common::workspaces::ensure_resource_identity_change_allowed(
db,
w_id,
path,
previous.as_ref(),
value.as_ref(),
)
.await?;
let mut tx = user_db.clone().begin(authed).await?;
// `RETURNING resource_type` rather than a second lookup: the advisory below has to know the
@@ -193,10 +193,10 @@
<Cell last>
{@const revokeScope = revokeScopeOf(grant)}
<!-- Reading a target's access needs no ownership of it, so a row
on one the caller does not own has nothing to offer. And the
database's own grants to `admin` are what every role here
connects with, so they are not the drawer's to take away. -->
{#if info.roles.includes(grant.grantee) && revokeScope && info.can_manage && !(target.kind === 'database' && grant.grantee === ADMIN_DATATABLE_ROLE)}
on one the caller does not own has nothing to offer. And what
`admin` holds is what every role here connects through, so it
is not the drawer's to take away wherever it appears. -->
{#if info.roles.includes(grant.grantee) && revokeScope && info.can_manage && grant.grantee !== ADMIN_DATATABLE_ROLE}
<Button
unifiedSize="xs"
variant="default"