Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,  Corey Huinker
 <corey.huinker@gmail.com>,  Tom Lane <tgl@sss.pgh.pa.us>,  Stephen Frost
 <sfrost@snowman.net>,  Bharath Rupireddy
 <bharath.rupireddyforpostgres@gmail.com>,  "David G. Johnston"
 <david.g.johnston@gmail.com>,  Kyotaro Horiguchi
 <horikyota.ntt@gmail.com>,  Michael Paquier <michael@paquier.xyz>,  Robert
 Haas <robertmhaas@gmail.com>,  "pgsql-hackers@postgresql.org"
 <pgsql-hackers@postgresql.org>
Subject: Re: predefined role(s) for VACUUM and ANALYZE
References: <20220930231834.GA366260@nathanxps13>
	<CADkLM=fg15_SYRGBcTFpoW-vVUTCkLfe+NTM0G32RjWY1W5Pvw@mail.gmail.com>
	<20221114234004.GA1771874@nathanxps13>
	<20221115050813.GA1953731@nathanxps13>
	<287b17b8-92f3-2bc2-6bcf-31dc1305b65a@dunslane.net>
	<20221117043952.GA116054@nathanxps13>
	<20221118170504.GA401589@nathanxps13>
	<20221119185004.GA539143@nathanxps13>
	<20221120165713.GA597801@nathanxps13>
	<0b00a6ff-1475-c0ba-15ec-5b5e381c6359@dunslane.net>
	<20221123235444.GA479104@nathanxps13>
Date: Tue, 06 Dec 2022 11:47:50 +0000
In-Reply-To: <20221123235444.GA479104@nathanxps13> (Nathan Bossart's message
	of "Wed, 23 Nov 2022 15:54:44 -0800")
Message-ID: <878rjkiwih.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: 
 <https://www.postgresql.org/message-id/878rjkiwih.fsf%40wibble.ilmari.org>
Precedence: bulk

--=-=-=
Content-Type: text/plain

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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=0001-Make-built-in-role-permission-checking-data-driven.patch

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


--=-=-=--