Make role grant system more consistent with other privileges.

Previously, membership of role A in role B could be recorded in the
catalog tables only once. This meant that a new grant of role A to
role B would overwrite the previous grant. For other object types, a
new grant of permission on an object - in this case role A - exists
along side the existing grant provided that the grantor is different.
Either grant can be revoked independently of the other, and
permissions remain so long as at least one grant remains. Make role
grants work similarly.

Previously, when granting membership in a role, the superuser could
specify any role whatsoever as the grantor, but for other object types,
the grantor of record must be either the owner of the object, or a
role that currently has privileges to perform a similar GRANT.
Implement the same scheme for role grants, treating the bootstrap
superuser as the role owner since roles do not have owners. This means
that attempting to revoke a grant, or admin option on a grant, can now
fail if there are dependent privileges, and that CASCADE can be used
to revoke these. It also means that you can't grant ADMIN OPTION on
a role back to a user who granted it directly or indirectly to you,
similar to how you can't give WITH GRANT OPTION on a privilege back
to a role which granted it directly or indirectly to you.

Previously, only the superuser could specify GRANTED BY with a user
other than the current user. Relax that rule to allow the grantor
to be any role whose privileges the current user posseses. This
doesn't improve compatibility with what we do for other object types,
where support for GRANTED BY is entirely vestigial, but it makes this
feature more usable and seems to make sense to change at the same time
we're changing related behaviors.

Along the way, fix "ALTER GROUP group_name ADD USER user_name" to
require the same privileges as "GRANT group_name TO user_name".
Previously, CREATEROLE privileges were sufficient for either, but
only the former form was permissible with ADMIN OPTION on the role.
Now, either CREATEROLE or ADMIN OPTION on the role suffices for
either spelling.

Patch by me, reviewed by Stephen Frost.

Discussion: http://postgr.es/m/CA+TgmoaFr-RZeQ+WoQ5nKPv97oT9+aDgK_a5+qWHSgbDsMp1Vg@mail.gmail.com

doc/src/sgml/ref/alter_group.sgml

@@ -52,7 +52,11 @@ ALTER GROUP <replaceable class="parameter">group_name</replaceable> RENAME TO <r
    equivalent to granting or revoking membership in the role named as the
    <quote>group</quote>; so the preferred way to do this is to use
    <link linkend="sql-grant"><command>GRANT</command></link> or
-   <link linkend="sql-revoke"><command>REVOKE</command></link>.
+   <link linkend="sql-revoke"><command>REVOKE</command></link>. Note that
+   <command>GRANT</command> and <command>REVOKE</command> have additional
+   options which are not available with this command, such as the ability
+   to grant and revoke <literal>ADMIN OPTION</literal>, and the ability to
+   specify the grantor.
   </para>
 
   <para>

doc/src/sgml/ref/grant.sgml

@@ -267,8 +267,14 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
 
   <para>
    If <literal>GRANTED BY</literal> is specified, the grant is recorded as
-   having been done by the specified role.  Only database superusers may
-   use this option, except when it names the same role executing the command.
+   having been done by the specified role. A user can only attribute a grant
+   to another role if they possess the privileges of that role. The role
+   recorded as the grantor must have <literal>ADMIN OPTION</literal> on the
+   target role, unless it is the bootstrap superuser. When a grant is recorded
+   as having a grantor other than the bootstrap superuser, it depends on the
+   grantor continuing to posess <literal>ADMIN OPTION</literal> on the role;
+   so, if <literal>ADMIN OPTION</literal> is revoked, dependent grants must
+   be revoked as well.
   </para>
 
   <para>
@@ -333,7 +339,7 @@ GRANT <replaceable class="parameter">role_name</replaceable> [, ...] TO <replace
     owner of the affected object.  In particular, privileges granted via
     such a command will appear to have been granted by the object owner.
     (For role membership, the membership appears to have been granted
-    by the containing role itself.)
+    by the bootstrap superuser.)
    </para>
 
    <para>

doc/src/sgml/ref/revoke.sgml

@@ -198,9 +198,10 @@ REVOKE [ ADMIN OPTION FOR ]
   <para>
    When revoking membership in a role, <literal>GRANT OPTION</literal> is instead
    called <literal>ADMIN OPTION</literal>, but the behavior is similar.
-   This form of the command also allows a <literal>GRANTED BY</literal>
-   option, but that option is currently ignored (except for checking
-   the existence of the named role).
+   Note that, in releases prior to <productname>PostgreSQL</productname> 16,
+   dependent privileges were not tracked for grants of role membership,
+   and thus <literal>CASCADE</literal> had no effect for role membership.
+   This is no longer the case.
    Note also that this form of the command does not
    allow the noise word <literal>GROUP</literal>
    in <replaceable class="parameter">role_specification</replaceable>.
@@ -239,7 +240,10 @@ REVOKE [ ADMIN OPTION FOR ]
    <para>
     If a superuser chooses to issue a <command>GRANT</command> or <command>REVOKE</command>
     command, the command is performed as though it were issued by the
-    owner of the affected object.  Since all privileges ultimately come
+    owner of the affected object.  (Since roles do not have owners, in the
+    case of a <command>GRANT</command> of role membership, the command is
+    performed as though it were issued by the bootstrap superuser.)
+    Since all privileges ultimately come
     from the object owner (possibly indirectly via chains of grant options),
     it is possible for a superuser to revoke all privileges, but this might
     require use of <literal>CASCADE</literal> as stated above.

src/backend/commands/user.c

@@ -35,10 +35,32 @@
 #include "storage/lmgr.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
+#include "utils/catcache.h"
 #include "utils/fmgroids.h"
 #include "utils/syscache.h"
 #include "utils/timestamp.h"
 
+/*
+ * Removing a role grant - or the admin option on it - might recurse to
+ * dependent grants. We use these values to reason about what would need to
+ * be done in such cases.
+ *
+ * RRG_NOOP indicates a grant that would not need to be altered by the
+ * operation.
+ *
+ * RRG_REMOVE_ADMIN_OPTION indicates a grant that would need to have
+ * admin_option set to false by the operation.
+ *
+ * RRG_DELETE_GRANT indicates a grant that would need to be removed entirely
+ * by the operation.
+ */
+typedef enum
+{
+	RRG_NOOP,
+	RRG_REMOVE_ADMIN_OPTION,
+	RRG_DELETE_GRANT
+} RevokeRoleGrantAction;
+
 /* Potentially set by pg_upgrade_support functions */
 Oid			binary_upgrade_next_pg_authid_oid = InvalidOid;
 
@@ -54,7 +76,22 @@ static void AddRoleMems(const char *rolename, Oid roleid,
 						Oid grantorId, bool admin_opt);
 static void DelRoleMems(const char *rolename, Oid roleid,
 						List *memberSpecs, List *memberIds,
-						bool admin_opt);
+						Oid grantorId, bool admin_opt, DropBehavior behavior);
+static Oid	check_role_grantor(Oid currentUserId, Oid roleid, Oid grantorId,
+							   bool is_grant);
+static RevokeRoleGrantAction *initialize_revoke_actions(CatCList *memlist);
+static bool plan_single_revoke(CatCList *memlist,
+							   RevokeRoleGrantAction *actions,
+							   Oid member, Oid grantor,
+							   bool revoke_admin_option_only,
+							   DropBehavior behavior);
+static void plan_member_revoke(CatCList *memlist,
+							   RevokeRoleGrantAction *actions, Oid member);
+static void plan_recursive_revoke(CatCList *memlist,
+								  RevokeRoleGrantAction *actions,
+								  int index,
+								  bool revoke_admin_option_only,
+								  DropBehavior behavior);
 
 
 /* Check if current user has createrole privileges */
@@ -449,7 +486,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 			AddRoleMems(oldrolename, oldroleid,
 						thisrole_list,
 						thisrole_oidlist,
-						GetUserId(), false);
+						InvalidOid, false);
 
 			ReleaseSysCache(oldroletup);
 		}
@@ -461,10 +498,10 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 	 */
 	AddRoleMems(stmt->role, roleid,
 				adminmembers, roleSpecsToIds(adminmembers),
-				GetUserId(), true);
+				InvalidOid, true);
 	AddRoleMems(stmt->role, roleid,
 				rolemembers, roleSpecsToIds(rolemembers),
-				GetUserId(), false);
+				InvalidOid, false);
 
 	/* Post creation hook for new role */
 	InvokeObjectPostCreateHook(AuthIdRelationId, roleid, 0);
@@ -624,7 +661,8 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 	 * To mess with a superuser or replication role in any way you gotta be
 	 * superuser.  We also insist on superuser to change the BYPASSRLS
 	 * property.  Otherwise, if you don't have createrole, you're only allowed
-	 * to change your own password.
+	 * to (1) change your own password or (2) add members to a role for which
+	 * you have ADMIN OPTION.
 	 */
 	if (authform->rolsuper || dissuper)
 	{
@@ -649,12 +687,25 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 	}
 	else if (!have_createrole_privilege())
 	{
-		/* check the rest */
+		/* things you certainly can't do without CREATEROLE */
 		if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
-			drolemembers || dvalidUntil || !dpassword || roleid != GetUserId())
+			dvalidUntil)
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("permission denied")));
+
+		/* without CREATEROLE, can only change your own password */
+		if (dpassword && roleid != GetUserId())
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("must have CREATEROLE privilege to change another user's password")));
+
+		/* without CREATEROLE, can only add members to roles you admin */
+		if (drolemembers && !is_admin_of_role(GetUserId(), roleid))
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("must have admin option on role \"%s\" to add members",
+							rolename)));
 	}
 
 	/* Convert validuntil to internal form */
@@ -805,11 +856,11 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
 		if (stmt->action == +1) /* add members to role */
 			AddRoleMems(rolename, roleid,
 						rolemembers, roleSpecsToIds(rolemembers),
-						GetUserId(), false);
+						InvalidOid, false);
 		else if (stmt->action == -1)	/* drop members from role */
 			DelRoleMems(rolename, roleid,
 						rolemembers, roleSpecsToIds(rolemembers),
-						false);
+						InvalidOid, false, DROP_RESTRICT);
 	}
 
 	/*
@@ -1296,7 +1347,7 @@ GrantRole(GrantRoleStmt *stmt)
 	if (stmt->grantor)
 		grantor = get_rolespec_oid(stmt->grantor, false);
 	else
-		grantor = GetUserId();
+		grantor = InvalidOid;
 
 	grantee_ids = roleSpecsToIds(stmt->grantee_roles);
 
@@ -1330,7 +1381,7 @@ GrantRole(GrantRoleStmt *stmt)
 		else
 			DelRoleMems(rolename, roleid,
 						stmt->grantee_roles, grantee_ids,
-						stmt->admin_opt);
+						grantor, stmt->admin_opt, stmt->behavior);
 	}
 
 	/*
@@ -1431,7 +1482,7 @@ roleSpecsToIds(List *memberNames)
  * roleid: OID of role to add to
  * memberSpecs: list of RoleSpec of roles to add (used only for error messages)
  * memberIds: OIDs of roles to add
- * grantorId: who is granting the membership
+ * grantorId: who is granting the membership (InvalidOid if not set explicitly)
  * admin_opt: granting admin option?
  */
 static void
@@ -1443,6 +1494,7 @@ AddRoleMems(const char *rolename, Oid roleid,
 	TupleDesc	pg_authmem_dsc;
 	ListCell   *specitem;
 	ListCell   *iditem;
+	Oid			currentUserId = GetUserId();
 
 	Assert(list_length(memberSpecs) == list_length(memberIds));
 
@@ -1464,7 +1516,7 @@ AddRoleMems(const char *rolename, Oid roleid,
 	else
 	{
 		if (!have_createrole_privilege() &&
-			!is_admin_of_role(grantorId, roleid))
+			!is_admin_of_role(currentUserId, roleid))
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must have admin option on role \"%s\"",
@@ -1483,29 +1535,25 @@ AddRoleMems(const char *rolename, Oid roleid,
 		ereport(ERROR,
 				errmsg("role \"%s\" cannot have explicit members", rolename));
 
-	/*
-	 * The role membership grantor of record has little significance at
-	 * present.  Nonetheless, inasmuch as users might look to it for a crude
-	 * audit trail, let only superusers impute the grant to a third party.
-	 */
-	if (grantorId != GetUserId() && !superuser())
-		ereport(ERROR,
-				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("must be superuser to set grantor")));
+	/* Validate grantor (and resolve implicit grantor if not specified). */
+	grantorId = check_role_grantor(currentUserId, roleid, grantorId, true);
 
 	pg_authmem_rel = table_open(AuthMemRelationId, RowExclusiveLock);
 	pg_authmem_dsc = RelationGetDescr(pg_authmem_rel);
 
+	/*
+	 * Only allow changes to this role by one backend at a time, so that we
+	 * can check integrity constraints like the lack of circular ADMIN OPTION
+	 * grants without fear of race conditions.
+	 */
+	LockSharedObject(AuthIdRelationId, roleid, 0,
+					 ShareUpdateExclusiveLock);
+
+	/* Preliminary sanity checks. */
 	forboth(specitem, memberSpecs, iditem, memberIds)
 	{
 		RoleSpec   *memberRole = lfirst_node(RoleSpec, specitem);
 		Oid			memberid = lfirst_oid(iditem);
-		HeapTuple	authmem_tuple;
-		HeapTuple	tuple;
-		Datum		new_record[Natts_pg_auth_members] = {0};
-		bool		new_record_nulls[Natts_pg_auth_members] = {0};
-		bool		new_record_repl[Natts_pg_auth_members] = {0};
-		Form_pg_auth_members authmem_form;
 
 		/*
 		 * pg_database_owner is never a role member.  Lifting this restriction
@@ -1543,14 +1591,94 @@ AddRoleMems(const char *rolename, Oid roleid,
 					(errcode(ERRCODE_INVALID_GRANT_OPERATION),
 					 errmsg("role \"%s\" is a member of role \"%s\"",
 							rolename, get_rolespec_name(memberRole))));
+	}
+
+	/*
+	 * Disallow attempts to grant ADMIN OPTION back to a user who granted it
+	 * to you, similar to what check_circularity does for ACLs. We want the
+	 * chains of grants to remain acyclic, so that it's always possible to use
+	 * REVOKE .. CASCADE to clean up all grants that depend on the one being
+	 * revoked.
+	 *
+	 * NB: This check might look redundant with the check for membership loops
+	 * above, but it isn't. That's checking for role-member loop (e.g. A is a
+	 * member of B and B is a member of A) while this is checking for a
+	 * member-grantor loop (e.g. A gave ADMIN OPTION on X to B and now B, who
+	 * has no other source of ADMIN OPTION on X, tries to give ADMIN OPTION on
+	 * X back to A).
+	 */
+	if (admin_opt && grantorId != BOOTSTRAP_SUPERUSERID)
+	{
+		CatCList   *memlist;
+		RevokeRoleGrantAction *actions;
+		int			i;
+
+		/* Get the list of members for this role. */
+		memlist = SearchSysCacheList1(AUTHMEMROLEMEM,
+									  ObjectIdGetDatum(roleid));
+
+		/*
+		 * Figure out what would happen if we removed all existing grants to
+		 * every role to which we've been asked to make a new grant.
+		 */
+		actions = initialize_revoke_actions(memlist);
+		foreach(iditem, memberIds)
+		{
+			Oid			memberid = lfirst_oid(iditem);
+
+			if (memberid == BOOTSTRAP_SUPERUSERID)
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+						 errmsg("admin option cannot be granted back to your own grantor")));
+			plan_member_revoke(memlist, actions, memberid);
+		}
+
+		/*
+		 * If the result would be that the grantor role would no longer have
+		 * the ability to perform the grant, then the proposed grant would
+		 * create a circularity.
+		 */
+		for (i = 0; i < memlist->n_members; ++i)
+		{
+			HeapTuple	authmem_tuple;
+			Form_pg_auth_members authmem_form;
+
+			authmem_tuple = &memlist->members[i]->tuple;
+			authmem_form = (Form_pg_auth_members) GETSTRUCT(authmem_tuple);
+
+			if (actions[i] == RRG_NOOP &&
+				authmem_form->member == grantorId &&
+				authmem_form->admin_option)
+				break;
+		}
+		if (i >= memlist->n_members)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_GRANT_OPERATION),
+					 errmsg("admin option cannot be granted back to your own grantor")));
+
+		ReleaseSysCacheList(memlist);
+	}
+
+	/* Now perform the catalog updates. */
+	forboth(specitem, memberSpecs, iditem, memberIds)
+	{
+		RoleSpec   *memberRole = lfirst_node(RoleSpec, specitem);
+		Oid			memberid = lfirst_oid(iditem);
+		HeapTuple	authmem_tuple;
+		HeapTuple	tuple;
+		Datum		new_record[Natts_pg_auth_members] = {0};
+		bool		new_record_nulls[Natts_pg_auth_members] = {0};
+		bool		new_record_repl[Natts_pg_auth_members] = {0};
+		Form_pg_auth_members authmem_form;
 
 		/*
 		 * Check if entry for this role/member already exists; if so, give
 		 * warning unless we are adding admin option.
 		 */
-		authmem_tuple = SearchSysCache2(AUTHMEMROLEMEM,
+		authmem_tuple = SearchSysCache3(AUTHMEMROLEMEM,
 										ObjectIdGetDatum(roleid),
-										ObjectIdGetDatum(memberid));
+										ObjectIdGetDatum(memberid),
+										ObjectIdGetDatum(grantorId));
 		if (!HeapTupleIsValid(authmem_tuple))
 		{
 			authmem_form = NULL;
@@ -1562,8 +1690,9 @@ AddRoleMems(const char *rolename, Oid roleid,
 			if (!admin_opt || authmem_form->admin_option)
 			{
 				ereport(NOTICE,
-						(errmsg("role \"%s\" is already a member of role \"%s\"",
-								get_rolespec_name(memberRole), rolename)));
+						(errmsg("role \"%s\" has already been granted membership in role \"%s\" by role \"%s\"",
+								get_rolespec_name(memberRole), rolename,
+								GetUserNameFromId(grantorId, false))));
 				ReleaseSysCache(authmem_tuple);
 				continue;
 			}
@@ -1577,28 +1706,12 @@ AddRoleMems(const char *rolename, Oid roleid,
 
 		if (HeapTupleIsValid(authmem_tuple))
 		{
-			new_record_repl[Anum_pg_auth_members_grantor - 1] = true;
 			new_record_repl[Anum_pg_auth_members_admin_option - 1] = true;
 			tuple = heap_modify_tuple(authmem_tuple, pg_authmem_dsc,
 									  new_record,
 									  new_record_nulls, new_record_repl);
 			CatalogTupleUpdate(pg_authmem_rel, &tuple->t_self, tuple);
 
-			if (authmem_form->grantor != grantorId)
-			{
-				Oid		   *oldmembers = palloc(sizeof(Oid));
-				Oid		   *newmembers = palloc(sizeof(Oid));
-
-				/* updateAclDependencies wants to pfree array inputs */
-				oldmembers[0] = authmem_form->grantor;
-				newmembers[0] = grantorId;
-
-				updateAclDependencies(AuthMemRelationId, authmem_form->oid,
-									  0, InvalidOid,
-									  1, oldmembers,
-									  1, newmembers);
-			}
-
 			ReleaseSysCache(authmem_tuple);
 		}
 		else
@@ -1637,17 +1750,23 @@ AddRoleMems(const char *rolename, Oid roleid,
  * roleid: OID of role to del from
  * memberSpecs: list of RoleSpec of roles to del (used only for error messages)
  * memberIds: OIDs of roles to del
+ * grantorId: who is revoking the membership
  * admin_opt: remove admin option only?
+ * behavior: RESTRICT or CASCADE behavior for recursive removal
  */
 static void
 DelRoleMems(const char *rolename, Oid roleid,
 			List *memberSpecs, List *memberIds,
-			bool admin_opt)
+			Oid grantorId, bool admin_opt, DropBehavior behavior)
 {
 	Relation	pg_authmem_rel;
 	TupleDesc	pg_authmem_dsc;
 	ListCell   *specitem;
 	ListCell   *iditem;
+	Oid			currentUserId = GetUserId();
+	CatCList   *memlist;
+	RevokeRoleGrantAction *actions;
+	int			i;
 
 	Assert(list_length(memberSpecs) == list_length(memberIds));
 
@@ -1669,40 +1788,69 @@ DelRoleMems(const char *rolename, Oid roleid,
 	else
 	{
 		if (!have_createrole_privilege() &&
-			!is_admin_of_role(GetUserId(), roleid))
+			!is_admin_of_role(currentUserId, roleid))
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("must have admin option on role \"%s\"",
 							rolename)));
 	}
 
+	/* Validate grantor (and resolve implicit grantor if not specified). */
+	grantorId = check_role_grantor(currentUserId, roleid, grantorId, false);
+
 	pg_authmem_rel = table_open(AuthMemRelationId, RowExclusiveLock);
 	pg_authmem_dsc = RelationGetDescr(pg_authmem_rel);
 
+	/*
+	 * Only allow changes to this role by one backend at a time, so that we
+	 * can check for things like dependent privileges without fear of race
+	 * conditions.
+	 */
+	LockSharedObject(AuthIdRelationId, roleid, 0,
+					 ShareUpdateExclusiveLock);
+
+	memlist = SearchSysCacheList1(AUTHMEMROLEMEM, ObjectIdGetDatum(roleid));
+	actions = initialize_revoke_actions(memlist);
+
+	/*
+	 * We may need to recurse to dependent privileges if DROP_CASCADE was
+	 * specified, or refuse to perform the operation if dependent privileges
+	 * exist and DROP_RESTRICT was specified. plan_single_revoke() will figure
+	 * out what to do with each catalog tuple.
+	 */
 	forboth(specitem, memberSpecs, iditem, memberIds)
 	{
 		RoleSpec   *memberRole = lfirst(specitem);
 		Oid			memberid = lfirst_oid(iditem);
-		HeapTuple	authmem_tuple;
-		Form_pg_auth_members authmem_form;
 
-		/*
-		 * Find entry for this role/member
-		 */
-		authmem_tuple = SearchSysCache2(AUTHMEMROLEMEM,
-										ObjectIdGetDatum(roleid),
-										ObjectIdGetDatum(memberid));
-		if (!HeapTupleIsValid(authmem_tuple))
+		if (!plan_single_revoke(memlist, actions, memberid, grantorId,
+								admin_opt, behavior))
 		{
 			ereport(WARNING,
-					(errmsg("role \"%s\" is not a member of role \"%s\"",
-							get_rolespec_name(memberRole), rolename)));
+					(errmsg("role \"%s\" has not been granted membership in role \"%s\" by role \"%s\"",
+							get_rolespec_name(memberRole), rolename,
+							GetUserNameFromId(grantorId, false))));
 			continue;
 		}
+	}
 
+	/*
+	 * We now know what to do with each catalog tuple: it should either be
+	 * left alone, deleted, or just have the admin_option flag cleared.
+	 * Perform the appropriate action in each case.
+	 */
+	for (i = 0; i < memlist->n_members; ++i)
+	{
+		HeapTuple	authmem_tuple;
+		Form_pg_auth_members authmem_form;
+
+		if (actions[i] == RRG_NOOP)
+			continue;
+
+		authmem_tuple = &memlist->members[i]->tuple;
 		authmem_form = (Form_pg_auth_members) GETSTRUCT(authmem_tuple);
 
-		if (!admin_opt)
+		if (actions[i] == RRG_DELETE_GRANT)
 		{
 			/*
 			 * Remove the entry altogether, after first removing its
@@ -1729,15 +1877,298 @@ DelRoleMems(const char *rolename, Oid roleid,
 									  new_record_nulls, new_record_repl);
 			CatalogTupleUpdate(pg_authmem_rel, &tuple->t_self, tuple);
 		}
-
-		ReleaseSysCache(authmem_tuple);
-
-		/* CCI after each change, in case there are duplicates in list */
-		CommandCounterIncrement();
 	}
 
+	ReleaseSysCacheList(memlist);
+
 	/*
 	 * Close pg_authmem, but keep lock till commit.
 	 */
 	table_close(pg_authmem_rel, NoLock);
 }
+
+/*
+ * Sanity-check, or infer, the grantor for a GRANT or REVOKE statement
+ * targeting a role.
+ *
+ * The grantor must always be either a role with ADMIN OPTION on the role in
+ * which membership is being granted, or the bootstrap superuser. This is
+ * similar to the restriction enforced by select_best_grantor, except that
+ * roles don't have owners, so we regard the bootstrap superuser as the
+ * implicit owner.
+ *
+ * If the grantor was not explicitly specified by the user, grantorId should
+ * be passed as InvalidOid, and this function will infer the user to be
+ * recorded as the grantor. In many cases, this will be the current user, but
+ * things get more complicated when the current user doesn't possess ADMIN
+ * OPTION on the role but rather relies on having CREATEROLE privileges, or
+ * on inheriting the privileges of a role which does have ADMIN OPTION. See
+ * below for details.
+ *
+ * If the grantor was specified by the user, then it must be a user that
+ * can legally be recorded as the grantor, as per the rule stated above.
+ * This is an integrity constraint, not a permissions check, and thus even
+ * superusers are subject to this restriction. However, there is also a
+ * permissions check: to specify a role as the grantor, the current user
+ * must possess the privileges of that role. Superusers will always pass
+ * this check, but for non-superusers it may lead to an error.
+ *
+ * The return value is the OID to be regarded as the grantor when executing
+ * the operation.
+ */
+static Oid
+check_role_grantor(Oid currentUserId, Oid roleid, Oid grantorId, bool is_grant)
+{
+	/* If the grantor ID was not specified, pick one to use. */
+	if (!OidIsValid(grantorId))
+	{
+		/*
+		 * Grants where the grantor is recorded as the bootstrap superuser do
+		 * not depend on any other existing grants, so always default to this
+		 * interpretation when possible.
+		 */
+		if (has_createrole_privilege(currentUserId))
+			return BOOTSTRAP_SUPERUSERID;
+
+		/*
+		 * Otherwise, the grantor must either have ADMIN OPTION on the role or
+		 * inherit the privileges of a role which does. In the former case,
+		 * record the grantor as the current user; in the latter, pick one of
+		 * the roles that is "most directly" inherited by the current role
+		 * (i.e. fewest "hops").
+		 *
+		 * (We shouldn't fail to find a best grantor, because we've already
+		 * established that the current user has permission to perform the
+		 * operation.)
+		 */
+		grantorId = select_best_admin(currentUserId, roleid);
+		if (!OidIsValid(grantorId))
+			elog(ERROR, "no possible grantors");
+		return grantorId;
+	}
+
+	/*
+	 * If an explicit grantor is specified, it must be a role whose privileges
+	 * the current user possesses.
+	 *
+	 * It should also be a role that has ADMIN OPTION on the target role, but
+	 * we check this condition only in case of GRANT. For REVOKE, no matching
+	 * grant should exist anyway, but if it somehow does, let the user get rid
+	 * of it.
+	 */
+	if (is_grant)
+	{
+		if (!has_privs_of_role(currentUserId, grantorId))
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("permission denied to grant privileges as role \"%s\"",
+							GetUserNameFromId(grantorId, false))));
+
+		if (grantorId != BOOTSTRAP_SUPERUSERID &&
+			select_best_admin(grantorId, roleid) != grantorId)
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("grantor must have ADMIN OPTION on \"%s\"",
+							GetUserNameFromId(roleid, false))));
+	}
+	else
+	{
+		if (!has_privs_of_role(currentUserId, grantorId))
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("permission denied to revoke privileges granted by role \"%s\"",
+							GetUserNameFromId(grantorId, false))));
+	}
+
+	/*
+	 * If a grantor was specified explicitly, always attribute the grant to
+	 * that role (unless we error out above).
+	 */
+	return grantorId;
+}
+
+/*
+ * Initialize an array of RevokeRoleGrantAction objects.
+ *
+ * 'memlist' should be a list of all grants for the target role.
+ *
+ * This constructs an array indicating that no actions are to be performed;
+ * that is, every element is initially RRG_NOOP.
+ */
+static RevokeRoleGrantAction *
+initialize_revoke_actions(CatCList *memlist)
+{
+	RevokeRoleGrantAction *result;
+	int			i;
+
+	if (memlist->n_members == 0)
+		return NULL;
+
+	result = palloc(sizeof(RevokeRoleGrantAction) * memlist->n_members);
+	for (i = 0; i < memlist->n_members; i++)
+		result[i] = RRG_NOOP;
+	return result;
+}
+
+/*
+ * Figure out what we would need to do in order to revoke a grant, or just the
+ * admin option on a grant, given that there might be dependent privileges.
+ *
+ * 'memlist' should be a list of all grants for the target role.
+ *
+ * Whatever actions prove to be necessary will be signalled by updating
+ * 'actions'.
+ *
+ * If behavior is DROP_RESTRICT, an error will occur if there are dependent
+ * role membership grants; if DROP_CASCADE, those grants will be scheduled
+ * for deletion.
+ *
+ * The return value is true if the matching grant was found in the list,
+ * and false if not.
+ */
+static bool
+plan_single_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
+				   Oid member, Oid grantor, bool revoke_admin_option_only,
+				   DropBehavior behavior)
+{
+	int			i;
+
+	for (i = 0; i < memlist->n_members; ++i)
+	{
+		HeapTuple	authmem_tuple;
+		Form_pg_auth_members authmem_form;
+
+		authmem_tuple = &memlist->members[i]->tuple;
+		authmem_form = (Form_pg_auth_members) GETSTRUCT(authmem_tuple);
+
+		if (authmem_form->member == member &&
+			authmem_form->grantor == grantor)
+		{
+			plan_recursive_revoke(memlist, actions, i,
+								  revoke_admin_option_only, behavior);
+			return true;
+		}
+	}
+
+	return false;
+}
+
+/*
+ * Figure out what we would need to do in order to revoke all grants to
+ * a given member, given that there might be dependent privileges.
+ *
+ * 'memlist' should be a list of all grants for the target role.
+ *
+ * Whatever actions prove to be necessary will be signalled by updating
+ * 'actions'.
+ */
+static void
+plan_member_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
+				   Oid member)
+{
+	int			i;
+
+	for (i = 0; i < memlist->n_members; ++i)
+	{
+		HeapTuple	authmem_tuple;
+		Form_pg_auth_members authmem_form;
+
+		authmem_tuple = &memlist->members[i]->tuple;
+		authmem_form = (Form_pg_auth_members) GETSTRUCT(authmem_tuple);
+
+		if (authmem_form->member == member)
+			plan_recursive_revoke(memlist, actions, i, false, DROP_CASCADE);
+	}
+}
+
+/*
+ * Workhorse for figuring out recursive revocation of role grants.
+ *
+ * This is similar to what recursive_revoke() does for ACLs.
+ */
+static void
+plan_recursive_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
+					  int index,
+					  bool revoke_admin_option_only, DropBehavior behavior)
+{
+	bool		would_still_have_admin_option = false;
+	HeapTuple	authmem_tuple;
+	Form_pg_auth_members authmem_form;
+	int			i;
+
+	/* If it's already been done, we can just return. */
+	if (actions[index] == RRG_DELETE_GRANT)
+		return;
+	if (actions[index] == RRG_REMOVE_ADMIN_OPTION &&
+		revoke_admin_option_only)
+		return;
+
+	/* Locate tuple data. */
+	authmem_tuple = &memlist->members[index]->tuple;
+	authmem_form = (Form_pg_auth_members) GETSTRUCT(authmem_tuple);
+
+	/*
+	 * If the existing tuple does not have admin_option set, then we do not
+	 * need to recurse. If we're just supposed to clear that bit we don't need
+	 * to do anything at all; if we're supposed to remove the grant, we need
+	 * to do something, but only to the tuple, and not any others.
+	 */
+	if (!revoke_admin_option_only)
+	{
+		actions[index] = RRG_DELETE_GRANT;
+		if (!authmem_form->admin_option)
+			return;
+	}
+	else
+	{
+		if (!authmem_form->admin_option)
+			return;
+		actions[index] = RRG_REMOVE_ADMIN_OPTION;
+	}
+
+	/* Determine whether the member would still have ADMIN OPTION. */
+	for (i = 0; i < memlist->n_members; ++i)
+	{
+		HeapTuple	am_cascade_tuple;
+		Form_pg_auth_members am_cascade_form;
+
+		am_cascade_tuple = &memlist->members[i]->tuple;
+		am_cascade_form = (Form_pg_auth_members) GETSTRUCT(am_cascade_tuple);
+
+		if (am_cascade_form->member == authmem_form->member &&
+			am_cascade_form->admin_option && actions[i] == RRG_NOOP)
+		{
+			would_still_have_admin_option = true;
+			break;
+		}
+	}
+
+	/* If the member would still have ADMIN OPTION, we need not recurse. */
+	if (would_still_have_admin_option)
+		return;
+
+	/*
+	 * Recurse to grants that are not yet slated for deletion which have this
+	 * member as the grantor.
+	 */
+	for (i = 0; i < memlist->n_members; ++i)
+	{
+		HeapTuple	am_cascade_tuple;
+		Form_pg_auth_members am_cascade_form;
+
+		am_cascade_tuple = &memlist->members[i]->tuple;
+		am_cascade_form = (Form_pg_auth_members) GETSTRUCT(am_cascade_tuple);
+
+		if (am_cascade_form->grantor == authmem_form->member &&
+			actions[i] != RRG_DELETE_GRANT)
+		{
+			if (behavior == DROP_RESTRICT)
+				ereport(ERROR,
+						(errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+						 errmsg("dependent privileges exist"),
+						 errhint("Use CASCADE to revoke them too.")));
+
+			plan_recursive_revoke(memlist, actions, i, false, behavior);
+		}
+	}
+}

src/backend/parser/gram.y

@@ -7870,6 +7870,7 @@ RevokeRoleStmt:
 					n->admin_opt = false;
 					n->granted_roles = $2;
 					n->grantee_roles = $4;
+					n->grantor = $5;
 					n->behavior = $6;
 					$$ = (Node *) n;
 				}
@@ -7881,6 +7882,7 @@ RevokeRoleStmt:
 					n->admin_opt = true;
 					n->granted_roles = $5;
 					n->grantee_roles = $7;
+					n->grantor = $8;
 					n->behavior = $9;
 					$$ = (Node *) n;
 				}

src/backend/utils/adt/acl.c

@@ -4791,9 +4791,7 @@ has_rolinherit(Oid roleid)
  * Get a list of roles that the specified roleid is a member of
  *
  * Type ROLERECURSE_PRIVS recurses only through roles that have rolinherit
- * set, while ROLERECURSE_MEMBERS recurses through all roles.  This sets
- * *is_admin==true if and only if role "roleid" has an ADMIN OPTION membership
- * in role "admin_of".
+ * set, while ROLERECURSE_MEMBERS recurses through all roles.
  *
  * Since indirect membership testing is relatively expensive, we cache
  * a list of memberships.  Hence, the result is only guaranteed good until
@@ -4801,10 +4799,15 @@ has_rolinherit(Oid roleid)
  *
  * For the benefit of select_best_grantor, the result is defined to be
  * in breadth-first order, ie, closer relationships earlier.
+ *
+ * If admin_of is not InvalidOid, this function sets *admin_role, either
+ * to the OID of the first role in the result list that directly possesses
+ * ADMIN OPTION on the role corresponding to admin_of, or to InvalidOid if
+ * there is no such role.
  */
 static List *
 roles_is_member_of(Oid roleid, enum RoleRecurseType type,
-				   Oid admin_of, bool *is_admin)
+				   Oid admin_of, Oid *admin_role)
 {
 	Oid			dba;
 	List	   *roles_list;
@@ -4812,7 +4815,9 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
 	List	   *new_cached_roles;
 	MemoryContext oldctx;
 
-	Assert(OidIsValid(admin_of) == PointerIsValid(is_admin));
+	Assert(OidIsValid(admin_of) == PointerIsValid(admin_role));
+	if (admin_role != NULL)
+		*admin_role = InvalidOid;
 
 	/* If cache is valid and ADMIN OPTION not sought, just return the list */
 	if (cached_role[type] == roleid && !OidIsValid(admin_of) &&
@@ -4873,8 +4878,8 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
 			 */
 			if (otherid == admin_of &&
 				((Form_pg_auth_members) GETSTRUCT(tup))->admin_option &&
-				OidIsValid(admin_of))
-				*is_admin = true;
+				OidIsValid(admin_of) && !OidIsValid(*admin_role))
+				*admin_role = memberid;
 
 			/*
 			 * Even though there shouldn't be any loops in the membership
@@ -5014,7 +5019,7 @@ is_member_of_role_nosuper(Oid member, Oid role)
 bool
 is_admin_of_role(Oid member, Oid role)
 {
-	bool		result = false;
+	Oid			admin_role;
 
 	if (superuser_arg(member))
 		return true;
@@ -5023,8 +5028,30 @@ is_admin_of_role(Oid member, Oid role)
 	if (member == role)
 		return false;
 
-	(void) roles_is_member_of(member, ROLERECURSE_MEMBERS, role, &result);
-	return result;
+	(void) roles_is_member_of(member, ROLERECURSE_MEMBERS, role, &admin_role);
+	return OidIsValid(admin_role);
+}
+
+/*
+ * Find a role whose privileges "member" inherits which has ADMIN OPTION
+ * on "role", ignoring super-userness.
+ *
+ * There might be more than one such role; prefer one which involves fewer
+ * hops. That is, if member has ADMIN OPTION, prefer that over all other
+ * options; if not, prefer a role from which member inherits more directly
+ * over more indirect inheritance.
+ */
+Oid
+select_best_admin(Oid member, Oid role)
+{
+	Oid			admin_role;
+
+	/* By policy, a role cannot have WITH ADMIN OPTION on itself. */
+	if (member == role)
+		return InvalidOid;
+
+	(void) roles_is_member_of(member, ROLERECURSE_PRIVS, role, &admin_role);
+	return admin_role;
 }
 
 

src/backend/utils/cache/syscache.c

@@ -213,22 +213,22 @@ static const struct cachedesc cacheinfo[] = {
 	},
 	{AuthMemRelationId,			/* AUTHMEMMEMROLE */
 		AuthMemMemRoleIndexId,
-		2,
+		3,
 		{
 			Anum_pg_auth_members_member,
 			Anum_pg_auth_members_roleid,
-			0,
+			Anum_pg_auth_members_grantor,
 			0
 		},
 		8
 	},
 	{AuthMemRelationId,			/* AUTHMEMROLEMEM */
 		AuthMemRoleMemIndexId,
-		2,
+		3,
 		{
 			Anum_pg_auth_members_roleid,
 			Anum_pg_auth_members_member,
-			0,
+			Anum_pg_auth_members_grantor,
 			0
 		},
 		8

src/bin/pg_dump/pg_dumpall.c

@@ -21,6 +21,7 @@
 #include "catalog/pg_authid_d.h"
 #include "common/connect.h"
 #include "common/file_utils.h"
+#include "common/hashfn.h"
 #include "common/logging.h"
 #include "common/string.h"
 #include "dumputils.h"
@@ -31,6 +32,28 @@
 /* version string we expect back from pg_dump */
 #define PGDUMP_VERSIONSTR "pg_dump (PostgreSQL) " PG_VERSION "\n"
 
+static uint32 hash_string_pointer(char *s);
+
+typedef struct
+{
+	uint32		status;
+	uint32		hashval;
+	char	   *rolename;
+} RoleNameEntry;
+
+#define SH_PREFIX	rolename
+#define SH_ELEMENT_TYPE	RoleNameEntry
+#define SH_KEY_TYPE	char *
+#define SH_KEY		rolename
+#define SH_HASH_KEY(tb, key)	hash_string_pointer(key)
+#define SH_EQUAL(tb, a, b)		(strcmp(a, b) == 0)
+#define SH_STORE_HASH
+#define SH_GET_HASH(tb, a)		(a)->hashval
+#define SH_SCOPE	static inline
+#define SH_RAW_ALLOCATOR	pg_malloc0
+#define SH_DECLARE
+#define SH_DEFINE
+#include "lib/simplehash.h"
 
 static void help(void);
 
@@ -925,45 +948,150 @@ dumpRoleMembership(PGconn *conn)
 {
 	PQExpBuffer buf = createPQExpBuffer();
 	PGresult   *res;
-	int			i;
+	int			start = 0,
+				end,
+				total;
+	bool		dump_grantors;
 
-	printfPQExpBuffer(buf, "SELECT ur.rolname AS roleid, "
+	/*
+	 * Previous versions of PostgreSQL didn't used to track the grantor very
+	 * carefully in the backend, and the grantor could be any user even if
+	 * they didn't have ADMIN OPTION on the role, or a user that no longer
+	 * existed. To avoid dump and restore failures, don't dump the grantor
+	 * when talking to an old server version.
+	 */
+	dump_grantors = (PQserverVersion(conn) >= 160000);
+
+	/* Generate and execute query. */
+	printfPQExpBuffer(buf, "SELECT ur.rolname AS role, "
 					  "um.rolname AS member, "
-					  "a.admin_option, "
-					  "ug.rolname AS grantor "
+					  "ug.oid AS grantorid, "
+					  "ug.rolname AS grantor, "
+					  "a.admin_option "
 					  "FROM pg_auth_members a "
 					  "LEFT JOIN %s ur on ur.oid = a.roleid "
 					  "LEFT JOIN %s um on um.oid = a.member "
 					  "LEFT JOIN %s ug on ug.oid = a.grantor "
 					  "WHERE NOT (ur.rolname ~ '^pg_' AND um.rolname ~ '^pg_')"
-					  "ORDER BY 1,2,3", role_catalog, role_catalog, role_catalog);
+					  "ORDER BY 1,2,4", role_catalog, role_catalog, role_catalog);
 	res = executeQuery(conn, buf->data);
 
 	if (PQntuples(res) > 0)
 		fprintf(OPF, "--\n-- Role memberships\n--\n\n");
 
-	for (i = 0; i < PQntuples(res); i++)
+	/*
+	 * We can't dump these GRANT commands in arbitary order, because a role
+	 * that is named as a grantor must already have ADMIN OPTION on the
+	 * role for which it is granting permissions, except for the boostrap
+	 * superuser, who can always be named as the grantor.
+	 *
+	 * We handle this by considering these grants role by role. For each role,
+	 * we initially consider the only allowable grantor to be the boostrap
+	 * superuser. Every time we grant ADMIN OPTION on the role to some user,
+	 * that user also becomes an allowable grantor. We make repeated passes
+	 * over the grants for the role, each time dumping those whose grantors
+	 * are allowable and which we haven't done yet. Eventually this should
+	 * let us dump all the grants.
+	 */
+	total = PQntuples(res);
+	while (start < total)
 	{
-		char	   *roleid = PQgetvalue(res, i, 0);
-		char	   *member = PQgetvalue(res, i, 1);
-		char	   *option = PQgetvalue(res, i, 2);
+		char	   *role = PQgetvalue(res, start, 0);
+		int			i;
+		bool	   *done;
+		int			remaining;
+		int			prev_remaining = 0;
+		rolename_hash *ht;
+
+		/* All memberships for a single role should be adjacent. */
+		for (end = start; end < total; ++end)
+		{
+			char   *otherrole;
+
+			otherrole = PQgetvalue(res, end, 0);
+			if (strcmp(role, otherrole) != 0)
+				break;
+		}
 
-		fprintf(OPF, "GRANT %s", fmtId(roleid));
-		fprintf(OPF, " TO %s", fmtId(member));
-		if (*option == 't')
-			fprintf(OPF, " WITH ADMIN OPTION");
+		role = PQgetvalue(res, start, 0);
+		remaining = end - start;
+		done = pg_malloc0(remaining * sizeof(bool));
+		ht = rolename_create(remaining, NULL);
 
 		/*
-		 * We don't track the grantor very carefully in the backend, so cope
-		 * with the possibility that it has been dropped.
+		 * Make repeated passses over the grants for this role until all have
+		 * been dumped.
 		 */
-		if (!PQgetisnull(res, i, 3))
+		while (remaining > 0)
 		{
-			char	   *grantor = PQgetvalue(res, i, 3);
+			/*
+			 * We should make progress on every iteration, because a notional
+			 * graph whose vertices are grants and whose edges point from
+			 * grantors to members should be connected and acyclic. If we fail
+			 * to make progress, either we or the server have messed up.
+			 */
+			if (remaining == prev_remaining)
+			{
+				pg_log_error("could not find a legal dump ordering for memberships in role \"%s\"",
+							 role);
+				PQfinish(conn);
+				exit_nicely(1);
+			}
 
-			fprintf(OPF, " GRANTED BY %s", fmtId(grantor));
+			/* Make one pass over the grants for this role. */
+			for (i = start; i < end; ++i)
+			{
+				char	   *member;
+				char	   *admin_option;
+				char	   *grantorid;
+				char	   *grantor;
+				bool		found;
+
+				/* If we already did this grant, don't do it again. */
+				if (done[i - start])
+					continue;
+
+				member = PQgetvalue(res, i, 1);
+				grantorid = PQgetvalue(res, i, 2);
+				grantor = PQgetvalue(res, i, 3);
+				admin_option = PQgetvalue(res, i, 4);
+
+				/*
+				 * If we're not dumping grantors or if the grantor is the
+				 * bootstrap superuser, it's fine to dump this now. Otherwise,
+				 * it's got to be someone who has already been granted ADMIN
+				 * OPTION.
+				 */
+				if (dump_grantors &&
+					atooid(grantorid) != BOOTSTRAP_SUPERUSERID &&
+					rolename_lookup(ht, grantor) == NULL)
+					continue;
+
+				/* Remember that we did this so that we don't do it again. */
+				done[i - start] = true;
+				--remaining;
+
+				/*
+				 * If ADMIN OPTION is being granted, remember that grants
+				 * listing this member as the grantor can now be dumped.
+				 */
+				if (*admin_option == 't')
+					rolename_insert(ht, member, &found);
+
+				/* Generate the actual GRANT statement. */
+				fprintf(OPF, "GRANT %s", fmtId(role));
+				fprintf(OPF, " TO %s", fmtId(member));
+				if (*admin_option == 't')
+					fprintf(OPF, " WITH ADMIN OPTION");
+				if (dump_grantors)
+					fprintf(OPF, " GRANTED BY %s", fmtId(grantor));
+				fprintf(OPF, ";\n");
+			}
 		}
-		fprintf(OPF, ";\n");
+
+		rolename_destroy(ht);
+		pg_free(done);
+		start = end;
 	}
 
 	PQclear(res);
@@ -1748,3 +1876,14 @@ dumpTimestamp(const char *msg)
 	if (strftime(buf, sizeof(buf), PGDUMP_STRFTIME_FMT, localtime(&now)) != 0)
 		fprintf(OPF, "-- %s %s\n\n", msg, buf);
 }
+
+/*
+ * Helper function for rolenamehash hash table.
+ */
+static uint32
+hash_string_pointer(char *s)
+{
+	unsigned char *ss = (unsigned char *) s;
+
+	return hash_bytes(ss, strlen(s));
+}

src/include/catalog/catversion.h

@@ -57,6 +57,6 @@
  */
 
 /*							yyyymmddN */
-#define CATALOG_VERSION_NO	202208181
+#define CATALOG_VERSION_NO	202208221
 
 #endif

src/include/catalog/pg_auth_members.h

@@ -44,8 +44,8 @@ CATALOG(pg_auth_members,1261,AuthMemRelationId) BKI_SHARED_RELATION BKI_ROWTYPE_
 typedef FormData_pg_auth_members *Form_pg_auth_members;
 
 DECLARE_UNIQUE_INDEX_PKEY(pg_auth_members_oid_index, 9385, AuthMemOidIndexId, on pg_auth_members using btree(oid oid_ops));
-DECLARE_UNIQUE_INDEX(pg_auth_members_role_member_index, 2694, AuthMemRoleMemIndexId, on pg_auth_members using btree(roleid oid_ops, member oid_ops));
-DECLARE_UNIQUE_INDEX(pg_auth_members_member_role_index, 2695, AuthMemMemRoleIndexId, on pg_auth_members using btree(member oid_ops, roleid oid_ops));
+DECLARE_UNIQUE_INDEX(pg_auth_members_role_member_index, 2694, AuthMemRoleMemIndexId, on pg_auth_members using btree(roleid oid_ops, member oid_ops, grantor oid_ops));
+DECLARE_UNIQUE_INDEX(pg_auth_members_member_role_index, 2695, AuthMemMemRoleIndexId, on pg_auth_members using btree(member oid_ops, roleid oid_ops, grantor oid_ops));
 DECLARE_INDEX(pg_auth_members_grantor_index, 9384, AuthMemGrantorIndexId, on pg_auth_members using btree(grantor oid_ops));
 
 #endif							/* PG_AUTH_MEMBERS_H */

src/include/utils/acl.h

@@ -212,6 +212,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 Oid	select_best_admin(Oid member, 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);

src/test/regress/expected/create_role.out

@@ -103,21 +103,9 @@ 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, can't drop regress_createrole yet, due to outstanding grants
-DROP ROLE regress_createrole;
-ERROR:  role "regress_createrole" cannot be dropped because some objects depend on it
-DETAIL:  privileges for membership of role regress_read_all_data in role pg_read_all_data
-privileges for membership of role regress_write_all_data in role pg_write_all_data
-privileges for membership of role regress_monitor in role pg_monitor
-privileges for membership of role regress_read_all_settings in role pg_read_all_settings
-privileges for membership of role regress_read_all_stats in role pg_read_all_stats
-privileges for membership of role regress_stat_scan_tables in role pg_stat_scan_tables
-privileges for membership of role regress_read_server_files in role pg_read_server_files
-privileges for membership of role regress_write_server_files in role pg_write_server_files
-privileges for membership of role regress_execute_server_program in role pg_execute_server_program
-privileges for membership of role regress_signal_backend in role pg_signal_backend
 -- ok, should be able to drop non-superuser roles we created
 DROP ROLE regress_createdb;
+DROP ROLE regress_createrole;
 DROP ROLE regress_login;
 DROP ROLE regress_inherit;
 DROP ROLE regress_connection_limit;
@@ -137,8 +125,6 @@ DROP ROLE regress_read_server_files;
 DROP ROLE regress_write_server_files;
 DROP ROLE regress_execute_server_program;
 DROP ROLE regress_signal_backend;
--- ok, dropped the other roles first so this is ok now
-DROP ROLE regress_createrole;
 -- fail, role still owns database objects
 DROP ROLE regress_tenant;
 ERROR:  role "regress_tenant" cannot be dropped because some objects depend on it

src/test/regress/expected/privileges.out

@@ -33,6 +33,54 @@ CREATE USER regress_priv_user8;
 CREATE USER regress_priv_user9;
 CREATE USER regress_priv_user10;
 CREATE ROLE regress_priv_role;
+-- circular ADMIN OPTION grants should be disallowed
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
+GRANT regress_priv_user1 TO regress_priv_user3 WITH ADMIN OPTION GRANTED BY regress_priv_user2;
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION GRANTED BY regress_priv_user3;
+ERROR:  admin option cannot be granted back to your own grantor
+-- need CASCADE to revoke grant or admin option if dependent grants exist
+REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2; -- fail
+ERROR:  dependent privileges exist
+HINT:  Use CASCADE to revoke them too.
+REVOKE regress_priv_user1 FROM regress_priv_user2; -- fail
+ERROR:  dependent privileges exist
+HINT:  Use CASCADE to revoke them too.
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+       member       | admin_option 
+--------------------+--------------
+ regress_priv_user2 | t
+ regress_priv_user3 | t
+(2 rows)
+
+BEGIN;
+REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2 CASCADE;
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+       member       | admin_option 
+--------------------+--------------
+ regress_priv_user2 | f
+(1 row)
+
+ROLLBACK;
+REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+ member | admin_option 
+--------+--------------
+(0 rows)
+
+-- inferred grantor must be a role with ADMIN OPTION
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
+GRANT regress_priv_user2 TO regress_priv_user3;
+SET ROLE regress_priv_user3;
+GRANT regress_priv_user1 TO regress_priv_user4;
+SELECT grantor::regrole FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole and member = 'regress_priv_user4'::regrole;
+      grantor       
+--------------------
+ regress_priv_user2
+(1 row)
+
+RESET ROLE;
+REVOKE regress_priv_user2 FROM regress_priv_user3;
+REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
 -- test GRANTED BY with DROP OWNED and REASSIGN OWNED
 GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
 GRANT regress_priv_user1 TO regress_priv_user3 GRANTED BY regress_priv_user2;
@@ -68,15 +116,17 @@ CREATE USER regress_priv_user5;
 GRANT pg_read_all_data TO regress_priv_user6;
 GRANT pg_write_all_data TO regress_priv_user7;
 GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;
+GRANT regress_priv_user9 TO regress_priv_user8;
 SET SESSION AUTHORIZATION regress_priv_user8;
 GRANT pg_read_all_settings TO regress_priv_user9 WITH ADMIN OPTION;
 SET SESSION AUTHORIZATION regress_priv_user9;
 GRANT pg_read_all_settings TO regress_priv_user10;
 SET SESSION AUTHORIZATION regress_priv_user8;
-REVOKE pg_read_all_settings FROM regress_priv_user10;
+REVOKE pg_read_all_settings FROM regress_priv_user10 GRANTED BY regress_priv_user9;
 REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user9;
 REVOKE pg_read_all_settings FROM regress_priv_user9;
 RESET SESSION AUTHORIZATION;
+REVOKE regress_priv_user9 FROM regress_priv_user8;
 REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user8;
 SET SESSION AUTHORIZATION regress_priv_user8;
 SET ROLE pg_read_all_settings;
@@ -87,11 +137,20 @@ DROP USER regress_priv_user10;
 DROP USER regress_priv_user9;
 DROP USER regress_priv_user8;
 CREATE GROUP regress_priv_group1;
-CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
+CREATE GROUP regress_priv_group2 WITH ADMIN regress_priv_user1 USER regress_priv_user2;
 ALTER GROUP regress_priv_group1 ADD USER regress_priv_user4;
+GRANT regress_priv_group2 TO regress_priv_user2 GRANTED BY regress_priv_user1;
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2;
+NOTICE:  role "regress_priv_user2" has already been granted membership in role "regress_priv_group2" by role "regress_priv_user1"
 ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2;	-- duplicate
-NOTICE:  role "regress_priv_user2" is already a member of role "regress_priv_group2"
+NOTICE:  role "regress_priv_user2" has already been granted membership in role "regress_priv_group2" by role "regress_priv_user1"
+ALTER GROUP regress_priv_group2 DROP USER regress_priv_user2;
+ALTER USER regress_priv_user2 PASSWORD 'verysecret'; -- not permitted
+ERROR:  must have CREATEROLE privilege to change another user's password
+RESET SESSION AUTHORIZATION;
 ALTER GROUP regress_priv_group2 DROP USER regress_priv_user2;
+REVOKE ADMIN OPTION FOR regress_priv_group2 FROM regress_priv_user1;
 GRANT regress_priv_group2 TO regress_priv_user4 WITH ADMIN OPTION;
 -- prepare non-leakproof function for later
 CREATE FUNCTION leak(integer,integer) RETURNS boolean
@@ -99,9 +158,13 @@ CREATE FUNCTION leak(integer,integer) RETURNS boolean
   LANGUAGE internal IMMUTABLE STRICT;  -- but deliberately not LEAKPROOF
 ALTER FUNCTION leak(integer,integer) OWNER TO regress_priv_user1;
 -- test owner privileges
+GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY regress_priv_role; -- error, doesn't have ADMIN OPTION
+ERROR:  grantor must have ADMIN OPTION on "regress_priv_role"
 GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY CURRENT_ROLE;
 REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY foo; -- error
-REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY regress_priv_user2; -- error
+ERROR:  role "foo" does not exist
+REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY regress_priv_user2; -- warning, noop
+WARNING:  role "regress_priv_user1" has not been granted membership in role "regress_priv_role" by role "regress_priv_user2"
 REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY CURRENT_USER;
 REVOKE regress_priv_role FROM regress_priv_user1 GRANTED BY CURRENT_ROLE;
 DROP ROLE regress_priv_role;
@@ -1746,7 +1809,7 @@ SET SESSION AUTHORIZATION regress_priv_user1;
 GRANT regress_priv_group2 TO regress_priv_user5; -- fails: no ADMIN OPTION
 ERROR:  must have admin option on role "regress_priv_group2"
 SELECT dogrant_ok();			-- ok: SECURITY DEFINER conveys ADMIN
-NOTICE:  role "regress_priv_user5" is already a member of role "regress_priv_group2"
+NOTICE:  role "regress_priv_user5" has already been granted membership in role "regress_priv_group2" by role "regress_priv_user4"
  dogrant_ok 
 ------------
  

src/test/regress/sql/create_role.sql

@@ -98,11 +98,9 @@ DROP ROLE regress_nosuch_recursive;
 DROP ROLE regress_nosuch_admin_recursive;
 DROP ROLE regress_plainrole;
 
--- fail, can't drop regress_createrole yet, due to outstanding grants
-DROP ROLE regress_createrole;
-
 -- ok, should be able to drop non-superuser roles we created
 DROP ROLE regress_createdb;
+DROP ROLE regress_createrole;
 DROP ROLE regress_login;
 DROP ROLE regress_inherit;
 DROP ROLE regress_connection_limit;
@@ -123,9 +121,6 @@ DROP ROLE regress_write_server_files;
 DROP ROLE regress_execute_server_program;
 DROP ROLE regress_signal_backend;
 
--- ok, dropped the other roles first so this is ok now
-DROP ROLE regress_createrole;
-
 -- fail, role still owns database objects
 DROP ROLE regress_tenant;
 

src/test/regress/sql/privileges.sql

@@ -37,6 +37,32 @@ CREATE USER regress_priv_user9;
 CREATE USER regress_priv_user10;
 CREATE ROLE regress_priv_role;
 
+-- circular ADMIN OPTION grants should be disallowed
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
+GRANT regress_priv_user1 TO regress_priv_user3 WITH ADMIN OPTION GRANTED BY regress_priv_user2;
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION GRANTED BY regress_priv_user3;
+
+-- need CASCADE to revoke grant or admin option if dependent grants exist
+REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2; -- fail
+REVOKE regress_priv_user1 FROM regress_priv_user2; -- fail
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+BEGIN;
+REVOKE ADMIN OPTION FOR regress_priv_user1 FROM regress_priv_user2 CASCADE;
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+ROLLBACK;
+REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
+SELECT member::regrole, admin_option FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole;
+
+-- inferred grantor must be a role with ADMIN OPTION
+GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
+GRANT regress_priv_user2 TO regress_priv_user3;
+SET ROLE regress_priv_user3;
+GRANT regress_priv_user1 TO regress_priv_user4;
+SELECT grantor::regrole FROM pg_auth_members WHERE roleid = 'regress_priv_user1'::regrole and member = 'regress_priv_user4'::regrole;
+RESET ROLE;
+REVOKE regress_priv_user2 FROM regress_priv_user3;
+REVOKE regress_priv_user1 FROM regress_priv_user2 CASCADE;
+
 -- test GRANTED BY with DROP OWNED and REASSIGN OWNED
 GRANT regress_priv_user1 TO regress_priv_user2 WITH ADMIN OPTION;
 GRANT regress_priv_user1 TO regress_priv_user3 GRANTED BY regress_priv_user2;
@@ -67,6 +93,7 @@ CREATE USER regress_priv_user5;
 GRANT pg_read_all_data TO regress_priv_user6;
 GRANT pg_write_all_data TO regress_priv_user7;
 GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;
+GRANT regress_priv_user9 TO regress_priv_user8;
 
 SET SESSION AUTHORIZATION regress_priv_user8;
 GRANT pg_read_all_settings TO regress_priv_user9 WITH ADMIN OPTION;
@@ -75,11 +102,12 @@ SET SESSION AUTHORIZATION regress_priv_user9;
 GRANT pg_read_all_settings TO regress_priv_user10;
 
 SET SESSION AUTHORIZATION regress_priv_user8;
-REVOKE pg_read_all_settings FROM regress_priv_user10;
+REVOKE pg_read_all_settings FROM regress_priv_user10 GRANTED BY regress_priv_user9;
 REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user9;
 REVOKE pg_read_all_settings FROM regress_priv_user9;
 
 RESET SESSION AUTHORIZATION;
+REVOKE regress_priv_user9 FROM regress_priv_user8;
 REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user8;
 
 SET SESSION AUTHORIZATION regress_priv_user8;
@@ -94,12 +122,19 @@ DROP USER regress_priv_user9;
 DROP USER regress_priv_user8;
 
 CREATE GROUP regress_priv_group1;
-CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
+CREATE GROUP regress_priv_group2 WITH ADMIN regress_priv_user1 USER regress_priv_user2;
 
 ALTER GROUP regress_priv_group1 ADD USER regress_priv_user4;
 
+GRANT regress_priv_group2 TO regress_priv_user2 GRANTED BY regress_priv_user1;
+SET SESSION AUTHORIZATION regress_priv_user1;
+ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2;
 ALTER GROUP regress_priv_group2 ADD USER regress_priv_user2;	-- duplicate
 ALTER GROUP regress_priv_group2 DROP USER regress_priv_user2;
+ALTER USER regress_priv_user2 PASSWORD 'verysecret'; -- not permitted
+RESET SESSION AUTHORIZATION;
+ALTER GROUP regress_priv_group2 DROP USER regress_priv_user2;
+REVOKE ADMIN OPTION FOR regress_priv_group2 FROM regress_priv_user1;
 GRANT regress_priv_group2 TO regress_priv_user4 WITH ADMIN OPTION;
 
 -- prepare non-leakproof function for later
@@ -110,9 +145,10 @@ ALTER FUNCTION leak(integer,integer) OWNER TO regress_priv_user1;
 
 -- test owner privileges
 
+GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY regress_priv_role; -- error, doesn't have ADMIN OPTION
 GRANT regress_priv_role TO regress_priv_user1 WITH ADMIN OPTION GRANTED BY CURRENT_ROLE;
 REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY foo; -- error
-REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY regress_priv_user2; -- error
+REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY regress_priv_user2; -- warning, noop
 REVOKE ADMIN OPTION FOR regress_priv_role FROM regress_priv_user1 GRANTED BY CURRENT_USER;
 REVOKE regress_priv_role FROM regress_priv_user1 GRANTED BY CURRENT_ROLE;
 DROP ROLE regress_priv_role;