From 932b9032d365c1ffb8f663d445d6ff927e14bd16 Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Fri, 11 Sep 2026 10:48:01 +0200 Subject: [PATCH] fix: track grant sources and revoke only what the connection can take back Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DsU2Lf6wYQJ9o8ASKRgCmK --- backend/ee-repo-ref.txt | 2 +- .../src/datatable_acl.rs | 381 +++++++++++++++--- .../src/datatable_acl_oss.rs | 6 +- backend/windmill-api/openapi.yaml | 23 +- .../datatableAcl/PgAclEditor.svelte | 16 +- .../components/datatableAcl/aclScopes.test.ts | 86 +++- .../lib/components/datatableAcl/aclScopes.ts | 25 +- 7 files changed, 452 insertions(+), 87 deletions(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 2fd92b9245..6068a60a86 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -1d87a86160e5ba203ce7e8af5799f8af1fd90586 +5bc3830f04ff2de110c862eb31689cb4e1724d66 diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index cd06ef5d29..23efd478bd 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -216,6 +216,21 @@ pub struct AclGrant { /// objects that do not exist yet. #[serde(skip_serializing_if = "Option::is_none")] pub future: Option, + /// Where the grant comes from, each role once: who granted it, or for a default privilege the + /// role whose future objects it covers. A revoke takes it back from every one of them. + pub sources: Vec, +} + +/// One role a grant comes from. +#[derive(Serialize, Debug, PartialEq)] +pub struct AclSource { + /// Under the same names as [`AclGrant::grantee`]. + pub role: 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 + /// revocable from here. + pub reachable: bool, } #[derive(Serialize, Debug)] @@ -502,6 +517,226 @@ async fn read_former_owner_defaults( })) } +/// What the catalog holds that a plan depends on, read before planning so the planner stays pure. +#[derive(Debug, Default, PartialEq)] +pub(crate) struct CatalogFacts { + /// Every role that may create objects here, but the one the change is about. + pub(crate) other_pg_roles: Vec, + /// For a schema's change of owner: what moves along with it. + pub(crate) existing_objects: Vec, + /// For a schema's change of owner: the defaults it takes back from the owner it replaces. + pub(crate) former_owner: Option, + /// For a revoke: each grant it takes back. + pub(crate) revoked_grants: Vec, +} + +/// A grant a revoke takes back, as the catalog records it: what `source` gave on `object` (the +/// target itself when `None`), or for a default privilege on what `source` creates later. +#[derive(Debug, PartialEq)] +pub(crate) struct RevokedGrant { + pub(crate) object: Option, + /// The role that made the grant: its grantor, or the creating role of a default privilege. + pub(crate) source: String, + pub(crate) privileges: Vec, +} + +/// An `aclexplode` row's source and whether this connection can take back what it gave, on an +/// object owned by `$owner`. A REVOKE speaks for the grantor Postgres picks itself — the owner, +/// when the connection acts for the owner, and otherwise the connection — and `GRANTED BY` names +/// nobody else, so a grant any other role made stays whatever the connection runs. +macro_rules! grant_source { + ($owner:literal) => { + concat!( + "pg_get_userbyid(a.grantor), a.grantor = CASE WHEN pg_has_role(", + $owner, + ", 'USAGE') THEN ", + $owner, + " ELSE (SELECT oid FROM pg_roles WHERE rolname = current_user) END" + ) + }; +} + +/// What relation `$2` of schema `$1` grants role `$3`, as (source, whether this connection can +/// take it back, privilege). +const RELATION_GRANTS: &str = concat!( + "SELECT ", + grant_source!("c.relowner"), + ", a.privilege_type + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace, + 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 + AND c.relkind = ANY(ARRAY['r','p','v','m','S','f']::\"char\"[]) + AND a.grantee = (SELECT oid FROM pg_roles WHERE rolname = $3)" +); + +/// The grants a revoke takes back, read from the catalog rather than from the request: one per +/// object and source that gave `pg_role` any of `privileges`. Refused when a source's grant is out +/// of this connection's reach (`grant_source!`, or for a default privilege a creating role it does +/// not act for): the revoke would leave that grant in place. +async fn read_revoked_grants( + client: &tokio_postgres::Client, + dbname: &str, + target: &AclTarget, + scope: GrantScope, + objects: &[AclObject], + privileges: &[String], + pg_role: &str, +) -> Result> { + let read_error = |e: tokio_postgres::Error| { + Error::internal_err(format!( + "Failed to read what the revoke takes back: {}", + pg_error_message(&e) + )) + }; + // (object, how it reads in a refusal, one row per source and privilege) + let mut read = Vec::new(); + match (scope, target) { + (scope, AclTarget::Schema { schema }) if scope.is_future() => { + let (objtype, plural) = match scope { + GrantScope::FutureTables => ("r", "tables"), + GrantScope::FutureSequences => ("S", "sequences"), + _ => ("f", "functions"), + }; + let rows = client + .query( + "SELECT pg_get_userbyid(d.defaclrole), pg_has_role(d.defaclrole, 'USAGE'), + a.privilege_type + FROM pg_default_acl d + JOIN pg_namespace n ON n.oid = d.defaclnamespace, + aclexplode(d.defaclacl) a + WHERE n.nspname = $1 AND d.defaclobjtype::text = $2 + AND a.grantee = (SELECT oid FROM pg_roles WHERE rolname = $3)", + &[schema, &objtype, &pg_role], + ) + .await + .map_err(read_error)?; + read.push(( + None, + format!("{plural} created later in schema {schema}"), + rows, + )); + } + (GrantScope::Target, _) if objects.is_empty() => { + let rows = match target { + AclTarget::Database => { + client + .query( + concat!( + "SELECT ", + grant_source!("d.datdba"), + ", a.privilege_type + FROM pg_database d, + aclexplode(COALESCE(d.datacl, acldefault('d', d.datdba))) a + WHERE d.datname = current_database() + AND a.grantee = (SELECT oid FROM pg_roles WHERE rolname = $1)" + ), + &[&pg_role], + ) + .await + } + AclTarget::Schema { schema } => { + client + .query( + concat!( + "SELECT ", + grant_source!("n.nspowner"), + ", a.privilege_type + FROM pg_namespace n, + aclexplode(COALESCE(n.nspacl, acldefault('n', n.nspowner))) a + WHERE n.nspname = $1 + AND a.grantee = (SELECT oid FROM pg_roles WHERE rolname = $2)" + ), + &[schema, &pg_role], + ) + .await + } + AclTarget::Table { schema, table } => { + client + .query(RELATION_GRANTS, &[schema, table, &pg_role]) + .await + } + } + .map_err(read_error)?; + read.push((None, target.label(dbname), rows)); + } + (GrantScope::Target, AclTarget::Schema { schema }) => { + for object in objects { + let rows = match object_keyword(&object.kind)? { + "ROUTINE" => { + client + .query( + concat!( + "SELECT ", + grant_source!("p.proowner"), + ", a.privilege_type + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace, + aclexplode(COALESCE(p.proacl, acldefault('f', p.proowner))) a + WHERE n.nspname = $1 AND p.proname = $2 + AND pg_get_function_identity_arguments(p.oid) = $3 + AND a.grantee = (SELECT oid FROM pg_roles WHERE rolname = $4)" + ), + &[ + schema, + &object.name, + &object.args.as_deref().unwrap_or(""), + &pg_role, + ], + ) + .await + } + _ => { + client + .query(RELATION_GRANTS, &[schema, &object.name, &pg_role]) + .await + } + } + .map_err(read_error)?; + read.push(( + Some(object.clone()), + format!("{} {schema}.{}", object.kind.to_lowercase(), object.name), + rows, + )); + } + } + // Every other scope and target is the planner's to refuse. + _ => {} + } + + let wanted: Vec = privileges.iter().map(|p| p.to_uppercase()).collect(); + let mut revoked = Vec::new(); + for (object, label, rows) in read { + let mut by_source: BTreeMap)> = BTreeMap::new(); + for row in rows { + let privilege: String = row.get(2); + if wanted.contains(&privilege) { + by_source + .entry(row.get(0)) + .or_insert_with(|| (row.get(1), vec![])) + .1 + .push(privilege); + } + } + for (source, (reachable, mut privileges)) in by_source { + if !reachable { + return Err(Error::BadRequest(format!( + "{} on {label} was granted to {} by {source}, and Postgres takes a grant \ + back only through the role that made it, which this data table's \ + connection cannot speak for here. Revoke it as {source}.", + privileges.join(", "), + role_name_of(pg_role), + ))); + } + privileges.sort(); + privileges.dedup(); + revoked.push(RevokedGrant { object: object.clone(), source, privileges }); + } + } + Ok(revoked) +} + /// The keyword a `REVOKE ... ON` takes for one object, checked rather than interpolated: it lands /// in SQL unquoted. pub(crate) fn object_keyword(kind: &str) -> Result<&'static str> { @@ -717,14 +952,19 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res // 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`. The // owner's own entries are left out: what it holds comes with ownership, which the owner shows, - // not with a grant a revoke here could take back. + // not with a grant a revoke here could take back. Each row ends with its source — the grantor, + // or a default privilege's creating role — and whether this connection can take back what it + // gave. 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(COALESCE(d.datacl, acldefault('d', d.datdba))) a - WHERE d.datname = current_database() AND a.grantee <> d.datdba", + concat!( + "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, ", + grant_source!("d.datdba"), + " FROM pg_database d, aclexplode(COALESCE(d.datacl, acldefault('d', d.datdba))) a + WHERE d.datname = current_database() AND a.grantee <> d.datdba" + ), &[], ) .await @@ -732,10 +972,13 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res AclTarget::Schema { schema } => { let mut out = 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_namespace n, aclexplode(COALESCE(n.nspacl, acldefault('n', n.nspowner))) a - WHERE n.nspname = $1 AND a.grantee <> n.nspowner", + concat!( + "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, ", + grant_source!("n.nspowner"), + " FROM pg_namespace n, aclexplode(COALESCE(n.nspacl, acldefault('n', n.nspowner))) a + WHERE n.nspname = $1 AND a.grantee <> n.nspowner" + ), &[schema], ) .await @@ -743,17 +986,20 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res out.extend( client .query( - "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, - a.privilege_type, c.relname, NULL::text, - CASE c.relkind WHEN 'S' THEN 'SEQUENCE' ELSE 'TABLE' END, - NULL::text - FROM pg_class c - JOIN pg_namespace n ON n.oid = c.relnamespace, - 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.relkind = ANY(ARRAY['r','p','v','m','S','f']::\"char\"[]) - AND a.grantee <> c.relowner", + concat!( + "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, + a.privilege_type, c.relname, NULL::text, + CASE c.relkind WHEN 'S' THEN 'SEQUENCE' ELSE 'TABLE' END, + NULL::text, ", + grant_source!("c.relowner"), + " FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace, + 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.relkind = ANY(ARRAY['r','p','v','m','S','f']::\"char\"[]) + AND a.grantee <> c.relowner" + ), &[schema], ) .await @@ -764,14 +1010,17 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res .query( // 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, - 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(COALESCE(p.proacl, acldefault('f', p.proowner))) a - WHERE n.nspname = $1 AND a.grantee <> p.proowner", + concat!( + "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, + a.privilege_type, p.proname, NULL::text, + CASE p.prokind WHEN 'p' THEN 'PROCEDURE' ELSE 'FUNCTION' END, + pg_get_function_identity_arguments(p.oid), ", + grant_source!("p.proowner"), + " FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace, + aclexplode(COALESCE(p.proacl, acldefault('f', p.proowner))) a + WHERE n.nspname = $1 AND a.grantee <> p.proowner" + ), &[schema], ) .await @@ -783,9 +1032,11 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res // `USAGE` on a type is what lets a role use it in a column. Only a type the // schema holds in its own right has an acl: an array, a row type and a // multirange answer to their element, table or range. - "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, - a.privilege_type, t.typname, NULL::text, 'TYPE', NULL::text - FROM pg_type t + concat!( + "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, + a.privilege_type, t.typname, NULL::text, 'TYPE', NULL::text, ", + grant_source!("t.typowner"), + " FROM pg_type t JOIN pg_depend d ON d.classid = 'pg_type'::regclass AND d.objid = t.oid AND d.refclassid = 'pg_namespace'::regclass AND d.deptype = 'n', aclexplode(COALESCE(t.typacl, acldefault('T', t.typowner))) a @@ -794,7 +1045,8 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res AND NOT EXISTS ( SELECT 1 FROM pg_depend x WHERE x.classid = 'pg_type'::regclass AND x.objid = t.oid - AND x.objsubid = 0 AND x.deptype = 'i')", + AND x.objsubid = 0 AND x.deptype = 'i')" + ), &[schema], ) .await @@ -803,11 +1055,14 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res out.extend( client .query( + // What a creating role set is taken back `FOR ROLE` that role, which only + // a role acting for it may do. "SELECT CASE WHEN a.grantee = 0 THEN 'PUBLIC' ELSE pg_get_userbyid(a.grantee) END, a.privilege_type, NULL::text, CASE d.defaclobjtype WHEN 'r' THEN 'TABLES' WHEN 'S' THEN 'SEQUENCES' - WHEN 'f' THEN 'FUNCTIONS' ELSE 'TYPES' END, NULL::text, NULL::text + WHEN 'f' THEN 'FUNCTIONS' ELSE 'TYPES' END, NULL::text, NULL::text, + pg_get_userbyid(d.defaclrole), pg_has_role(d.defaclrole, 'USAGE') FROM pg_default_acl d JOIN pg_namespace n ON n.oid = d.defaclnamespace, aclexplode(d.defaclacl) a @@ -821,28 +1076,32 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res } AclTarget::Table { schema, table } => 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_class c - JOIN pg_namespace n ON n.oid = c.relnamespace, - 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 AND a.grantee <> c.relowner", + concat!( + "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, ", + grant_source!("c.relowner"), + " FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace, + 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 AND a.grantee <> c.relowner" + ), &[schema, table], ) .await .map_err(grant_read_error)?, }; - // One row per privilege — and, for default privileges, one per creating role. Fold them back - // into one entry per grantee and object. + // 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. let mut folded: BTreeMap< ( String, Option<(String, String, Option)>, Option, ), - Vec, + (Vec, BTreeMap), > = BTreeMap::new(); for row in rows.drain(..) { let grantee: String = row.get(0); @@ -851,7 +1110,9 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res let future: Option = row.get(3); let object_kind: Option = row.get(4); let object_args: Option = row.get(5); - folded + let source: String = row.get(6); + let reachable: bool = row.get(7); + let (privileges, sources) = folded .entry(( role_name_of(&grantee), object.map(|name| { @@ -863,12 +1124,13 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res }), future, )) - .or_default() - .push(privilege); + .or_default(); + privileges.push(privilege); + sources.insert(role_name_of(&source), reachable); } Ok(folded .into_iter() - .map(|((grantee, object, future), mut privileges)| { + .map(|((grantee, object, future), (mut privileges, sources))| { privileges.sort(); privileges.dedup(); AclGrant { @@ -876,6 +1138,10 @@ async fn read_grants(client: &tokio_postgres::Client, target: &AclTarget) -> Res privileges, object: object.map(|(name, kind, args)| AclObject { name, kind, args }), future, + sources: sources + .into_iter() + .map(|(role, reachable)| AclSource { role, reachable }) + .collect(), } }) .collect()) @@ -942,6 +1208,15 @@ async fn build_plan( ), _ => (vec![], None), }; + let revoked_grants = match &change { + AclChange::Revoke { privileges, scope, objects, .. } => { + read_revoked_grants( + client, dbname, target, *scope, objects, privileges, &pg_role, + ) + .await? + } + _ => vec![], + }; if matches!(change, AclChange::SetOwner { .. }) { if let Some((object, owner)) = unmanaged_owner(client, target).await? { return Err(Error::BadRequest(format!( @@ -950,15 +1225,9 @@ async fn build_plan( ))); } } - let mut plan = crate::datatable_acl_oss::plan_statements( - target, - &change, - dbname, - &pg_role, - &other_pg_roles, - &existing_objects, - former_owner.as_ref(), - )?; + let facts = CatalogFacts { other_pg_roles, existing_objects, former_owner, revoked_grants }; + let mut plan = + crate::datatable_acl_oss::plan_statements(target, &change, dbname, &pg_role, &facts)?; if matches!(change, AclChange::SetOwner { .. }) { if let Some(missing) = missing_owner_privilege(client, target, &pg_role).await? { plan.warnings.push(format!( diff --git a/backend/windmill-api-workspaces/src/datatable_acl_oss.rs b/backend/windmill-api-workspaces/src/datatable_acl_oss.rs index d7c0ee98c3..c82de6754e 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl_oss.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl_oss.rs @@ -22,7 +22,7 @@ pub(crate) fn ensure_acl_planner() -> windmill_common::error::Result<()> { #[cfg(not(all(feature = "private", feature = "enterprise")))] use { - crate::datatable_acl::{AclChange, AclPlan, AclTarget, FormerOwnerDefaults, OwnedObject}, + crate::datatable_acl::{AclChange, AclPlan, AclTarget, CatalogFacts}, windmill_common::error::{Error, Result}, }; @@ -40,9 +40,7 @@ pub(crate) fn plan_statements( _change: &AclChange, _dbname: &str, _pg_role: &str, - _other_pg_roles: &[String], - _existing_objects: &[OwnedObject], - _former_owner: Option<&FormerOwnerDefaults>, + _facts: &CatalogFacts, ) -> Result { ensure_acl_planner()?; Err(Error::internal_err( diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 9c651fd152..f62b4e0dce 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -32742,7 +32742,7 @@ components: AclGrant: type: object - required: [grantee, privileges] + required: [grantee, privileges, sources] properties: grantee: type: string @@ -32755,6 +32755,27 @@ components: future: type: string description: set for a default privilege, naming the kind of object it covers + sources: + type: array + description: >- + the roles the grant comes from, each once — who granted it, or for a default privilege + the role whose future objects it covers. A revoke takes it back from every one of them. + items: + $ref: "#/components/schemas/AclSource" + + AclSource: + type: object + required: [role, reachable] + properties: + role: + type: string + reachable: + type: boolean + description: >- + whether the data table's connection can take back what role gave. On an object that is + the owner, when the connection acts for the owner, and otherwise the connection itself; + for a default privilege, a creating role the connection acts for. A grant with a source + out of reach is not revocable from here. DatatableAclInfo: type: object diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index 3035455469..a9b2df5a12 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -17,7 +17,8 @@ grantScopeLabel, groupGrants, revocablePrivileges, - revokeScopeOf + revokeScopeOf, + unreachableSources } from './aclScopes' let { @@ -197,12 +198,23 @@ {#each grantRows as grant (grantKey(grant))} {@const revokeScope = revokeScopeOf(grant)} {@const revocable = revocablePrivileges(grant, target)} + {@const unreachable = unreachableSources(grant)} {grant.grantee} {grant.privileges.join(', ')} - {grantScopeLabel(grant)} + + {grantScopeLabel(grant)} + {#if unreachable.length > 0} + + from {unreachable.join(', ')} + + {/if} + diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts index 82e642c383..29e8d00e2d 100644 --- a/frontend/src/lib/components/datatableAcl/aclScopes.test.ts +++ b/frontend/src/lib/components/datatableAcl/aclScopes.test.ts @@ -1,15 +1,27 @@ import { describe, expect, it } from 'vitest' import type { AclGrant } from '$lib/gen' -import { grantKey, groupGrants, revocablePrivileges, revokeScopeOf } from './aclScopes' +import { + grantKey, + groupGrants, + revocablePrivileges, + revokeScopeOf, + unreachableSources +} from './aclScopes' const table = (name: string) => ({ name, kind: 'TABLE' }) +const from = (...roles: string[]) => roles.map((role) => ({ role, reachable: true })) +const byAdmin = (grant: Omit): AclGrant => ({ + ...grant, + sources: from('admin') +}) 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] + objects: [object], + sources: from('admin') }) expect(grantKey(row(table('orders')))).not.toBe( grantKey(row({ name: 'orders', kind: 'FUNCTION', args: '' })) @@ -28,58 +40,96 @@ describe('groupGrants', () => { { grantee: 'analytics', privileges: ['SELECT'], object: { name: 's', kind: 'SEQUENCE' } }, { 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 + future: undefined, + sources }, { grantee: 'operator', privileges: ['SELECT'], objects: [table('orders')], - future: undefined + future: undefined, + sources }, { grantee: 'analytics', privileges: ['INSERT', 'SELECT'], objects: [table('events')], - future: undefined + future: undefined, + sources }, { grantee: 'analytics', privileges: ['SELECT'], objects: [{ name: 's', kind: 'SEQUENCE' }], - future: undefined + future: undefined, + sources }, - { grantee: 'analytics', privileges: ['SELECT'], objects: [], future: 'TABLES' }, - { grantee: 'analytics', privileges: ['USAGE'], objects: [], future: undefined } + { grantee: 'analytics', privileges: ['SELECT'], objects: [], future: 'TABLES', sources }, + { grantee: 'analytics', privileges: ['USAGE'], objects: [], future: undefined, sources } ]) }) + + // A revoke takes the row back from every source, so the row must name them all. + it('keeps every source of the grants it folds', () => { + const grants: AclGrant[] = [ + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('orders'), + sources: from('admin') + }, + { + grantee: 'analytics', + privileges: ['SELECT'], + object: table('salaries'), + sources: from('admin', 'operator') + } + ] + expect(groupGrants(grants)[0].sources).toEqual(from('admin', 'operator')) + }) }) describe('revoke of a row', () => { + const row = (future?: string) => ({ + grantee: 'analytics', + privileges: ['SELECT'], + objects: [], + future, + sources: from('admin') + }) + 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([ + const database = { ...row(), privileges: ['CONNECT', 'CREATE'] } + expect(revocablePrivileges(database, { kind: 'database' })).toEqual(['CREATE']) + expect(revocablePrivileges(database, { 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() 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. + it('offers none for a row with a source out of reach', () => { + const partly = { + ...row('TABLES'), + sources: [...from('admin'), { role: 'postgres', reachable: false }] + } + expect(revokeScopeOf(partly)).toBeUndefined() + expect(unreachableSources(partly)).toEqual(['postgres']) + expect(unreachableSources(row('TABLES'))).toEqual([]) + }) }) diff --git a/frontend/src/lib/components/datatableAcl/aclScopes.ts b/frontend/src/lib/components/datatableAcl/aclScopes.ts index 746b149714..75dbeaf88c 100644 --- a/frontend/src/lib/components/datatableAcl/aclScopes.ts +++ b/frontend/src/lib/components/datatableAcl/aclScopes.ts @@ -1,4 +1,4 @@ -import type { AclGrant, AclTarget } from '$lib/gen' +import type { AclGrant, AclSource, AclTarget } from '$lib/gen' /** The role a data table connects as without roles — `custom_instance_user` in Postgres. */ export const ADMIN_ROLE = 'admin' @@ -102,6 +102,8 @@ export type GroupedGrant = { privileges: string[] objects: NonNullable[] future?: string + /** Every role the row's grants come from, each once. */ + sources: AclSource[] } export function groupGrants(grants: AclGrant[]): GroupedGrant[] { @@ -118,18 +120,30 @@ export function groupGrants(grants: AclGrant[]): GroupedGrant[] { : undefined if (existing) { existing.objects.push(grant.object!) + for (const source of grant.sources) { + if (!existing.sources.some((s) => s.role === source.role)) { + existing.sources.push(source) + } + } } else { rows.push({ grantee: grant.grantee, privileges: grant.privileges, objects: grant.object ? [grant.object] : [], - future: grant.future + future: grant.future, + sources: [...grant.sources] }) } } 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) +} + /** 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 { @@ -141,10 +155,11 @@ export function grantKey(grant: GroupedGrant): string { ].join('|') } -/** 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. */ +/** 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. */ 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(