Re: predefined role(s) for VACUUM and ANALYZE - Mailing list pgsql-hackers

Nathan Bossart <nathandbossart@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?= <ilmari@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