Re: [PATCH v2] use has_privs_for_role for predefined roles

On 3/20/22 12:38, Stephen Frost wrote:
> Greetings,
>
> On Sun, Mar 20, 2022 at 18:31 Joshua Brindle
> <joshua(dot)brindle(at)crunchydata(dot)com <mailto:joshua(dot)brindle(at)crunchydata(dot)com>>
> wrote:
>
> On Sun, Mar 20, 2022 at 12:27 PM Joe Conway <mail(at)joeconway(dot)com
> <mailto:mail(at)joeconway(dot)com>> wrote:
> >
> > On 3/3/22 11:26, Joshua Brindle wrote:
> > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway <mail(at)joeconway(dot)com
> <mailto:mail(at)joeconway(dot)com>> wrote:
> > >>
> > >> On 2/10/22 14:28, Nathan Bossart wrote:
> > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
> > >> >> On 2/9/22 13:13, Nathan Bossart wrote:
> > >> >>> I do wonder if users find the differences between
> predefined roles and role
> > >> >>> attributes confusing.  INHERIT doesn't govern role
> attributes, but it will
> > >> >>> govern predefined roles when this patch is applied.  Maybe
> the role
> > >> >>> attribute system should eventually be deprecated in favor
> of using
> > >> >>> predefined roles for everything.  Or perhaps the
> predefined roles should be
> > >> >>> converted to role attributes.
> > >> >>
> > >> >> Yep, I was suggesting that the latter would have been
> preferable to me while
> > >> >> Robert seemed to prefer the former. Honestly I could be
> happy with either of
> > >> >> those solutions, but as I alluded to that is probably a
> discussion for the
> > >> >> next development cycle since I don't see us doing that big
> a change in this
> > >> >> one.
> > >> >
> > >> > I agree.  I still think Joshua's proposed patch is a
> worthwhile improvement
> > >> > for v15.
> > >>
> > >> +1
> > >>
> > >> I am planning to get into it in detail this weekend. So far I have
> > >> really just ensured it merges cleanly and passes make world.
> > >
> > > Rebased patch to apply to master attached.
> >
> > Well longer than I planned, but finally took a closer look.
> >
> > I made one minor editorial fix to Joshua's patch, rebased to current
> > master, and added two missing call sites that presumably are
> related to
> > recent commits for pg_basebackup.
>
> FWIW I pinged Stephen when I saw the basebackup changes included
> is_member_of and he didn't think they necessarily needed to be changed
> since they aren't accessible to human and you can't SET ROLE on a
> replication connection in order to access the role where inheritance
> was blocked.
>
> Yeah, though that should really be very clearly explained in comments
> around that code as anything that uses is_member should really be viewed
> as an exception.  I also wouldn’t be against using has_privs there
> anyway and saying that you shouldn’t be trying to connect as one role on
> a replication connection and using the privs of another when you don’t
> automatically inherit those rights, but on the assumption that the
> committer intended is_member there because SET ROLE isn’t something one
> does on replication connections, I’m alright with it being as is.

Robert -- any opinion on this? If I am not mistaken it is code that you
are actively working on.

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development