> On 27 Dec 2021, at 23:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> 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.
Agreed.
> 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.
I'll craft a revert of b2a459edf when I'm less tired, a proper fix can then be
worked on separately.
--
Daniel Gustafsson https://vmware.com/