Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Subject: Re: BUG #17346: pg_upgrade fails with role granted by other role
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <1672903.1640645528@sss.pgh.pa.us>
Date: Tue, 28 Dec 2021 00:26:17 +0100
Cc: andrewbille@gmail.com,
 pgsql-bugs@lists.postgresql.org,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <8F7DB6E9-E3A4-411B-AFCC-BF3642ACF309@yesql.se>
References: <17346-f72b28bd1a341060@postgresql.org>
 <1489048.1640620947@sss.pgh.pa.us>
 <9F4DFF73-49E6-4A6D-9D1F-60B1643FB99E@yesql.se>
 <1672903.1640645528@sss.pgh.pa.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/8F7DB6E9-E3A4-411B-AFCC-BF3642ACF309%40yesql.se>
Precedence: bulk

> On 27 Dec 2021, at 23:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>=20
> 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.
>=20
>> 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?
>=20
> 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.
>=20
> 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/