agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Nathan Bossart <nathan@postgresql.org>
Subject: [PATCH v5 1/1] Fix authorization check for role membership changes.
Date: Fri, 7 Aug 2026 16:50:25 -0500
Presently, check_role_membership_authorization() decides whether
the current user may grant or revoke membership in a role by
calling is_admin_of_role(), which recurses through all grants,
while the grantor to record for the resulting entry is chosen by
select_best_admin(), which recurses only through inherited grants.
When the two disagree, the premission check passes and the grantor
lookup then comes up empty, so the user sees an internal "no
possible grantors" error. ALTER GROUP ... ADD USER reaches the
same error through the separate check in AlterRole().
To fix, teach both checks to search the same way
select_best_admin() does via a new has_admin_privs_of_role(). The
new check passes exactly when the grantor lookup was going to
succeed, so nothing that works today starts failing; the internal
error simply becomes a proper permission error. Note that this
leaves the other callers of is_admin_of_role() alone, so a role
reachable only through a non-inherited grant can still be dropped,
renamed, or altered. Whether that ought to change as well is left
as a future exercise.
Oversight in commit ce6b672e44.
Reported-by: ChangAo Chen <cca5507@qq.com>
Author: ChangAo Chen <cca5507@qq.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Reviewed-by: Jacob Champion <jacob.champion@enterprisedb.com>
Discussion: https://postgr.es/m/tencent_ADCE2B34B230A9B631854806104FEF40C105%40qq.com
Backpatch-through: 16
---
src/backend/commands/user.c | 4 ++--
src/backend/utils/adt/acl.c | 27 ++++++++++++++++++++++++
src/include/utils/acl.h | 1 +
src/test/regress/expected/privileges.out | 9 ++++++++
src/test/regress/sql/privileges.sql | 5 +++++
5 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index be11c49f919..04b270c08a8 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -825,7 +825,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
}
/* To add or drop members, you need ADMIN OPTION. */
- if (drolemembers && !is_admin_of_role(currentUserId, roleid))
+ if (drolemembers && !has_admin_privs_of_role(currentUserId, roleid))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied to alter role"),
@@ -2165,7 +2165,7 @@ check_role_membership_authorization(Oid currentUserId, Oid roleid,
/*
* Otherwise, must have admin option on the role to be changed.
*/
- if (!is_admin_of_role(currentUserId, roleid))
+ if (!has_admin_privs_of_role(currentUserId, roleid))
{
if (is_grant)
ereport(ERROR,
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index e2547d719ed..a4d52e80970 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -5442,6 +5442,8 @@ is_member_of_role_nosuper(Oid member, Oid role)
* Is member an admin of role? That is, is member the role itself (subject to
* restrictions below), a member (directly or indirectly) WITH ADMIN OPTION,
* or a superuser?
+ *
+ * See also has_admin_privs_of_role() below.
*/
bool
is_admin_of_role(Oid member, Oid role)
@@ -5459,6 +5461,31 @@ is_admin_of_role(Oid member, Oid role)
return OidIsValid(admin_role);
}
+/*
+ * Does member hold ADMIN OPTION on role, either directly or through a role
+ * whose privileges member inherits?
+ *
+ * Unlike is_admin_of_role(), this does not recurse through grants that are not
+ * inherited. Callers that must go on to record a grantor for the operation
+ * should use this rather than is_admin_of_role(), since select_best_admin()
+ * searches the same way.
+ */
+bool
+has_admin_privs_of_role(Oid member, Oid role)
+{
+ Oid admin_role;
+
+ if (superuser_arg(member))
+ return true;
+
+ /* By policy, a role cannot have WITH ADMIN OPTION on itself. */
+ if (member == role)
+ return false;
+
+ (void) roles_is_member_of(member, ROLERECURSE_PRIVS, role, &admin_role);
+ return OidIsValid(admin_role);
+}
+
/*
* Find a role whose privileges "member" inherits which has ADMIN OPTION
* on "role", ignoring super-userness.
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 0b9b04e78ee..e0f70ccb4c3 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -216,6 +216,7 @@ extern void check_can_set_role(Oid member, Oid role);
extern bool is_member_of_role(Oid member, Oid role);
extern bool is_member_of_role_nosuper(Oid member, Oid role);
extern bool is_admin_of_role(Oid member, Oid role);
+extern bool has_admin_privs_of_role(Oid member, Oid role);
extern Oid select_best_admin(Oid member, Oid role);
extern Oid get_role_oid(const char *rolname, bool missing_ok);
extern Oid get_role_oid_or_public(const char *rolname);
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 5e3c9510490..5c28aacdb51 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -78,6 +78,15 @@ SELECT grantor::regrole FROM pg_auth_members WHERE roleid = 'regress_priv_user1'
regress_priv_user2
(1 row)
+RESET ROLE;
+REVOKE INHERIT OPTION FOR regress_priv_user2 FROM regress_priv_user3;
+SET ROLE regress_priv_user3;
+GRANT regress_priv_user1 TO regress_priv_user5; -- fail
+ERROR: permission denied to grant role "regress_priv_user1"
+DETAIL: Only roles with the ADMIN option on role "regress_priv_user1" may grant this role.
+ALTER GROUP regress_priv_user1 ADD USER regress_priv_user5; -- fail
+ERROR: permission denied to alter role
+DETAIL: Only roles with the ADMIN option on role "regress_priv_user1" may add or drop members.
RESET ROLE;
REVOKE regress_priv_user2 FROM regress_priv_user3;
REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index d3e87fa617f..6a7c4ca92e5 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -60,6 +60,11 @@ SET ROLE regress_priv_user3;
GRANT regress_priv_user1 TO regress_priv_user4;
SELECT grantor::regrole FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole and member = 'regress_priv_user4'::regrole;
RESET ROLE;
+REVOKE INHERIT OPTION FOR regress_priv_user2 FROM regress_priv_user3;
+SET ROLE regress_priv_user3;
+GRANT regress_priv_user1 TO regress_priv_user5; -- fail
+ALTER GROUP regress_priv_user1 ADD USER regress_priv_user5; -- fail
+RESET ROLE;
REVOKE regress_priv_user2 FROM regress_priv_user3;
REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
--
2.50.1 (Apple Git-155)
--yMN78K0KJ1SI7U4o--
view thread (134+ messages) latest in thread
Message-ID: <no-message-id-1179718@localhost>
Permalink: ../../no-message-id-1179718@localhost/
Also on: postgresql.org/message-id/no-message-id-1179718@localhost
reply
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: nathan@postgresql.org
Subject: Re: [PATCH v5 1/1] Fix authorization check for role membership changes.
In-Reply-To: <no-message-id-1179718@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox