Users/Groups -> Roles

Greetings,

(Sent this earlier, but afraid it may have gotten caught by the
too-big bug, so I'm reposting without the files attached, they can
all be found at: http://kenobi.snowman.net/~sfrost/pg_role/ ; there
are also gzip and uncompressed versions of the unified / context
diffs there for those who don't care for bzip2)

Attached please find files and patches associated with moving from the
User/Group system currently in place to Roles, as discussed
previously. The files are:

pg_authid.h
New system table, contains role information
To be placed in src/include/catalog, replacing pg_shadow.h

pg_auth_members.h
New system table, contains role/membership information
To be placed in src/include/catalog, replacing pg_group.h

role_2005062701.ctx.patch.bz2
Main patch, generated via cvs -z3 diff -c | bzip2
(gzip didn't quite get it under the 100K mark for the list)

role_milestones
List of milestones associated with my work on Roles support
'*' indicates that the milestone has been met/completed
'?' indicates uncertainty about if something should be done
No marker indicates an item which needs to be done
Note: Documentation needs to be updated

Again, my apologies for not getting this in sooner, it's been a little
hectic around here of late. I'm anxious to get feedback, comments,
corrections, etc.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

Attached please find files and patches associated with moving from the
User/Group system currently in place to Roles, as discussed
previously.

I have cleaned this up a bit and committed it. I normally wouldn't
commit an incomplete patch, but this change is blocking Alvaro's work
on dependencies for shared objects, so I felt it was best to get the
catalog changes in now. That will let Alvaro work on dependencies
while I sort out the unfinished bits of roles, which I intend to do
over the next day or so.

Many thanks for your work on this!

regards, tom lane

Dear Stephen,

Attached please find files and patches associated with moving from the
User/Group system currently in place to Roles, as discussed
previously. The files are:

pg_authid.h
New system table, contains role information
To be placed in src/include/catalog, replacing pg_shadow.h

pg_auth_members.h
New system table, contains role/membership information
To be placed in src/include/catalog, replacing pg_group.h

I've looked very quickly at the patch. ISTM that the proposed patch is a
reworking of the user/group stuff, which are both unified for a new "role"
concept where a user is a kind of role and a role can be a member of
another role. Well, why not.

Some added files seems not to be provided in the patch :

sh> bzgrep pg_authid.h ./role_2005062701.ctx.patch.bz2
? src/include/catalog/pg_authid.h
! pg_namespace.h pg_conversion.h pg_database.h pg_authid.h pg_auth_members.h \
! #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"
+ #include "catalog/pg_authid.h"
+ #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"
! #include "catalog/pg_authid.h"

Or maybe I missed something, but I could not find the pg_authid file?
the '?' line in the diff seems to suggest an unexpected file.

Anyway, from what I can see in the patch it seems that the roles are per
cluster, and not per catalog. So this is not so conceptually different
from user/group as already provided in pg.

What would have been much more interesting for me would be a per catalog
role, so that rights could be administrated locally in each database. I'm
not sure how to provide such a feature, AFAICS the current version does
not give me new abilities wrt right management.

Have a nice day,

--
Fabien.

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

Attached please find files and patches associated with moving from the
User/Group system currently in place to Roles, as discussed
previously.

I have cleaned this up a bit and committed it. I normally wouldn't
commit an incomplete patch, but this change is blocking Alvaro's work
on dependencies for shared objects, so I felt it was best to get the
catalog changes in now. That will let Alvaro work on dependencies
while I sort out the unfinished bits of roles, which I intend to do
over the next day or so.

Great, glad to hear it. I hope you got a chance to look over the open
items in the 'milestones' file. I'd really like to see the grammar be
fixed to match SQL spec for GRANT ROLE/REVOKE ROLE. I think an approach
to take there might be to try and get GrantRoleStmt and GrantStmt to use
the same productions at the end of the line if possible or something
along those lines.

Also, I've been looking through the diff between my tree and what you
committed to CVS and had a couple comments (just my 2c: I think it would
have been alot easier using SVN to see exaclty what was different from
my patch vs. other changes since my last CVS up):

First, sorry about the gratuitous name changes, it helped me find
every place I needed to look at the code and think about if it needed
to be changed in some way (ie: Int32GetDatum -> ObjectIdGetDatum,
etc). I had planned on changing some of them back to minimize the
patch but kind of ran out of time.

Second, looks like I missed fixing an owner check in pg_proc.c
Current CVS has, line 269:
if (GetUserId() != oldproc->proowner && !superuser())
Which is not a sufficient owner check. This should by fixed by doing
a proper pg_proc_ownercheck, ie:
if (!pg_proc_ownercheck(HeapTupleGetOid(oldtup), GetUserId()))

Third, I feel it's incorrect to only allow superuser() to change
ownership of objects under a role-based system. Users must be able to
create objects owned by a role they're in (as opposed to owned only
by themselves). Without this there is no way for a given role to
allow other roles to perform owner-level actions on objects which they
create. The point of adding roles was to allow owner-level actions on
objects to more than a single user or the superuser. Requiring the
superuser to get involved with every table creation defeats much of
the point.

This should really be possible either by explicitly changing the
ownership of an object using ALTER ... OWNER, or by a SET ROLE
followed by CREATE TABLE, etc. SET ROLE is defined by the SQL
specification, though we don't support it specifically yet (shouldn't
be too difficult to add now though). Certainly if we accept that
SET ROLE should be supported and that objects then created should be
owned by the role set in SET ROLE we should be willing to support
non-superusers doing ALTER ... OWNER given that they could effectively
do the same thing via SET ROLE (though with much more difficulty,
which has no appreciable gain).

Fourth, not that I use it, but, it looks like my changes to
src/interfaces/ecpg/preproc/preproc.y were lost. Not sure if that was
intentional or not (I wouldn't think so... I do wish ecpg could just
be the differences necessary for ecpg and be based off the main parser
somehow, but that'd be a rather large change). Oh, and in that same
boat, src/tools/pgindent/pgindent also appears to not have gotten the
changes that I made.

Many thanks for your work on this!

Happy to have helped though frustrated that you seem to have removed
the part that I was originally looking for. I don't feel that's
justification for having it (I feel I've addressed that above) but it
certainly would have been nice to be aware of that earlier and perhaps
to have discussed the issues around it a bit more before being so
close to the feature freeze (I know, alot my fault, but there it is).

Thanks,

Stephen

Hi Fabien,

* Fabien COELHO (fabien.coelho@ensmp.fr) wrote:

I've looked very quickly at the patch. ISTM that the proposed patch is a
reworking of the user/group stuff, which are both unified for a new "role"
concept where a user is a kind of role and a role can be a member of
another role. Well, why not.

Right, it's a beginning to proper 'Role' support as defined by the SQL
specification.

Some added files seems not to be provided in the patch :

pg_authid.h and pg_auth_members.h were attached to the email. They're
also available at http://kenobi.snowman.net/~sfrost/pg_role/ ; but the
patch has already been applied by Tom to CVS HEAD (well, with lots of
modifications and whatnot), so you probably should just take a look at
that.

Anyway, from what I can see in the patch it seems that the roles are per
cluster, and not per catalog. So this is not so conceptually different
from user/group as already provided in pg.

It's conceptually different from users/groups in that it's roles, which
aren't the same thing. You're right, it's still per-cluster though.

What would have been much more interesting for me would be a per catalog
role, so that rights could be administrated locally in each database. I'm
not sure how to provide such a feature, AFAICS the current version does
not give me new abilities wrt right management.

I understand your concerns here and while I agree with the basic idea
that per-catalog role sets would be nice it wasn't what I had set out to
do with this patch. Perhaps what you're asking for will be added later
on. Some things this patch does do though are:

Allow role ownership. This role can also have members, and doesn't
necessairly have to be allowed to log in. Members of a role which owns
an object have owner-level rights on that object (so, fe: roles user1,
user2 and group1 where user1 and user2 are members of group1, a table
owned by group1 can be vacuumed, have columns added/removed, have
indexes create on it, etc, by user1 or user2).

Allow granting roles to other roles based on the 'with admin option'.
This means you don't have to be a superuser to add a member to a role
which you have the 'admin option' on.

There's other things (startup may be a bit faster since the pg_auth file
is sorted by the backend instead of during each startup, etc) but the
above were the types of things that I was looking to do mainly.

I'd like to see it possible to distinguish between 'superuser' and
'createrole' permissions, but I didn't get to that point with the roles
support (it's really a seperate issue anyway).

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

Also, I've been looking through the diff between my tree and what you
committed to CVS and had a couple comments

First, sorry about the gratuitous name changes, it helped me find
every place I needed to look at the code and think about if it needed
to be changed in some way (ie: Int32GetDatum -> ObjectIdGetDatum,
etc). I had planned on changing some of them back to minimize the
patch but kind of ran out of time.

No problem, I figured that was why you'd done it, but changing them back
helped me to understand the patch also ;-)

Second, looks like I missed fixing an owner check in pg_proc.c

Got it. I was wondering if there were more --- might be worth checking
all the superuser() calls.

Third, I feel it's incorrect to only allow superuser() to change
ownership of objects under a role-based system.

I took that out because it struck me as a likely security hole; we don't
allow non-superuser users to give away objects now, and we shouldn't
allow non-superuser roles to do so either. Moreover the tests you had
were inconsistent (not same test everyplace).

Users must be able to
create objects owned by a role they're in (as opposed to owned only
by themselves).

This is what SET SESSION AUTHORIZATION/SET ROLE is for, no? You set the
auth to a role you are allowed to be in, then create the object. I do
notice that we don't have this yet, but it's surely a required piece of
the puzzle.

Fourth, not that I use it, but, it looks like my changes to
src/interfaces/ecpg/preproc/preproc.y were lost. Not sure if that was
intentional or not

Yeah, it was. I leave it to Michael Meskes to sync ecpg with the main
parser; on the occasions where I've tried to do it for him, things
didn't work out well.

I do wish ecpg could just
be the differences necessary for ecpg and be based off the main parser
somehow,

Me too, but I haven't seen a way yet.

src/tools/pgindent/pgindent also appears to not have gotten the
changes that I made.

That's an automatically generated list; there's no need to edit it.

regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

Second, looks like I missed fixing an owner check in pg_proc.c

Got it. I was wondering if there were more --- might be worth checking
all the superuser() calls.

Yeah, let's come up with a decision about what exactly we should allow
and then perhaps I can go through all of the superuser() calls and see
what needs fixing up.

Third, I feel it's incorrect to only allow superuser() to change
ownership of objects under a role-based system.

I took that out because it struck me as a likely security hole; we don't
allow non-superuser users to give away objects now, and we shouldn't
allow non-superuser roles to do so either. Moreover the tests you had
were inconsistent (not same test everyplace).

Sorry about them being inconsistent, I didn't intend for them to be.
I went through a couple of iterations of them trying to do the check the
'right' way. Thinking back on it, even the checks I ended up with were
wrong (in the superuser case), though I think they were closer.
Basically my thought was to allow the same thing you could do w/ SET
ROLE, etc:

If you are the owner of the object to be changed (following the normal
owner checking rules) AND would still be considered the owner of the
object *after* the change, then you can change the ownership.

The code I had for this was:

if (!pg_class_ownercheck(tuple,GetUserId()) ||
!is_role_member(newowner,GetUserId()))

That needs a check for superuser though because while the test will pass
on the 'pg_class_ownercheck' side, it won't on the 'is_role_member' side
(currently anyway, I suppose a superuser could be considered to be in
any role, so we could change is_role_member to always return true for
superusers, that'd probably make pg_group look ugly though, either way):

if (!superuser() && !(pg_class_ownercheck(tupe,GetUserId()) &&
is_role_member(newowner,GetUserId())))

I think that's the correct check and can be done the same way for pretty
much all of the objects. Were there other security concerns you had?
I'd be happy to look through the superuser() checks in commands/ and
develop a patch following what I described above, as well as looking for
other cases where we should be using the *_ownercheck() functions.

One place I recall seeing one and not being sure if it should be a new
*_ownercheck() function or not was in the 2PC patch- twophase.c, line
380:

if (user != gxact->owner && !superuser_arg(user))

Wasn't sure if that made sense to have *_ownercheck, or, even if we
added one for it, if it made sense to check is_member_of_role() for
prepared transactions. I don't think SQL has anything to say about it,
anyone know what other DBs do here?

Users must be able to
create objects owned by a role they're in (as opposed to owned only
by themselves).

This is what SET SESSION AUTHORIZATION/SET ROLE is for, no? You set the
auth to a role you are allowed to be in, then create the object. I do
notice that we don't have this yet, but it's surely a required piece of
the puzzle.

(Technically I think SET SESSION AUTHORIZATION is different from SET
ROLE, but anyway)

Right, that's another way to do it (as I mentioned), and that lets you
do ownership changes, but they're much more painful:

CONNECT AS joe;
CREATE TABLE abc as SELECT name,avg(a),sum(b) FROM reallybigtable;
-- Whoops, I meant for abc to be owned by role C so sally can add her
-- column to it later, or vacuum/analyze it, whatever
GRANT SELECT ON abc TO C; -- Might not be necessary
ALTER TABLE abc RENAME TO abc_temp;
SET ROLE C;
CREATE TABLE abc AS SELECT * FROM abc_temp; -- Could be big :(
SET ROLE NONE; -- Might be just 'SET ROLE;'? Gotta check the spec
DROP TABLE abc_temp;

I don't really see the point in making users go through all of these
hoops to do an ownership change. In the end, it's the same result near
as I can tell...

Yeah, it was. I leave it to Michael Meskes to sync ecpg with the main
parser; on the occasions where I've tried to do it for him, things
didn't work out well.

Ah, ok.

src/tools/pgindent/pgindent also appears to not have gotten the
changes that I made.

That's an automatically generated list; there's no need to edit it.

Hah, silly me.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

The code I had for this was:

if (!pg_class_ownercheck(tuple,GetUserId()) ||
!is_role_member(newowner,GetUserId()))

That needs a check for superuser though because while the test will pass
on the 'pg_class_ownercheck' side, it won't on the 'is_role_member' side

Um, right, that was another problem I had with it --- at one point the
regression tests were failing because the superuser wasn't allowed to
reassign object ownership ...

I'm still fairly concerned about the security implications of letting
ordinary users reassign object ownership. The fact that SET ROLE would
let you *create* an object with ownership X is a long way away from
saying that you should be allowed to change an *existing* object to have
ownership X. This is particularly so if you are a member of a couple of
different roles with different memberships: you will be able to cause
objects to become effectively owned by certain other people, or make
them stop being effectively owned by those people. I don't have a clear
trouble case in mind at the moment, but this sure sounds like the stuff
of routine security-hole reports. (Altering the ownership of a SECURITY
DEFINER function, in particular, sounds like a great path for a cracker
to pursue.)

One place I recall seeing one and not being sure if it should be a new
*_ownercheck() function or not was in the 2PC patch- twophase.c, line
380:

This one I think we can leave...

regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

That needs a check for superuser though because while the test will pass
on the 'pg_class_ownercheck' side, it won't on the 'is_role_member' side

Um, right, that was another problem I had with it --- at one point the
regression tests were failing because the superuser wasn't allowed to
reassign object ownership ...

Yeah, sorry about that.

I'm still fairly concerned about the security implications of letting
ordinary users reassign object ownership. The fact that SET ROLE would
let you *create* an object with ownership X is a long way away from
saying that you should be allowed to change an *existing* object to have
ownership X. This is particularly so if you are a member of a couple of
different roles with different memberships: you will be able to cause
objects to become effectively owned by certain other people, or make
them stop being effectively owned by those people. I don't have a clear
trouble case in mind at the moment, but this sure sounds like the stuff
of routine security-hole reports. (Altering the ownership of a SECURITY
DEFINER function, in particular, sounds like a great path for a cracker
to pursue.)

SET ROLE also lets you *drop* an object owned by that role. Or alter
it. Or CREATE OR REPLACE FUNCTION ...

I can understand your concern. The specific use case I'm thinking about
is where a user creates an object, does some work on it, and then wants
to change its ownership to be owned by a role which that user is in. I
find myself doing that a fair bit (as superuser atm). One thing I don't
like about limiting it to that is that you then can't go back without
the whole drop/create business or getting an admin.

This also isn't stuff that couldn't be done through other means, even in
the SECURITY DEFINER function case, you just need to drop, set role,
create. Having a role with members be able to own objects isn't meant
to replace the privileges system and I don't expect people to try to use
it to.

I can perhaps see a special case for SECURITY DEFINER functions but if
we're going to special case them I'd think we'd need to make them only
be creatable/modifiable at all by superusers or add another flag to the
role to allow that.

Thanks,

Stephen

On Tue, Jun 28, 2005 at 14:45:06 -0400,
Stephen Frost <sfrost@snowman.net> wrote:

If you are the owner of the object to be changed (following the normal
owner checking rules) AND would still be considered the owner of the
object *after* the change, then you can change the ownership.

That still isn't a good idea, because the new owner may not have had
access to create the object you just gave to them. Or you may not have
had access to drop the object you just gave away. That is going to
be a security hole.

On Tue, Jun 28, 2005 at 14:52:07 -0500,
Bruno Wolff III <bruno@wolff.to> wrote:

On Tue, Jun 28, 2005 at 14:45:06 -0400,
Stephen Frost <sfrost@snowman.net> wrote:

If you are the owner of the object to be changed (following the normal
owner checking rules) AND would still be considered the owner of the
object *after* the change, then you can change the ownership.

That still isn't a good idea, because the new owner may not have had
access to create the object you just gave to them. Or you may not have
had access to drop the object you just gave away. That is going to
be a security hole.

Thinking about it some more, drops wouldn't be an issue since the owner
can always drop objects.

Creating objects in particular schemas or databases is not something that
all roles may be able to do.

* Bruno Wolff III (bruno@wolff.to) wrote:

On Tue, Jun 28, 2005 at 14:45:06 -0400,
Stephen Frost <sfrost@snowman.net> wrote:

If you are the owner of the object to be changed (following the normal
owner checking rules) AND would still be considered the owner of the
object *after* the change, then you can change the ownership.

That still isn't a good idea, because the new owner may not have had
access to create the object you just gave to them. Or you may not have
had access to drop the object you just gave away. That is going to
be a security hole.

If you're considered the owner of an object then you have access to drop
it already. You have to be a member of the role to which you're
changing the ownership. That role not having permission to create the
object in place is an interesting question. That's an issue for SET
ROLE too, to some extent I think, do you still have your role's
permissions after you've SET ROLE to another role? If not then you'd
have to grant CREATE on the schema to the role in order to create
objects owned by that role, and I don't think that's necessairly
something you'd want to do.

Thanks,

Stephen

Stephen Frost wrote:

I can perhaps see a special case for SECURITY DEFINER functions but if
we're going to special case them I'd think we'd need to make them only
be creatable/modifiable at all by superusers or add another flag to the
role to allow that.

I agree that owner changes of SECURITY DEFINER functions seem dangerous. I
would follow Stephen's idea that SECURITY DEFINER functions should only be
creatable/modifiable by superusers.

This would be similar to unix, where setting the suid/sgid bits is usually
only allowed to root.

Best Regards,
Michael Paesold

Stephen Frost wrote:

If you're considered the owner of an object then you have access to drop
it already. You have to be a member of the role to which you're
changing the ownership. That role not having permission to create the
object in place is an interesting question. That's an issue for SET
ROLE too, to some extent I think, do you still have your role's
permissions after you've SET ROLE to another role?

For me this would be the "natural" way how SET ROLE would behave. This is
unix'ism again, but using setuid to become another user, you loose the
privileges of the old user context.
Therefore SET ROLE should not inherit privileges from the other role. This
seems to be the safes approach.

Nevertheless, what does the standard say?

If not then you'd
have to grant CREATE on the schema to the role in order to create
objects owned by that role, and I don't think that's necessairly
something you'd want to do.

Right, that's an issue. But since the new role will be the *owner* of the
object, it *should* really have create-privileges in that schema. So the
above way seems to be correct anyway.

Best Regards,
Michael Paesold

* Bruno Wolff III (bruno@wolff.to) wrote:

Thinking about it some more, drops wouldn't be an issue since the owner
can always drop objects.

Right.

Creating objects in particular schemas or databases is not something that
all roles may be able to do.

Yeah, I'm not entirely sure what I think about this issue. If you're
not allowed to change ownership of objects and SET ROLE drops your
regular ROLE's privileges then the role which owns the object originally
(and which you're required to be in) must have had create access to that
schema at some point.

I can see requiring the role that's changing the ownership to have
create access to the schema in which the object that's being changed is
in.

Thanks,

Stephen

* Michael Paesold (mpaesold@gmx.at) wrote:

Stephen Frost wrote:

If you're considered the owner of an object then you have access to drop
it already. You have to be a member of the role to which you're
changing the ownership. That role not having permission to create the
object in place is an interesting question. That's an issue for SET
ROLE too, to some extent I think, do you still have your role's
permissions after you've SET ROLE to another role?

For me this would be the "natural" way how SET ROLE would behave. This is
unix'ism again, but using setuid to become another user, you loose the
privileges of the old user context.
Therefore SET ROLE should not inherit privileges from the other role. This
seems to be the safes approach.

Nevertheless, what does the standard say?

Hmm, it says there's a stack and that the thing on top is what's
currently used, so it sounds like it would drop the privs too, but imv
it's not entirely clear.

If not then you'd
have to grant CREATE on the schema to the role in order to create
objects owned by that role, and I don't think that's necessairly
something you'd want to do.

Right, that's an issue. But since the new role will be the *owner* of the
object, it *should* really have create-privileges in that schema. So the
above way seems to be correct anyway.

I'm not entirely sure that you'd necessairly want the role to have
create privileges on the schema even when it owns things in the schema
but the more I think about it that doesn't seem all that unreasonable
either. I don't think it'd be very difficult to add such a check to the
ALTER OWNER code too though.

In general, and perhaps as a unix'ism to some extent, I don't
particularly like having to su to people. To get all the other
permissions which the role has you don't have to 'su' currently, and
personally I like that and think that's correct for a role-based
environment (unlike unix where you have users and groups).

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Bruno Wolff III (bruno@wolff.to) wrote:

Creating objects in particular schemas or databases is not something that
all roles may be able to do.

Yeah, I'm not entirely sure what I think about this issue.

We have a precedent, which is that RENAME checks for create rights.
If you want to lean on the argument that this is just a shortcut for
dropping the object and then recreating it somewhere else, then you
need (a) the right to drop the object --- which is inherent in being
the old owner, and (b) the right to create the new object, which means
that (b1) you can become the role you wish to have owning the object,
and (b2) *as that role* you would have the rights needed to create the
object.

Stephen's original analysis covers (a) and (b1) but not (b2). With (b2)
I'd agree that it's just a useful shortcut.

I don't see a need to treat SECURITY DEFINER functions as
superuser-only. We've had that facility since 7.3 or so and no one
has complained that it's too dangerous.

regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Bruno Wolff III (bruno@wolff.to) wrote:

Creating objects in particular schemas or databases is not something that
all roles may be able to do.

Yeah, I'm not entirely sure what I think about this issue.

We have a precedent, which is that RENAME checks for create rights.

Ah, ok. Precedent is good.

If you want to lean on the argument that this is just a shortcut for
dropping the object and then recreating it somewhere else, then you
need (a) the right to drop the object --- which is inherent in being
the old owner, and (b) the right to create the new object, which means
that (b1) you can become the role you wish to have owning the object,
and (b2) *as that role* you would have the rights needed to create the
object.

Stephen's original analysis covers (a) and (b1) but not (b2). With (b2)
I'd agree that it's just a useful shortcut.

Right. Ok, I'll develop a patch which covers (a), (b1) and (b2). I'll
also go through all of the superuser() calls in src/backend/commands/
and check for other places we may need *_ownercheck calls.

I expect to have the patch done either tonight or tommorow.

Thanks,

Stephen

Greetings,

Attached please find a patch to change how the permissions checking
for alter-owner is done. With roles there can be more than one
'owner' of an object and therefore it becomes sensible to allow
specific cases of ownership change for non-superusers.

The permission checks for change-owner follow the alter-rename
precedent that the new owner must have permission to create the object
in the schema.

The roles patch previously applied did not require the role for
which a database is being created to have createdb privileges, or for
the role for which a schema is being created to have create
privileges on the database (the role doing the creation did have to
have those privileges though, of course).

For 'container' type objects this seems reasonable. 'container' type
objects are unlike others in a few ways, but one of the more notable
differences for this case is that an owner may be specified as part of
the create command.

To support cleaning up the various checks, I also went ahead and
modified is_member_of_role() to always return true when asked if a
superuser is in a given role. This seems reasonable, won't affect
what's actually seen in the various tables, and allows us to eliminate
explicit superuser() checks in a number of places.

I have also reviewed the other superuser() calls in
src/backend/commands/ and feel pretty comfortable that they're all
necessary, reasonable, and don't need to be replaced with
*_ownercheck or other calls.

The specific changes which have been changed, by file:
aggregatecmds.c, alter-owner:
alter-owner checks:
User is owner of the to-be-changed object
User is a member of the new owner's role
New owner is permitted to create objects in the schema
Superuser() requirement removed

conversioncmds.c, rename:
rename-checks:
Changed from superuser() or same-roleId to pg_conversion_ownercheck
alter-owner checks:
User is owner of the to-be-changed object
User is a member of the new owner's role
New owner is permitted to create objects in the schema
Superuser() requirement removed

dbcommands.c:
Moved superuser() check to have_createdb_privilege
Cleaned up permissions checking in createdb and rename
alter-owner checks:
User is owner of the database
User is a member of the new owner's role
User has createdb privilege

functioncmds.c:
alter-owner checks:
User is owner of the function
User is a member of the new owner's role
New owner is permitted to create objects in the schema

opclasscmds.c:
alter-owner checks:
User is owner of the object
User is a member of the new owner's role
New owner has permission to create objects in the schema

operatorcmds.c:
alter-owner checks:
User is owner of the object
User is a member of the new owner's role
New owner has permission to create objects in the schema

schemacmds.c:
Cleaned up create schema identify changing/setting/checking
(This code was quite different from all the other create functions,
these changes make it much more closely match createdb)
alter-owner checks:
User is owner of the schema
User is a member of the new owner's role
User has create privilege on database

tablecmds.c:
alter-owner checks:
User is owner of the object
User is a member of the new owner's role
New owner has permission to create objects in the schema

tablespace.c:
alter-owner checks:
User is owner of the tablespace
User is a member of the new owner's role
(No create-tablespace permission to check, tablespaces must be
created by superusers and so alter-owner here really only matters
if the superuser changed the tablespace owner to a non-superuser
and then that non-superuser wants to change the ownership to yet
another user, the other option would be to continue to force
superuser-only for tablespace owner changes but I'm not sure I
see the point if the superuser trusts the non-superuser enough to
give them a tablespace...)

typecmds.c:
alter-owner checks:
User is owner of the object
User is a member of the new owner's role
New owner has permission to create objects in the schema

Many thanks. As always, comments, questions, concerns, please let me
know.

Thanks again,

Stephen