Add a SET option to the GRANT command.

Similar to how the INHERIT option controls whether or not the
permissions of the granted role are automatically available to the
grantee, the new SET permission controls whether or not the grantee
may use the SET ROLE command to assume the privileges of the granted
role.

In addition, the new SET permission controls whether or not it
is possible to transfer ownership of objects to the target role
or to create new objects owned by the target role using commands
such as CREATE DATABASE .. OWNER. We could alternatively have made
this controlled by the INHERIT option, or allow it when either
option is given. An advantage of this approach is that if you
are granted a predefined role with INHERIT TRUE, SET FALSE, you
can't go and create objects owned by that role.

The underlying theory here is that the ability to create objects
as a target role is not a privilege per se, and thus does not
depend on whether you inherit the target role's privileges. However,
it's surely something you could do anyway if you could SET ROLE
to the target role, and thus making it contingent on whether you
have that ability is reasonable.

Design review by Nathan Bossat, Wolfgang Walther, Jeff Davis,
Peter Eisentraut, and Stephen Frost.

Discussion: http://postgr.es/m/CA+Tgmob+zDSRS6JXYrgq0NWdzCXuTNzT5eK54Dn2hhgt17nm8A@mail.gmail.com

10 files changed, 131 insertions, 39 deletions

diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index b2089d785b6..10b6fe19a2c 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -999,7 +999,7 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
 							   objname);
 			}
 			/* Must be able to become new owner */
-			check_is_member_of_role(GetUserId(), new_ownerId);
+			check_can_set_role(GetUserId(), new_ownerId);
 
 			/* New owner must have CREATE privilege on namespace */
 			if (OidIsValid(namespaceId))
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index a67ea86619c..6eb87427181 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -941,7 +941,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("permission denied to create database")));
 
-	check_is_member_of_role(GetUserId(), datdba);
+	check_can_set_role(GetUserId(), datdba);
 
 	/*
 	 * Lookup database (template) to be cloned, and obtain share lock on it.
@@ -2495,7 +2495,7 @@ AlterDatabaseOwner(const char *dbname, Oid newOwnerId)
 						   dbname);
 
 		/* Must be able to become new owner */
-		check_is_member_of_role(GetUserId(), newOwnerId);
+		check_can_set_role(GetUserId(), newOwnerId);
 
 		/*
 		 * must have createdb rights
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index 55b0be9e1d1..28b5c75f110 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -363,7 +363,7 @@ AlterForeignServerOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 							   NameStr(form->srvname));
 
 			/* Must be able to become new owner */
-			check_is_member_of_role(GetUserId(), newOwnerId);
+			check_can_set_role(GetUserId(), newOwnerId);
 
 			/* New owner must have USAGE privilege on foreign-data wrapper */
 			aclresult = object_aclcheck(ForeignDataWrapperRelationId, form->srvfdw, newOwnerId, ACL_USAGE);
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 940655b9be0..20fa72c5c80 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1911,7 +1911,7 @@ AlterPublicationOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 						   NameStr(form->pubname));
 
 		/* Must be able to become new owner */
-		check_is_member_of_role(GetUserId(), newOwnerId);
+		check_can_set_role(GetUserId(), newOwnerId);
 
 		/* New owner must have CREATE privilege on database */
 		aclresult = object_aclcheck(DatabaseRelationId, MyDatabaseId, newOwnerId, ACL_CREATE);
diff --git a/src/backend/commands/schemacmds.c b/src/backend/commands/schemacmds.c
index b03f07a2322..12cbfba7d06 100644
--- a/src/backend/commands/schemacmds.c
+++ b/src/backend/commands/schemacmds.c
@@ -97,7 +97,7 @@ CreateSchemaCommand(CreateSchemaStmt *stmt, const char *queryString,
 		aclcheck_error(aclresult, OBJECT_DATABASE,
 					   get_database_name(MyDatabaseId));
 
-	check_is_member_of_role(saved_uid, owner_uid);
+	check_can_set_role(saved_uid, owner_uid);
 
 	/* Additional check to protect reserved schema names */
 	if (!allowSystemTableMods && IsReservedName(schemaName))
@@ -370,7 +370,7 @@ AlterSchemaOwner_internal(HeapTuple tup, Relation rel, Oid newOwnerId)
 						   NameStr(nspForm->nspname));
 
 		/* Must be able to become new owner */
-		check_is_member_of_role(GetUserId(), newOwnerId);
+		check_can_set_role(GetUserId(), newOwnerId);
 
 		/*
 		 * must have create-schema rights
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f0068078520..845208d662b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13833,7 +13833,7 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing, LOCKMODE lock
 								   RelationGetRelationName(target_rel));
 
 				/* Must be able to become new owner */
-				check_is_member_of_role(GetUserId(), newOwnerId);
+				check_can_set_role(GetUserId(), newOwnerId);
 
 				/* New owner must have CREATE privilege on namespace */
 				aclresult = object_aclcheck(NamespaceRelationId, namespaceOid, newOwnerId,
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index ecc8b3f44c9..7770a86bee0 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3745,7 +3745,7 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
 				aclcheck_error_type(ACLCHECK_NOT_OWNER, typTup->oid);
 
 			/* Must be able to become new owner */
-			check_is_member_of_role(GetUserId(), newOwnerId);
+			check_can_set_role(GetUserId(), newOwnerId);
 
 			/* New owner must have CREATE privilege on namespace */
 			aclresult = object_aclcheck(NamespaceRelationId, typTup->typnamespace,
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 2369cc600c7..8b6543edee2 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -51,8 +51,8 @@
  * RRG_REMOVE_ADMIN_OPTION indicates a grant that would need to have
  * admin_option set to false by the operation.
  *
- * RRG_REMOVE_INHERIT_OPTION indicates a grant that would need to have
- * inherit_option set to false by the operation.
+ * Similarly, RRG_REMOVE_INHERIT_OPTION and RRG_REMOVE_SET_OPTION indicate
+ * grants that would need to have the corresponding options set to false.
  *
  * RRG_DELETE_GRANT indicates a grant that would need to be removed entirely
  * by the operation.
@@ -62,6 +62,7 @@ typedef enum
 	RRG_NOOP,
 	RRG_REMOVE_ADMIN_OPTION,
 	RRG_REMOVE_INHERIT_OPTION,
+	RRG_REMOVE_SET_OPTION,
 	RRG_DELETE_GRANT
 } RevokeRoleGrantAction;
 
@@ -73,10 +74,12 @@ typedef struct
 	unsigned	specified;
 	bool		admin;
 	bool		inherit;
+	bool		set;
 } GrantRoleOptions;
 
 #define GRANT_ROLE_SPECIFIED_ADMIN			0x0001
 #define GRANT_ROLE_SPECIFIED_INHERIT		0x0002
+#define GRANT_ROLE_SPECIFIED_SET			0x0004
 
 /* GUC parameter */
 int			Password_encryption = PASSWORD_TYPE_SCRAM_SHA_256;
@@ -1389,6 +1392,12 @@ GrantRole(ParseState *pstate, GrantRoleStmt *stmt)
 			if (parse_bool(optval, &popt.inherit))
 				continue;
 		}
+		else if (strcmp(opt->defname, "set") == 0)
+		{
+			popt.specified |= GRANT_ROLE_SPECIFIED_SET;
+			if (parse_bool(optval, &popt.set))
+				continue;
+		}
 		else
 			ereport(ERROR,
 					errcode(ERRCODE_SYNTAX_ERROR),
@@ -1776,6 +1785,16 @@ AddRoleMems(const char *rolename, Oid roleid,
 				at_least_one_change = true;
 			}
 
+			if ((popt->specified & GRANT_ROLE_SPECIFIED_SET) != 0
+				&& authmem_form->set_option != popt->set)
+			{
+				new_record[Anum_pg_auth_members_set_option - 1] =
+					BoolGetDatum(popt->set);
+				new_record_repl[Anum_pg_auth_members_set_option - 1] =
+					true;
+				at_least_one_change = true;
+			}
+
 			if (!at_least_one_change)
 			{
 				ereport(NOTICE,
@@ -1798,9 +1817,15 @@ AddRoleMems(const char *rolename, Oid roleid,
 			Oid			objectId;
 			Oid		   *newmembers = palloc(sizeof(Oid));
 
-			/* Set admin option if user set it to true, otherwise not. */
+			/*
+			 * The values for these options can be taken directly from 'popt'.
+			 * Either they were specified, or the defaults as set by
+			 * InitGrantRoleOptions are correct.
+			 */
 			new_record[Anum_pg_auth_members_admin_option - 1] =
 				BoolGetDatum(popt->admin);
+			new_record[Anum_pg_auth_members_set_option - 1] =
+				BoolGetDatum(popt->set);
 
 			/*
 			 * If the user specified a value for the inherit option, use
@@ -1989,6 +2014,13 @@ DelRoleMems(const char *rolename, Oid roleid,
 				new_record_repl[Anum_pg_auth_members_inherit_option - 1] =
 					true;
 			}
+			else if (actions[i] == RRG_REMOVE_SET_OPTION)
+			{
+				new_record[Anum_pg_auth_members_set_option - 1] =
+					BoolGetDatum(false);
+				new_record_repl[Anum_pg_auth_members_set_option - 1] =
+					true;
+			}
 			else
 				elog(ERROR, "unknown role revoke action");
 
@@ -2182,6 +2214,11 @@ plan_single_revoke(CatCList *memlist, RevokeRoleGrantAction *actions,
 				 */
 				actions[i] = RRG_REMOVE_INHERIT_OPTION;
 			}
+			else if ((popt->specified & GRANT_ROLE_SPECIFIED_SET) != 0)
+			{
+				/* Here too, no need to recurse. */
+				actions[i] = RRG_REMOVE_SET_OPTION;
+			}
 			else
 			{
 				bool	revoke_admin_option_only;
@@ -2331,4 +2368,5 @@ InitGrantRoleOptions(GrantRoleOptions *popt)
 	popt->specified = 0;
 	popt->admin = false;
 	popt->inherit = false;
+	popt->set = true;
 }
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 791bac6715c..00d8d54d820 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -939,7 +939,7 @@ check_role(char **newval, void **extra, GucSource source)
 		 * leader's state.
 		 */
 		if (!InitializingParallelWorker &&
-			!is_member_of_role(GetSessionUserId(), roleid))
+			!member_can_set_role(GetSessionUserId(), roleid))
 		{
 			if (source == PGC_S_TEST)
 			{
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 8bdb9461b7f..d4d68f97243 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -67,16 +67,17 @@ typedef struct
  *
  * Each element of cached_roles is an OID list of constituent roles for the
  * corresponding element of cached_role (always including the cached_role
- * itself).  One cache has ROLERECURSE_PRIVS semantics, and the other has
- * ROLERECURSE_MEMBERS semantics.
+ * itself).  There's a separate cache for each RoleRecurseType, with the
+ * corresponding semantics.
  */
 enum RoleRecurseType
 {
-	ROLERECURSE_PRIVS = 0,		/* recurse through inheritable grants */
-	ROLERECURSE_MEMBERS = 1		/* recurse unconditionally */
+	ROLERECURSE_MEMBERS = 0,	/* recurse unconditionally */
+	ROLERECURSE_PRIVS = 1,		/* recurse through inheritable grants */
+	ROLERECURSE_SETROLE = 2		/* recurse through grants with set_option */
 };
-static Oid	cached_role[] = {InvalidOid, InvalidOid};
-static List *cached_roles[] = {NIL, NIL};
+static Oid	cached_role[] = {InvalidOid, InvalidOid, InvalidOid};
+static List *cached_roles[] = {NIL, NIL, NIL};
 static uint32 cached_db_hash;
 
 
@@ -4691,10 +4692,13 @@ convert_role_priv_string(text *priv_type_text)
 	static const priv_map role_priv_map[] = {
 		{"USAGE", ACL_USAGE},
 		{"MEMBER", ACL_CREATE},
+		{"SET", ACL_SET},
 		{"USAGE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
 		{"USAGE WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
 		{"MEMBER WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
 		{"MEMBER WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
+		{"SET WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
+		{"SET WITH ADMIN OPTION", ACL_GRANT_OPTION_FOR(ACL_CREATE)},
 		{NULL, 0}
 	};
 
@@ -4723,6 +4727,11 @@ pg_role_aclcheck(Oid role_oid, Oid roleid, AclMode mode)
 		if (has_privs_of_role(roleid, role_oid))
 			return ACLCHECK_OK;
 	}
+	if (mode & ACL_SET)
+	{
+		if (member_can_set_role(roleid, role_oid))
+			return ACLCHECK_OK;
+	}
 	return ACLCHECK_NO_PRIV;
 }
 
@@ -4771,15 +4780,17 @@ RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
 	}
 
 	/* Force membership caches to be recomputed on next use */
-	cached_role[ROLERECURSE_PRIVS] = InvalidOid;
 	cached_role[ROLERECURSE_MEMBERS] = InvalidOid;
+	cached_role[ROLERECURSE_PRIVS] = InvalidOid;
+	cached_role[ROLERECURSE_SETROLE] = InvalidOid;
 }
 
 /*
  * Get a list of roles that the specified roleid is a member of
  *
- * Type ROLERECURSE_PRIVS recurses only through inheritable grants,
- * while ROLERECURSE_MEMBERS recurses through all grants.
+ * Type ROLERECURSE_MEMBERS recurses through all grants; ROLERECURSE_PRIVS
+ * recurses only through inheritable grants; and ROLERECURSE_SETROLe recurses
+ * only through grants with set_option.
  *
  * Since indirect membership testing is relatively expensive, we cache
  * a list of memberships.  Hence, the result is only guaranteed good until
@@ -4870,6 +4881,10 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
 			if (type == ROLERECURSE_PRIVS && !form->inherit_option)
 				continue;
 
+			/* If we're supposed to ignore non-SET grants, do so. */
+			if (type == ROLERECURSE_SETROLE && !form->set_option)
+				continue;
+
 			/*
 			 * Even though there shouldn't be any loops in the membership
 			 * graph, we must test for having already seen this role. It is
@@ -4909,9 +4924,10 @@ roles_is_member_of(Oid roleid, enum RoleRecurseType type,
 /*
  * Does member have the privileges of role (directly or indirectly)?
  *
- * This is defined not to recurse through grants that are not inherited;
- * in such cases, membership implies the ability to do SET ROLE, but
- * the privileges are not available until you've done so.
+ * This is defined not to recurse through grants that are not inherited,
+ * and only inherited grants confer the associated privileges automatically.
+ *
+ * See also member_can_set_role, below.
  */
 bool
 has_privs_of_role(Oid member, Oid role)
@@ -4933,49 +4949,87 @@ has_privs_of_role(Oid member, Oid role)
 						   role);
 }
 
-
 /*
- * Is member a member of role (directly or indirectly)?
+ * Can member use SET ROLE to this role?
  *
- * This is defined to recurse through grants whether they are inherited or not.
+ * There must be a chain of grants from 'member' to 'role' each of which
+ * permits SET ROLE; that is, each of which has set_option = true.
  *
- * Do not use this for privilege checking, instead use has_privs_of_role()
+ * It doesn't matter whether the grants are inheritable. That's a separate
+ * question; see has_privs_of_role.
+ *
+ * This function should be used to determine whether the session user can
+ * use SET ROLE to become the target user. We also use it to determine whether
+ * the session user can change an existing object to be owned by the target
+ * user, or create new objects owned by the target user.
  */
 bool
-is_member_of_role(Oid member, Oid role)
+member_can_set_role(Oid member, Oid role)
 {
 	/* Fast path for simple case */
 	if (member == role)
 		return true;
 
-	/* Superusers have every privilege, so are part of every role */
+	/* Superusers have every privilege, so can always SET ROLE */
 	if (superuser_arg(member))
 		return true;
 
 	/*
-	 * Find all the roles that member is a member of, including multi-level
-	 * recursion, then see if target role is any one of them.
+	 * Find all the roles that member can access via SET ROLE, including
+	 * multi-level recursion, then see if target role is any one of them.
 	 */
-	return list_member_oid(roles_is_member_of(member, ROLERECURSE_MEMBERS,
+	return list_member_oid(roles_is_member_of(member, ROLERECURSE_SETROLE,
 											  InvalidOid, NULL),
 						   role);
 }
 
 /*
- * check_is_member_of_role
- *		is_member_of_role with a standard permission-violation error if not
+ * Permission violation eror unless able to SET ROLE to target role.
  */
 void
-check_is_member_of_role(Oid member, Oid role)
+check_can_set_role(Oid member, Oid role)
 {
-	if (!is_member_of_role(member, role))
+	if (!member_can_set_role(member, role))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-				 errmsg("must be member of role \"%s\"",
+				 errmsg("must be able to SET ROLE \"%s\"",
 						GetUserNameFromId(role, false))));
 }
 
 /*
+ * Is member a member of role (directly or indirectly)?
+ *
+ * This is defined to recurse through grants whether they are inherited or not.
+ *
+ * Do not use this for privilege checking, instead use has_privs_of_role().
+ * Don't use it for determining whether it's possible to SET ROLE to some
+ * other role; for that, use member_can_set_role(). And don't use it for
+ * determining whether it's OK to create an object owned by some other role:
+ * use member_can_set_role() for that, too.
+ *
+ * In short, calling this function is the wrong thing to do nearly everywhere.
+ */
+bool
+is_member_of_role(Oid member, Oid role)
+{
+	/* Fast path for simple case */
+	if (member == role)
+		return true;
+
+	/* Superusers have every privilege, so are part of every role */
+	if (superuser_arg(member))
+		return true;
+
+	/*
+	 * Find all the roles that member is a member of, including multi-level
+	 * recursion, then see if target role is any one of them.
+	 */
+	return list_member_oid(roles_is_member_of(member, ROLERECURSE_MEMBERS,
+											  InvalidOid, NULL),
+						   role);
+}
+
+/*
  * Is member a member of role, not considering superuserness?
  *
  * This is identical to is_member_of_role except we ignore superuser