v6-0003-Give-role-owners-control-over-owned-roles.patch

From 0b670278d7bc64641e6f8b77f9506705d92ceb02 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Tue, 18 Jan 2022 10:18:12 -0800
Subject: [PATCH v6 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.
---
 src/backend/catalog/aclchk.c                  |  49 +-------
 src/backend/catalog/objectaddress.c           |  22 +---
 src/backend/commands/schemacmds.c             |   2 +-
 src/backend/commands/user.c                   |  29 +++--
 src/backend/utils/adt/acl.c                   | 118 ++++++++++++++++++
 src/include/utils/acl.h                       |   1 +
 .../expected/dummy_seclabel.out               |  12 +-
 .../dummy_seclabel/sql/dummy_seclabel.sql     |  12 +-
 src/test/regress/expected/create_role.out     |  68 ++++------
 src/test/regress/sql/create_role.sql          |  44 +++----
 10 files changed, 205 insertions(+), 152 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index e5c7324340..1e0ee503e4 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -5440,61 +5440,16 @@ pg_statistics_object_ownercheck(Oid stat_oid, Oid roleid)
 bool
 pg_role_ownercheck(Oid owned_role_oid, Oid owner_roleid)
 {
-	HeapTuple		tuple;
-	Form_pg_authid	authform;
-	Oid				owner_oid;
-
 	/* Superusers bypass all permission checking. */
 	if (superuser_arg(owner_roleid))
 		return true;
 
-	/* Otherwise, look up the owner of the role */
-	tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(owned_role_oid));
-	if (!HeapTupleIsValid(tuple))
-		ereport(ERROR,
-				(errcode(ERRCODE_UNDEFINED_OBJECT),
-				 errmsg("role with OID %u does not exist",
-						owned_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 == owner_roleid);
+	/* Otherwise, check the role ownership hierarchy */
+	return is_owner_of_role_nosuper(owner_roleid, owned_role_oid);
 }
 
 /*
  * 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.)
  */
 bool
 has_createrole_privilege(Oid roleid)
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index f30c742d48..1a47f28829 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2596,25 +2596,9 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
 							   NameListToString(castNode(List, object)));
 			break;
 		case OBJECT_ROLE:
-
-			/*
-			 * We treat roles as being "owned" by those with CREATEROLE priv,
-			 * except that superusers are only owned by superusers.
-			 */
-			if (superuser_arg(address.objectId))
-			{
-				if (!superuser_arg(roleid))
-					ereport(ERROR,
-							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-							 errmsg("must be superuser")));
-			}
-			else
-			{
-				if (!has_createrole_privilege(roleid))
-					ereport(ERROR,
-							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-							 errmsg("must have CREATEROLE privilege")));
-			}
+			if (!pg_role_ownercheck(address.objectId, roleid))
+				aclcheck_error(ACLCHECK_NOT_OWNER, objtype,
+							   strVal(object));
 			break;
 		case OBJECT_TSPARSER:
 		case OBJECT_TSTEMPLATE:
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index 984000a5bc..e5405db658 100644
--- 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
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 3903791ff0..3a24934679 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -693,7 +693,8 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 	{
 		/* check the rest */
 		if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
-			drolemembers || dvalidUntil || !dpassword || roleid != GetUserId())
+			drolemembers || dvalidUntil || !dpassword ||
+			!pg_role_ownercheck(roleid, GetUserId()))
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("permission denied")));
@@ -898,7 +899,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")));
@@ -950,11 +952,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.
@@ -1026,6 +1023,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);
 
@@ -1784,6 +1787,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
index 0a16f8156c..97336db058 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -4832,6 +4832,111 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
 }
 
 
+/*
+ * 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;
+}
+
 /*
  * Does member have the privileges of role (directly or indirectly)?
  *
@@ -4850,6 +4955,10 @@ has_privs_of_role(Oid member, Oid role)
 	if (superuser_arg(member))
 		return true;
 
+	/* Owners of roles have every privilege the owned role has */
+	if (pg_role_ownercheck(role, member))
+		return true;
+
 	/*
 	 * Find all the roles that member has the privileges of, including
 	 * multi-level recursion, then see if target role is any one of them.
@@ -4921,6 +5030,15 @@ is_member_of_role_nosuper(Oid member, Oid role)
 						   role);
 }
 
+/*
+ * 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);
+}
 
 /*
  * Is member an admin of role?	That is, is member the role itself (subject to
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 42f85d0e84..572cae0f27 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -209,6 +209,7 @@ extern bool has_privs_of_role(Oid member, Oid role);
 extern bool is_member_of_role(Oid member, Oid role);
 extern bool is_member_of_role_nosuper(Oid member, Oid role);
 extern bool is_admin_of_role(Oid member, Oid role);
+extern bool is_owner_of_role_nosuper(Oid owner, Oid role);
 extern void check_is_member_of_role(Oid member, Oid role);
 extern Oid	get_role_oid(const char *rolename, bool missing_ok);
 extern Oid	get_role_oid_or_public(const char *rolename);
diff --git a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
index b2d898a7d1..93cf82b750 100644
--- a/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
+++ b/src/test/modules/dummy_seclabel/expected/dummy_seclabel.out
@@ -7,8 +7,11 @@ SET client_min_messages TO 'warning';
 DROP ROLE IF EXISTS regress_dummy_seclabel_user1;
 DROP ROLE IF EXISTS regress_dummy_seclabel_user2;
 RESET client_min_messages;
-CREATE USER regress_dummy_seclabel_user1 WITH CREATEROLE;
+CREATE USER regress_dummy_seclabel_user0 WITH CREATEROLE;
+SET SESSION AUTHORIZATION regress_dummy_seclabel_user0;
+CREATE USER regress_dummy_seclabel_user1;
 CREATE USER regress_dummy_seclabel_user2;
+RESET SESSION AUTHORIZATION;
 CREATE TABLE dummy_seclabel_tbl1 (a int, b text);
 CREATE TABLE dummy_seclabel_tbl2 (x int, y text);
 CREATE VIEW dummy_seclabel_view1 AS SELECT * FROM dummy_seclabel_tbl2;
@@ -19,7 +22,7 @@ ALTER TABLE dummy_seclabel_tbl2 OWNER TO regress_dummy_seclabel_user2;
 --
 -- Test of SECURITY LABEL statement with a plugin
 --
-SET SESSION AUTHORIZATION regress_dummy_seclabel_user1;
+SET SESSION AUTHORIZATION regress_dummy_seclabel_user0;
 SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS 'classified';			-- OK
 SECURITY LABEL ON COLUMN dummy_seclabel_tbl1.a IS 'unclassified';		-- OK
 SECURITY LABEL ON COLUMN dummy_seclabel_tbl1 IS 'unclassified';	-- fail
@@ -29,6 +32,7 @@ ERROR:  '...invalid label...' is not a valid security label
 SECURITY LABEL FOR 'dummy' ON TABLE dummy_seclabel_tbl1 IS 'unclassified';	-- OK
 SECURITY LABEL FOR 'unknown_seclabel' ON TABLE dummy_seclabel_tbl1 IS 'classified';	-- fail
 ERROR:  security label provider "unknown_seclabel" is not loaded
+SET SESSION AUTHORIZATION regress_dummy_seclabel_user1;
 SECURITY LABEL ON TABLE dummy_seclabel_tbl2 IS 'unclassified';	-- fail (not owner)
 ERROR:  must be owner of table dummy_seclabel_tbl2
 SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS 'secret';		-- fail (not superuser)
@@ -42,7 +46,7 @@ SECURITY LABEL ON TABLE dummy_seclabel_tbl2 IS 'classified';			-- OK
 --
 -- Test for shared database object
 --
-SET SESSION AUTHORIZATION regress_dummy_seclabel_user1;
+SET SESSION AUTHORIZATION regress_dummy_seclabel_user0;
 SECURITY LABEL ON ROLE regress_dummy_seclabel_user1 IS 'classified';			-- OK
 SECURITY LABEL ON ROLE regress_dummy_seclabel_user1 IS '...invalid label...';	-- fail
 ERROR:  '...invalid label...' is not a valid security label
@@ -55,7 +59,7 @@ SECURITY LABEL ON ROLE regress_dummy_seclabel_user3 IS 'unclassified';	-- fail (
 ERROR:  role "regress_dummy_seclabel_user3" does not exist
 SET SESSION AUTHORIZATION regress_dummy_seclabel_user2;
 SECURITY LABEL ON ROLE regress_dummy_seclabel_user2 IS 'unclassified';	-- fail (not privileged)
-ERROR:  must have CREATEROLE privilege
+ERROR:  must be owner of role regress_dummy_seclabel_user2
 RESET SESSION AUTHORIZATION;
 --
 -- Test for various types of object
diff --git a/src/test/modules/dummy_seclabel/sql/dummy_seclabel.sql b/src/test/modules/dummy_seclabel/sql/dummy_seclabel.sql
index 8c347b6a68..bf575343cf 100644
--- a/src/test/modules/dummy_seclabel/sql/dummy_seclabel.sql
+++ b/src/test/modules/dummy_seclabel/sql/dummy_seclabel.sql
@@ -11,8 +11,12 @@ DROP ROLE IF EXISTS regress_dummy_seclabel_user2;
 
 RESET client_min_messages;
 
-CREATE USER regress_dummy_seclabel_user1 WITH CREATEROLE;
+CREATE USER regress_dummy_seclabel_user0 WITH CREATEROLE;
+
+SET SESSION AUTHORIZATION regress_dummy_seclabel_user0;
+CREATE USER regress_dummy_seclabel_user1;
 CREATE USER regress_dummy_seclabel_user2;
+RESET SESSION AUTHORIZATION;
 
 CREATE TABLE dummy_seclabel_tbl1 (a int, b text);
 CREATE TABLE dummy_seclabel_tbl2 (x int, y text);
@@ -26,7 +30,7 @@ ALTER TABLE dummy_seclabel_tbl2 OWNER TO regress_dummy_seclabel_user2;
 --
 -- Test of SECURITY LABEL statement with a plugin
 --
-SET SESSION AUTHORIZATION regress_dummy_seclabel_user1;
+SET SESSION AUTHORIZATION regress_dummy_seclabel_user0;
 
 SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS 'classified';			-- OK
 SECURITY LABEL ON COLUMN dummy_seclabel_tbl1.a IS 'unclassified';		-- OK
@@ -34,6 +38,8 @@ SECURITY LABEL ON COLUMN dummy_seclabel_tbl1 IS 'unclassified';	-- fail
 SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS '...invalid label...';	-- fail
 SECURITY LABEL FOR 'dummy' ON TABLE dummy_seclabel_tbl1 IS 'unclassified';	-- OK
 SECURITY LABEL FOR 'unknown_seclabel' ON TABLE dummy_seclabel_tbl1 IS 'classified';	-- fail
+
+SET SESSION AUTHORIZATION regress_dummy_seclabel_user1;
 SECURITY LABEL ON TABLE dummy_seclabel_tbl2 IS 'unclassified';	-- fail (not owner)
 SECURITY LABEL ON TABLE dummy_seclabel_tbl1 IS 'secret';		-- fail (not superuser)
 SECURITY LABEL ON TABLE dummy_seclabel_tbl3 IS 'unclassified';	-- fail (not found)
@@ -45,7 +51,7 @@ SECURITY LABEL ON TABLE dummy_seclabel_tbl2 IS 'classified';			-- OK
 --
 -- Test for shared database object
 --
-SET SESSION AUTHORIZATION regress_dummy_seclabel_user1;
+SET SESSION AUTHORIZATION regress_dummy_seclabel_user0;
 
 SECURITY LABEL ON ROLE regress_dummy_seclabel_user1 IS 'classified';			-- OK
 SECURITY LABEL ON ROLE regress_dummy_seclabel_user1 IS '...invalid label...';	-- fail
diff --git a/src/test/regress/expected/create_role.out b/src/test/regress/expected/create_role.out
index 66d4c29cf7..2c42c8ad8d 100644
--- a/src/test/regress/expected/create_role.out
+++ b/src/test/regress/expected/create_role.out
@@ -58,8 +58,9 @@ CREATE ROLE regress_inherit INHERIT;
 CREATE ROLE regress_connection_limit CONNECTION LIMIT 5;
 CREATE ROLE regress_encrypted_password PASSWORD NULL;
 CREATE ROLE regress_password_null
-  CREATEDB CREATEROLE LOGIN INHERIT CONNECTION LIMIT 2 ENCRYPTED PASSWORD 'foo'
-  IN ROLE regress_createdb, regress_createrole, regress_login;
+  CREATEDB CREATEROLE INHERIT CONNECTION LIMIT 2 ENCRYPTED PASSWORD 'foo'
+  IN ROLE regress_createdb, regress_createrole;
+COMMENT ON ROLE regress_password_null IS 'no login test role';
 \du+ regress_createdb
                                        List of roles
     Role name     |       Owner        |       Attributes        | Member of | Description 
@@ -98,11 +99,11 @@ CREATE ROLE regress_password_null
  regress_encrypted_password | regress_role_admin | Cannot login | {}        | 
 
 \du+ regress_password_null
-                                                              List of roles
-       Role name       |       Owner        |       Attributes       |                      Member of                      | Description 
------------------------+--------------------+------------------------+-----------------------------------------------------+-------------
- regress_password_null | regress_role_admin | Create role, Create DB+| {regress_createdb,regress_createrole,regress_login} | 
-                       |                    | 2 connections          |                                                     | 
+                                                                 List of roles
+       Role name       |       Owner        |              Attributes              |               Member of               |    Description     
+-----------------------+--------------------+--------------------------------------+---------------------------------------+--------------------
+ regress_password_null | regress_role_admin | Create role, Create DB, Cannot login+| {regress_createdb,regress_createrole} | no login test role
+                       |                    | 2 connections                        |                                       | 
 
 -- ok, backwards compatible noise words should be ignored
 CREATE ROLE regress_noiseword SYSID 12345;
@@ -143,21 +144,18 @@ CREATE TABLE tenant_table (i integer);
 CREATE INDEX tenant_idx ON tenant_table(i);
 CREATE VIEW tenant_view AS SELECT * FROM pg_catalog.pg_class;
 REVOKE ALL PRIVILEGES ON tenant_table FROM PUBLIC;
--- fail, these objects belonging to regress_tenant
+-- ok, owning role can manage owned role's objects
 SET SESSION AUTHORIZATION regress_createrole;
 DROP INDEX tenant_idx;
-ERROR:  must be owner of index tenant_idx
 ALTER TABLE tenant_table ADD COLUMN t text;
-ERROR:  must be owner of table tenant_table
 DROP TABLE tenant_table;
-ERROR:  must be owner of table tenant_table
+-- fail, not a member of target role
 ALTER VIEW tenant_view OWNER TO regress_role_admin;
-ERROR:  must be owner of view tenant_view
+ERROR:  must be member of role "regress_role_admin"
+-- ok
 DROP VIEW tenant_view;
-ERROR:  must be owner of view tenant_view
--- fail, cannot take ownership of these objects from regress_tenant
+-- ok, can take ownership objects from owned roles
 REASSIGN OWNED BY regress_tenant TO regress_createrole;
-ERROR:  permission denied to reassign objects
 -- ok, having CREATEROLE is enough to create roles in privileged roles
 CREATE ROLE regress_read_all_data IN ROLE pg_read_all_data;
 CREATE ROLE regress_write_all_data IN ROLE pg_write_all_data;
@@ -169,14 +167,14 @@ CREATE ROLE regress_read_server_files IN ROLE pg_read_server_files;
 CREATE ROLE regress_write_server_files IN ROLE pg_write_server_files;
 CREATE ROLE regress_execute_server_program IN ROLE pg_execute_server_program;
 CREATE ROLE regress_signal_backend IN ROLE pg_signal_backend;
--- fail, cannot take ownership of these objects from regress_createrole
-SET SESSION AUTHORIZATION regress_role_admin;
-ALTER ROLE regress_plainrole OWNER TO regress_role_admin;
-ERROR:  must be owner of role regress_plainrole
-REASSIGN OWNED BY regress_plainrole TO regress_role_admin;
-ERROR:  permission denied to reassign objects
--- superuser can do it, though
+-- fail, cannot create ownership cycles
 RESET SESSION AUTHORIZATION;
+REASSIGN OWNED BY regress_role_admin TO regress_tenant;
+ERROR:  role "regress_createrole" may not both own and be owned by role "regress_tenant"
+ALTER ROLE regress_role_admin OWNER TO regress_tenant;
+ERROR:  role "regress_role_admin" may not both own and be owned by role "regress_tenant"
+-- ok, can take ownership from owned roles
+SET SESSION AUTHORIZATION regress_role_admin;
 ALTER ROLE regress_plainrole OWNER TO regress_role_admin;
 REASSIGN OWNED BY regress_plainrole TO regress_role_admin;
 -- ok, superuser roles can drop superuser roles they own
@@ -187,14 +185,6 @@ SET SESSION AUTHORIZATION regress_role_admin;
 DROP ROLE regress_nosuch_replication_bypassrls;
 DROP ROLE regress_nosuch_replication;
 DROP ROLE regress_nosuch_bypassrls;
-DROP ROLE regress_nosuch_super;
-ERROR:  role "regress_nosuch_super" does not exist
-DROP ROLE regress_nosuch_dbowner;
-ERROR:  role "regress_nosuch_dbowner" does not exist
-DROP ROLE regress_nosuch_recursive;
-ERROR:  role "regress_nosuch_recursive" does not exist
-DROP ROLE regress_nosuch_admin_recursive;
-ERROR:  role "regress_nosuch_admin_recursive" does not exist
 DROP ROLE regress_plainrole;
 -- fail, cannot drop roles that own other roles
 DROP ROLE regress_createrole;
@@ -222,6 +212,7 @@ DROP ROLE regress_noiseword;
 DROP ROLE regress_inroles;
 DROP ROLE regress_adminroles;
 DROP ROLE regress_rolecreator;
+DROP ROLE regress_tenant;
 DROP ROLE regress_read_all_data;
 DROP ROLE regress_write_all_data;
 DROP ROLE regress_monitor;
@@ -232,28 +223,15 @@ DROP ROLE regress_read_server_files;
 DROP ROLE regress_write_server_files;
 DROP ROLE regress_execute_server_program;
 DROP ROLE regress_signal_backend;
--- fail, role still owns database objects
-DROP ROLE regress_tenant;
-ERROR:  role "regress_tenant" cannot be dropped because some objects depend on it
-DETAIL:  owner of table tenant_table
-owner of view tenant_view
--- fail, role still owns other roles
-DROP ROLE regress_createrole;
-ERROR:  role "regress_createrole" cannot be dropped because some objects depend on it
-DETAIL:  owner of role regress_tenant
 -- fail, cannot drop ourself nor superusers
 DROP ROLE regress_role_super;
 ERROR:  must be superuser to drop superusers
 DROP ROLE regress_role_admin;
 ERROR:  current user cannot be dropped
--- ok
-RESET SESSION AUTHORIZATION;
-DROP INDEX tenant_idx;
-DROP TABLE tenant_table;
-DROP VIEW tenant_view;
-DROP ROLE regress_tenant;
+-- ok, no more owned roles remain
 DROP ROLE regress_createrole;
 -- fail, cannot drop role with remaining privileges
+RESET SESSION AUTHORIZATION;
 DROP ROLE regress_role_admin;
 ERROR:  role "regress_role_admin" cannot be dropped because some objects depend on it
 DETAIL:  privileges for database regression
diff --git a/src/test/regress/sql/create_role.sql b/src/test/regress/sql/create_role.sql
index 091b116dd6..00e63b7d93 100644
--- a/src/test/regress/sql/create_role.sql
+++ b/src/test/regress/sql/create_role.sql
@@ -39,8 +39,9 @@ CREATE ROLE regress_inherit INHERIT;
 CREATE ROLE regress_connection_limit CONNECTION LIMIT 5;
 CREATE ROLE regress_encrypted_password PASSWORD NULL;
 CREATE ROLE regress_password_null
-  CREATEDB CREATEROLE LOGIN INHERIT CONNECTION LIMIT 2 ENCRYPTED PASSWORD 'foo'
-  IN ROLE regress_createdb, regress_createrole, regress_login;
+  CREATEDB CREATEROLE INHERIT CONNECTION LIMIT 2 ENCRYPTED PASSWORD 'foo'
+  IN ROLE regress_createdb, regress_createrole;
+COMMENT ON ROLE regress_password_null IS 'no login test role';
 
 \du+ regress_createdb
 \du+ regress_createrole
@@ -95,15 +96,19 @@ CREATE INDEX tenant_idx ON tenant_table(i);
 CREATE VIEW tenant_view AS SELECT * FROM pg_catalog.pg_class;
 REVOKE ALL PRIVILEGES ON tenant_table FROM PUBLIC;
 
--- fail, these objects belonging to regress_tenant
+-- ok, owning role can manage owned role's objects
 SET SESSION AUTHORIZATION regress_createrole;
 DROP INDEX tenant_idx;
 ALTER TABLE tenant_table ADD COLUMN t text;
 DROP TABLE tenant_table;
+
+-- fail, not a member of target role
 ALTER VIEW tenant_view OWNER TO regress_role_admin;
+
+-- ok
 DROP VIEW tenant_view;
 
--- fail, cannot take ownership of these objects from regress_tenant
+-- ok, can take ownership objects from owned roles
 REASSIGN OWNED BY regress_tenant TO regress_createrole;
 
 -- ok, having CREATEROLE is enough to create roles in privileged roles
@@ -118,13 +123,13 @@ CREATE ROLE regress_write_server_files IN ROLE pg_write_server_files;
 CREATE ROLE regress_execute_server_program IN ROLE pg_execute_server_program;
 CREATE ROLE regress_signal_backend IN ROLE pg_signal_backend;
 
--- fail, cannot take ownership of these objects from regress_createrole
-SET SESSION AUTHORIZATION regress_role_admin;
-ALTER ROLE regress_plainrole OWNER TO regress_role_admin;
-REASSIGN OWNED BY regress_plainrole TO regress_role_admin;
-
--- superuser can do it, though
+-- fail, cannot create ownership cycles
 RESET SESSION AUTHORIZATION;
+REASSIGN OWNED BY regress_role_admin TO regress_tenant;
+ALTER ROLE regress_role_admin OWNER TO regress_tenant;
+
+-- ok, can take ownership from owned roles
+SET SESSION AUTHORIZATION regress_role_admin;
 ALTER ROLE regress_plainrole OWNER TO regress_role_admin;
 REASSIGN OWNED BY regress_plainrole TO regress_role_admin;
 
@@ -137,10 +142,6 @@ SET SESSION AUTHORIZATION regress_role_admin;
 DROP ROLE regress_nosuch_replication_bypassrls;
 DROP ROLE regress_nosuch_replication;
 DROP ROLE regress_nosuch_bypassrls;
-DROP ROLE regress_nosuch_super;
-DROP ROLE regress_nosuch_dbowner;
-DROP ROLE regress_nosuch_recursive;
-DROP ROLE regress_nosuch_admin_recursive;
 DROP ROLE regress_plainrole;
 
 -- fail, cannot drop roles that own other roles
@@ -157,6 +158,7 @@ DROP ROLE regress_noiseword;
 DROP ROLE regress_inroles;
 DROP ROLE regress_adminroles;
 DROP ROLE regress_rolecreator;
+DROP ROLE regress_tenant;
 DROP ROLE regress_read_all_data;
 DROP ROLE regress_write_all_data;
 DROP ROLE regress_monitor;
@@ -168,25 +170,15 @@ DROP ROLE regress_write_server_files;
 DROP ROLE regress_execute_server_program;
 DROP ROLE regress_signal_backend;
 
--- fail, role still owns database objects
-DROP ROLE regress_tenant;
-
--- fail, role still owns other roles
-DROP ROLE regress_createrole;
-
 -- fail, cannot drop ourself nor superusers
 DROP ROLE regress_role_super;
 DROP ROLE regress_role_admin;
 
--- ok
-RESET SESSION AUTHORIZATION;
-DROP INDEX tenant_idx;
-DROP TABLE tenant_table;
-DROP VIEW tenant_view;
-DROP ROLE regress_tenant;
+-- ok, no more owned roles remain
 DROP ROLE regress_createrole;
 
 -- fail, cannot drop role with remaining privileges
+RESET SESSION AUTHORIZATION;
 DROP ROLE regress_role_admin;
 
 -- ok, can drop role if we revoke privileges first
-- 
2.21.1 (Apple Git-122.3)