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
