Date: Fri, 1 Jul 2022 14:12:25 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Joe Conway <mail@joeconway.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	"Bossart, Nathan" <bossartn@amazon.com>,
	Stephen Frost <sfrost@snowman.net>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: replacing role-level NOINHERIT with a grant-level option
Message-ID: <20220701211225.GA418166@nathanxps13>
References: <20220629231939.GA362451@nathanxps13>
 <CA+Tgmobuqgr3XKujMwgsEZUxBHJA8jZSH2tVp7PC1cifePANtw@mail.gmail.com>
 <20220630232931.GA367181@nathanxps13>
 <CA+TgmoZWWqB66t4K6Mqv6TDttO1YJRJhOJO77B5H9Z8f28yc8Q@mail.gmail.com>
 <20220701025830.GA369935@nathanxps13>
 <12e2f0cb-08ff-c65f-d31e-76cea1af70ec@joeconway.com>
 <CA+TgmoZRO3tO_w+ROHW6Xe1fmQfzVyPuZ5OtbYX40w3YBQkMwA@mail.gmail.com>
 <442d7887-645d-0e31-e971-7a8dc8952d2f@joeconway.com>
 <CA+Tgmoaw_k2jooV214S=2bOBDPPLo7Nnq27-UGp6MNTfk11x5g@mail.gmail.com>
 <CA+TgmoYbjr6F6V3zVp0p4Jjd8jZf9TuCV1iHZ8w-fw9NGanW9w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoYbjr6F6V3zVp0p4Jjd8jZf9TuCV1iHZ8w-fw9NGanW9w@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20220701211225.GA418166%40nathanxps13>
Precedence: bulk

On Fri, Jul 01, 2022 at 02:59:53PM -0400, Robert Haas wrote:
> And here is a patch that makes it work that way. In this version, you
> can GRANT foo TO bar WITH INHERIT { TRUE | FALSE | DEFAULT }, and
> DEFAULT means that role-level [NO]INHERIT property is controlling.
> Otherwise, the grant-level option overrides the role-level option.

I haven't spent a ton of time reviewing the patch yet, but I did read
through it and thought it looked pretty good.

> I have some hesitations about this approach. On the positive side, I
> believe it's fully backward compatible, and that's something to like.
> On the negative side, I've always felt that the role-level properties
> - [NO]INHERIT, [NO]CREATEROLE, [NO]SUPERUSER, [NO]CREATEDB were hacky
> kludges, and I'd be happier they all went away in favor of more
> principled ways of controlling those behaviors. I think that the
> previous design moves us in the direction of being able to eventually
> remove [NO]INHERIT, whereas this, if anything, entrenches it.

+1.  As mentioned upthread [0], I think attributes like CREATEROLE,
SUPERUSER, and CREATEDB could be replaced with predefined roles.  However,
since role attributes aren't inherited, that would result in a behavior
change.  I'm curious what you have in mind.  It might be worth spinning off
a new thread for this sooner than later.

> It might even encourage people to add *more* role-level Boolean flags.
> For instance, say we add another grant-level property that controls
> whether or not you can SET ROLE to the granted role. Well, it somebody
> going to then say that, for symmetry with CREATE ROLE .. [NO]INHERIT
> we need CREATE ROLE .. [NO]SETROLE? I think such additions would be
> actively harmful, not only because they add complexity, but also
> because I think they are fundamentally the wrong way to think about
> these kinds of properties.

Perhaps there should just be a policy against adding new role-level
options.  Instead, new functionality should be provided via predefined
roles or grant-level options.  There is some precedent for this.  For
example, VACUUM has several options that are only usable via the
parenthesized syntax, and the unparenthesized syntax is marked deprecated
in the documentation.  (Speaking of which, is it finally time to remove the
unparenthesized syntax or add WARNINGs when it is used?)  For [NO]INHERIT
and WITH INHERIT DEFAULT, presumably we could do something similar.  Down
the road, those would be removed in favor of only using grant-level
options.

[0] https://postgr.es/m/20220602180755.GA2402942%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com