Thread: Roles - SET ROLE Updated

* 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 a much better version of the SET ROLE work.  I'm reasonably happy
with it.  The only parts I don't like are that I had to do some ugly
things in gram.y to avoid making NONE reserved, and I can't seem to see
how to avoid having ROLE be reserved (I understand it was reserved in
SQL99 but not in SQL2003...).

Another issue that I noticed is that when I created a role which didn't
have login permissions, SET ROLE to that role and then created a table,
the 'owner' for the object shown by \d came up NULL.  This is almost
certainly because \d is using pg_user which filters out roles which
can't log in.  Personally, I disagree with pg_user not having all roles
in it but regardless this needs to be fixed and it'd probably just be
best to update psql to use pg_authid and pg_auth_members, have a \dr,
\dm, etc.  I'll try to work on that next unless someone else is already.

    Thanks,

        Stephen

* Stephen Frost (sfrost@snowman.net) wrote:
> * 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 a much better version of the SET ROLE work.  I'm reasonably happy
> with it.  The only parts I don't like are that I had to do some ugly
> things in gram.y to avoid making NONE reserved, and I can't seem to see
> how to avoid having ROLE be reserved (I understand it was reserved in
> SQL99 but not in SQL2003...).

Updated yet again, fixing a bug in the prior one that caused it to not
work properly, and some additional things:

Added a 'has_role' function that's basically is_member_of_role for the
masses.  Updated information_schema to use has_role for permissions
checks in addition to the straight '=' owner-check.  Also fixed up
enabled_roles and applicable_roles views.  This depends somewhat on part
of my other patch where I modified is_member_of_role to always return
true for superuser().  If that doesn't end up being done then we'll need
to add some explicit superuser() checks in the SetCurrentRoleId() logic.

    Thanks,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
>> Here's a much better version of the SET ROLE work.  I'm reasonably happy
>> with it.  The only parts I don't like are that I had to do some ugly
>> things in gram.y to avoid making NONE reserved, and I can't seem to see
>> how to avoid having ROLE be reserved (I understand it was reserved in
>> SQL99 but not in SQL2003...).

> Updated yet again, fixing a bug in the prior one that caused it to not
> work properly, and some additional things:

I don't think this patch works; it certainly doesn't do what I'd expect
to happen with SECURITY DEFINER functions.  At the very least you'd need
to make fmgr_security_definer save/restore the current role setting.
But I doubt that this is even the direction we want to head in.

After rereading SQL99 4.31, I don't think there is any need to
distinguish CURRENT_USER from CURRENT_ROLE, mainly because our
implementation does not distinguish users from roles at all.
(Which I think is good.)  So ISTM we should not change GetUserId()
as you propose, but leave it alone and implement SetRole approximately
like SetSessionUserId is implemented, ie, setting a background value
that may sometimes get copied into CurrentUserId.  The "stack" aspect
only matters to the extent that SetRoleId has precedence over
SetSessionUserId for determining the outside-a-transaction value of
CurrentUserId.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> After rereading SQL99 4.31, I don't think there is any need to
> distinguish CURRENT_USER from CURRENT_ROLE, mainly because our
> implementation does not distinguish users from roles at all.

CURRENT_USER and CURRENT_ROLE can have different values, as I understand
SQL2003, and there are places where one is used instead of the other
(such as with the 'grantor' in grants, according to SQL2003 the
'grantor' should be the CURRENT_USER, regardless of if CURRENT_ROLE is
set or not).  I believe this is a seperate issue from how we implement
the accounts themselves (where we don't differentiate between users and
roles, which is fine).

> (Which I think is good.)  So ISTM we should not change GetUserId()
> as you propose, but leave it alone and implement SetRole approximately
> like SetSessionUserId is implemented, ie, setting a background value
> that may sometimes get copied into CurrentUserId.  The "stack" aspect
> only matters to the extent that SetRoleId has precedence over
> SetSessionUserId for determining the outside-a-transaction value of
> CurrentUserId.

SQL2003 also states that CURRENT_ROLE is NULL initially.  I suppose we
could implement CURRENT_ROLE as a check to see if CurrentUserId is
different from CurrentRoleId and return NULL in that case and then just
always use CurrentRoleId (or CurrentUserId, whichever).  That would
avoid having to change how GetUserId() works though this doesn't seem
like a huge change to the patch itself.  Do you want me to rework the
patch along these lines or are you already working on it?  I've been
having a bit of computer trouble but I think I could get the patch
changed/updated by Monday.

    Thanks,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> After rereading SQL99 4.31, I don't think there is any need to
>> distinguish CURRENT_USER from CURRENT_ROLE, mainly because our
>> implementation does not distinguish users from roles at all.

> CURRENT_USER and CURRENT_ROLE can have different values, as I understand
> SQL2003, and there are places where one is used instead of the other

It's possible for CURRENT_ROLE to be null according to the spec; if you
like we could implement that as returning what the current outer-level
SET ROLE value is (which would then make it semantically more like
SESSION_USER than CURRENT_USER).  I don't think CURRENT_USER should ever
be allowed to be null, or to be different from the active authorization
identifier, first because it's silly and second because it will break
existing applications that depend on CURRENT_USER for authorization
checking.

Given that we don't really distinguish users and roles, I would be
inclined to make the same argument for CURRENT_ROLE too, leaving
SHOW ROLE (and its function equivalent) as the only way to see what
you SET ROLE to.  But it's less likely to break existing apps if we
don't.

> (such as with the 'grantor' in grants, according to SQL2003 the
> 'grantor' should be the CURRENT_USER, regardless of if CURRENT_ROLE is
> set or not).

Exactly.  CURRENT_USER has to be the active authorization identifier.

> Do you want me to rework the
> patch along these lines or are you already working on it?

I'm working on it ...

            regards, tom lane

BTW, I realized we do not support granting roles to PUBLIC:

regression=# create role r;
CREATE ROLE
regression=# grant r to public;
ERROR:  role "public" does not exist

but as far as I can tell SQL99 expects this to work.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> BTW, I realized we do not support granting roles to PUBLIC:
>
> regression=# create role r;
> CREATE ROLE
> regression=# grant r to public;
> ERROR:  role "public" does not exist
>
> but as far as I can tell SQL99 expects this to work.

Indeed, I believe you're correct, sorry about missing that.

    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> BTW, I realized we do not support granting roles to PUBLIC:
>>
>> regression=# create role r;
>> CREATE ROLE
>> regression=# grant r to public;
>> ERROR:  role "public" does not exist
>>
>> but as far as I can tell SQL99 expects this to work.

> Indeed, I believe you're correct, sorry about missing that.

However, on second thought I'm not sure that this is sensible anyway.

Consider that every role is implicitly a member of PUBLIC --- so isn't
the above a creation of a circular membership loop, which is (for good
reason) forbidden by the spec?

            regards, tom lane

Another issue: I like the has_role() function and in fact think it needs
to come in multiple variants just like has_table_privilege and friends:

    has_role(name, name)
    has_role(name, oid)
    has_role(oid, name)
    has_role(oid, oid)
    has_role(name)        -- implicitly has_role(current_user, ...)
    has_role(oid)

However I'm a bit dubious about whether "has_role" isn't an invasion of
application namespace.  pg_has_role would be better, but we have the
(mis) precedent of has_table_privilege.  What do you think about calling
it "has_role_privilege"?

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> However, on second thought I'm not sure that this is sensible anyway.
>
> Consider that every role is implicitly a member of PUBLIC --- so isn't
> the above a creation of a circular membership loop, which is (for good
> reason) forbidden by the spec?

Ah, yes, you're right.  I won't claim to have considered that in the
original working of the patch though. :)

    Stephen

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Another issue: I like the has_role() function and in fact think it needs
> to come in multiple variants just like has_table_privilege and friends:
>
>     has_role(name, name)
>     has_role(name, oid)
>     has_role(oid, name)
>     has_role(oid, oid)
>     has_role(name)        -- implicitly has_role(current_user, ...)
>     has_role(oid)
>
> However I'm a bit dubious about whether "has_role" isn't an invasion of
> application namespace.  pg_has_role would be better, but we have the
> (mis) precedent of has_table_privilege.  What do you think about calling
> it "has_role_privilege"?

I thought about that originally.  It seemed a bit long to me and I felt
that having the 'privilege' of a role wasn't quite the same as having a
'role', but honestly I'm not terribly picky and on reflection a role
*is* like other objects in the catalog (I originally hadn't considered
it such), so, that's fine with me...

has_role() was another reason I was thinking about having a seperate
function for 'is_member_of_role' which didn't pollute the cache, just a
side-note.

    Thanks,

        Stephen

Tom Lane wrote:

>
>However I'm a bit dubious about whether "has_role" isn't an invasion of
>application namespace.  pg_has_role would be better, but we have the
>(mis) precedent of has_table_privilege.  What do you think about calling
>it "has_role_privilege"?
>
>
>
>

Do we need to follow a bad precedent for the sake of consistency? If
forced to choose, in general I would prefer to sacrifice consistency.

cheers

andrew (old Emersonian)