diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index fe958776ee..8c171c970c 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -c88cd64f5f70e8704c73699bbffd29c8cbdb7c68 +09b3b12b6633445593e5846b5b61d6ae4ef7f88d diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index a295d53517..5b05146560 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -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. diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 951fad050c..2aa0913b34 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -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> { // `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, diff --git a/backend/windmill-common/src/datatable_roles.rs b/backend/windmill-common/src/datatable_roles.rs index 21f75df08d..430469cd5e 100644 --- a/backend/windmill-common/src/datatable_roles.rs +++ b/backend/windmill-common/src/datatable_roles.rs @@ -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)> = 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)], +) -> Vec { + 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""#, + ] + ); + } } diff --git a/frontend/src/lib/components/datatableAcl/AclTargetPicker.svelte b/frontend/src/lib/components/datatableAcl/AclTargetPicker.svelte index 505c5733ed..3d2a511ac1 100644 --- a/frontend/src/lib/components/datatableAcl/AclTargetPicker.svelte +++ b/frontend/src/lib/components/datatableAcl/AclTargetPicker.svelte @@ -1,52 +1,26 @@
({ 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" /> diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index 5ce2d274fb..8ee68e17ed 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -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 @@ - {#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)} diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts index e8061abd21..7d1e69aeb0 100644 --- a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts +++ b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts @@ -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', () => { diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.ts b/frontend/src/lib/components/datatableAcl/aclScopes.ts index 76690427c1..501e801aeb 100644 --- a/frontend/src/lib/components/datatableAcl/aclScopes.ts +++ b/frontend/src/lib/components/datatableAcl/aclScopes.ts @@ -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)) diff --git a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte index ef2ddff569..4919811382 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTablePermissionsButton.svelte @@ -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([]) + let aclTables = $state([]) + + // 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 @@
aclSchema, + (s) => { + aclSchema = s + aclTables = [] + } + } bind:table={aclTable} /> {#key JSON.stringify(aclTarget)} - + {/key} {/if}