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 */
--
Sent via pgsql-bugs mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
