Re: [PATCH v2] use has_privs_for_role for predefined roles

On 3/21/22 16:15, Joe Conway wrote:
> 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.

Lacking any feedback from Robert, I removed my changes related to
basebackup and pushed Joshua's patch with one minor editorial fix. What
to do with the basebackup changes can go on the open items list for pg15
I guess.

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