Why is_admin_of_role() use ROLERECURSE_MEMBERS rather than ROLERECURSE_PRIVS?

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

Hi,

Fix "create_role.sql" in v2.

--
Regards,
ChangAo Chen

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

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:

Added Robert to the thread.

Yeah, something doesn't seem right here. As mentioned upthread, the
following ERROR in check_role_grantor() is easily reached:

* 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");

This was added by commit ce6b672e44, which first took effect in v16. IIUC
earlier versions simply check is_admin_of_role() and mark the grantor as
the current role, at least when adding members. I haven't found any other
e-mails or documentation about this.

Changing is_admin_of_role() to use ROLERECURSE_PRIVS would make things more
restrictive (e.g., the DROP ROLE in the test fails), which has the
potential to break existing scripts. But it does seem intuitive that if
you don't INHERIT a roles privileges, you don't inherit its ADMIN rights
either.

--
nathan

On Mon, Mar 16, 2026 at 5:19 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

This was added by commit ce6b672e44, which first took effect in v16. IIUC
earlier versions simply check is_admin_of_role() and mark the grantor as
the current role, at least when adding members. I haven't found any other
e-mails or documentation about this.

Thanks, Nathan, for pointing me to this thread.

Changing is_admin_of_role() to use ROLERECURSE_PRIVS would make things more
restrictive (e.g., the DROP ROLE in the test fails), which has the
potential to break existing scripts. But it does seem intuitive that if
you don't INHERIT a roles privileges, you don't inherit its ADMIN rights
either.

I've been viewing ROLERECURSE_MEMBERS vs. ROLERECURSE_PRIVS vs.
ROLERECURSE_SETROLE as somewhat orthogonal to ADMIN vs. MEMBER. Look
at pg_role_aclcheck(): it tests is_member_of_role(), which uses
ROLERECURSE_MEMBERS, for ACL_CREATE. So it seems like it would be
surprising if when checking ACL_GRANT_OPTION_FOR(ACL_CREATE), it
switched to using ROLERECURSE_PRIVS. On the other hand,
check_role_grantor() checks has_privs_of_role(), which I strongly
suspect is why select_best_admin(), called just after that check, uses
ROLERECURSE_PRIVS.

But I think my mental model here is actually a bit incoherent for
ADMIN. We get into this problem because
check_role_membership_authorization() decides whether you're allowed
to tinker with a role using is_admin_of_role(), which searches with
ROLERECURSE_MEMBERS, and then we call AddRoleMems() which calls
check_role_grantor() which calls select_best_admin(), which uses a
ROLERECURSE_PRIVS search and it expects that to succeed.

I'm pretty strongly disinclined to change the meaning of
is_admin_of_role() in released code. That affects more than this call
site. When this code was under development, one of the use cases that
was booted was a user management bot who should be able to run ALTER
ROLE but should not automatically exercise the privilege of any
created roles. If we standardize on ROLERECURSE_PRIVS, that use case
doesn't work any more. You now have to inherit a role's privileges or
AlterRole() will fail.

One idea could be that non-membership changes to roles continue to
work as they do today, but membership changes use ROLERECURSE_PRIVS.
So we'd have is_admin_of_role() and inherits_admin_privs_for_role()
and be careful to use the right one in each case. This seems a little
weird, but I'm not sure what would be better.

--
Robert Haas
EDB: http://www.enterprisedb.com

I'm pretty strongly disinclined to change the meaning of
is_admin_of_role() in released code. That affects more than this call
site. When this code was under development, one of the use cases that
was booted was a user management bot who should be able to run ALTER
ROLE but should not automatically exercise the privilege of any
created roles. If we standardize on ROLERECURSE_PRIVS, that use case
doesn't work any more. You now have to inherit a role's privileges or
AlterRole() will fail.

This use case makes sense to me.

One idea could be that non-membership changes to roles continue to
work as they do today, but membership changes use ROLERECURSE_PRIVS.
So we'd have is_admin_of_role() and inherits_admin_privs_for_role()
and be careful to use the right one in each case. This seems a little
weird, but I'm not sure what would be better.

Attach a patch done like this.

--
Regards,
ChangAo Chen

On Wed, Apr 29, 2026 at 04:46:05PM +0800, cca5507 wrote:

I'm pretty strongly disinclined to change the meaning of
is_admin_of_role() in released code. That affects more than this call
site. When this code was under development, one of the use cases that
was booted was a user management bot who should be able to run ALTER
ROLE but should not automatically exercise the privilege of any
created roles. If we standardize on ROLERECURSE_PRIVS, that use case
doesn't work any more. You now have to inherit a role's privileges or
AlterRole() will fail.

This use case makes sense to me.

One idea could be that non-membership changes to roles continue to
work as they do today, but membership changes use ROLERECURSE_PRIVS.
So we'd have is_admin_of_role() and inherits_admin_privs_for_role()
and be careful to use the right one in each case. This seems a little
weird, but I'm not sure what would be better.

Attach a patch done like this.

The patch seems to resolve the reported case. I don't like how the new
function is named "has_admin_option_on_role()" because it sounds like it
means the exact same thing as "is_admin_of_role()". IMHO Robert's
suggestion of inherits_admin_privs_of_role() would be better.

I don't have any better ideas for how to solve it, but I also fear for the
day when I have to explain these subtle differences in behavior to a casual
user...

--
nathan

On Wed, May 6, 2026 at 9:48 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

I don't have any better ideas for how to solve it, but I also fear for the
day when I have to explain these subtle differences in behavior to a casual
user...

(spitballing)

Would it help to name the functions according to what they intend to
let the caller do? is_admin_of_role() -> can_administer_role();
inherits_admin_privs_of_role() -> is_administering_role()?

Alternatively, we have the is_member_* vs has_privs_* division
already, so has_admin_privs_over_role(), or something?

--Jacob

I'm pretty strongly disinclined to change the meaning of
is_admin_of_role() in released code. That affects more than this call
site. When this code was under development, one of the use cases that
was booted was a user management bot who should be able to run ALTER
ROLE but should not automatically exercise the privilege of any
created roles. If we standardize on ROLERECURSE_PRIVS, that use case
doesn't work any more. You now have to inherit a role's privileges or
AlterRole() will fail.

After thinking more about this case, it seems still work even if we use
ROLERECURSE_PRIVS in is_admin_of_role():

```
postgres=# create role bot createrole;
CREATE ROLE
postgres=# set session authorization bot;
SET
postgres=> create role user1;
CREATE ROLE
postgres=> reset session authorization;
RESET
postgres=# select pg_has_role('bot', 'user1', 'USAGE');
pg_has_role
-------------
f
(1 row)

postgres=# select pg_has_role('bot', 'user1', 'MEMBER');
pg_has_role
-------------
t
(1 row)

postgres=# select pg_has_role('bot', 'user1', 'MEMBER WITH ADMIN OPTION');
pg_has_role
-------------
t
(1 row)

postgres=# select * from pg_auth_members where member = (select oid from pg_authid where rolname = 'bot');
oid | roleid | member | grantor | admin_option | inherit_option | set_option
-------+--------+--------+---------+--------------+----------------+------------
16393 | 16392 | 16391 | 10 | t | f | f
(1 row)

```

The bot doesn't have privs of user1 by default, but is admin of user1. Changing
is_admin_of_role() to use ROLERECURSE_PRIVS only affects the case of indirect
inheritance. Do I miss something?

--
Regards,
ChangAo Chen

On Thu, May 07, 2026 at 12:04:44PM +0800, cca5507 wrote:

The bot doesn't have privs of user1 by default, but is admin of user1. Changing
is_admin_of_role() to use ROLERECURSE_PRIVS only affects the case of indirect
inheritance. Do I miss something?

I think that's right. roles_is_member_of() sets *admin_role before
checking inheritance:

if (otherid == admin_of && form->admin_option &&
OidIsValid(admin_of) && !OidIsValid(*admin_role))
*admin_role = memberid;

So direct membership with admin but without inherit/set still allows you to
administrate the role.

At all rates, I think I'm inclined to proceed with something like v4. That
fixes the error message without changing any other behavior, which is
probably about the best we can do on the back-branches. Perhaps we should
consider changing is_admin_of_role() to use ROLERECURSE_PRIVS in v20, but I
don't sense much appetite for that, so... maybe we leave that thread loose
for now.

If nobody objects, I'll take care of committing/back-patching, hopefully in
the near future.

--
nathan

On Fri, Aug 07, 2026 at 04:36:20PM -0500, Nathan Bossart wrote:

At all rates, I think I'm inclined to proceed with something like v4. That
fixes the error message without changing any other behavior, which is
probably about the best we can do on the back-branches. Perhaps we should
consider changing is_admin_of_role() to use ROLERECURSE_PRIVS in v20, but I
don't sense much appetite for that, so... maybe we leave that thread loose
for now.

If nobody objects, I'll take care of committing/back-patching, hopefully in
the near future.

Concretely, like the attached.

--
nathan

Concretely, like the attached.

Thanks for the v5 patch, LGTM!

--
Regards,
ChangAo Chen

Committed.

--
nathan