Обсуждение: CREATEROLE does not permit commenting on newly-created roles
Salut,
I'm making use of database-side comments to document the oddities and
intended uses for various database symbols. While this is mostly going
well, it's falling down a bit for documenting role objects. For the
record, I'm running the EnterpriseDB Postgres 9.0.3 package on OS X
10.6 as well as Martin Pitt's Postgres 9.0 PPA on Ubuntu 10.10
(x86_64).
Steps to reproduce:
1. Create a role with CREATEROLE to act as an owner
("comment_createrole" in the repro transcript below).
2. Log in as this role and issue the following statements:
CREATE ROLE commented_role;
COMMENT ON ROLE commented_role IS 'A comment';
Expected results:
The new role has the comment 'A comment' applied to it.
Actual results:
psql:repro.sql:2: ERROR: must be member of role "commented_role" to
comment upon it
While I realize that roles have no owner, and therefore no obvious
person who should be able to fiddle the comment, being unable to
comment on newly-created roles without joining them really limits the
usefulness of role comments for database administrators. It
intuitively (to me, anyways) feels like CREATEROLE should permit
commenting on (non-superuser) roles.
-o
P.S. A complete reproduction:
$ cat repro.sql
CREATE ROLE commented_role;
COMMENT ON ROLE commented_role IS 'A comment';
$ createdb pg-comment-role
$ psql -U postgres pg-comment-role
psql (9.0.3)
Type "help" for help.
pg-comment-role=# CREATE ROLE comment_createrole WITH LOGIN CREATEROLE
PASSWORD 'comment_createrole';
CREATE ROLE
pg-comment-role=# \q
$ psql -h localhost -U comment_createrole pg-comment-role
psql (9.0.3)
Type "help" for help.
pg-comment-role=> \i repro.sql
CREATE ROLE
psql:repro.sql:2: ERROR: must be member of role "commented_role" to
comment upon it
Em 07-03-2011 16:53, Owen Jacobson escreveu: > psql:repro.sql:2: ERROR: must be member of role "commented_role" to > comment upon it > This isn't a bug; let say it is a limitation (and a documented one [1]). Unfortunately only the role, superuser or its members can add/drop comments. [1] http://www.postgresql.org/docs/9.0/static/sql-comment.html -- Euler Taveira de Oliveira http://www.timbira.com/
Excerpts from Euler Taveira de Oliveira's message of mar mar 08 02:06:13 -0300 2011: > Em 07-03-2011 16:53, Owen Jacobson escreveu: > > psql:repro.sql:2: ERROR: must be member of role "commented_role" to > > comment upon it > > > This isn't a bug; let say it is a limitation (and a documented one [1]). > Unfortunately only the role, superuser or its members can add/drop comments. Maybe it would be good to have a COMMENT clause on the CREATE ROLE command. It would be inconsistent with the rest of the comment system, but this privilege problem is inconsistent too. -- Ãlvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Excerpts from Euler Taveira de Oliveira's message of mar mar 08 02:06:13 -0300 2011:
>> Em 07-03-2011 16:53, Owen Jacobson escreveu:
>>> psql:repro.sql:2: ERROR: must be member of role "commented_role" to
>>> comment upon it
>> This isn't a bug; let say it is a limitation (and a documented one [1]).
>> Unfortunately only the role, superuser or its members can add/drop comments.
> Maybe it would be good to have a COMMENT clause on the CREATE ROLE
> command. It would be inconsistent with the rest of the comment system,
> but this privilege problem is inconsistent too.
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.
regards, tom lane
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 */
On Tue, Mar 8, 2011 at 11:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > 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. =A0They are not owners of the role in any >> meaningful sense --- for instance, they can't drop it. =A0It'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). =A0The changes here bring COMMENT ON ROLE into > line with the permission requirements for other operations on roles > that require ownership-like permissions. =A0This patch modifies > check_object_ownership, which means it affects three call sites at > present: > > =A0 =A0 =A0 =A0COMMENT ON ROLE > > =A0 =A0 =A0 =A0ALTER EXTENSION ADD/DROP (but the target object cannot be = a role) > > =A0 =A0 =A0 =A0SECURITY 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. =A0Who would want a mere member of some > group role to be able to set that role's security label? > > Comments, objections? I think it's a good change, but we should make sure to release-note it properly, along with the change you made for PLs. --=20 Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Mar 8, 2011 at 11:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In particular, I suggest the attached patch (code-complete, but sans
>> documentation changes).
> I think it's a good change, but we should make sure to release-note it
> properly,
I had already drafted a commit message:
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.
Per complaint from Owen Jacobson and subsequent discussion.
How that gets boiled down into a release note will depend on whoever
writes the release notes.
> along with the change you made for PLs.
Hrm?
regards, tom lane
On Wed, Mar 9, 2011 at 12:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > How that gets boiled down into a release note will depend on whoever > writes the release notes. > >> along with the change you made for PLs. In 9.0, to comment on a procedural language, you must be superuser. But your recent commit changed it to check for language ownership. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 9, 2011 at 12:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> along with the change you made for PLs.
>> Huh?
> In 9.0, to comment on a procedural language, you must be superuser.
> But your recent commit changed it to check for language ownership.
Oh. That's just a bug fix: it should have been changed when
we invented pg_language.lanowner.
regards, tom lane
I wrote:
> In particular, I suggest the attached patch (code-complete, but sans
> documentation changes).
Applied with doc fixes. I waited till after alpha4 was cut, though.
regards, tom lane