Re: CREATEROLE and role ownership hierarchies

On 1/5/22 19:05, Mark Dilger wrote:
>
>> On Jan 4, 2022, at 12:47 PM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
>>
>>> I was able to reproduce that using REASSIGN OWNED BY to cause a user to own itself.  Is that how you did it, or is
thereyet another way to get into that state?
 
>> I did:
>> ALTER ROLE brindle OWNER TO brindle;
> Ok, thanks.  I have rebased, fixed both REASSIGN OWNED BY and ALTER ROLE .. OWNER TO cases, and added regression
coveragefor them.
 
>
> The last patch set to contain significant changes was v2, with v3 just being a rebase.  Relative to those sets:
>
> 0001 -- rebased.
> 0002 -- rebased; extend AlterRoleOwner_internal to disallow making a role its own immediate owner.
> 0003 -- rebased; extend AlterRoleOwner_internal to disallow cycles in the role ownership graph.
> 0004 -- rebased.
> 0005 -- new; removes the broken pg_auth_members.grantor field.


In general this looks good. Some nitpicks:


+/*
+ * Ownership check for a role (specified by OID)
+ */
+bool
+pg_role_ownercheck(Oid role_oid, Oid roleid)


This is a bit confusing. Let's rename these params so it's clear which
is the owner and which the owned role.


+ * Note: In versions prior to PostgreSQL version 15, roles did not have
owners
+ * per se; instead we used this test in places where an ownership-like
+ * permissions test was needed for a role.


No need to talk about what we used to do. People who want to know can
look back at older branches.


+bool
+has_rolinherit_privilege(Oid roleid)
+{


This and similar functions should have header comments.


+   /* Owners of roles have every privilge the owned role has */

s/privlge/privilege/


+CREATE ROLE regress_role_1 CREATEDB CREATEROLE REPLICATION BYPASSRLS;


I don't really like this business of just numbering large numbers of
roles in the tests. Let's give them more meaningful names.


+   Role owners can change any of these settings on roles they own except


I would say "on roles they directly or indirectly own", here and
similarly in one or two other places.


...


I will probably do one or two more passes over the patches, but as I say
in general they look fairly good.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com