fix: gate a revoke on the sources of what it takes, not the whole row

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DsU2Lf6wYQJ9o8ASKRgCmK

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<String>,
     /// 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<String>)>,
             Option<String>,
         ),
-        (Vec<String>, BTreeMap<String, bool>),
+        (Vec<String>, BTreeMap<String, (bool, Vec<String>)>),
     > = 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,

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: >-

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 @@
 					<span class="text-xs font-semibold text-emphasis">Owner</span>
 					<span class="text-xs text-secondary">
 						{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.'}
 					</span>
 				</div>
@@ -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)}
 							<Row>
 								<Cell first>{grant.grantee}</Cell>
 								<Cell wrap
@@ -206,19 +208,27 @@
 								>
 								<Cell>
 									{grantScopeLabel(grant)}
-									{#if unreachable.length > 0}
+									{#if blocked.length > 0}
 										<span
 											class="text-2xs text-secondary"
 											title="Only this role can take the grant back: Postgres revokes a grant through the role that made it"
 										>
-											from {unreachable.join(', ')}
+											from {blocked.join(', ')}
+										</span>
+									{/if}
+									{#if uncovered.length > 0}
+										<span
+											class="text-2xs text-secondary"
+											title="A default privilege covers only the roles it was granted for: grant it again to cover these"
+										>
+											· not for what {uncovered.join(', ')} create
 										</span>
 									{/if}
 								</Cell>
 								<Cell last>
 									<!-- What `admin` holds is what every role here connects through, so it is not
 									this editor's to take away. -->
-									{#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}
 										<Button
 											unifiedSize="xs"
 											variant="subtle"

frontend/src/lib/components/datatableAcl/aclScopes.test.ts

@@ -1,18 +1,23 @@
 import { describe, expect, it } from 'vitest'
 import type { AclGrant } from '$lib/gen'
 import {
+	blockingSources,
 	grantKey,
 	groupGrants,
 	revocablePrivileges,
 	revokeScopeOf,
-	unreachableSources
+	uncoveredCreators
 } from './aclScopes'
 
 const table = (name: string) => ({ name, kind: 'TABLE' })
-const from = (...roles: string[]) => roles.map((role) => ({ role, reachable: true }))
+const by = (role: string, privileges: string[], reachable = true) => ({
+	role,
+	privileges,
+	reachable
+})
 const byAdmin = (grant: Omit<AclGrant, 'sources'>): AclGrant => ({
 	...grant,
-	sources: from('admin')
+	sources: [by('admin', grant.privileges)]
 })
 
 describe('grantKey', () => {
@@ -21,7 +26,7 @@ describe('grantKey', () => {
 			grantee: 'analytics',
 			privileges: ['SELECT'],
 			objects: [object],
-			sources: from('admin')
+			sources: [by('admin', ['SELECT'])]
 		})
 		expect(grantKey(row(table('orders')))).not.toBe(
 			grantKey(row({ name: 'orders', kind: 'FUNCTION', args: '' }))
@@ -41,58 +46,38 @@ describe('groupGrants', () => {
 			{ grantee: 'analytics', privileges: ['SELECT'], future: 'TABLES' },
 			{ grantee: 'analytics', privileges: ['USAGE'] }
 		].map(byAdmin)
-		const sources = from('admin')
-		expect(groupGrants(grants)).toEqual([
-			{
-				grantee: 'analytics',
-				privileges: ['SELECT'],
-				objects: [table('orders'), table('salaries')],
-				future: undefined,
-				sources
-			},
-			{
-				grantee: 'operator',
-				privileges: ['SELECT'],
-				objects: [table('orders')],
-				future: undefined,
-				sources
-			},
-			{
-				grantee: 'analytics',
-				privileges: ['INSERT', 'SELECT'],
-				objects: [table('events')],
-				future: undefined,
-				sources
-			},
-			{
-				grantee: 'analytics',
-				privileges: ['SELECT'],
-				objects: [{ name: 's', kind: 'SEQUENCE' }],
-				future: undefined,
-				sources
-			},
-			{ grantee: 'analytics', privileges: ['SELECT'], objects: [], future: 'TABLES', sources },
-			{ grantee: 'analytics', privileges: ['USAGE'], objects: [], future: undefined, sources }
+		const rows = groupGrants(grants)
+		expect(rows.map((r) => [r.grantee, r.privileges, r.objects, r.future])).toEqual([
+			['analytics', ['SELECT'], [table('orders'), table('salaries')], undefined],
+			['operator', ['SELECT'], [table('orders')], undefined],
+			['analytics', ['INSERT', 'SELECT'], [table('events')], undefined],
+			['analytics', ['SELECT'], [{ name: 's', kind: 'SEQUENCE' }], undefined],
+			['analytics', ['SELECT'], [], 'TABLES'],
+			['analytics', ['USAGE'], [], undefined]
 		])
 	})
 
-	// A revoke takes the row back from every source, so the row must name them all.
+	// A revoke takes the row back from every source, so the row must name them all, with what each
+	// gave.
 	it('keeps every source of the grants it folds', () => {
 		const grants: AclGrant[] = [
 			{
 				grantee: 'analytics',
 				privileges: ['SELECT'],
 				object: table('orders'),
-				sources: from('admin')
+				sources: [by('admin', ['SELECT'])]
 			},
 			{
 				grantee: 'analytics',
 				privileges: ['SELECT'],
 				object: table('salaries'),
-				sources: from('admin', 'operator')
+				sources: [by('admin', ['SELECT']), by('operator', ['SELECT'])]
 			}
 		]
-		expect(groupGrants(grants)[0].sources).toEqual(from('admin', 'operator'))
+		expect(groupGrants(grants)[0].sources).toEqual([
+			by('admin', ['SELECT']),
+			by('operator', ['SELECT'])
+		])
 	})
 })
 
@@ -102,7 +87,7 @@ describe('revoke of a row', () => {
 		privileges: ['SELECT'],
 		objects: [],
 		future,
-		sources: from('admin')
+		sources: [by('admin', ['SELECT'])]
 	})
 
 	it('takes back only what the editor may revoke on the database', () => {
@@ -121,39 +106,63 @@ describe('revoke of a row', () => {
 		expect(revokeScopeOf({ ...row(), objects: [{ name: 'mood', kind: 'TYPE' }] })).toBeUndefined()
 	})
 
+	// Postgres takes a grant back only through its source: offering the revoke would promise what
+	// the plan then refuses. But only the sources of what is revoked count: the catalog's CONNECT
+	// on the database comes from its owner, out of reach, and must not hold back a CREATE the
+	// editor granted.
+	it('is held back only by a source out of reach for what it takes', () => {
+		const database = {
+			...row(),
+			privileges: ['CONNECT', 'CREATE'],
+			sources: [by('postgres', ['CONNECT'], false), by('admin', ['CREATE'])]
+		}
+		const revocable = revocablePrivileges(database, { kind: 'database' })
+		expect(blockingSources(database, revocable)).toEqual([])
+		expect(blockingSources(database, ['CONNECT'])).toEqual(['postgres'])
+		const partly = {
+			...row('TABLES'),
+			sources: [by('admin', ['SELECT']), by('postgres', ['SELECT'], false)]
+		}
+		expect(blockingSources(partly, ['SELECT'])).toEqual(['postgres'])
+	})
+
 	// Whether a grant can be taken back depends on its object, so a row folding several objects is
 	// only revocable if each of its grants is.
-	it('offers none for a folded row with a source out of reach on any of its objects', () => {
+	it('is held back by a source out of reach on any of the objects it folds', () => {
 		const grants: AclGrant[] = [
 			{
 				grantee: 'analytics',
 				privileges: ['SELECT'],
 				object: table('orders'),
-				sources: from('admin')
+				sources: [by('admin', ['SELECT'])]
 			},
 			{
 				grantee: 'analytics',
 				privileges: ['SELECT'],
 				object: table('salaries'),
-				sources: [{ role: 'admin', reachable: false }]
+				sources: [by('admin', ['SELECT'], false)]
 			}
 		]
 		const [folded] = groupGrants(grants)
 		expect(folded.objects).toHaveLength(2)
-		expect(revokeScopeOf(folded)).toBeUndefined()
+		expect(blockingSources(folded, ['SELECT'])).toEqual(['admin'])
 		// Folding reads the grants, never rewrites them.
 		expect(grants[0].sources[0].reachable).toBe(true)
 	})
+})
 
-	// Postgres takes a grant back only through its source: offering the revoke would promise what
-	// the plan then refuses.
-	it('offers none for a row with a source out of reach', () => {
-		const partly = {
-			...row('TABLES'),
-			sources: [...from('admin'), { role: 'postgres', reachable: false }]
+describe('uncoveredCreators', () => {
+	// A default privilege binds only the creating roles it was granted for: a role added since is
+	// left out until the grant is made again.
+	it('names the roles a created-later row leaves out', () => {
+		const future = {
+			grantee: 'analytics',
+			privileges: ['SELECT'],
+			objects: [],
+			future: 'TABLES',
+			sources: [by('admin', ['SELECT']), by('analytics', ['SELECT'])]
 		}
-		expect(revokeScopeOf(partly)).toBeUndefined()
-		expect(unreachableSources(partly)).toEqual(['postgres'])
-		expect(unreachableSources(row('TABLES'))).toEqual([])
+		expect(uncoveredCreators(future, ['admin', 'analytics', 'late'])).toEqual(['late'])
+		expect(uncoveredCreators({ ...future, future: undefined }, ['late'])).toEqual([])
 	})
 })

frontend/src/lib/components/datatableAcl/aclScopes.ts

@@ -102,7 +102,7 @@ export type GroupedGrant = {
 	privileges: string[]
 	objects: NonNullable<AclGrant['object']>[]
 	future?: string
-	/** Every role the row's grants come from, each once. */
+	/** Every role the row's grants come from, each once, with what it gave. */
 	sources: AclSource[]
 }
 
@@ -124,8 +124,12 @@ export function groupGrants(grants: AclGrant[]): GroupedGrant[] {
 				const known = existing.sources.find((s) => s.role === source.role)
 				// Whether a role's grant can be taken back depends on the object it is on, so a row
 				// holds a source as reachable only if it is on every object the row folds.
-				if (known) known.reachable &&= source.reachable
-				else existing.sources.push({ ...source })
+				if (known) {
+					known.reachable &&= source.reachable
+					known.privileges = [...new Set([...known.privileges, ...source.privileges])].sort()
+				} else {
+					existing.sources.push({ ...source, privileges: [...source.privileges] })
+				}
 			}
 		} else {
 			rows.push({
@@ -133,17 +137,26 @@ export function groupGrants(grants: AclGrant[]): GroupedGrant[] {
 				privileges: grant.privileges,
 				objects: grant.object ? [grant.object] : [],
 				future: grant.future,
-				sources: grant.sources.map((s) => ({ ...s }))
+				sources: grant.sources.map((s) => ({ ...s, privileges: [...s.privileges] }))
 			})
 		}
 	}
 	return rows
 }
 
-/** The roles a row comes from that this data table's connection cannot act for. Only they can take
- * those grants back, so the editor offers no revoke for the row. */
-export function unreachableSources(grant: GroupedGrant): string[] {
-	return grant.sources.filter((s) => !s.reachable).map((s) => s.role)
+/** The roles that gave some of `privileges` and that this data table's connection cannot act for.
+ * Only they can take those grants back, so a revoke of `privileges` is not offered. */
+export function blockingSources(grant: GroupedGrant, privileges: string[]): string[] {
+	return grant.sources
+		.filter((s) => !s.reachable && s.privileges.some((p) => privileges.includes(p)))
+		.map((s) => s.role)
+}
+
+/** Which of `roles` a "created later" row does not cover. A default privilege binds only the
+ * creating roles it was granted for, so what the others create stays out of it. */
+export function uncoveredCreators(grant: GroupedGrant, roles: string[]): string[] {
+	if (!grant.future) return []
+	return roles.filter((r) => !grant.sources.some((s) => s.role === r))
 }
 
 /** A row's identity. Two rows may share a grantee and an object name — a table `orders` and a
@@ -157,11 +170,10 @@ export function grantKey(grant: GroupedGrant): string {
 	].join('|')
 }
 
-/** The scope a revoke of this row takes, or `undefined` when there is none here: a source out of
- * reach, or privileges on types, present and default, which nothing here grants and the API has no
- * scope for. */
+/** The scope a revoke of this row takes, or `undefined` when the builder cannot express it —
+ * Postgres also records privileges on types, present and default, which nothing here grants and
+ * the API has no scope for. */
 export function revokeScopeOf(grant: GroupedGrant): AclScope | undefined {
-	if (unreachableSources(grant).length > 0) return undefined
 	if (!grant.future) return grant.objects.some((o) => o.kind === 'TYPE') ? undefined : 'target'
 	const scope = `future_${grant.future.toLowerCase()}`
 	return (['future_tables', 'future_sequences', 'future_functions'] as const).find(