Adjust the permissions required for COMMENT ON ROLE.

Formerly, any member of a role could change the role's comment, as of
course could superusers; but holders of CREATEROLE privilege could not,
unless they were also members.  This led to the odd situation that a
CREATEROLE holder could create a role but then could not comment on it.
It also seems a bit dubious to let an unprivileged user change his own
comment, let alone those of group roles he belongs to.  So, change the
rule to be "you must be superuser to comment on a superuser role, or
hold CREATEROLE to comment on non-superuser roles".  This is the same
as the privilege check for creating/dropping roles, and thus fits much
better with the rule for other object types, namely that only the owner
of an object can comment on it.

In passing, clean up the documentation for COMMENT a little bit.

Per complaint from Owen Jacobson and subsequent discussion.

4 files changed, 52 insertions, 21 deletions

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index a98f918a239..48fa6d48b7f 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4736,6 +4736,36 @@ pg_extension_ownercheck(Oid ext_oid, Oid roleid)
 }
 
 /*
+ * 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)
+{
+	bool		result = false;
+	HeapTuple	utup;
+
+	/* Superusers bypass all permission checking. */
+	if (superuser_arg(roleid))
+		return true;
+
+	utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+	if (HeapTupleIsValid(utup))
+	{
+		result = ((Form_pg_authid) GETSTRUCT(utup))->rolcreaterole;
+		ReleaseSysCache(utup);
+	}
+	return result;
+}
+
+/*
  * Fetch pg_default_acl entry for given role, namespace and object type
  * (object type must be given in pg_default_acl's encoding).
  * Returns NULL if no such entry.
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index b8b89ab7c19..880b95df020 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -808,13 +808,6 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
 				aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TABLESPACE,
 							   NameListToString(objname));
 			break;
-		case OBJECT_ROLE:
-			if (!has_privs_of_role(roleid, address.objectId))
-				ereport(ERROR,
-						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-						 errmsg("must be member of role \"%s\"",
-								NameListToString(objname))));
-			break;
 		case OBJECT_TSDICTIONARY:
 			if (!pg_ts_dict_ownercheck(address.objectId, roleid))
 				aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TSDICTIONARY,
@@ -825,6 +818,26 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
 				aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TSCONFIGURATION,
 							   NameListToString(objname));
 			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")));
+			}
+			break;
 		case OBJECT_FDW:
 		case OBJECT_TSPARSER:
 		case OBJECT_TSTEMPLATE:
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 63f22d8adc2..f13eb2891e2 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -58,20 +58,7 @@ static void DelRoleMems(const char *rolename, Oid roleid,
 static bool
 have_createrole_privilege(void)
 {
-	bool		result = false;
-	HeapTuple	utup;
-
-	/* Superusers can always do everything */
-	if (superuser())
-		return true;
-
-	utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(GetUserId()));
-	if (HeapTupleIsValid(utup))
-	{
-		result = ((Form_pg_authid) GETSTRUCT(utup))->rolcreaterole;
-		ReleaseSysCache(utup);
-	}
-	return result;
+	return has_createrole_privilege(GetUserId());
 }
 
 
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index c0f7b64d806..e96323efcc7 100644
--- a/src/include/utils/acl.h
+++ b/src/include/utils/acl.h
@@ -317,5 +317,6 @@ extern bool pg_ts_dict_ownercheck(Oid dict_oid, Oid roleid);
 extern bool pg_ts_config_ownercheck(Oid cfg_oid, Oid roleid);
 extern bool pg_foreign_server_ownercheck(Oid srv_oid, Oid roleid);
 extern bool pg_extension_ownercheck(Oid ext_oid, Oid roleid);
+extern bool has_createrole_privilege(Oid roleid);
 
 #endif   /* ACL_H */