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

fix(datatables): check what a change reaches, not only what it names

Browse Source 
Owning the target is not owning what a change through it covers: a scope that
reads IN SCHEMA names every object in the schema, and handing a schema over
takes them all with it. Postgres would have skipped the ones the caller does
not own — these statements run as the data table's admin, so it will not.
Refuse, naming the object that is not theirs, and let a workspace admin
through as before.

A default-privilege rule speaks for the role that creates the objects, so a
non-admin now only writes them for the roles they may run as.

Also: the revoke button follows can_manage like the grant builder already did,
the copy path resolves a data table as admin for an admin (dumping as a
restricted role silently omits what it cannot read), and two doc comments now
sit on the function they describe.
This commit is contained in:
Diego Imbert
2026-09-01 23:00:51 +02:00
parent 8a949471d0
commit 6768e2a9e9
5 changed files with 214 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/ee-repo-ref.txt
+1 -1
View File
@@ -1 +1 @@
67c80ba9088de8cec92a3a528b637648f9b9b3a0
9a61895fa24f1e5a13ee174353ccbe2ad76a24f5
backend/windmill-api-workspaces/src/datatable_acl.rs
+157 -30
View File
@@ -294,6 +294,123 @@ async fn connect_as_caller(
Ok((client, CallerConnection { dbname, current_user }))
}
/// The first object a change reaches that the connection's role does not own, if
/// any — the reason a caller may be refused even on a target they do own.
///
/// The scopes that read `IN SCHEMA` cover the whole schema whoever owns what is
/// in it, and `SET OWNER` on a schema moves every object in it; a revoke may
/// also name objects one at a time. `Target` scope on a table or a schema is the
/// target itself, which [`can_manage_target`] has already answered.
async fn first_unmanageable_object(
client: &tokio_postgres::Client,
target: &AclTarget,
change: &AclChange,
) -> Result<Option<String>> {
let Some(schema) = target.schema() else {
return Ok(None);
};
// A revoke that names its objects reaches exactly those.
if let AclChange::Revoke { objects, .. } = change {
if !objects.is_empty() {
let named: Vec<String> = objects
.iter()
.map(|o| match &o.args {
Some(args) => format!("{}({args})", o.name),
None => o.name.clone(),
})
.collect();
let row = client
.query_opt(
"SELECT name FROM (
SELECT c.relname AS name, pg_has_role(c.relowner, 'USAGE') AS mine
FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE n.nspname = $1 AND c.relname = ANY($2)
UNION ALL
SELECT p.proname || '(' || pg_get_function_identity_arguments(p.oid) || ')',
pg_has_role(p.proowner, 'USAGE')
FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace
WHERE n.nspname = $1
AND p.proname || '(' || pg_get_function_identity_arguments(p.oid) || ')'
= ANY($2)
) o WHERE NOT o.mine ORDER BY name LIMIT 1",
&[&schema, &named],
)
.await
.map_err(|e| {
Error::internal_err(format!(
"Failed to read the owners of schema '{schema}': {}",
pg_error_message(&e)
))
})?;
return Ok(row.map(|row| format!("{schema}.{}", row.get::<_, String>(0))));
}
}
// The object classes the change touches, as `relkind`s and whether routines
// are included — Postgres groups views and foreign tables under TABLES.
let (relkinds, routines): (&[&str], bool) = match change {
AclChange::SetOwner { .. } if matches!(target, AclTarget::Schema { .. }) => {
(&["r", "p", "v", "m", "S", "f"], true)
}
AclChange::SetOwner { .. } => return Ok(None),
AclChange::Grant { scope, .. } | AclChange::Revoke { scope, .. } => match scope {
GrantScope::AllTables => (&["r", "p", "v", "f"], false),
GrantScope::AllSequences => (&["S"], false),
GrantScope::AllFunctions => (&[], true),
// A future grant creates no statement about an existing object.
GrantScope::FutureTables
| GrantScope::FutureSequences
| GrantScope::FutureFunctions
| GrantScope::Target => return Ok(None),
},
};
let relkinds: Vec<String> = relkinds.iter().map(|k| k.to_string()).collect();
let row = client
.query_opt(
"SELECT name FROM (
SELECT c.relname AS name, pg_has_role(c.relowner, 'USAGE') AS mine
FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE n.nspname = $1 AND c.relkind::text = ANY($2)
UNION ALL
SELECT p.proname || '(' || pg_get_function_identity_arguments(p.oid) || ')',
pg_has_role(p.proowner, 'USAGE')
FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace
WHERE $3 AND n.nspname = $1
) o WHERE NOT o.mine ORDER BY name LIMIT 1",
&[&schema, &relkinds, &routines],
)
.await
.map_err(|e| {
Error::internal_err(format!(
"Failed to read the owners of schema '{schema}': {}",
pg_error_message(&e)
))
})?;
Ok(row.map(|row| format!("{schema}.{}", row.get::<_, String>(0))))
}
/// The data table roles `authed` may themselves run as.
async fn usable_role_names(
db: &DB,
w_id: &str,
datatable_name: &str,
authed: &ApiAuthed,
) -> Result<Vec<String>> {
let authed_ref = authed.to_authed_ref();
let datatable = read_datatable(db, w_id, datatable_name).await?;
Ok(match datatable.permissions.filter(|p| p.enabled) {
Some(p) => p
.roles
.iter()
.filter(|(_, role)| can_use_datatable_role(role, &authed_ref))
.map(|(name, _)| name.clone())
.collect(),
None => vec![ADMIN_DATATABLE_ROLE.to_string()],
})
}
/// Whether the connection's role is a member of the target's owner, which is
/// what "may change its access" means here.
async fn can_manage_target(client: &tokio_postgres::Client, target: &AclTarget) -> Result<bool> {
@@ -632,17 +749,7 @@ async fn get_datatable_acl(
// Every role is named, since the list is not what is private — what each of
// them may reach is.
let authed_ref = authed.to_authed_ref();
let datatable = read_datatable(&db, &w_id, &datatable_name).await?;
let usable_roles: Vec<String> = match datatable.permissions.filter(|p| p.enabled) {
Some(p) => p
.roles
.iter()
.filter(|(_, role)| can_use_datatable_role(role, &authed_ref))
.map(|(name, _)| name.clone())
.collect(),
None => vec![ADMIN_DATATABLE_ROLE.to_string()],
};
let usable_roles = usable_role_names(&db, &w_id, &datatable_name, &authed).await?;
Ok(Json(DatatableAclInfo {
owner: windmill_role_of(&roles, &owner),
@@ -813,15 +920,38 @@ async fn build_acl_plan(
crate::datatable_permissions::require_datatable_permissions_license().await?;
let (client, conn) =
connect_as_caller(db, authed, w_id, datatable_name, req.role.as_deref()).await?;
// The objects a revoke names come from the request; the ones it is checked
// and planned against come from the catalog.
let change = match &req.change {
AclChange::Revoke { role, privileges, scope, objects } => AclChange::Revoke {
role: role.clone(),
privileges: privileges.clone(),
scope: *scope,
objects: resolve_acl_objects(&client, &req.target, objects).await?,
},
change => change.clone(),
};
// What the caller's own role may change. Postgres cannot enforce the rule we
// want on its own — handing an object to a role you are not a member of is
// refused outright, and granting on one you own needs the grant option — so
// this is the check, and the statements run as the data table's admin below.
if !authed.is_admin && !can_manage_target(&client, &req.target).await? {
return Err(Error::NotAuthorized(format!(
"{} is owned by a role you are not a member of",
req.target.label(&conn.dbname)
)));
if !authed.is_admin {
if !can_manage_target(&client, &req.target).await? {
return Err(Error::NotAuthorized(format!(
"{} is owned by a role you are not a member of",
req.target.label(&conn.dbname)
)));
}
// Owning the target is not owning what a change through it reaches: a
// schema-wide scope names every object in the schema, and handing a
// schema over takes them all with it. Postgres would have skipped the
// ones the caller does not own; these statements run as admin, so it
// will not.
if let Some(unreachable) = first_unmanageable_object(&client, &req.target, &change).await? {
return Err(Error::NotAuthorized(format!(
"This change also covers {unreachable}, which is owned by a role you are not a member of"
)));
}
}
let roles = role_map(
db,
@@ -835,10 +965,18 @@ async fn build_acl_plan(
AclChange::Grant { role, .. } | AclChange::Revoke { role, .. } => role,
};
let pg_role = pg_role_of(&roles, role_name)?;
// The roles a plan may also write about: what a schema's new owner is kept
// in reach of, and whose future objects a `created later` grant covers. Both
// are `ALTER DEFAULT PRIVILEGES FOR ROLE <other>`, which speaks for that role
// — so a non-admin only gets the ones they may run as.
let usable = usable_role_names(db, w_id, datatable_name, authed).await?;
let other_pg_roles: Vec<String> = roles
.values()
.filter(|pg| pg.as_str() != pg_role.as_str())
.cloned()
.iter()
.filter(|(name, pg)| {
pg.as_str() != pg_role.as_str()
&& (authed.is_admin || usable.iter().any(|u| u == name.as_str()))
})
.map(|(_, pg)| pg.clone())
.collect();
let existing_objects = match (&req.change, &req.target) {
(AclChange::SetOwner { .. }, AclTarget::Schema { schema }) => {
@@ -846,17 +984,6 @@ async fn build_acl_plan(
}
_ => vec![],
};
// The objects a revoke names come from the request; the ones it plans with
// come from the catalog.
let change = match &req.change {
AclChange::Revoke { role, privileges, scope, objects } => AclChange::Revoke {
role: role.clone(),
privileges: privileges.clone(),
scope: *scope,
objects: resolve_acl_objects(&client, &req.target, objects).await?,
},
change => change.clone(),
};
let plan = crate::datatable_acl_oss::plan_statements(
&req.target,
&change,
backend/windmill-api-workspaces/src/workspaces.rs
+42 -3
View File
@@ -2987,9 +2987,48 @@ pub(crate) async fn resolve_pg_source_checked(
authed: &ApiAuthed,
w_id: &str,
source: &str,
) -> Result<PgDatabase> {
resolve_pg_source_as(db, user_db, authed, w_id, source, false).await
}
/// As [`resolve_pg_source_checked`], but a workspace admin reaches a data table
/// as `admin` rather than as the role it defaults to.
///
/// For the paths that copy a whole database: dumping as a restricted default
/// role silently leaves out every table that role cannot read, which is a
/// truncated copy rather than an error. A non-admin still resolves as their own
/// role — `admin` is not theirs to ask for — so this hands out nothing.
pub(crate) async fn resolve_pg_source_for_copy(
db: &DB,
user_db: &UserDB,
authed: &ApiAuthed,
w_id: &str,
source: &str,
) -> Result<PgDatabase> {
resolve_pg_source_as(db, user_db, authed, w_id, source, authed.is_admin).await
}
async fn resolve_pg_source_as(
db: &DB,
user_db: &UserDB,
authed: &ApiAuthed,
w_id: &str,
source: &str,
as_admin: bool,
) -> Result<PgDatabase> {
let db_resource = if let Some(name) = source.strip_prefix("datatable://") {
get_datatable_resource_as_default_role(db, authed, w_id, name).await?
if as_admin {
get_datatable_resource_from_db(
db,
w_id,
name,
Some(ADMIN_DATATABLE_ROLE),
DatatableAccess::Authed(authed.to_authed_ref()),
)
.await?
} else {
get_datatable_resource_as_default_role(db, authed, w_id, name).await?
}
} else if let Some(path) = source.strip_prefix("$res:") {
let db_with_authed = windmill_common::db::DbWithOptAuthed::from_authed(
authed,
@@ -3466,9 +3505,9 @@ async fn import_pg_database(
}
let schema_only = req.fork_behavior == DataTableForkBehavior::SchemaOnly;
let source_pg = resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.source).await?;
let source_pg = resolve_pg_source_for_copy(&db, &user_db, &authed, &w_id, &req.source).await?;
let mut target_pg =
resolve_pg_source_checked(&db, &user_db, &authed, &w_id, &req.target).await?;
resolve_pg_source_for_copy(&db, &user_db, &authed, &w_id, &req.target).await?;
if let Some(ref override_dbname) = req.target_dbname_override {
if !windmill_api_auth::is_super_admin_authed(&db, &authed).await? {
backend/windmill-common/src/workspaces.rs
+11 -10
View File
@@ -1297,10 +1297,11 @@ pub async fn get_datatable_resource_from_db_unchecked(
.await
}
/// Resolve a data table's connection credentials as `role` (default `admin`),
/// checking that `access` is allowed to use it when the data table is
/// permissioned. On an unpermissioned data table only `admin` is accepted, and
/// the resolution is the same as the unchecked one.
/// Resolve a data table's connection credentials as `role`, checking that
/// `access` is allowed to use it when the data table is permissioned. Absent
/// means the data table's configured default role, which is `admin` only until a
/// workspace names another one. On an unpermissioned data table only `admin` is
/// accepted, and the resolution is the same as the unchecked one.
pub async fn get_datatable_resource_from_db(
db: &DB,
w_id: &str,
@@ -1336,12 +1337,6 @@ pub async fn get_datatable_replication_resource_from_db_unchecked(
.await
}
/// Resolve which postgres login the data table should be reached through, and
/// authorize it.
///
/// Returns the `(user, password)` to swap into the connection, or `None` when
/// the data table's own credentials are to be used — which is every
/// unpermissioned data table, and the `admin` role of a permissioned one.
/// Look up the role a resolution asks for, without authorizing it.
///
/// `Ok(None)` means the data table is unpermissioned and resolves through its own
@@ -1379,6 +1374,12 @@ fn datatable_role_entry<'a>(
Ok(Some((role_name.as_str(), role_entry)))
}
/// Resolve which postgres login the data table should be reached through, and
/// authorize it.
///
/// Returns the `(user, password)` to swap into the connection, or `None` when
/// the data table's own credentials are to be used — which is every
/// unpermissioned data table, and the `admin` role of a permissioned one.
async fn resolve_datatable_role(
db: &DB,
w_id: &str,
frontend/src/lib/components/datatableAcl/PgAclEditor.svelte
+3 -1
View File
@@ -191,7 +191,9 @@
<Cell>{grantScopeLabel(grant)}</Cell>
<Cell last>
{@const revokeScope = revokeScopeOf(grant)}
{#if info.roles.includes(grant.grantee) && revokeScope}
<!-- Reading a target's access needs no ownership of it, so a row
on one the caller does not own has nothing to offer. -->
{#if info.roles.includes(grant.grantee) && revokeScope && info.can_manage}
<Button
unifiedSize="xs"
variant="default"