From 40bd4be333953213c755c6c4cf103750f8963fa3 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Fri, 11 Sep 2026 15:06:04 +0200 Subject: [PATCH] fix: gate a revoke on the sources of what it takes, not the whole row Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DsU2Lf6wYQJ9o8ASKRgCmK --- .../src/datatable_acl.rs | 38 +++++- backend/windmill-api/openapi.yaml | 120 +++++++++++++++--- .../datatableAcl/PgAclEditor.svelte | 24 +++- .../components/datatableAcl/aclScopes.test.ts | 117 +++++++++-------- .../lib/components/datatableAcl/aclScopes.ts | 36 ++++-- 5 files changed, 239 insertions(+), 96 deletions(-) diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 41230fa3c8..d51ae9acdb 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -226,6 +226,9 @@ pub struct AclGrant { pub struct AclSource { /// Under the same names as [`AclGrant::grantee`]. pub role: String, + /// What `role` gave of the grant's privileges. A revoke is held back only by a source out of + /// reach that gave some of what it takes back. + pub privileges: Vec, /// Whether this data table's connection can take back what `role` gave: on an object only the /// owner's grants when it acts for the owner, or else its own (`grant_source!`); for a default /// privilege, a creating role it acts for. A grant with any source out of reach is not @@ -1093,15 +1096,15 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res }; // One row per privilege and source — and, for default privileges, per creating role. Fold them - // back into one entry per grantee and object that keeps every source: a revoke takes the grant - // back from each of them. + // back into one entry per grantee and object that keeps every source and what each gave: a + // revoke takes a privilege back from every source that gave it. let mut folded: BTreeMap< ( String, Option<(String, String, Option)>, Option, ), - (Vec, BTreeMap), + (Vec, BTreeMap)>), > = BTreeMap::new(); for row in rows.drain(..) { let grantee: String = row.get(0); @@ -1125,8 +1128,12 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res future, )) .or_default(); + sources + .entry(role_name_of(&source)) + .or_insert_with(|| (reachable, vec![])) + .1 + .push(privilege.clone()); privileges.push(privilege); - sources.insert(role_name_of(&source), reachable); } Ok(folded .into_iter() @@ -1140,7 +1147,11 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res future, sources: sources .into_iter() - .map(|(role, reachable)| AclSource { role, reachable }) + .map(|(role, (reachable, mut privileges))| { + privileges.sort(); + privileges.dedup(); + AclSource { role, privileges, reachable } + }) .collect(), } }) @@ -1582,6 +1593,23 @@ mod tests { .await .unwrap(); assert!(held_none.is_empty(), "{held_none:?}"); + // Each source says what it gave: that, and not the whole row, is what a revoke of some of + // its privileges is held back by. + let grants = read_grants( + &client, + &AclTarget::Schema { schema: "granted".to_string() }, + ) + .await + .unwrap(); + let on_g = grants + .iter() + .find(|g| { + g.grantee == "pg_read_all_data" && g.object.as_ref().is_some_and(|o| o.name == "g") + }) + .unwrap(); + assert_eq!(on_g.sources.len(), 1, "{:?}", on_g.sources); + assert_eq!(on_g.sources[0].privileges, ["INSERT", "SELECT"]); + assert!(on_g.sources[0].reachable); } /// A kind of object the list misses stays with its old owner while the schema changes hands, diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index f62b4e0dce..0381151a44 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -32648,28 +32648,82 @@ components: type: string AclTarget: + description: what access is read or changed on + oneOf: + - $ref: "#/components/schemas/AclTargetDatabase" + - $ref: "#/components/schemas/AclTargetSchema" + - $ref: "#/components/schemas/AclTargetTable" + discriminator: + propertyName: kind + mapping: + database: "#/components/schemas/AclTargetDatabase" + schema: "#/components/schemas/AclTargetSchema" + table: "#/components/schemas/AclTargetTable" + + AclTargetDatabase: type: object required: [kind] properties: kind: type: string - enum: [database, schema, table] + enum: [database] + + AclTargetSchema: + type: object + required: [kind, schema] + properties: + kind: + type: string + enum: [schema] + schema: + type: string + + AclTargetTable: + type: object + required: [kind, schema, table] + properties: + kind: + type: string + enum: [table] schema: type: string - description: required for a schema or table target table: type: string AclChange: + description: one change to plan or apply + oneOf: + - $ref: "#/components/schemas/AclChangeSetOwner" + - $ref: "#/components/schemas/AclChangeGrant" + - $ref: "#/components/schemas/AclChangeRevoke" + discriminator: + propertyName: type + mapping: + set_owner: "#/components/schemas/AclChangeSetOwner" + grant: "#/components/schemas/AclChangeGrant" + revoke: "#/components/schemas/AclChangeRevoke" + + AclChangeSetOwner: type: object + description: >- + hands the target to role — for a schema, with everything already in it but an extension's + members, which stay with the extension required: [type, role] properties: type: type: string - enum: [set_owner, grant, revoke] - description: >- - set_owner hands the target to role — for a schema, with everything already in it but - an extension's members, which stay with the extension. + enum: [set_owner] + role: + type: string + description: a data table role of the instance, or admin + + AclChangeGrant: + type: object + required: [type, role, privileges, scope] + properties: + type: + type: string + enum: [grant] role: type: string description: a data table role of the instance, or admin @@ -32678,23 +32732,46 @@ components: items: type: string scope: + $ref: "#/components/schemas/AclGrantScope" + + AclChangeRevoke: + type: object + required: [type, role, privileges, scope] + properties: + type: type: string - enum: - [ - target, - all_tables, - all_sequences, - all_functions, - future_tables, - future_sequences, - future_functions, - ] + enum: [revoke] + role: + type: string + description: a data table role of the instance, other than admin + privileges: + type: array + items: + type: string + scope: + $ref: "#/components/schemas/AclGrantScope" objects: type: array - description: objects inside the target a revoke covers, empty for the target itself + description: >- + objects inside the target the revoke covers, empty for the target itself. Only with the + target scope; a revoke on all objects of a kind is refused, since it cannot say which + grants it takes back. items: $ref: "#/components/schemas/AclObject" + AclGrantScope: + type: string + enum: + [ + target, + all_tables, + all_sequences, + all_functions, + future_tables, + future_sequences, + future_functions, + ] + AclChangeRequest: type: object required: [target, change] @@ -32765,10 +32842,17 @@ components: AclSource: type: object - required: [role, reachable] + required: [role, privileges, reachable] properties: role: type: string + privileges: + type: array + description: >- + what role gave of the grant's privileges. A revoke is held back only by a source out of + reach that gave some of what it takes back. + items: + type: string reachable: type: boolean description: >- diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index 829cefea36..ca0f801bfb 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -13,12 +13,13 @@ import PgGrantBuilder from './PgGrantBuilder.svelte' import { ADMIN_ROLE, + blockingSources, grantKey, grantScopeLabel, groupGrants, revocablePrivileges, revokeScopeOf, - unreachableSources + uncoveredCreators } from './aclScopes' let { @@ -42,7 +43,7 @@ workspace: ws, datatableName: dt, kind: t.kind, - schema: t.schema, + schema: t.kind === 'database' ? undefined : t.schema, table: t.kind === 'table' ? t.table : undefined }) onLoaded?.(t, loaded) @@ -132,7 +133,7 @@ Owner {target.kind === 'schema' - ? 'The role that owns the schema and everything already in it, except what belongs to an extension, which stays with the extension. Changing it also keeps the new owner in reach of what the roles there are now create here later.' + ? 'The role that owns the schema and everything already in it, except what belongs to an extension, which stays with the extension. Changing it also keeps the new owner in reach of what the current roles create here from then on; a role added afterwards is not covered.' : 'The role that owns the table. Its owner may always read and write it, and is who ALTER and DROP answer to.'} @@ -198,7 +199,8 @@ {#each grantRows as grant (grantKey(grant))} {@const revokeScope = revokeScopeOf(grant)} {@const revocable = revocablePrivileges(grant, target)} - {@const unreachable = unreachableSources(grant)} + {@const blocked = blockingSources(grant, revocable)} + {@const uncovered = uncoveredCreators(grant, info.roles)} {grant.grantee} {grantScopeLabel(grant)} - {#if unreachable.length > 0} + {#if blocked.length > 0} - from {unreachable.join(', ')} + from {blocked.join(', ')} + + {/if} + {#if uncovered.length > 0} + + · not for what {uncovered.join(', ')} create {/if} - {#if info.editable && revokeScope && revocable.length > 0 && info.roles.includes(grant.grantee) && grant.grantee !== ADMIN_ROLE} + {#if info.editable && revokeScope && revocable.length > 0 && blocked.length === 0 && info.roles.includes(grant.grantee) && grant.grantee !== ADMIN_ROLE}