diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 51d500ccd2..ed5cfebffe 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -210,10 +210,11 @@ pub enum AclChange { role: String, privileges: Vec, scope: GrantScope, - /// One object inside the target. `ON ALL TABLES` grants read back per - /// object, so they are revoked per object too. + /// Objects inside the target, empty for the target itself. `ON ALL + /// TABLES` grants read back per object, so they are revoked per object — + /// and the same privileges on several of them are revoked together. #[serde(default)] - object: Option, + objects: Vec, }, } @@ -384,10 +385,13 @@ fn plan_statements( AclChange::Grant { privileges, scope, .. } | AclChange::Revoke { privileges, scope, .. } => { let revoking = matches!(change, AclChange::Revoke { .. }); - let object = match change { - AclChange::Revoke { object, .. } => object.as_ref(), - _ => None, + let objects: &[AclObject] = match change { + AclChange::Revoke { objects, .. } => objects, + _ => &[], }; + // Every object of one revoke is the same kind of thing, so the first + // decides which privileges are legal for all of them. + let object = objects.first(); // A named object decides which privileges are legal, not the scope: // `ON ALL TABLES` grants read back per object and revoke per object. let allowed = match object { @@ -434,16 +438,18 @@ fn plan_statements( privileges, plural, schema, role ) }), - (_, None) if object.is_some() => { - let object = object.expect("checked"); - Some(format!( - "REVOKE {} ON {} {}.{} FROM {}", - privileges, - object_keyword(&object.kind)?, - schema, - quote_ident(&object.name), - role - )) + (_, None) if !objects.is_empty() => { + for object in objects { + statements.push(format!( + "REVOKE {} ON {} {}.{} FROM {}", + privileges, + object_keyword(&object.kind)?, + schema, + quote_ident(&object.name), + role + )); + } + None } (_, None) => Some(if revoking { format!( @@ -958,7 +964,7 @@ mod tests { role: "analyst".to_string(), privileges: vec!["select".to_string()], scope: GrantScope::AllTables, - object: None, + objects: vec![], }, "dt_probe", "wm_analyst_1", @@ -980,10 +986,10 @@ mod tests { role: "analyst".to_string(), privileges: vec!["SELECT".to_string()], scope: GrantScope::Target, - object: Some(AclObject { + objects: vec![AclObject { name: "orders".to_string(), kind: "TABLE".to_string(), - }), + }], }, "dt_probe", "wm_analyst_1", @@ -1047,6 +1053,34 @@ mod tests { } } + #[test] + fn several_objects_are_revoked_together() { + let plan = plan_statements( + &schema(), + &AclChange::Revoke { + role: "analyst".to_string(), + privileges: vec!["SELECT".to_string()], + scope: GrantScope::Target, + objects: vec![ + AclObject { name: "a".to_string(), kind: "TABLE".to_string() }, + AclObject { name: "b".to_string(), kind: "TABLE".to_string() }, + ], + }, + "dt_probe", + "wm_analyst_1", + &[], + &[], + ) + .unwrap(); + assert_eq!( + plan.statements, + [ + r#"REVOKE SELECT ON TABLE "analytics"."a" FROM "wm_analyst_1""#, + r#"REVOKE SELECT ON TABLE "analytics"."b" FROM "wm_analyst_1""#, + ] + ); + } + #[test] fn a_privilege_the_object_does_not_have_is_refused() { for (scope, privilege) in [ diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index c447fe9d40..0830b6ffbc 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -34001,8 +34001,11 @@ components: future_sequences, future_functions ] - object: - $ref: "#/components/schemas/AclObject" + objects: + type: array + description: objects inside the target a revoke covers, empty for the target itself + items: + $ref: "#/components/schemas/AclObject" AclChangeRequest: type: object diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index 2111b2ccd9..b21b65fe74 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -13,7 +13,7 @@ import Cell from '../table/Cell.svelte' import { Trash2 } from 'lucide-svelte' import PgGrantBuilder from './PgGrantBuilder.svelte' - import { grantScopeLabel, type AclScope } from './aclScopes' + import { grantScopeLabel, groupGrants, type AclScope } from './aclScopes' let { workspace, @@ -48,6 +48,10 @@ let pending = $state< { change: AclChange; statements: string[]; warnings: string[]; title: string } | undefined >(undefined) + + /** A revoke listed per object takes them all: say where one of them is + * managed on its own. */ + const pendingCoversObjects = $derived((pending?.change.objects?.length ?? 0) > 1) let planning = $state(false) let applying = $state(false) @@ -88,6 +92,7 @@ const info: DatatableAclInfo | undefined = $derived(acl.current) const roleItems = $derived((info?.roles ?? []).map((r) => ({ value: r, label: r }))) + const grantRows = $derived(groupGrants(info?.grants ?? [])) {#if acl.error} @@ -152,7 +157,7 @@ `Granted ${privileges.join(', ')} to ${role}` )} /> - {#if info.grants.length === 0} + {#if grantRows.length === 0} No grants yet. {:else} @@ -165,7 +170,9 @@ - {#each info.grants as grant (grant.grantee + (grant.object?.name ?? '') + (grant.future ?? ''))} + {#each grantRows as grant (grant.grantee + grant.objects + .map((o) => o.name) + .join() + (grant.future ?? ''))} {grant.grantee} {grant.privileges.join(', ')} @@ -189,7 +196,7 @@ scope: grant.future ? (`future_${grant.future.toLowerCase()}` as AclScope) : 'target', - object: grant.object + objects: grant.objects }, `Revoked ${grant.privileges.join(', ')} from ${grant.grantee}` )} @@ -216,6 +223,11 @@ onCanceled={() => (pending = undefined)} >
+ {#if pendingCoversObjects} + + Permissions on a single table are managed in that table's own permissions drawer. + + {/if} {#each pending?.warnings ?? [] as warning} {warning} {/each} diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.ts b/frontend/src/lib/components/datatableAcl/aclScopes.ts index a723fd519f..56481cc2aa 100644 --- a/frontend/src/lib/components/datatableAcl/aclScopes.ts +++ b/frontend/src/lib/components/datatableAcl/aclScopes.ts @@ -95,9 +95,47 @@ export function scopeSql(scope: AclScope, target: AclTarget, dbname?: string): s } } -/** How an existing grant reads back: the object it covers, in one phrase. */ -export function grantScopeLabel(grant: AclGrant): string { +/** One row of the grants table: the same privileges on several objects read as + * one line, since granting them per object is what `ON ALL TABLES` does. */ +export type GroupedGrant = { + grantee: string + privileges: string[] + objects: NonNullable[] + future?: string +} + +export function groupGrants(grants: AclGrant[]): GroupedGrant[] { + const rows: GroupedGrant[] = [] + for (const grant of grants) { + const existing = grant.object + ? rows.find( + (r) => + r.grantee === grant.grantee && + r.future === grant.future && + r.objects[0]?.kind === grant.object?.kind && + r.privileges.join() === grant.privileges.join() + ) + : undefined + if (existing) { + existing.objects.push(grant.object!) + } else { + rows.push({ + grantee: grant.grantee, + privileges: grant.privileges, + objects: grant.object ? [grant.object] : [], + future: grant.future + }) + } + } + return rows +} + +/** How a row reads back: what it covers, in one phrase. */ +export function grantScopeLabel(grant: GroupedGrant): string { if (grant.future) return `${grant.future.toLowerCase()} created later` - if (grant.object) return `${grant.object.kind.toLowerCase()} ${grant.object.name}` + if (grant.objects.length === 1) + return `${grant.objects[0].kind.toLowerCase()} ${grant.objects[0].name}` + if (grant.objects.length > 1) + return `${grant.objects.length} ${grant.objects[0].kind.toLowerCase()}s` return 'itself' }