Re: replacing role-level NOINHERIT with a grant-level option

Greetings,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Mon, Feb 7, 2022 at 11:13 AM Joe Conway <mail(at)joeconway(dot)com> wrote:
> > > It seems to me that the INHERIT role flag isn't very well-considered.
> > > Inheritance, or the lack of it, ought to be decided separately for
> > > each inherited role. However, that would be a major architectural
> > > change.
> >
> > Agreed -- that would be useful.
>
> Is this a kind of change people would support? Here's a quick sketch:

On the whole, moving things to pg_auth_members when they're about
relationships between roles makes more sense to me too (ISTR mentioning
that somewhere or at least thinking about it but not sure where and it
doesn't really matter).

> It's been proposed elsewhere by Stephen and others that we ought to
> have the ability to grant ADMIN OPTION on a role without granting
> membership in that role. There's some overlap between these two
> proposals. If your concern is just about accident prevention, you will
> be happy to use this instead of that. If you want real access
> restrictions, you will want that, not this. I think that's OK. Doing
> this first doesn't mean we can't do that later. What about the
> reverse? Could we add ADMIN-without-membership first, and then decide
> whether to do this later? Maybe so, but I have come to feel that
> NOINHERIT is such an ugly wart that we'll be happier the sooner we
> kill it. It seems to make every discussion that we have about any
> other potential change in this area more difficult, and I venture that
> ADMIN-without-membership wouldn't turn out to be an exception.

Being able to segregate the control over privileges from the control
over objects is definitely helpful in some environments. I don't see
any strong reason that one must happen before the other and am happy to
defer to whomever has cycles to work on these things to sort that out.

> Based on previous discussions I think that, long term, we're probably
> headed toward a future where role grants have a bunch of different
> flags, each of which controls a single behavior: whether you can
> implicitly use the privileges of the role, whether you can access them
> via SET ROLE, whether you can grant the role to others, or revoke it
> from them, etc. I don't know exactly what the final list will look
> like, and hopefully it won't be so long that it makes us all wish we
> were dead, but there doesn't seem to be any possibility that implicit
> inheritance of permissions won't be in that list. I spent a few
> minutes considering whether I ought to instead propose that we just
> nuke NOINHERIT from orbit and replace it with absolutely nothing, and
> concluded that such a proposal had no hope of attracting consensus.

I agree that a future where there's more granularity in terms of role
grants would be a better place than where we are now. I'd be -1 on just
removing 'NOINHERIT', to no one's surprise, I'm sure.

> Perhaps the idea of replacing it with that is more powerful and at
> least IMHO cleaner will.

-EPARSEFAIL (tho I'm guessing you were intending to say that replacing
the role-attribute noinherit with something more powerful would be a
generally agreeable way forward, which I would generally support)

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Thu, Jun 2, 2022 at 12:26 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > 1. Extend the GRANT role_name TO role_name [ WITH ADMIN OPTION ] with
> > a new, optional clause, something like WITH NO INHERIT or WITH
> > NOINHERIT or WITHOUT INHERIT.
>
> I just realized that, with ADMIN OPTION, you can change your mind
> after the initial grant, and we probably would want to allow the same
> kind of thing here. The existing syntax is:

Yes.

> 1. GRANT foo TO bar [WITH ADMIN OPTION];
> 2. REVOKE foo FROM bar;
> 3. REVOKE ADMIN OPTION FOR foo FROM bar;
>
> To grant the admin option later, you just use (1) again and this time
> include WITH ADMIN OPTION. To revoke it without removing the grant
> entirely, you use (3). This could easily be generalized to any number
> of options all of which are Boolean properties and all of which have a
> default value of false, but INHERIT is a Boolean property with a
> default value of true, and calling the property NOINHERIT to dodge
> that issue seems dumb. I'd like to find a way to extend the syntax
> that can accommodate not only INHERIT as an option, but any other
> options we might want to add in the future, and maybe even leave room
> for non-Boolean properties, just in case. The question of which
> options we think it valuable to add is separate from what the syntax
> ought to be, so for syntax discussion purposes let's suppose we want
> to add three new options: FRUIT, which can be strawberry, banana, or
> kiwi; CHOCOLATE, a Boolean whose default value is true; and SARDINES,
> another Boolean whose default value is false. Then:
>
> GRANT foo TO bar WITH FRUIT STRAWBERRY;
> GRANT foo TO bar WITH CHOCOLATE FALSE;
> GRANT foo TO bar WITH SARDINES TRUE;
> GRANT foo TO bar WITH SARDINES; -- same as previous
> GRANT foo TO bar WITH SARDINES OPTION; -- also same as previous
> GRANT foo TO bar WITH FRUIT KIWI, SARDINES; -- multiple options
> GRANT foo TO bar WITH CHOCOLATE OPTION, SARDINES OPTION; -- dubious combination
>
> In other words, you write a comma-separated list of option names. Each
> option name can be followed by an option value or by the word OPTION.
> If it is followed by the word OPTION or by nothing, the option value
> is taken to be true. This would mean that, going forward, any of the
> following would work: (a) GRANT foo TO bar WITH ADMIN OPTION, (b)
> GRANT foo TO BAR WITH ADMIN, (c) GRANT foo TO BAR WITH ADMIN TRUE.

Seems generally reasonable... though I'm thinking that we should make
OPTION only be accepted for ADMIN as TRUE rather than having it
magically work for all the other things because ... why would we? What
if we decided later that we wanted a FRUIT OPTION (to use your example
of FRUIT being an enum). I don't feel very strongly on that point
though (but what if you wrote FRUIT OPTION and FRUIT isn't able to just
be TRUE? Would that blow up, or?).

> To revoke a grant entirely, you just say REVOKE foo FROM bar, as now.
> To change an option for an existing grant, you can re-execute the
> grant statement with a different WITH clause. Any options that are
> explicitly mentioned will be changed to have the associated values;
> unmentioned options will retain their existing values. If you want to
> change the value of a Boolean option to false, you have a second
> option, which is to write "REVOKE option_name OPTION FOR foo FROM
> bar," which means exactly the same thing as "GRANT foo TO bar WITH
> option_name FALSE".

I'm a bit concerned about this because, iiuc, it would mean:

GRANT foo TO bar WITH FRUIT KIWI, SARDINES;
GRANT foo TO bar WITH FRUIT STRAWBERRY;

would mean that the GRANT of FRUIT would then *only* have STRAWBERRY,
right? The first GRANT in this example would essentially be entirely
undone by the second GRANT. Having GRANT remove things would be new, I
believe. An alternative would be to have GRANT continue to always be
'additive' and require using REVOKE to remove them, ala:

REVOKE FRUIT OPTION FOR foo FROM bar;
GRANT foo TO bar WITH FRUIT STRAWBERRY;

Another tricky item to consider here is the "implicitly includes role
membership" part of the GRANT. That is, the spec explicitly says:

GRANT foo TO bar WITH ADMIN OPTION;

Means that 'foo' is grant'd to 'bar' (plus the admin option thing is
done). In your proposal, does:

GRANT foo TO bar WITH FRUIT STRAWBERRY;

mean that 'foo' is grant'd to 'bar' too? Seems to be closest to current
usage and the spec's ideas on these things. I'm thinking that could be
dealt with by having a MEMBERSHIP option (which would be a separate
column in pg_auth_members and default would be 'true') but otherwise
using exactly what you have here, eg:

GRANT foo TO bar;
GRANT foo TO bar WITH MEMBERSHIP; -- identical to above
GRANT foo TO bar WITH MEMBERSHIP TRUE; -- identical to above
GRANT foo TO bar WITH MEMBERSHIP OPTION; -- identical to above
GRANT foo TO bar WITH MEMBERSHIP FALSE; -- no-op
GRANT foo TO bar WITH MEMBERSHIP FALSE, FRUIT STRAWBERRY; -- only fruit
GRANT foo TO bar WITH FRUIT STRAWBERRY; -- with membership and fruit

Note also that the spec defines a WITH HIERARCHY OPTION too (but it's
explicitly defined for table privileges not roles, but still part of the
overall GRANT syntax) and after those WITH's has a GRANTED BY syntax
(for both roles and object privileges, which are actually distinct
things in the spec), just when thinking about what will/won't work in
the grammar.

> In terms of what options to offer, the most obvious idea is to just
> add INHERIT as a Boolean option which is true by default. We could go
> further and also add a SET option, with the idea that INHERIT OPTION
> controls whether you can exercise the privileges of the role without
> SET ROLE, and SET OPTION controls whether you can switch to that role
> using the SET ROLE command. Those two things together would give us a
> way to get to the admin-without-membership concept that we have
> previously discussed: GRANT foo TO BAR WITH ADMIN TRUE, INHERIT FALSE,
> SET FALSE sounds like it should do the trick.

Guess this is pretty close to what I just suggested above, but using
"SET" for that strikes me as quite generic while MEMBERSHIP is clearer.

Not really sure how I feel about what the results of:

GRANT foo TO bar;
GRANT foo TO bar WITH MEMBERSHIP FALSE;

should be with this but I think I'm leaning towards the "GRANT isn't for
removing stuff" side. A GRANT is for giving things, REVOKE is for
taking things away, meaning that the second statement above would,
again, be a no-op.

> I briefly considered suggesting that the way to set a Boolean-valued
> option to false ought to be to write "NO option_name" rather than
> "option_name FALSE", since it reads more naturally, but I proposed
> this instead because it's more like what we do for other options lists
> (cf. EXPLAIN, VACUUM, COPY).

The spec has WITH ADMIN OPTION and WITH HIERARCHY OPTION, so going in
that general direction seems a bit better. Also, as mentioned, GRANT
doesn't really do 'subtractive' things, so having a 'NO' in there seems
to go against the grain.

Thanks!

Stephen