Re: CREATEROLE does not permit commenting on newly-created roles

I wrote:
> I thought there was nothing particularly unreasonable about Owen's
> suggestion: let users with the CREATEROLE attribute comment on any role.
> I don't think COMMENT added to CREATE ROLE would be a very nice fix
> (aside from being ugly, what if you want to change the comment later?).

> It strikes me actually that letting members of the role comment on it
> is not an amazingly good idea.  They are not owners of the role in any
> meaningful sense --- for instance, they can't drop it.  It'd be more
> reasonable and consistent to say that only superusers and holders of
> CREATEROLE can do COMMENT ON ROLE.

In particular, I suggest the attached patch (code-complete, but sans
documentation changes).  The changes here bring COMMENT ON ROLE into
line with the permission requirements for other operations on roles
that require ownership-like permissions.  This patch modifies
check_object_ownership, which means it affects three call sites at
present:

    COMMENT ON ROLE

    ALTER EXTENSION ADD/DROP (but the target object cannot be a role)

    SECURITY LABEL IS (also couldn't be a role, at the moment)

The SECURITY LABEL case, even though it's presently unimplemented,
seems to me to be a darn good argument for redefining the notion
of "role ownership" like this.  Who would want a mere member of some
group role to be able to set that role's security label?

Comments, objections?

            regards, tom lane

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index a98f918..48fa6d4 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*************** pg_extension_ownercheck(Oid ext_oid, Oid
*** 4736,4741 ****
--- 4736,4771 ----
  }

  /*
+  * 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 b8b89ab..880b95d 100644
*** a/src/backend/catalog/objectaddress.c
--- b/src/backend/catalog/objectaddress.c
*************** check_object_ownership(Oid roleid, Objec
*** 808,820 ****
                  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,
--- 808,813 ----
*************** check_object_ownership(Oid roleid, Objec
*** 825,830 ****
--- 818,843 ----
                  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 63f22d8..f13eb28 100644
*** a/src/backend/commands/user.c
--- b/src/backend/commands/user.c
*************** static void DelRoleMems(const char *role
*** 58,77 ****
  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;
  }


--- 58,64 ----
  static bool
  have_createrole_privilege(void)
  {
!     return has_createrole_privilege(GetUserId());
  }


diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index c0f7b64..e96323e 100644
*** a/src/include/utils/acl.h
--- b/src/include/utils/acl.h
*************** extern bool pg_ts_dict_ownercheck(Oid di
*** 317,321 ****
--- 317,322 ----
  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 */