Re: [PATCHES] Users/Groups -> Roles

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)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