Re: [PATCH v2] use has_privs_for_role for predefined roles

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,

Stephen