Re: [PATCH v2] use has_privs_for_role for predefined roles

On Mon, Nov 8, 2021 at 3:44 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Joshua Brindle (joshua.brindle@crunchydata.com) wrote:
> > On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle <joshua.brindle@crunchydata.com> wrote:
> > > On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
> > > > <joshua.brindle@crunchydata.com> wrote:
> > > > > Attached is an updated version of the patch to replace
> > > > > is_member_of_role with has_privs_for_role for predefined roles. It
> > > > > does not remove is_member_of_role from acl.h but it does add a warning
> > > > > not to use that function for privilege checking.
> > > > >
> > > > > Please consider this for the upcoming commitfest.
> > > >
> > > > I am not sure I understand what the advantage of this is supposed to be.
> > >
> > > Pre-defined roles currently do not operate the same way other roles do
> > > with respect to inheritance. The patchfile has an explanation and
> > > examples, I wasn't sure if that needed to be repeated in the email or
> > > not.
> >
> > And FWIW several predefined role patches on the list currently
> > correctly use has_privs_for_role rather than is_memver_of_role so this
> > brings the older roles to be consistent with the newer ones being
> > proposed.
>
> Right, we really should be consistent here and we're not and that's not
> a good thing.  Further, if you're logged in as an unprivileged role and
> expect to not see things that you shouldn't, then it's not good for that
> to end up happening because you've been GRANT'd a more privileged, but
> noinherit, role that was given some predefined roles.  This doesn't end
> up being an actual security issue as you could just SET ROLE to that
> more privileged role, so it's not that you couldn't see that data, just
> that you should have had to SET ROLE to do so.
>
> Reviewing the actual patch, it generally looks good to me except that
> you missed updating this comment:
>
> Superusers or members of pg_read_all_stats members are allowed

Thanks for the review, attached is an update with that comment fixed
and also sgml documentation changes that I missed earlier.