Re: [PATCHES] Roles - SET ROLE Updated

* Stephen Frost (sfrost@snowman.net) wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> > If we don't do it that way then we have a bunch of API that breaks down:
> > all of the has_foo_privilege functions stop working, because they don't
> > have a signature that allows both a user and a role to be passed to
> > them.
>
> It seems like there might be a way to solve this better but I'm not
> coming up with it yet.

Alright, I've thought about this some more.  I think there's two ways to
deal with this.  One is the solution you proposed below where we have
per-role 'rolinherit' and answers are based off of that.  The other is
to just base it off of the question of if 'SET ROLE all;' is allowed or
not.  If not then we basically use 'rolcanlogin' as a proxy for
'rolinherit'.  Otherwise the logic stays the same as it is currently.

I like your solution more but I see it as something to do to go above
what the spec calls for, not as a replacement for doing what the spec
requires.  I *think* with your solution we wouldn't need the explicit
'SET ROLE all;'-allowed option, as that would just correspond to setting
rolinherit = !rolcanlogin;.

> > I think we do need to invent the concept of roles (pg_authid entries)
> > that either do or do not inherit privileges from roles they are members
> > of.  A non-inheriting role can do SET ROLE to gain the privileges of a
> > role it is a member of, but it does not have those privileges without
> > SET ROLE.  We could drive this off rolcanlogin but it's probably better
> > to invent an additional flag column in pg_authid (call it rolinherit,
> > maybe).  Users by default would not inherit, which puts us a great deal
> > closer to the spec's semantics.

This sounds like a good addition, though to retain backwards
compatibility to group-based systems I'd think we'd either have to have
rolinherit on by default or cause pg_dump to turn it on when coming from
group-based systems (the latter would probably be better, if it's
possible).

> Sorry if it's not entirely coherent, I've been thinking about this alot
> over the past couple of hours.  I'm going to sleep on it and hopefully
> write up something better tommorow.  Basically my thinking is that in
> general the 'list of roles current enabled' is what we've currently got
> already and that works.  We're just changing how and what gets
> added/removed from it.

In the end I think we can have just one top-level to work from, we just
have to add CURRENT_USER to the list of roles after the resolution if
it's not in it already and it wasn't used as the base.  Otherwise I
think what I had pretty much works except that we do 'SET ROLE all;'
initially, which just sets CURRENT_ROLE = CURRENT_USER and does the
role-cache generation off that.  When a SET ROLE is done we change
CURRENT_ROLE to the new role and regenerate the role-cache based off the
CURRENT_ROLE and add CURRENT_USER to the list at the end.  This does
mean that SET ROLE can't work off the cache, but that doesn't seem all
that terrible to me.
Thanks,
    Stephen