Re: [PATCHES] Roles - SET ROLE Updated

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