Re: [PATCHES] Roles - SET ROLE Updated

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > To support having certain roles turned on and certain roles turned off
> > would be some additional effort. I think we'd need a list of
> > 'ENABLED_ROLES' and then correct recursion based off of that list
> > instead of just starting from a single point like we do now.
>
> That seems fairly ugly and messy though, because it wouldn't readily
> translate to the case of SECURITY DEFINER procedures.

SECURITY DEFINER is just another level in the stack, one which is above
all others. At least, in my thinking anyway. We don't actually need a
stack, just need to save off the old user/role list and reinstate it
after the SECURITY DEFINER function.

> Let's review the bidding. As I presently understand it:
[...]

That all looks right.

> What I would like to do is say that there is exactly one active
> authorization identifier at any instant. That means that if you SET
> ROLE then you no longer have the privileges attached to SESSION_USER
> (except the right to SET ROLE to a different one of his roles).
> This is contrary to spec but it seems to be a pretty widely accepted
> way of doing things; and it makes SET ROLE effectively equivalent to
> the change in privileges associated with entering a SECURITY DEFINER
> procedure owned by that role, so there's a much cleaner conceptual
> model.

I'd rather get away from the idea of only one 'active authorization
identifier'. That's rather limiting and I don't really see the point.
All that's changing is what seeds the initial list of roles prior to
doing the full hierarchical resolution to populate the full list of
roles. The original patch used CURRENT_USER or CURRENT_ROLE if it was
set. This just adds CURRENT_USER back in at the end if we used
CURRENT_ROLE initially. Except in a 'SET ROLE all' case, when we seed
with CURRENT_USER, as I had originally.

> Furthermore, I still feel that both CURRENT_USER and CURRENT_ROLE should
> reflect this single active authorization id. The spec's distinction is
> artificial and doesn't gain anything, except the ability to break code
> that assumes CURRENT_USER is always meaningful.

There is a distinction there though, at least as the SQL spec works, and
really there is when you consider that CURRENT_USER is really the
SESSION_USER generally.

> 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.

> 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.

I really don't care for this idea as an alternative to what the spec
calls for. I don't think it puts us much closer to the spec's semantics
unless you let a user change pg_authid wrt this and that seems like
quite a bad idea.

> Thoughts?

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.

Thanks,

Stephen