Nathan Bossart <nathandboss...@gmail.com> writes: > diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c > index 3b5ea3c137..bd967eaa78 100644 > --- a/src/backend/catalog/aclchk.c > +++ b/src/backend/catalog/aclchk.c > @@ -4202,6 +4202,26 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, > AclMode mask, > has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA)) > result |= (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)); > > + /* > + * Check if ACL_VACUUM is being checked and, if so, and not already set > as > + * part of the result, then check if the user is a member of the > + * pg_vacuum_all_tables role, which allows VACUUM on all relations. > + */ > + if (mask & ACL_VACUUM && > + !(result & ACL_VACUUM) && > + has_privs_of_role(roleid, ROLE_PG_VACUUM_ALL_TABLES)) > + result |= ACL_VACUUM; > + > + /* > + * Check if ACL_ANALYZE is being checked and, if so, and not already > set as > + * part of the result, then check if the user is a member of the > + * pg_analyze_all_tables role, which allows ANALYZE on all relations. > + */ > + if (mask & ACL_ANALYZE && > + !(result & ACL_ANALYZE) && > + has_privs_of_role(roleid, ROLE_PG_ANALYZE_ALL_TABLES)) > + result |= ACL_ANALYZE; > + > return result; > }
These checks are getting rather repetitive, how about a data-driven approach, along the lines of the below patch? I'm not quite happy with the naming of the struct and its members (and maybe it should be in a header?), suggestions welcome. - ilmari
>From 34bac3aced60931b2e995c5e1e6269f40c0828f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org> Date: Thu, 1 Dec 2022 11:49:14 +0000 Subject: [PATCH] Make built-in role permission checking data-driven --- src/backend/catalog/aclchk.c | 64 +++++++++++++------------------- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 27 insertions(+), 38 deletions(-) diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index bd967eaa78..434bd39124 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -4084,6 +4084,22 @@ pg_class_aclmask(Oid table_oid, Oid roleid, return pg_class_aclmask_ext(table_oid, roleid, mask, how, NULL); } +/* + * Actions that built-in roles can perform unconditionally + */ +typedef struct RoleAcl +{ + Oid role; + AclMode mode; +} RoleAcl; + +static const RoleAcl builtin_role_acls[] = { + {ROLE_PG_READ_ALL_DATA, ACL_SELECT}, + {ROLE_PG_WRITE_ALL_DATA, ACL_INSERT | ACL_UPDATE | ACL_DELETE}, + {ROLE_PG_VACUUM_ALL_TABLES, ACL_VACUUM}, + {ROLE_PG_ANALYZE_ALL_TABLES, ACL_ANALYZE}, +}; + /* * Routine for examining a user's privileges for a table * @@ -4182,45 +4198,17 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask, ReleaseSysCache(tuple); /* - * Check if ACL_SELECT is being checked and, if so, and not set already as - * part of the result, then check if the user is a member of the - * pg_read_all_data role, which allows read access to all relations. + * For each built-in role, check if its permissions are being checked and, + * if so, and not set already as part of the result, then check if the + * user is a member of the role, and allow the action if so. */ - if (mask & ACL_SELECT && !(result & ACL_SELECT) && - has_privs_of_role(roleid, ROLE_PG_READ_ALL_DATA)) - result |= ACL_SELECT; - - /* - * Check if ACL_INSERT, ACL_UPDATE, or ACL_DELETE is being checked and, if - * so, and not set already as part of the result, then check if the user - * is a member of the pg_write_all_data role, which allows - * INSERT/UPDATE/DELETE access to all relations (except system catalogs, - * which requires superuser, see above). - */ - if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE) && - !(result & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) && - has_privs_of_role(roleid, ROLE_PG_WRITE_ALL_DATA)) - result |= (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)); - - /* - * Check if ACL_VACUUM is being checked and, if so, and not already set as - * part of the result, then check if the user is a member of the - * pg_vacuum_all_tables role, which allows VACUUM on all relations. - */ - if (mask & ACL_VACUUM && - !(result & ACL_VACUUM) && - has_privs_of_role(roleid, ROLE_PG_VACUUM_ALL_TABLES)) - result |= ACL_VACUUM; - - /* - * Check if ACL_ANALYZE is being checked and, if so, and not already set as - * part of the result, then check if the user is a member of the - * pg_analyze_all_tables role, which allows ANALYZE on all relations. - */ - if (mask & ACL_ANALYZE && - !(result & ACL_ANALYZE) && - has_privs_of_role(roleid, ROLE_PG_ANALYZE_ALL_TABLES)) - result |= ACL_ANALYZE; + for (int i = 0; i < lengthof(builtin_role_acls); i++) + { + const RoleAcl *const builtin = &builtin_role_acls[i]; + if (mask & builtin->mode && !(result & builtin->mode) && + has_privs_of_role(roleid, builtin->role)) + result |= (mask & builtin->mode); + } return result; } diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 58daeca831..1c36d241db 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2340,6 +2340,7 @@ RewriteState RmgrData RmgrDescData RmgrId +RoleAcl RoleNameItem RoleSpec RoleSpecType -- 2.34.1