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

fix: read effective ACLs and keep future grants for roles added later

Browse Source 
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DsU2Lf6wYQJ9o8ASKRgCmK
This commit is contained in:
Diego Imbert
2026-09-10 17:30:48 +02:00
co-authored by Claude Opus 5
parent c45f3bc47f
commit da2aaa0a7f
9 changed files with 267 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/ee-repo-ref.txt
+1 -1
View File
@@ -1 +1 @@
c88cd64f5f70e8704c73699bbffd29c8cbdb7c68
09b3b12b6633445593e5846b5b61d6ae4ef7f88d
backend/windmill-api-settings/src/lib.rs
+23
View File
@@ -2635,6 +2635,7 @@ async fn create_datatable_role(
catalog.insert(id.clone(), entry);
tx.commit().await?;
converge_connect_grants_everywhere(&db, &catalog).await;
copy_default_privileges_everywhere(&db, &req.name).await;
windmill_common::feature_usage::log_feature_usage("datatable", "role_created", "");
audit_log(
@@ -2749,6 +2750,28 @@ async fn delete_datatable_role(
Ok(Json(()))
}
/// Best-effort, like the `CONNECT` convergence below: a database this misses leaves the new role's
/// objects there granting nobody anything, which re-applying the "created later" grant in the ACL
/// editor repairs.
async fn copy_default_privileges_everywhere(db: &DB, name: &str) {
let dbnames = match windmill_common::datatable_roles::registered_instance_databases(db).await {
Ok(dbnames) => dbnames,
Err(e) => {
tracing::warn!("Could not list instance databases to copy default privileges: {e}");
return;
}
};
for dbname in dbnames {
if let Err(e) =
windmill_common::datatable_roles::copy_default_privileges_to(db, &dbname, name).await
{
tracing::warn!(
"Could not copy default privileges to data table role '{name}' on '{dbname}': {e}"
);
}
}
}
/// Best-effort `CONNECT` convergence over the instance database registry. A database that is
/// unreachable right now is repaired the next time one of its data tables is administered, so a
/// role creation is not held hostage by an unrelated database being down.
backend/windmill-api-workspaces/src/datatable_acl.rs
+53 -9
View File
@@ -420,7 +420,8 @@ pub(crate) fn object_keyword(kind: &str) -> Result<&'static str> {
match kind.to_uppercase().as_str() {
"TABLE" | "VIEW" | "MATERIALIZED VIEW" | "FOREIGN TABLE" => Ok("TABLE"),
"SEQUENCE" => Ok("SEQUENCE"),
"FUNCTION" => Ok("FUNCTION"),
// `FUNCTION` names no procedure; `ROUTINE` names either.
"FUNCTION" | "PROCEDURE" | "ROUTINE" => Ok("ROUTINE"),
other => Err(Error::BadRequest(format!("Unknown object kind '{other}'"))),
}
}
@@ -436,7 +437,8 @@ async fn read_schema_objects(
FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE n.nspname = $1 AND c.relkind = ANY(ARRAY['r','p','v','m','S','f']::\"char\"[])
UNION ALL
SELECT 'FUNCTION', p.proname, pg_get_function_identity_arguments(p.oid)
SELECT CASE p.prokind WHEN 'p' THEN 'PROCEDURE' ELSE 'FUNCTION' END, p.proname,
pg_get_function_identity_arguments(p.oid)
FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace
WHERE n.nspname = $1",
&[&schema],
@@ -624,13 +626,14 @@ async fn get_datatable_acl(
async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Result<Vec<AclGrant>> {
// `aclexplode` turns an acl array into one row per (grantee, privilege); grantee 0 is PUBLIC,
// which has no name to resolve.
// which has no name to resolve. A NULL acl is not "no access" but Postgres's built-in default —
// the owner holds everything and, on a routine, PUBLIC may EXECUTE — hence `acldefault`.
let mut rows = match target {
AclTarget::Database => client
.query(
"SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END,
a.privilege_type, NULL::text, NULL::text, NULL::text, NULL::text
FROM pg_database d, aclexplode(d.datacl) a
FROM pg_database d, aclexplode(COALESCE(d.datacl, acldefault('d', d.datdba))) a
WHERE d.datname = current_database()",
&[],
)
@@ -641,7 +644,7 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res
.query(
"SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END,
a.privilege_type, NULL::text, NULL::text, NULL::text, NULL::text
FROM pg_namespace n, aclexplode(n.nspacl) a
FROM pg_namespace n, aclexplode(COALESCE(n.nspacl, acldefault('n', n.nspowner))) a
WHERE n.nspname = $1",
&[schema],
)
@@ -656,7 +659,8 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res
NULL::text
FROM pg_class c
JOIN pg_namespace n ON n.oid = c.relnamespace,
aclexplode(c.relacl) a
aclexplode(COALESCE(c.relacl, acldefault(
CASE c.relkind WHEN 'S' THEN 's' ELSE 'r' END::\"char\", c.relowner))) a
WHERE n.nspname = $1",
&[schema],
)
@@ -669,11 +673,12 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res
// Routines carry their own acl in `pg_proc`; without this a grant made here
// would vanish on the next read and could never be revoked back.
"SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END,
a.privilege_type, p.proname, NULL::text, 'FUNCTION',
a.privilege_type, p.proname, NULL::text,
CASE p.prokind WHEN 'p' THEN 'PROCEDURE' ELSE 'FUNCTION' END,
pg_get_function_identity_arguments(p.oid)
FROM pg_proc p
JOIN pg_namespace n ON n.oid = p.pronamespace,
aclexplode(p.proacl) a
aclexplode(COALESCE(p.proacl, acldefault('f', p.proowner))) a
WHERE n.nspname = $1",
&[schema],
)
@@ -705,7 +710,8 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res
a.privilege_type, NULL::text, NULL::text, NULL::text, NULL::text
FROM pg_class c
JOIN pg_namespace n ON n.oid = c.relnamespace,
aclexplode(c.relacl) a
aclexplode(COALESCE(c.relacl, acldefault(
CASE c.relkind WHEN 'S' THEN 's' ELSE 'r' END::\"char\", c.relowner))) a
WHERE n.nspname = $1 AND c.relname = $2",
&[schema, table],
)
@@ -1029,6 +1035,44 @@ async fn apply_datatable_acl(
}
}
// A schema's objects were listed before the transaction opened; one created since would stay
// with its old owner while the schema changes hands.
if let (AclChange::SetOwner { role }, AclTarget::Schema { schema }) = (&req.change, &req.target)
{
let new_owner = pg_role_of(role, &catalog)?;
let straggler = pg_tx
.query_opt(
"SELECT name FROM (
SELECT c.relname::text AS name, c.relowner AS owner
FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE n.nspname = $1
AND c.relkind = ANY(ARRAY['r','p','v','m','S','f']::\"char\"[])
UNION ALL
SELECT p.proname || '(' || pg_get_function_identity_arguments(p.oid) || ')',
p.proowner
FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace
WHERE n.nspname = $1
) o
WHERE owner <> (SELECT oid FROM pg_roles WHERE rolname = $2)
ORDER BY name LIMIT 1",
&[schema, &new_owner],
)
.await
.map_err(|e| {
Error::internal_err(format!(
"Failed to check what the schema holds: {}",
pg_error_message(&e)
))
})?;
if let Some(row) = straggler {
return Err(Error::BadRequest(format!(
"{schema}.{} appeared while this ran and would keep its old owner, so nothing was \
applied. Plan it again.",
row.get::<_, String>(0)
)));
}
}
let target_label = req.target.label(&dbname);
audit_log(
&mut *tx,
backend/windmill-common/src/datatable_roles.rs
+119
View File
@@ -453,6 +453,95 @@ pub async fn drop_instance_role(
Ok(())
}
/// `ALTER DEFAULT PRIVILEGES` binds only what its own creating role makes, so a role added after a
/// "created later" grant would create objects that grant nobody anything. The ACL editor writes
/// every such grant for `custom_instance_user` as well, which makes its default privileges the
/// record of that intent: a new role is given the same ones in `dbname`.
///
/// Authorization: rewrites default privileges with the server's own credentials and checks
/// nothing. Callers MUST restrict this to superadmin paths.
pub async fn copy_default_privileges_to(db: &DB, dbname: &str, name: &str) -> Result<()> {
validate_role_name(name)?;
crate::validate_dbname(dbname)?;
let base = crate::PgDatabase::parse_uri(&crate::get_database_url().await?.as_str().await)?;
let creds = crate::PgDatabase { dbname: dbname.to_string(), ..base };
let (client, connection) = creds.connect(Some(db)).await?;
let join_handle = tokio::spawn(async move { connection.await });
let result = async {
let rows = client
.query(
"SELECT n.nspname::text, d.defaclobjtype::text,
CASE WHEN a.grantee = 0 THEN NULL ELSE pg_get_userbyid(a.grantee)::text END,
a.privilege_type
FROM pg_default_acl d JOIN pg_namespace n ON n.oid = d.defaclnamespace,
aclexplode(d.defaclacl) a
WHERE d.defaclrole = (SELECT oid FROM pg_roles WHERE rolname = $1)",
&[&CUSTOM_INSTANCE_USER],
)
.await?;
let entries: Vec<(String, String, Option<String>, String)> = rows
.iter()
.map(|row| (row.get(0), row.get(1), row.get(2), row.get(3)))
.collect();
for statement in default_privilege_copies(name, &entries) {
client.batch_execute(&statement).await?;
}
Ok::<(), tokio_postgres::Error>(())
}
.await;
drop(client);
crate::shutdown_pg_connection(join_handle).await?;
result.map_err(|e| {
Error::internal_err(format!(
"Copying default privileges to role '{name}' in '{dbname}': {}",
crate::error::pg_error_message(&e)
))
})
}
/// The statements giving `role` the default privileges listed — (schema, `defaclobjtype`, grantee,
/// privilege), grantee `None` for PUBLIC — one per schema, kind of object and grantee. Privileges
/// are Postgres's own keywords, read back from its catalog.
fn default_privilege_copies(
role: &str,
entries: &[(String, String, Option<String>, String)],
) -> Vec<String> {
let mut grouped: BTreeMap<(&str, &str, Option<&str>), Vec<&str>> = BTreeMap::new();
for (schema, objtype, grantee, privilege) in entries {
let plural = match objtype.as_str() {
"r" => "TABLES",
"S" => "SEQUENCES",
"f" => "FUNCTIONS",
"T" => "TYPES",
_ => continue,
};
if grantee.as_deref() == Some(role) {
continue;
}
grouped
.entry((schema.as_str(), plural, grantee.as_deref()))
.or_default()
.push(privilege.as_str());
}
grouped
.into_iter()
.map(|((schema, plural, grantee), mut privileges)| {
privileges.sort();
privileges.dedup();
format!(
"ALTER DEFAULT PRIVILEGES FOR ROLE {} IN SCHEMA {} GRANT {} ON {} TO {}",
quote_ident(role),
quote_ident(schema),
privileges.join(", "),
plural,
grantee
.map(quote_ident)
.unwrap_or_else(|| "PUBLIC".to_string())
)
})
.collect()
}
#[cfg(test)]
mod tests {
use super::*;
@@ -489,4 +578,34 @@ mod tests {
catalog.get_mut("id1").unwrap().enabled = true;
assert_eq!(role_id_by_name(&catalog, "analytics").unwrap(), "id1");
}
#[test]
fn a_new_role_is_given_the_admin_connections_default_privileges() {
let entry = |schema: &str, objtype: &str, grantee: Option<&str>, privilege: &str| {
(
schema.to_string(),
objtype.to_string(),
grantee.map(str::to_string),
privilege.to_string(),
)
};
let statements = default_privilege_copies(
"late",
&[
entry("public", "r", Some("analytics"), "SELECT"),
entry("public", "r", Some("analytics"), "INSERT"),
entry("public", "f", None, "EXECUTE"),
// A grant to itself says nothing, and `n` has no per-schema form.
entry("public", "S", Some("late"), "USAGE"),
entry("public", "n", Some("analytics"), "USAGE"),
],
);
assert_eq!(
statements,
[
r#"ALTER DEFAULT PRIVILEGES FOR ROLE "late" IN SCHEMA "public" GRANT EXECUTE ON FUNCTIONS TO PUBLIC"#,
r#"ALTER DEFAULT PRIVILEGES FOR ROLE "late" IN SCHEMA "public" GRANT INSERT, SELECT ON TABLES TO "analytics""#,
]
);
}
}
frontend/src/lib/components/datatableAcl/AclTargetPicker.svelte
+8 -36
View File
@@ -1,52 +1,26 @@
<script lang="ts">
import { WorkspaceService } from '$lib/gen'
import { resource } from 'runed'
import Select from '../select/Select.svelte'
let {
workspace,
datatable,
schemas,
tables,
schema = $bindable(),
table = $bindable()
}: {
workspace: string
datatable: string
/** The database's schemas, as the editor last read them. */
schemas: string[]
/** The picked schema's tables, as the editor last read them. */
tables: string[]
/** Unset for the database itself. */
schema?: string
/** Unset for the whole schema. */
table?: string
} = $props()
const schemas = resource(
() => [workspace, datatable] as const,
async ([ws, dt]) =>
(
await WorkspaceService.getDatatableAcl({
workspace: ws,
datatableName: dt,
kind: 'database'
})
).children
)
const tables = resource(
() => [workspace, datatable, schema] as const,
async ([ws, dt, s]) =>
s
? (
await WorkspaceService.getDatatableAcl({
workspace: ws,
datatableName: dt,
kind: 'schema',
schema: s
})
).children
: []
)
</script>
<div class="flex flex-wrap items-center gap-2">
<Select
items={(schemas.current ?? []).map((s) => ({ value: s, label: s }))}
items={schemas.map((s) => ({ value: s, label: s }))}
bind:value={
() => schema,
(s) => {
@@ -56,17 +30,15 @@
}
placeholder="The database itself"
clearable
loading={schemas.loading}
size="sm"
class="w-56"
/>
{#if schema}
<Select
items={(tables.current ?? []).map((t) => ({ value: t, label: t }))}
items={tables.map((t) => ({ value: t, label: t }))}
bind:value={table}
placeholder="The whole schema"
clearable
loading={tables.loading}
size="sm"
class="w-56"
/>
frontend/src/lib/components/datatableAcl/PgAclEditor.svelte
+11 -6
View File
@@ -13,6 +13,7 @@
import PgGrantBuilder from './PgGrantBuilder.svelte'
import {
ADMIN_ROLE,
grantKey,
grantScopeLabel,
groupGrants,
revocablePrivileges,
@@ -22,24 +23,30 @@
let {
workspace,
datatable,
target
target,
onLoaded
}: {
workspace: string
datatable: string
/** What owner and grants are read and written for. */
target: AclTarget
/** Each read, with the target it was made for: it also lists what the target holds. */
onLoaded?: (target: AclTarget, info: DatatableAclInfo) => void
} = $props()
const acl = resource(
() => [workspace, datatable, target] as const,
async ([ws, dt, t]) =>
await WorkspaceService.getDatatableAcl({
async ([ws, dt, t]) => {
const loaded = await WorkspaceService.getDatatableAcl({
workspace: ws,
datatableName: dt,
kind: t.kind,
schema: t.schema,
table: t.kind === 'table' ? t.table : undefined
})
onLoaded?.(t, loaded)
return loaded
}
)
// Nothing is written before its SQL has been shown, and the apply runs exactly that SQL: the
@@ -187,9 +194,7 @@
</tr>
</Head>
<tbody class="divide-y">
{#each grantRows as grant (grant.grantee + grant.objects
.map((o) => `${o.name}(${o.args ?? ''})`)
.join() + (grant.future ?? ''))}
{#each grantRows as grant (grantKey(grant))}
{@const revokeScope = revokeScopeOf(grant)}
{@const revocable = revocablePrivileges(grant, target)}
<Row>
frontend/src/lib/components/datatableAcl/aclScopes.test.ts
+14 -1
View File
@@ -1,9 +1,22 @@
import { describe, expect, it } from 'vitest'
import type { AclGrant } from '$lib/gen'
import { groupGrants, revocablePrivileges, revokeScopeOf } from './aclScopes'
import { grantKey, groupGrants, revocablePrivileges, revokeScopeOf } from './aclScopes'
const table = (name: string) => ({ name, kind: 'TABLE' })
describe('grantKey', () => {
it('tells apart a table and a function of the same name', () => {
const row = (object: { name: string; kind: string; args?: string }) => ({
grantee: 'analytics',
privileges: ['SELECT'],
objects: [object]
})
expect(grantKey(row(table('orders')))).not.toBe(
grantKey(row({ name: 'orders', kind: 'FUNCTION', args: '' }))
)
})
})
describe('groupGrants', () => {
// A row's revoke names every object in it, so a row must only hold what one revoke may take.
it('folds the same privileges on objects of one kind, and nothing else', () => {
frontend/src/lib/components/datatableAcl/aclScopes.ts
+14 -2
View File
@@ -130,6 +130,17 @@ export function groupGrants(grants: AclGrant[]): GroupedGrant[] {
return rows
}
/** A row's identity. Two rows may share a grantee and an object name — a table `orders` and a
* function `orders()` — so the kind and the privileges are part of it too. */
export function grantKey(grant: GroupedGrant): string {
return [
grant.grantee,
grant.future ?? '',
grant.privileges.join(','),
...grant.objects.map((o) => `${o.kind}:${o.name}(${o.args ?? ''})`)
].join('|')
}
/** The scope a revoke of this row takes, or `undefined` when the builder cannot express it —
* Postgres also records default privileges on types, which nothing here grants and the API has no
* scope for. */
@@ -141,8 +152,9 @@ export function revokeScopeOf(grant: GroupedGrant): AclScope | undefined {
)
}
/** The privileges of a row a revoke may take back. On the database that leaves out `CONNECT`,
* which the role catalog grants and would grant again. */
/** The privileges of a row a revoke may take back. On the database that is `CREATE` alone:
* `CONNECT` belongs to the role catalog, which would grant it again, and `TEMPORARY` is not one
* the editor hands out — a row holding only those has nothing to revoke here. */
export function revocablePrivileges(grant: GroupedGrant, target: AclTarget): string[] {
if (target.kind === 'database' && grant.objects.length === 0) {
return grant.privileges.filter((p) => DATABASE_PRIVILEGES.includes(p))
frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte
+24 -4
View File
@@ -12,6 +12,7 @@
UserService,
WorkspaceService,
type AclTarget,
type DatatableAclInfo,
type DatatablePermissions,
type InstanceDatatableRole
} from '$lib/gen'
@@ -61,6 +62,17 @@
: { kind: 'schema', schema: aclSchema }
: { kind: 'database' }
)
let aclSchemas = $state<string[]>([])
let aclTables = $state<string[]>([])
// The editor's read of a database lists its schemas, and of a schema its tables — which is what
// the picker offers, so the picker reads nothing of its own. A read for a target since left
// behind is dropped.
function onAclLoaded(target: AclTarget, loaded: DatatableAclInfo) {
if (JSON.stringify(target) !== JSON.stringify(aclTarget)) return
if (target.kind === 'database') aclSchemas = loaded.children
else if (target.kind === 'schema') aclTables = loaded.children
}
async function load() {
loading = true
@@ -142,6 +154,8 @@
export function open() {
aclSchema = undefined
aclTable = undefined
aclSchemas = []
aclTables = []
drawer?.openDrawer()
load()
}
@@ -300,13 +314,19 @@
</span>
</div>
<AclTargetPicker
{workspace}
{datatable}
bind:schema={aclSchema}
schemas={aclSchemas}
tables={aclTables}
bind:schema={
() => aclSchema,
(s) => {
aclSchema = s
aclTables = []
}
}
bind:table={aclTable}
/>
{#key JSON.stringify(aclTarget)}
<PgAclEditor {workspace} {datatable} target={aclTarget} />
<PgAclEditor {workspace} {datatable} target={aclTarget} onLoaded={onAclLoaded} />
{/key}
</div>
{/if}