Re: [PATCHES] Roles - SET ROLE Updated
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) ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Roles - SET ROLE Updated
* Tom Lane ([EMAIL PROTECTED]) 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 signature.asc Description: Digital signature
Re: [PATCHES] Roles - SET ROLE Updated
* Tom Lane ([EMAIL PROTECTED]) 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 signature.asc Description: Digital signature
Re: [PATCHES] Roles - SET ROLE Updated
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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Roles - SET ROLE Updated
Stephen Frost <[EMAIL PROTECTED]> writes: > * Tom Lane ([EMAIL PROTECTED]) 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Roles - SET ROLE Updated
* Tom Lane ([EMAIL PROTECTED]) 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 signature.asc Description: Digital signature
Re: [PATCHES] Roles - SET ROLE Updated
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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Roles - SET ROLE Updated
Stephen Frost <[EMAIL PROTECTED]> writes: > * Tom Lane ([EMAIL PROTECTED]) 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Roles - SET ROLE Updated
* Tom Lane ([EMAIL PROTECTED]) 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 signature.asc Description: Digital signature
Re: [PATCHES] Roles - SET ROLE Updated
Stephen Frost <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Roles - SET ROLE Updated
* Stephen Frost ([EMAIL PROTECTED]) wrote:
> * Tom Lane ([EMAIL PROTECTED]) wrote:
> > Stephen Frost <[EMAIL PROTECTED]> 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
Index: src/backend/catalog/aclchk.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.114
diff -c -r1.114 aclchk.c
*** src/backend/catalog/aclchk.c28 Jun 2005 19:51:21 - 1.114
--- src/backend/catalog/aclchk.c3 Jul 2005 18:30:18 -
***
*** 76,88
* GRANT or REVOKE, we pretend he is the object owner.This ensures
that
* all granted privileges appear to flow from the object owner, and there
* are never multiple "original sources" of a privilege.
*/
static Oid
select_grantor(Oid ownerId)
{
Oid grantorId;
! grantorId = GetUserId();
/* fast path if no difference */
if (grantorId == ownerId)
--- 76,93
* GRANT or REVOKE, we pretend he is the object owner.This ensures
that
* all granted privileges appear to flow from the object owner, and there
* are never multiple "original sources" of a privilege.
+ *
+ * NOTE: SQL spec defines that, unless 'GRANTED BY' is set, the grantor
+ * is the CURRENT_USER if it is set and the CURRENT_ROLE otherwise.
+ * CURRENT_USER is always set for us, so we use that. SQL spec does not
+ * show a way which CURRENT_USER could become unset.
*/
static Oid
select_grantor(Oid ownerId)
{
Oid grantorId;
! grantorId = GetCurrentUserId();
/* fast path if no difference */
if (grantorId == ownerId)
Index: src/backend/catalog/information_schema.sql
===
RCS file: /projects/cvsroot/pgsql/src/backend/catalog/information_schema.sql,v
retrieving revision 1.29
diff -c -r1.29 information_schema.sql
*** src/backend/catalog/information_schema.sql 28 Jun 2005 05:08:52 -
1.29
--- src/backend/catalog/information_schema.sql 3 Jul 2005 18:30:18 -
***
*** 216,222
FROM ((pg_auth_members m join pg_authid a ON (m.roleid = a.oid))
join pg_authid b ON (m.member = b.oid))
! WHERE b.rolname = current_user;
GRANT SELECT ON applicable_roles TO PUBLIC;
--- 216,222
FROM ((pg_auth_members m join pg_authid a ON (m.roleid = a.oid))
join pg_authid b ON (m.member = b.oid))
! WHERE has_role(current_user,b.rolname);
GRANT SELECT ON applicable_roles TO PUBLIC;
***
*** 239,245
pg_user u
WHERE rs.oid = con.connamespace
AND u.usesysid = coalesce(c.relowner, t.typowner)
! AND u.usename = current_user
AND con.contype = 'c';
GRANT SELECT ON check_constraints TO PUBLIC;
--- 239,245
pg_user u
WHERE rs.oid = con.connamespace
AND u.usesysid = coalesce(c.relowner, t.typowner)
! AND (u.usename = current_user or
has_role(COALESCE(CURRENT_ROLE,CURRENT_USER),u.usename))
AND con.contype = 'c';
GRANT SELECT ON check_constraints TO PUBLIC;
***
*** 271,277
AND c.relkind IN ('r', 'v')
AND a.attnum > 0
AND NOT a.attisdropped
! AND u.usename = current_user;
GRANT SELECT ON column_domain_usage TO PUBLIC;
--- 271,277
AND c.relkind IN ('r', 'v')
AND a.attnum > 0
AND NOT a.attisdropped
! AND (u.usename = current_user or
has_role(COALESCE(CURRENT_ROLE,CURRENT_USER),u.usename));
GRANT SELECT ON column_domain_usage TO PUBLIC;
***
*** 316,322
-
