On Fri, Apr 15, 2016 at 8:56 AM, Stephen Frost <sfr...@snowman.net> wrote:
> Robert, > > * Robert Haas (robertmh...@gmail.com) wrote: > > On Wed, Apr 13, 2016 at 6:53 PM, Stephen Frost <sfr...@snowman.net> > wrote: > > > Requiring that SET ROLE be allowed will mean that many more paths must > > > be checked and adjusted, such as in all of the CreateObject statements > > > and potentionally many other paths that I'm not thinking of here, not > > > the least of which are all of the *existing* checks near > > > get_rolespec_oid/tuple which will require additional checks for the > > > CURRENT_USER case to see if the current user is a default role. > > > > I don't get it. I think that these new roles should work just like > > any other roles, except for existing at initdb time. I don't see why > > they would require checking or adjusting any code-paths at all. It > > would presumably have made the patch you committed smaller and > > simpler. The only thing you'd need to do is (approximately) not dump > > them. > > Perhaps it would be helpful to return to the initial goal of these > default roles. > > We've identified certain privileges which are currently superuser-only > and we would like to be able to be GRANT those to non-superuser roles. > Where our GRANT system is able to provide the granularity desired, we > have simply removed the superuser checks, set up appropriate default > values and, through the "pg_dump dump catalog ACLs" patch, allowed > administrators to make the changes to those objects via the > 'GRANT privilege ON object TO role' system. > > For those cases where our GRANT system is unable to provide the > granularity desired and it isn't sensible to extend the GRANT system to > cover the exact granularity we wish to provide, we needed to come up > with an alternative approach. That approach is the use of special > default roles, where we can allow exactly the level of granularity > desired and give administrators a way to grant those privileges to > users. > I didn't fully comprehend this before, but now I understand why additional checks beyond simple "has inherited the necessary grant" are needed. The checks being performed are not for itemized granted capabilities Further, there seems to be no particular use-case which is satisfied by > allowing SET ROLE'ing to the default roles or for those roles to own > objects; indeed, it seems more likely to simply spark confusion and > ultimately may prevent administrators from making use of this system for > granting privileges as they are unable to GRANT just the specific > privilege they wish to. > > And I'm have trouble imaging what kind of corner-case could result from not allowing a role to assume ownership of a role it is a member of. While we do have the capability to required SET ROLE it is optional: any code paths dealing with grants have to assume that the current role receives permission via inheritance - and all we are doing here is ensuring that is the situation. It now occurs to me that perhaps we can improve on this situation in the > future by formally supporting a role attribute that is "do not allow SET > ROLE to this user" and then changing the default roles to have that > attribute set (and disallowing users from changing it). That's just > additional flexibility over what the current patch does, however, but > perhaps it helps illustrate that there are cases where only the > privileges of the role are intended to be GRANT'd and not the ability to > SET ROLE to the role or to create objects as that role. > That is where my first response was heading but it was definitely scope creep so I didn't bring it up. Basically, an "INHERITONLY" modifier in addition to INHERIT and NOINHERIT. I had stated the having a role that neither provides inheritance nor changing-to is pointless but I am now unsure if that is true. It would depend upon whether a LOGIN role is one that is considered having been SET ROLE to or not. If not, then a "LOGINONLY" combination would work such that a role with that combination would only have whatever grants are given to it and is not allowed to be changed to by a different role - i.e., it could only be used by someone logging into the system specifically as that role. It likewise complements "NOLOGIN + LOGIN". It wouldn't directly preclude said role from itself SET ROLE'ing to a different role though. And, for all that, I do realize I'm using these terms somewhat contrary to reality since INHERIT is done on the receiver and not the giver. The wording would have to change to reflect the POV of the giver. That is, INHERITONLY should be something done to a role that prevents it from being SET TO as opposed to NOINHERIT which forces a role to use SET TO to access the permissions of all roles in which it is a member. David J.
