Daniel Gustafsson <daniel@yesql.se> writes:
> On 27 Dec 2021, at 17:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A superuser, or really anyone who's a member of the user1 role,
>> ought to be able to do that (especially since it used to be allowed).
>> So it seems the permissions check was coded incorrectly.
> Reading the SQL spec for GRANT and REVOKE, and specifically the "Grantor
> Determination" subsection, it's not clear to me that this is wrong *per spec*
> and that any value except CURRENT_USER and CURRENT_ROLE is supported (which is
> what 6aaaa76bb implemented and the above referenced commit amended). Given the
> time of day I'm undercaffeinated for spec reading so I might be missing
> something though. Is <grantor> really handled differently for GRANT/REVOKE
> ROLE to PRIVILEGE?
Not sure. However, it is certainly true that we have long supported
other grantors in GRANT <role> ... GRANTED BY (at least back to 8.4,
I didn't try anything older). Whether that's within the spec or an
extension, breaking the case now isn't okay.
So the code change in b2a459edf is flat wrong AFAICS. The fact that
it didn't break any existing test cases says we've got a testing gap,
which probably ought to be filled.
ISTM that 6aaaa76bb left a lot on the table too; would it have been
that hard to support other roles in the cases it added, given
that (at least most of) the ACL infrastructure is there? I do agree
with the idea that GRANT <role> and GRANT <privilege> ought to
behave the same on this point --- but we have to do that by adding
capability, not subtracting it.
There do seem to be some bugs/limitations in the GRANT <role> case.
Trying this in v13:
regression=# create user user1;
CREATE ROLE
regression=# create user user2;
CREATE ROLE
regression=# create user user3;
CREATE ROLE
regression=# grant user3 to user2 with admin option;
GRANT ROLE
regression=# \c - user2
You are now connected to database "regression" as user "user2".
regression=> grant user3 to user1 granted by postgres;
ERROR: must be superuser to set grantor
regression=> grant user3 to user1 granted by user3;
ERROR: must have admin option on role "user3"
regression=> grant user3 to user1 granted by user2;
GRANT ROLE
So the "must be superuser" message is pretty misleading, it should
be more like "must be superuser to set grantor to another role".
And the second error is flat out wrong, since user2 *does* have that
admin option. Both of those errors are coming out of AddRoleMems,
but I didn't look at the code in detail.
regards, tom lane