Re: CREATEROLE and role ownership hierarchies

Greetings,

* Mark Dilger (mark(dot)dilger(at)enterprisedb(dot)com) wrote:
> > On Jan 4, 2022, at 12:47 PM, Joshua Brindle <joshua(dot)brindle(at)crunchydata(dot)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 there yet 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 coverage for 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.

> Subject: [PATCH v4 1/5] Add tests of the CREATEROLE attribute.

No particular issue with this one.

> Subject: [PATCH v4 2/5] Add owners to roles
>
> All roles now have owners. By default, roles belong to the role
> that created them, and initdb-time roles are owned by POSTGRES.

... database superuser, not 'POSTGRES'.

> +++ b/src/backend/catalog/aclchk.c
> @@ -5430,6 +5434,57 @@ pg_statistics_object_ownercheck(Oid stat_oid, Oid roleid)
> return has_privs_of_role(roleid, ownerId);
> }
>
> +/*
> + * Ownership check for a role (specified by OID)
> + */
> +bool
> +pg_role_ownercheck(Oid role_oid, Oid roleid)
> +{
> + HeapTuple tuple;
> + Form_pg_authid authform;
> + Oid owner_oid;
> +
> + /* Superusers bypass all permission checking. */
> + if (superuser_arg(roleid))
> + return true;
> +
> + /* Otherwise, look up the owner of the role */
> + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(role_oid));
> + if (!HeapTupleIsValid(tuple))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("role with OID %u does not exist",
> + role_oid)));
> + authform = (Form_pg_authid) GETSTRUCT(tuple);
> + owner_oid = authform->rolowner;
> +
> + /*
> + * Roles must necessarily have owners. Even the bootstrap user has an
> + * owner. (It owns itself). Other roles must form a proper tree.
> + */
> + if (!OidIsValid(owner_oid))
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u has invalid owner",
> + authform->rolname.data, authform->oid)));
> + if (authform->oid != BOOTSTRAP_SUPERUSERID &&
> + authform->rolowner == authform->oid)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u owns itself",
> + authform->rolname.data, authform->oid)));
> + if (authform->oid == BOOTSTRAP_SUPERUSERID &&
> + authform->rolowner != BOOTSTRAP_SUPERUSERID)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u owned by role with OID %u",
> + authform->rolname.data, authform->oid,
> + authform->rolowner)));
> + ReleaseSysCache(tuple);
> +
> + return (owner_oid == roleid);
> +}

Do we really need all of these checks on every call of this function..?
Also, there isn't much point in including the role OID twice in the last
error message, is there? Unless things have gotten quite odd, it's
goint to be the same value both times as we just proved to ourselves
that it is, in fact, the same value (and that it's not the
BOOTSTRAP_SUPERUSERID).

This function also doesn't actually do any kind of checking to see if
the role ownership forms a proper tree, so it seems a bit odd to have
the comment talking about that here where it's doing other checks.

> +++ b/src/backend/commands/user.c
> @@ -77,6 +79,9 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
> Datum new_record[Natts_pg_authid];
> bool new_record_nulls[Natts_pg_authid];
> Oid roleid;
> + Oid owner_uid;
> + Oid saved_uid;
> + int save_sec_context;

Seems a bit odd to introduce 'uid' into this file, which hasn't got any
such anywhere in it, and I'm not entirely sure that any of these are
actually needed..?

> @@ -108,6 +113,16 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
> DefElem *dvalidUntil = NULL;
> DefElem *dbypassRLS = NULL;
>
> + GetUserIdAndSecContext(&saved_uid, &save_sec_context);
> +
> + /*
> + * Who is supposed to own the new role?
> + */
> + if (stmt->authrole)
> + owner_uid = get_rolespec_oid(stmt->authrole, false);
> + else
> + owner_uid = saved_uid;
> +
> /* The defaults can vary depending on the original statement type */
> switch (stmt->stmt_type)
> {
> @@ -254,6 +269,10 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
> ereport(ERROR,
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("must be superuser to create superusers")));
> + if (!superuser_arg(owner_uid))
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("must be superuser to own superusers")));
> }
> else if (isreplication)
> {

So, we're telling a superuser (which is the only way you could get to
this point...) that they aren't allowed to create a superuser role which
is owned by a non-superuser... Why?

> @@ -310,6 +329,19 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
> errmsg("role \"%s\" already exists",
> stmt->role)));
>
> + /*
> + * If the requested authorization is different from the current user,
> + * temporarily set the current user so that the object(s) will be created
> + * with the correct ownership.
> + *
> + * (The setting will be restored at the end of this routine, or in case of
> + * error, transaction abort will clean things up.)
> + */
> + if (saved_uid != owner_uid)
> + SetUserIdAndSecContext(owner_uid,
> + save_sec_context | SECURITY_LOCAL_USERID_CHANGE);

Err, why is this needed? This looks copied from the CreateSchemaCommand
but, unlike with the create schema command, CreateRole doesn't actually
allow sub-commands to be run to create other objects in the way that
CreateSchemaCommand does.

> @@ -478,6 +513,9 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
> */
> table_close(pg_authid_rel, NoLock);
>
> + /* Reset current user and security context */
> + SetUserIdAndSecContext(saved_uid, save_sec_context);
> +
> return roleid;
> }

... ditto with this.

> @@ -1675,3 +1714,110 @@ DelRoleMems(const char *rolename, Oid roleid,
> +static void
> +AlterRoleOwner_internal(HeapTuple tup, Relation rel, Oid newOwnerId)
> +{
> + Form_pg_authid authForm;
> +
> + Assert(tup->t_tableOid == AuthIdRelationId);
> + Assert(RelationGetRelid(rel) == AuthIdRelationId);
> +
> + authForm = (Form_pg_authid) GETSTRUCT(tup);
> +
> + /*
> + * If the new owner is the same as the existing owner, consider the
> + * command to have succeeded. This is for dump restoration purposes.
> + */
> + if (authForm->rolowner != newOwnerId)
> + {
> + /* Otherwise, must be owner of the existing object */
> + if (!pg_role_ownercheck(authForm->oid, GetUserId()))
> + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_ROLE,
> + NameStr(authForm->rolname));
> +
> + /* Must be able to become new owner */
> + check_is_member_of_role(GetUserId(), newOwnerId);

Feels like we should be saying a bit more about why we check for role
membership vs. has_privs_of_role() here. I'm generally of the opinion
that membership is the right thing to check here, just feel like we
should try to explain more why that's the right thing.

> + /*
> + * must have CREATEROLE rights
> + *
> + * NOTE: This is different from most other alter-owner checks in that
> + * the current user is checked for create privileges instead of the
> + * destination owner. This is consistent with the CREATE case for
> + * roles. Because superusers will always have this right, we need no
> + * special case for them.
> + */
> + if (!have_createrole_privilege())
> + aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_ROLE,
> + NameStr(authForm->rolname));
> +

I would think we'd be trying to get away from the role attribute stuff.

> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y

> + CREATE ROLE RoleId AUTHORIZATION RoleSpec opt_with OptRoleList
> + {
> + CreateRoleStmt *n = makeNode(CreateRoleStmt);
> + n->stmt_type = ROLESTMT_ROLE;
> + n->role = $3;
> + n->authrole = $5;
> + n->options = $7;
> + $$ = (Node *)n;
> + }
> ;

...

> @@ -1218,6 +1229,10 @@ CreateOptRoleElem:
> {
> $$ = makeDefElem("addroleto", (Node *)$3, @1);
> }
> + | OWNER RoleSpec
> + {
> + $$ = makeDefElem("owner", (Node *)$2, @1);
> + }
> ;

Not sure why we'd have both AUTHORIZATION and OWNER for CREATE ROLE..?
We don't do that for other objects.

> diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql

> @@ -1,6 +1,7 @@
> -- ok, superuser can create users with any set of privileges
> CREATE ROLE regress_role_super SUPERUSER;
> CREATE ROLE regress_role_1 CREATEDB CREATEROLE REPLICATION BYPASSRLS;
> +GRANT CREATE ON DATABASE regression TO regress_role_1;

Seems odd to add this as part of this patch, or am I missing something?

> From 1784a5b51d4dbebf99798b5832d92b0f585feb08 Mon Sep 17 00:00:00 2001
> From: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>
> Date: Tue, 4 Jan 2022 11:42:27 -0800
> Subject: [PATCH v4 3/5] Give role owners control over owned roles
>
> Create a role ownership hierarchy. The previous commit added owners
> to roles. This goes further, making role ownership transitive. If
> role A owns role B, and role B owns role C, then role A can act as
> the owner of role C. Also, roles A and B can perform any action on
> objects belonging to role C that role C could itself perform.
>
> This is a preparatory patch for changing how CREATEROLE works.

This feels odd to have be an independent commit.

> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
> index ddd205d656..ef36fad700 100644
> --- a/src/backend/catalog/aclchk.c
> +++ b/src/backend/catalog/aclchk.c
> @@ -5440,61 +5440,20 @@ pg_statistics_object_ownercheck(Oid stat_oid, Oid roleid)
> bool
> pg_role_ownercheck(Oid role_oid, Oid roleid)
> {
> - HeapTuple tuple;
> - Form_pg_authid authform;
> - Oid owner_oid;
> -
> /* Superusers bypass all permission checking. */
> if (superuser_arg(roleid))
> return true;
>
> - /* Otherwise, look up the owner of the role */
> - tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(role_oid));
> - if (!HeapTupleIsValid(tuple))
> - ereport(ERROR,
> - (errcode(ERRCODE_UNDEFINED_OBJECT),
> - errmsg("role with OID %u does not exist",
> - role_oid)));
> - authform = (Form_pg_authid) GETSTRUCT(tuple);
> - owner_oid = authform->rolowner;
> -
> - /*
> - * Roles must necessarily have owners. Even the bootstrap user has an
> - * owner. (It owns itself). Other roles must form a proper tree.
> - */
> - if (!OidIsValid(owner_oid))
> - ereport(ERROR,
> - (errcode(ERRCODE_DATA_CORRUPTED),
> - errmsg("role \"%s\" with OID %u has invalid owner",
> - authform->rolname.data, authform->oid)));
> - if (authform->oid != BOOTSTRAP_SUPERUSERID &&
> - authform->rolowner == authform->oid)
> - ereport(ERROR,
> - (errcode(ERRCODE_DATA_CORRUPTED),
> - errmsg("role \"%s\" with OID %u owns itself",
> - authform->rolname.data, authform->oid)));
> - if (authform->oid == BOOTSTRAP_SUPERUSERID &&
> - authform->rolowner != BOOTSTRAP_SUPERUSERID)
> - ereport(ERROR,
> - (errcode(ERRCODE_DATA_CORRUPTED),
> - errmsg("role \"%s\" with OID %u owned by role with OID %u",
> - authform->rolname.data, authform->oid,
> - authform->rolowner)));
> - ReleaseSysCache(tuple);
> -
> - return (owner_oid == roleid);
> + /* Otherwise, check the role ownership hierarchy */
> + return is_owner_of_role_nosuper(roleid, role_oid);
> }

The function being basically entirely rewritten in this patch would be
one reason why it seems an odd split.

> /*
> * Check whether specified role has CREATEROLE privilege (or is a superuser)
> *
> - * Note: roles do not have owners per se; instead we use this test in
> - * places where an ownership-like permissions test is needed for a role.
> - * Be sure to apply it to the role trying to do the operation, not the
> - * role being operated on! Also note that this generally should not be
> - * considered enough privilege if the target role is a superuser.
> - * (We don't handle that consideration here because we want to give a
> - * separate error message for such cases, so the caller has to deal with it.)
> + * 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.
> */
> bool
> has_createrole_privilege(Oid roleid)

Surely this should be in the prior commit, if the split is kept..

> diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c

> @@ -363,7 +363,7 @@ AlterSchemaOwner_internal(HeapTuple tup, Relation rel, Oid newOwnerId)
> /*
> * must have create-schema rights
> *
> - * NOTE: This is different from other alter-owner checks in that the
> + * NOTE: This is different from most other alter-owner checks in that the
> * current user is checked for create privileges instead of the
> * destination owner. This is consistent with the CREATE case for
> * schemas. Because superusers will always have this right, we need

Not a fan of just dropping 'most' in here, doesn't really help someone
understand what is being talked about. I'd suggest adjusting the
comment to talk about alter-owner checks for objects which exist in
schemas, as that's really what is being referred to.

> diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
> index 14820744bf..11d5dffc90 100644
> --- a/src/backend/commands/user.c
> +++ b/src/backend/commands/user.c
> @@ -724,7 +724,7 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
> !rolemembers &&
> !validUntil &&
> dpassword &&
> - roleid == GetUserId()))
> + !pg_role_ownercheck(roleid, GetUserId())))
> ereport(ERROR,
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("permission denied")));
> @@ -925,7 +925,8 @@ AlterRoleSet(AlterRoleSetStmt *stmt)
> }
> else
> {
> - if (!have_createrole_privilege() && roleid != GetUserId())
> + if (!have_createrole_privilege() &&
> + !pg_role_ownercheck(roleid, GetUserId()))
> ereport(ERROR,
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("permission denied")));
> @@ -977,11 +978,6 @@ DropRole(DropRoleStmt *stmt)
> pg_auth_members_rel;
> ListCell *item;
>
> - if (!have_createrole_privilege())
> - ereport(ERROR,
> - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("permission denied to drop role")));
> -
> /*
> * Scan the pg_authid relation to find the Oid of the role(s) to be
> * deleted.
> @@ -1053,6 +1049,12 @@ DropRole(DropRoleStmt *stmt)
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("must be superuser to drop superusers")));
>
> + if (!have_createrole_privilege() &&
> + !pg_role_ownercheck(roleid, GetUserId()))
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("permission denied to drop role")));
> +
> /* DROP hook for the role being removed */
> InvokeObjectDropHook(AuthIdRelationId, roleid, 0);
>
> @@ -1811,6 +1813,18 @@ AlterRoleOwner_internal(HeapTuple tup, Relation rel, Oid newOwnerId)
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("role may not own itself")));
>
> + /*
> + * Must not create cycles in the role ownership hierarchy. If this
> + * role owns (directly or indirectly) the proposed new owner, disallow
> + * the ownership transfer.
> + */
> + if (is_owner_of_role_nosuper(authForm->oid, newOwnerId))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("role \"%s\" may not both own and be owned by role \"%s\"",
> + NameStr(authForm->rolname),
> + GetUserNameFromId(newOwnerId, false))));
> +
> authForm->rolowner = newOwnerId;
> CatalogTupleUpdate(rel, &tup->t_self, tup);

> diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c

> +/*
> + * Get a list of roles which own the given role, directly or indirectly.
> + *
> + * Each role has only one direct owner. The returned list contains the given
> + * role's owner, that role's owner, etc., up to the top of the ownership
> + * hierarchy, which is always the bootstrap superuser.
> + *
> + * Raises an error if any role ownership invariant is violated. Returns NIL if
> + * the given roleid is invalid.
> + */
> +static List *
> +roles_is_owned_by(Oid roleid)
> +{
> + List *owners_list = NIL;
> + Oid role_oid = roleid;
> +
> + /*
> + * Start with the current role and follow the ownership chain upwards until
> + * we reach the bootstrap superuser. To defend against getting into an
> + * infinite loop, we must check for ownership cycles. We choose to perform
> + * other corruption checks on the ownership structure while iterating, too.
> + */
> + while (OidIsValid(role_oid))
> + {
> + HeapTuple tuple;
> + Form_pg_authid authform;
> + Oid owner_oid;
> +
> + /* Find the owner of the current iteration's role */
> + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(role_oid));
> + if (!HeapTupleIsValid(tuple))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("role with OID %u does not exist", role_oid)));
> +
> + authform = (Form_pg_authid) GETSTRUCT(tuple);
> + owner_oid = authform->rolowner;
> +
> + /*
> + * Roles must necessarily have owners. Even the bootstrap user has an
> + * owner. (It owns itself).
> + */
> + if (!OidIsValid(owner_oid))
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u has invalid owner",
> + NameStr(authform->rolname), authform->oid)));
> +
> + /* The bootstrap user must own itself */
> + if (authform->oid == BOOTSTRAP_SUPERUSERID &&
> + owner_oid != BOOTSTRAP_SUPERUSERID)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u owned by role with OID %u",
> + NameStr(authform->rolname), authform->oid,
> + authform->rolowner)));
> +
> + /*
> + * Roles other than the bootstrap user must not be their own direct
> + * owners.
> + */
> + if (authform->oid != BOOTSTRAP_SUPERUSERID &&
> + authform->oid == owner_oid)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u owns itself",
> + NameStr(authform->rolname), authform->oid)));
> +
> + ReleaseSysCache(tuple);
> +
> + /* If we have reached the bootstrap user, we're done. */
> + if (role_oid == BOOTSTRAP_SUPERUSERID)
> + {
> + if (!owners_list)
> + owners_list = lappend_oid(owners_list, owner_oid);
> + break;
> + }
> +
> + /*
> + * For all other users, check they do not own themselves indirectly
> + * through an ownership cycle.
> + *
> + * Scanning the list each time through this loop results in overall
> + * quadratic work in the depth of the ownership chain, but we're
> + * not on a critical performance path, nor do we expect ownership
> + * hierarchies to be deep.
> + */
> + if (owners_list && list_member_oid(owners_list,
> + ObjectIdGetDatum(owner_oid)))
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("role \"%s\" with OID %u indirectly owns itself",
> + GetUserNameFromId(owner_oid, false),
> + owner_oid)));
> +
> + /* Done with sanity checks. Add this owner to the list. */
> + owners_list = lappend_oid(owners_list, owner_oid);
> +
> + /* Otherwise, iterate on this iteration's owner_oid. */
> + role_oid = owner_oid;
> + }
> +
> + return owners_list;
> +}

> @@ -4850,6 +4955,10 @@ has_privs_of_role(Oid member, Oid role)

> + /* Owners of roles have every privilge the owned role has */
> + if (pg_role_ownercheck(role, member))
> + return true;

Whoah, really? No, I don't agree with this, it's throwing away the
entire concept around inheritance of role rights and how you can have
roles which you can get the privileges of by doing a SET ROLE to them
but you don't automatically have those rights.

> +/*
> + * Is owner a direct or indirect owner of the role, not considering
> + * superuserness?
> + */
> +bool
> +is_owner_of_role_nosuper(Oid owner, Oid role)
> +{
> + return list_member_oid(roles_is_owned_by(role), owner);
> +}

Surely if you're a member of a role which owns another role, you should
be considered to be an owner of that role too..? Just checking if the
current role is a member of the roles which directly own the specified
role misses that case.

That is:

CREATE ROLE r1;
CREATE ROLE r2;

GRANT r2 to r1;

CREATE ROLE r3 AUTHORIZATION r2;

Surely, r1 is to be considered an owner of r3 in this case, but the
above check wouldn't consider that to be the case- it would only return
true if the current role is r2.

We do need some kind of direct membership check in the list of owners to
avoid creating loops, so maybe this function is kept as that and the
pg_role_ownership() check is changed to address the above case, but I
don't think we should just ignore role membership when it comes to role
ownership- we don't do that for any other kind of ownership check.

> Subject: [PATCH v4 4/5] Restrict power granted via CREATEROLE.

I would think this would be done independently of the other patches and
probably be first.

> diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml

> @@ -70,18 +70,18 @@ ALTER ROLE { <replaceable class="parameter">role_specification</replaceable> | A
> <link linkend="sql-revoke"><command>REVOKE</command></link> for that.)
> Attributes not mentioned in the command retain their previous settings.
> Database superusers can change any of these settings for any role.
> - Roles having <literal>CREATEROLE</literal> privilege can change any of these
> - settings except <literal>SUPERUSER</literal>, <literal>REPLICATION</literal>,
> - and <literal>BYPASSRLS</literal>; but only for non-superuser and
> - non-replication roles.
> - Ordinary roles can only change their own password.
> + Role owners can change any of these settings on roles they own except
> + <literal>SUPERUSER</literal>, <literal>REPLICATION</literal>, and
> + <literal>BYPASSRLS</literal>; but only for non-superuser and non-replication
> + roles, and only if the role owner does not alter the target role to have a
> + privilege which the role owner itself lacks. Ordinary roles can only change
> + their own password.
> </para>

Having contemplated this a bit more, I don't like it, and it's not how
things work when it comes to regular privileges.

Consider that I can currently GRANT someone UPDATE privileges on an
object, but they can't GRANT that privilege to someone else unless I
explicitly allow it. The same could certainly be said for roles-
perhaps I want to allow someone the privilege to create non-login roles,
but I don't want them to be able to create new login roles, even if they
themselves have LOGIN.

As another point, I might want to have an 'admin' role that I want
admins to SET ROLE to before they go creating other roles, because I
don't want them to be creating roles as their regular user and so that
those other roles are owned by the 'admin' role, but I don't want that
role to have the 'login' attribute.

In other words, we should really consider what role attributes a given
role has to be independent of what role attributes that role is allowed
to set on roles they create. I appreciate that "just whatever the
current role has" is simpler and less work but also will be difficult to
walk back from once it's in the wild.

> @@ -1457,7 +1449,7 @@ AddRoleMems(const char *rolename, Oid roleid,

> /*
> - * Check permissions: must have createrole or admin option on the role to
> + * Check permissions: must be owner or have admin option on the role to
> * be changed. To mess with a superuser role, you gotta be superuser.
> */
> if (superuser_arg(roleid))

...

> @@ -1467,9 +1459,9 @@ AddRoleMems(const char *rolename, Oid roleid,
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> errmsg("must be superuser to alter superusers")));
> }
> - else
> + else if (!superuser())
> {
> - if (!have_createrole_privilege() &&
> + if (!pg_role_ownercheck(roleid, grantorId) &&
> !is_admin_of_role(grantorId, roleid))
> ereport(ERROR,
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),

I'm not entirely sure about including owners here though I'm not
completely against it either. This conflation of what the 'admin'
privileges on a role means vs. the 'ownership' of a role is part of what
I dislike about having two distinct systems for saying who is allowed to
GRANT one role to another.

Also, if we're going to always consider owners to be admins of roles
they own, why not push that into is_admin_of_role()?

> Subject: [PATCH v4 5/5] Remove grantor field from pg_auth_members

While I do think we should fix the issue with dangling references, I
dislike just getting rid of this entirely. While I don't really agree
with the spec about running around DROP'ing objects when a user's
privilege to create those objects has been revoked, I do think we should
be REVOKE'ing rights when a user's right to GRANT has been revoked, and
tracking the information about who GRANT'd what role to what other role
is needed for that. Further, we track who GRANT'd access to what in the
regular ACL system and I don't like the idea of removing that for roles.

> We could fix the bug, but there is no clear solution to the problem
> that existing installations may have broken data. Since the field
> is not used for any purpose, removing it seems the best option.

Existing broken systems will have to eventually be upgraded and the
admin will have to deal with such cases then, so I don't really consider
this to be that big of an issue or reason to entirely remove this.

If we're going to do this, it should also be done independently of the
role ownership stuff too.

Thanks,

Stephen