Why is_admin_of_role() use ROLERECURSE_MEMBERS rather than ROLERECURSE_PRIVS?
Hi,
When reading the code, I find is_admin_of_role() use ROLERECURSE_MEMBERS while select_best_admin() use ROLERECURSE_PRIVS.
Why they are dismatch?
The following case will have is_admin_of_role() return true and select_best_admin() return InvalidOid:
create user u1;
create user u2;
create user u3;
create user u4;
grant u2 to u1 with admin true ;
grant u3 to u2 with admin true ;
revoke inherit option for u2 from u1 ;
set session authorization u1;
grant u3 to u4;
The "grant u3 to u4;" will report error "no possible grantors" rather than "permission denied to grant role".
Is this the expected behavior?
--
Regards,
ChangAo Chen
Hi,
According to the comment in check_role_grantor():
/*
* Otherwise, the grantor must either have ADMIN OPTION on the role or
* inherit the privileges of a role which does. In the former case,
* record the grantor as the current user; in the latter, pick one of
* the roles that is "most directly" inherited by the current role
* (i.e. fewest "hops").
*
* (We shouldn't fail to find a best grantor, because we've already
* established that the current user has permission to perform the
* operation.)
*/
grantorId = select_best_admin(currentUserId, roleid);
if (!OidIsValid(grantorId))
elog(ERROR, "no possible grantors");
But the "no possible grantors" error can happen in my test case.
The main reason is that is_admin_of_role() and select_best_admin() use different role recurse methods.
I think they should keep consistent, maybe both use ROLERECURSE_PRIVS? Thoughts?
--
Regards,
ChangAo Chen
Hi,
I attach a small patch for this.
Looking forward to your review.
--
Regards,
ChangAo Chen
Attachments:
v1-0001-Use-ROLERECURSE_PRIVS-in-is_admin_of_role.patchapplication/octet-stream; charset=utf-8; name=v1-0001-Use-ROLERECURSE_PRIVS-in-is_admin_of_role.patchDownload
From c3459c9dcd024734c684458bec1113883ada06e0 Mon Sep 17 00:00:00 2001
From: ChangAo Chen <cca5507@qq.com>
Date: Mon, 24 Nov 2025 11:03:58 +0800
Subject: [PATCH v1] Use ROLERECURSE_PRIVS in is_admin_of_role().
Currently we use different role recurse methods in is_admin_of_role()
and select_best_admin(), this could lead to an unexpected behavior.
For example, when is_admin_of_role() return true, select_best_admin()
can still return InvalidOid. To fix it, we use ROLERECURSE_PRIVS in
is_admin_of_role(), making it consistent with select_best_admin().
---
src/backend/utils/adt/acl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index fbcd64a2609..74fc73bc2be 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -5422,7 +5422,7 @@ is_admin_of_role(Oid member, Oid role)
if (member == role)
return false;
- (void) roles_is_member_of(member, ROLERECURSE_MEMBERS, role, &admin_role);
+ (void) roles_is_member_of(member, ROLERECURSE_PRIVS, role, &admin_role);
return OidIsValid(admin_role);
}
--
2.34.1
Hi,
Fix "create_role.sql" in v2.
--
Regards,
ChangAo Chen
Attachments:
v2-0001-Use-ROLERECURSE_PRIVS-in-is_admin_of_role.patchapplication/octet-stream; charset=utf-8; name=v2-0001-Use-ROLERECURSE_PRIVS-in-is_admin_of_role.patchDownload
From ea33903c1c79ce6bb149017f8203bee4eb359e2d Mon Sep 17 00:00:00 2001
From: ChangAo Chen <cca5507@qq.com>
Date: Tue, 23 Dec 2025 10:59:28 +0800
Subject: [PATCH v2] Use ROLERECURSE_PRIVS in is_admin_of_role().
Currently we use different role recurse methods in is_admin_of_role()
and select_best_admin(), this could lead to an unexpected behavior.
For example, when is_admin_of_role() return true, select_best_admin()
can still return InvalidOid. To fix it, we use ROLERECURSE_PRIVS in
is_admin_of_role(), making it consistent with select_best_admin().
---
src/backend/utils/adt/acl.c | 2 +-
src/test/regress/expected/create_role.out | 7 +++++++
src/test/regress/sql/create_role.sql | 6 ++++++
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 05d48412f82..d22e696a46a 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -5422,7 +5422,7 @@ is_admin_of_role(Oid member, Oid role)
if (member == role)
return false;
- (void) roles_is_member_of(member, ROLERECURSE_MEMBERS, role, &admin_role);
+ (void) roles_is_member_of(member, ROLERECURSE_PRIVS, role, &admin_role);
return OidIsValid(admin_role);
}
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index 46d4f9efe99..f3f9e647c9d 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -218,6 +218,13 @@ DROP ROLE regress_nosuch_recursive;
ERROR: role "regress_nosuch_recursive" does not exist
DROP ROLE regress_nosuch_admin_recursive;
ERROR: role "regress_nosuch_admin_recursive" does not exist
+-- fail, regress_role_admin is admin of regress_createrole, regress_createrole is admin
+-- of regress_plainrole, but regress_role_admin is not admin of regress_plainrole
+DROP ROLE regress_plainrole;
+ERROR: permission denied to drop role
+DETAIL: Only roles with the CREATEROLE attribute and the ADMIN option on role "regress_plainrole" may drop this role.
+-- ok, now regress_role_admin is admin of regress_plainrole
+GRANT regress_createrole TO regress_role_admin WITH INHERIT TRUE;
DROP ROLE regress_plainrole;
-- must revoke privileges before dropping role
REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE;
diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql
index 4491a28a8ae..bbeb86b29cd 100644
--- a/src/test/regress/sql/create_role.sql
+++ b/src/test/regress/sql/create_role.sql
@@ -174,6 +174,12 @@ DROP ROLE regress_nosuch_super;
DROP ROLE regress_nosuch_dbowner;
DROP ROLE regress_nosuch_recursive;
DROP ROLE regress_nosuch_admin_recursive;
+
+-- fail, regress_role_admin is admin of regress_createrole, regress_createrole is admin
+-- of regress_plainrole, but regress_role_admin is not admin of regress_plainrole
+DROP ROLE regress_plainrole;
+-- ok, now regress_role_admin is admin of regress_plainrole
+GRANT regress_createrole TO regress_role_admin WITH INHERIT TRUE;
DROP ROLE regress_plainrole;
-- must revoke privileges before dropping role
--
2.34.1
On Nov 18, 2025, at 16:41, cca5507 <cca5507@qq.com> wrote:
Hi,
When reading the code, I find is_admin_of_role() use ROLERECURSE_MEMBERS while select_best_admin() use ROLERECURSE_PRIVS.
Why they are dismatch?
The following case will have is_admin_of_role() return true and select_best_admin() return InvalidOid:
create user u1;
create user u2;
create user u3;
create user u4;
grant u2 to u1 with admin true ;
grant u3 to u2 with admin true ;
revoke inherit option for u2 from u1 ;
set session authorization u1;
grant u3 to u4;The "grant u3 to u4;" will report error "no possible grantors" rather than "permission denied to grant role".
Is this the expected behavior?
Let’s do a simpler test:
```
create user u1;
create user u2;
create user u3;
set session authorization u1;
grant u2 to u3;
```
In this test, u1 doesn’t administer u2, so when u1 runs “grant u2 to u3”, the error is “permission denied to grant role u2”.
Then back to ChangAo’s test, after revoking u2 from u1, u1 no longer can administer u3, so that when u1 runs “grant u2 to u3”, the error should also be “permission denied”. From this perspective, the current error “no possible grantors” is unexpected.
Reviewing v2, overall LGTM, my only nitpick is:
```
+-- ok, now regress_role_admin is admin of regress_plainrole
```
In this test comment, “now” is not needed. I think “now” is just from this patch’s perspective, but in the scope of the test script, this test case is just one test step. None of other comments in the same file have wordings of “now”, “then” or so.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi,
Update some comments in v3.
(CC Pretham, we discuss it in [1]/messages/by-id/CAJUn_kN+Mhbb8fYP5xxQCq1KEziOinM6HgYx4ts_pPDnQ2y1nQ@mail.gmail.com)
[1]: /messages/by-id/CAJUn_kN+Mhbb8fYP5xxQCq1KEziOinM6HgYx4ts_pPDnQ2y1nQ@mail.gmail.com
--
Regards,
ChangAo Chen
Attachments:
v3-0001-Use-ROLERECURSE_PRIVS-in-is_admin_of_role.patchapplication/octet-stream; charset=utf-8; name=v3-0001-Use-ROLERECURSE_PRIVS-in-is_admin_of_role.patchDownload
From aa2f2cd51d3d95708fe56167064ca697c7e39970 Mon Sep 17 00:00:00 2001
From: ChangAo Chen <cca5507@qq.com>
Date: Tue, 23 Dec 2025 10:59:28 +0800
Subject: [PATCH v3] Use ROLERECURSE_PRIVS in is_admin_of_role().
Currently we use different role recurse methods in is_admin_of_role()
and select_best_admin(), this could lead to an unexpected behavior.
For example, when is_admin_of_role() return true, select_best_admin()
can still return InvalidOid. To fix it, we use ROLERECURSE_PRIVS in
is_admin_of_role(), making it consistent with select_best_admin().
---
src/backend/utils/adt/acl.c | 2 +-
src/test/regress/expected/create_role.out | 8 ++++++++
src/test/regress/sql/create_role.sql | 7 +++++++
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 05d48412f82..d22e696a46a 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -5422,7 +5422,7 @@ is_admin_of_role(Oid member, Oid role)
if (member == role)
return false;
- (void) roles_is_member_of(member, ROLERECURSE_MEMBERS, role, &admin_role);
+ (void) roles_is_member_of(member, ROLERECURSE_PRIVS, role, &admin_role);
return OidIsValid(admin_role);
}
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index 46d4f9efe99..fe36586e49e 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -218,6 +218,14 @@ DROP ROLE regress_nosuch_recursive;
ERROR: role "regress_nosuch_recursive" does not exist
DROP ROLE regress_nosuch_admin_recursive;
ERROR: role "regress_nosuch_admin_recursive" does not exist
+-- fail, regress_role_admin is not admin of regress_plainrole
+DROP ROLE regress_plainrole;
+ERROR: permission denied to drop role
+DETAIL: Only roles with the CREATEROLE attribute and the ADMIN option on role "regress_plainrole" may drop this role.
+-- ok, regress_role_admin is admin of regress_createrole
+GRANT regress_createrole TO regress_role_admin WITH INHERIT TRUE;
+-- ok, regress_createrole is admin of regress_plainrole and regress_role_admin
+-- inherits permissions from regress_createrole
DROP ROLE regress_plainrole;
-- must revoke privileges before dropping role
REVOKE CREATE ON DATABASE regression FROM regress_createrole CASCADE;
diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql
index 4491a28a8ae..9247a90b4b4 100644
--- a/src/test/regress/sql/create_role.sql
+++ b/src/test/regress/sql/create_role.sql
@@ -174,6 +174,13 @@ DROP ROLE regress_nosuch_super;
DROP ROLE regress_nosuch_dbowner;
DROP ROLE regress_nosuch_recursive;
DROP ROLE regress_nosuch_admin_recursive;
+
+-- fail, regress_role_admin is not admin of regress_plainrole
+DROP ROLE regress_plainrole;
+-- ok, regress_role_admin is admin of regress_createrole
+GRANT regress_createrole TO regress_role_admin WITH INHERIT TRUE;
+-- ok, regress_createrole is admin of regress_plainrole and regress_role_admin
+-- inherits permissions from regress_createrole
DROP ROLE regress_plainrole;
-- must revoke privileges before dropping role
--
2.34.1
Hi,
the "permission denied" error does make sense from a user perspective as it
details that.only roles with admin option for u2 can grant it. the patch
also LGTM.
Regards,
Pretham
On Tue, Dec 23, 2025 at 11:52 AM cca5507 <cca5507@qq.com> wrote:
Show quoted text
Hi,
Update some comments in v3.
(CC Pretham, we discuss it in [1])
[1]:
/messages/by-id/CAJUn_kN+Mhbb8fYP5xxQCq1KEziOinM6HgYx4ts_pPDnQ2y1nQ@mail.gmail.com--
Regards,
ChangAo Chen