Thread: Users/Groups -> Roles

Greetings,

  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

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

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

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:
> > Tom, if you're watching, are you working on this?  I can probably spend
> > some time today on it, if that'd be helpful.
>
> I am not; I was hoping you'd deal with SET ROLE.  Is it really much
> different from SET SESSION AUTHORIZATION?

Here's what I've got done so far on SET ROLE.  I wasn't able to get as
much done as I'd hoped to.  This is mostly just to get something posted
before the end of today in case some might think it's more of a feature
than a bug needing to be fixed (which is what I'd consider it,
personally).  I'll try and work on it some this weekend, but in the US
it's a holiday weekend and I'm pretty busy. :/

    Thanks,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
>   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.

Applied with minor revisions.  The patch as submitted suffered a certain
amount of copy-and-paste-itis (eg, trying to use pg_type_ownercheck on
an opclass), and I really disliked using ACLCHECK_NOT_OWNER as the way
to report "you can't assign ownership to that role because you are not
a member of it".  So I made a separate error message for that case.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> >   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.
>
> Applied with minor revisions.  The patch as submitted suffered a certain
> amount of copy-and-paste-itis (eg, trying to use pg_type_ownercheck on
> an opclass), and I really disliked using ACLCHECK_NOT_OWNER as the way
> to report "you can't assign ownership to that role because you are not
> a member of it".  So I made a separate error message for that case.

Great, thanks!  Sorry about the copy-and-paste-itis...  Must have been a
case I wasn't sure about.  The different error message makes perfect
sense.  I see you also did the superuser-in-every-role change that I had
included, thanks.

When writing this patch it occurred to me that we nuke our
member-of-role cache for one-off lookups on occation.  I don't
particularly like that, especially when we *know* it's a one-off lookup,
so I was considering adding a function for the one-off lookup case but
I couldn't come up with a way to avoid a fair bit of mostly-the-same
code as the current cache-regen code, without making the cache-regen
code alot slower which would negate the point.

Just some thoughts.

    Thanks again,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> When writing this patch it occurred to me that we nuke our
> member-of-role cache for one-off lookups on occation.  I don't
> particularly like that, especially when we *know* it's a one-off lookup,

Yeah.  What I had been thinking about is that maybe it shouldn't be just
a one-element cache.  At the moment though we have no performance data
to justify complicating matters (heck, not even any to prove we need a
cache at all...)  It'd be smart to try to profile some realistic cases
before spending any time coding.

            regards, tom lane