From c45f3bc47fddb774065814f72727f51628333d29 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 10 Sep 2026 16:47:51 +0200 Subject: [PATCH] fix: plan only the owner moves Postgres will accept Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DsU2Lf6wYQJ9o8ASKRgCmK --- .../src/datatable_acl.rs | 66 +++++++++++++++++ .../components/datatableAcl/aclScopes.test.ts | 71 +++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 frontend/src/lib/components/datatableAcl/aclScopes.test.ts diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 971d95793e..951fad050c 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -357,11 +357,17 @@ async fn read_owned_objects( client: &tokio_postgres::Client, schema: &str, ) -> Result> { + // A sequence tied to a column — `serial`, identity, `OWNED BY` — follows its table's owner and + // refuses an `ALTER SEQUENCE ... OWNER` of its own, so it is left to the table's statement. let rows = client .query( "SELECT c.relname, c.relkind 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\"[]) + AND NOT (c.relkind = 'S' AND EXISTS ( + SELECT 1 FROM pg_depend d + WHERE d.classid = 'pg_class'::regclass AND d.objid = c.oid + AND d.refclassid = 'pg_class'::regclass AND d.deptype IN ('a', 'i'))) ORDER BY c.relname", &[&schema], ) @@ -814,6 +820,14 @@ async fn build_plan( } _ => vec![], }; + if matches!(change, AclChange::SetOwner { .. }) { + if let Some((object, owner)) = unmanaged_owner(client, target).await? { + return Err(Error::BadRequest(format!( + "{object} is owned by {owner}, which this data table's connection cannot act \ + for, so its owner cannot be changed from here" + ))); + } + } let mut plan = crate::datatable_acl_oss::plan_statements( target, &change, @@ -872,6 +886,58 @@ async fn missing_owner_privilege( Ok((!has).then_some(missing)) } +/// The first thing a change of owner would move that this connection cannot act for, with its +/// owner. Postgres lets only a member of the current owner move an object, and every change runs as +/// `custom_instance_user`, so an object it does not hold the owner of — `public`, owned by the +/// database's owner, above all — is refused here rather than at apply. Running as the instance's +/// own user instead would reach objects Windmill never created. +async fn unmanaged_owner( + client: &tokio_postgres::Client, + target: &AclTarget, +) -> Result> { + let row = match target { + AclTarget::Database => return Ok(None), + AclTarget::Table { schema, table } => { + client + .query_opt( + "SELECT n.nspname || '.' || c.relname, pg_get_userbyid(c.relowner) + FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = $1 AND c.relname = $2 AND NOT pg_has_role(c.relowner, 'USAGE')", + &[schema, table], + ) + .await + } + AclTarget::Schema { schema } => { + client + .query_opt( + "SELECT label, pg_get_userbyid(owner) FROM ( + SELECT 0 AS ord, 'schema ' || n.nspname AS label, n.nspowner AS owner + FROM pg_namespace n WHERE n.nspname = $1 + UNION ALL + SELECT 1, n.nspname || '.' || c.relname, c.relowner + 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 1, n.nspname || '.' || 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 NOT pg_has_role(owner, 'USAGE') ORDER BY ord, label LIMIT 1", + &[schema], + ) + .await + } + } + .map_err(|e| { + Error::internal_err(format!( + "Failed to read who owns what the change moves: {}", + pg_error_message(&e) + )) + })?; + Ok(row.map(|row| (row.get(0), row.get(1)))) +} + async fn plan_datatable_acl( authed: ApiAuthed, Extension(db): Extension, diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts new file mode 100644 index 0000000000..e8061abd21 --- /dev/null +++ b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from 'vitest' +import type { AclGrant } from '$lib/gen' +import { groupGrants, revocablePrivileges, revokeScopeOf } from './aclScopes' + +const table = (name: string) => ({ name, kind: 'TABLE' }) + +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', () => { + const grants: AclGrant[] = [ + { grantee: 'analytics', privileges: ['SELECT'], object: table('orders') }, + { grantee: 'analytics', privileges: ['SELECT'], object: table('salaries') }, + { grantee: 'operator', privileges: ['SELECT'], object: table('orders') }, + { grantee: 'analytics', privileges: ['INSERT', 'SELECT'], object: table('events') }, + { grantee: 'analytics', privileges: ['SELECT'], object: { name: 's', kind: 'SEQUENCE' } }, + { grantee: 'analytics', privileges: ['SELECT'], future: 'TABLES' }, + { grantee: 'analytics', privileges: ['USAGE'] } + ] + expect(groupGrants(grants)).toEqual([ + { + grantee: 'analytics', + privileges: ['SELECT'], + objects: [table('orders'), table('salaries')], + future: undefined + }, + { + grantee: 'operator', + privileges: ['SELECT'], + objects: [table('orders')], + future: undefined + }, + { + grantee: 'analytics', + privileges: ['INSERT', 'SELECT'], + objects: [table('events')], + future: undefined + }, + { + grantee: 'analytics', + privileges: ['SELECT'], + objects: [{ name: 's', kind: 'SEQUENCE' }], + future: undefined + }, + { grantee: 'analytics', privileges: ['SELECT'], objects: [], future: 'TABLES' }, + { grantee: 'analytics', privileges: ['USAGE'], objects: [], future: undefined } + ]) + }) +}) + +describe('revoke of a row', () => { + it('takes back only what the editor may revoke on the database', () => { + const row = { grantee: 'analytics', privileges: ['CONNECT', 'CREATE'], objects: [] } + expect(revocablePrivileges(row, { kind: 'database' })).toEqual(['CREATE']) + expect(revocablePrivileges(row, { kind: 'schema', schema: 'public' })).toEqual([ + 'CONNECT', + 'CREATE' + ]) + }) + + it('maps default privileges to their scope, and refuses the ones it has none for', () => { + const row = (future?: string) => ({ + grantee: 'analytics', + privileges: ['SELECT'], + objects: [], + future + }) + expect(revokeScopeOf(row())).toBe('target') + expect(revokeScopeOf(row('TABLES'))).toBe('future_tables') + expect(revokeScopeOf(row('TYPES'))).toBeUndefined() + }) +})