From 49a08ca1e968860fe02fa3331cc0aba361d76e02 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 9 Mar 2011 11:28:20 -0500 Subject: [PATCH] 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. --- doc/src/sgml/func.sgml | 12 +++---- doc/src/sgml/ref/comment.sgml | 52 ++++++++++++++++++----------- src/backend/catalog/aclchk.c | 30 +++++++++++++++++ src/backend/catalog/objectaddress.c | 27 +++++++++++---- src/backend/commands/user.c | 15 +-------- src/include/utils/acl.h | 1 + 6 files changed, 91 insertions(+), 46 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index c5d75c4c65..fe14a60426 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -13588,10 +13588,10 @@ SELECT typlen FROM pg_type WHERE oid = pg_typeof(33); - col_description returns the comment for a table column, - which is specified by the OID of its table and its column number. - obj_description cannot be used for table columns since - columns do not have OIDs of their own. + col_description returns the comment for a table + column, which is specified by the OID of its table and its column number. + (obj_description cannot be used for table columns + since columns do not have OIDs of their own.) @@ -13610,8 +13610,8 @@ SELECT typlen FROM pg_type WHERE oid = pg_typeof(33); shobj_description is used just like obj_description except it is used for retrieving comments on shared objects. Some system catalogs are global to all - databases within each cluster and their descriptions are stored globally - as well. + databases within each cluster, and the descriptions for objects in them + are stored globally as well. diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml index 2610fd5b8d..bc848b3069 100644 --- a/doc/src/sgml/ref/comment.sgml +++ b/doc/src/sgml/ref/comment.sgml @@ -65,11 +65,18 @@ COMMENT ON - To modify a comment, issue a new COMMENT command for the - same object. Only one comment string is stored for each object. - To remove a comment, write NULL in place of the text - string. - Comments are automatically dropped when the object is dropped. + Only one comment string is stored for each object, so to modify a comment, + issue a new COMMENT command for the same object. To remove a + comment, write NULL in place of the text string. + Comments are automatically dropped when their object is dropped. + + + + For most kinds of object, only the object's owner can set the comment. + Roles don't have owners, so the rule for COMMENT ON ROLE is + that you must be superuser to comment on a superuser role, or have the + CREATEROLE privilege to comment on non-superuser roles. + Of course, a superuser can comment on anything. @@ -93,15 +100,15 @@ COMMENT ON agg_name constraint_name function_name - op + operator_name rule_name trigger_name The name of the object to be commented. Names of tables, - aggregates, domains, foreign tables, functions, indexes, operators, - operator classes, operator families, sequences, text search objects, - types, and views can be schema-qualified. + aggregates, collations, conversions, domains, foreign tables, functions, + indexes, operators, operator classes, operator families, sequences, + text search objects, types, and views can be schema-qualified. @@ -137,7 +144,6 @@ COMMENT ON argmode - The mode of a function argument: IN, OUT, @@ -154,7 +160,6 @@ COMMENT ON argname - The name of a function argument. @@ -167,7 +172,6 @@ COMMENT ON argtype - The data type(s) of the function's arguments (optionally @@ -185,9 +189,20 @@ COMMENT ON + + left_type + right_type + + + The data type(s) of the operator's arguments (optionally + schema-qualified). Write NONE for the missing argument + of a prefix or postfix operator. + + + + PROCEDURAL - This is a noise word. @@ -212,12 +227,11 @@ COMMENT ON Notes - There is presently no security mechanism for comments: any user + There is presently no security mechanism for viewing comments: any user connected to a database can see all the comments for objects in - that database (although only superusers can change comments for - objects that they don't own). For shared objects such as - databases, roles, and tablespaces comments are stored globally - and any user connected to any database can see all the comments + that database. For shared objects such as + databases, roles, and tablespaces, comments are stored globally so any + user connected to any database in the cluster can see all the comments for shared objects. Therefore, don't put security-critical information in comments. @@ -257,7 +271,7 @@ COMMENT ON INDEX my_index IS 'Enforces uniqueness on employee ID'; COMMENT ON LANGUAGE plpython IS 'Python support for stored procedures'; COMMENT ON LARGE OBJECT 346344 IS 'Planning document'; COMMENT ON OPERATOR ^ (text, text) IS 'Performs intersection of two texts'; -COMMENT ON OPERATOR - (NONE, text) IS 'This is a prefix operator on text'; +COMMENT ON OPERATOR - (NONE, integer) IS 'Unary minus'; COMMENT ON OPERATOR CLASS int4ops USING btree IS '4 byte integer operators for btrees'; COMMENT ON OPERATOR FAMILY integer_ops USING btree IS 'all integer operators for btrees'; COMMENT ON ROLE my_role IS 'Administration group for finance tables'; diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index a98f918a23..48fa6d48b7 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -4735,6 +4735,36 @@ pg_extension_ownercheck(Oid ext_oid, Oid roleid) return has_privs_of_role(roleid, ownerId); } +/* + * 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). diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c index b8b89ab7c1..880b95df02 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 63f22d8adc..f13eb2891e 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 c0f7b64d80..e96323efcc 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 */ -- GitLab