On Mon, Nov 8, 2021 at 3:44 PM Stephen Frost <sfr...@snowman.net> wrote: > > Greetings, > > * Joshua Brindle (joshua.brin...@crunchydata.com) wrote: > > On Wed, Oct 27, 2021 at 5:20 PM Joshua Brindle > > <joshua.brin...@crunchydata.com> wrote: > > > On Wed, Oct 27, 2021 at 5:16 PM Robert Haas <robertmh...@gmail.com> wrote: > > > > On Wed, Oct 27, 2021 at 5:14 PM Joshua Brindle > > > > <joshua.brin...@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.
0001-use-has_privs_for_roles-for-predefined-role-checks.patch
Description: Binary data
