From: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>
Cc: Corey Huinker <corey.huinker@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: predefined role(s) for VACUUM and ANALYZE
Date: Tue, 06 Dec 2022 11:47:50 +0000
Message-ID: <878rjkiwih.fsf@wibble.ilmari.org> (raw)
In-Reply-To: <20221123235444.GA479104@nathanxps13>
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>
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
Attachments:
[text/x-diff] 0001-Make-built-in-role-permission-checking-data-driven.patch (3.7K, ../878rjkiwih.fsf@wibble.ilmari.org/2-0001-Make-built-in-role-permission-checking-data-driven.patch)
download | inline diff:
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.cindex 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.listindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ilmari@ilmari.org, nathandbossart@gmail.com, andrew@dunslane.net, corey.huinker@gmail.com, tgl@sss.pgh.pa.us, sfrost@snowman.net, bharath.rupireddyforpostgres@gmail.com, david.g.johnston@gmail.com, horikyota.ntt@gmail.com, michael@paquier.xyz, robertmhaas@gmail.com
Subject: Re: predefined role(s) for VACUUM and ANALYZE
In-Reply-To: <878rjkiwih.fsf@wibble.ilmari.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox