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 
> > 
> wrote:
> 
>     On Sun, Mar 20, 2022 at 12:27 PM Joe Conway      > wrote:
>      >
>      > On 3/3/22 11:26, Joshua Brindle wrote:
>      > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway      > 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