public inbox for [email protected]
help / color / mirror / Atom feedFrom: Stephen Frost <[email protected]>
To: Joshua Brindle <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: [PATCH v2] use has_privs_for_role for predefined roles
Date: Mon, 8 Nov 2021 15:44:50 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAGB+Vh7fgOfLtCG+01D8ch3icG2aEd38dvy9uavtKWrpf1_XZw@mail.gmail.com>
References: <CAGB+Vh4Zv_TvKt2tv3QNS6tUM_F_9icmuj0zjywwcgVi4PAhFA@mail.gmail.com>
<CA+TgmobiMfUoZ42ZDz_6Ne4zMznOjxfEb22W+jdNX29yL88agw@mail.gmail.com>
<CAGB+Vh4cCTey_TvE_O0CZu-D+6fVXC8a+C-UTzqkg8oN5YcOrQ@mail.gmail.com>
<CAGB+Vh7fgOfLtCG+01D8ch3icG2aEd38dvy9uavtKWrpf1_XZw@mail.gmail.com>
Greetings,
* Joshua Brindle ([email protected]) wrote:
> On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle <[email protected]> wrote:
> > On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <[email protected]> wrote:
> > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle
> > > <[email protected]> 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
Attachments:
[application/pgp-signature] signature.asc (819B, ../[email protected]/2-signature.asc)
download
view thread (13+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH v2] use has_privs_for_role for predefined roles
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox