Thread: [PATCH] remove is_member_of_role() from header, add can_set_role()
As a follow-on to Conflation of member/privs for predefined roles, this removes is_member_of_role from the header to dissuade it's use for privilege checking. Since SET ROLE must use membership rather than privileges a new, explicitly named can_set_role() function is exported. is_member_of_role_nosuper() still exists for the following purposes: - membership loop checking in user.c - membership matching for pg_hba.conf in hba.c Other uses of is_member_of_role_nosuper() should be avoided.
Attachment
> On Oct 27, 2021, at 9:26 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote: > > As a follow-on to Conflation of member/privs for predefined roles, > this removes is_member_of_role from the header to dissuade it's use > for privilege checking. Since SET ROLE must use membership rather than > privileges a new, explicitly named can_set_role() function is > exported. > > is_member_of_role_nosuper() still exists for the following purposes: > - membership loop checking in user.c > - membership matching for pg_hba.conf in hba.c > > Other uses of is_member_of_role_nosuper() should be avoided. > <0001-unexport-is_member_of_role-add-can_set_role.patch> I don't understand the purpose of this. You are defining can_set_role(member,role) as a simple wrapper around is_member_of_role(member,role). Couldn't the comment: + * + * Do not use this for privilege checking, instead use has_privs_of_role() be added to the header for is_member_of_role() without needing the new wrapper function? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Oct 27, 2021 at 1:12 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > > > > On Oct 27, 2021, at 9:26 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote: > > > > As a follow-on to Conflation of member/privs for predefined roles, > > this removes is_member_of_role from the header to dissuade it's use > > for privilege checking. Since SET ROLE must use membership rather than > > privileges a new, explicitly named can_set_role() function is > > exported. > > > > is_member_of_role_nosuper() still exists for the following purposes: > > - membership loop checking in user.c > > - membership matching for pg_hba.conf in hba.c > > > > Other uses of is_member_of_role_nosuper() should be avoided. > > <0001-unexport-is_member_of_role-add-can_set_role.patch> > > I don't understand the purpose of this. You are defining can_set_role(member,role) as a simple wrapper around is_member_of_role(member,role). Couldn't the comment: > > + * > + * Do not use this for privilege checking, instead use has_privs_of_role() > > be added to the header for is_member_of_role() without needing the new wrapper function? It could be, but the intent is to dissuade it from being used, so getting rid of it and making an explicit version that has a sole use seemed useful. It's possible that it's being used inappropriately out-of-tree so this would also prevent that.
On 10/27/21, 10:22 AM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote: > On Wed, Oct 27, 2021 at 1:12 PM Mark Dilger > <mark.dilger@enterprisedb.com> wrote: >> I don't understand the purpose of this. You are defining can_set_role(member,role) as a simple wrapper around is_member_of_role(member,role). Couldn't the comment: >> >> + * >> + * Do not use this for privilege checking, instead use has_privs_of_role() >> >> be added to the header for is_member_of_role() without needing the new wrapper function? > > It could be, but the intent is to dissuade it from being used, so > getting rid of it and making an explicit version that has a sole use > seemed useful. > > It's possible that it's being used inappropriately out-of-tree so this > would also prevent that. I think a comment about the intended usage is sufficient. However, renaming the function or replacing it with a wrapper might break extensions and encourage the authors to reevaluate. That could be a good thing. Nathan
"Bossart, Nathan" <bossartn@amazon.com> writes:
> On 10/27/21, 10:22 AM, "Joshua Brindle" <joshua.brindle@crunchydata.com> wrote:
>> On Wed, Oct 27, 2021 at 1:12 PM Mark Dilger
>> <mark.dilger@enterprisedb.com> wrote:
>>> I don't understand the purpose of this. You are defining can_set_role(member,role) as a simple wrapper around
is_member_of_role(member,role). Couldn't the comment:
> I think a comment about the intended usage is sufficient.
I agree with the position that a better function header comment is
sufficient. However, if we're to rename it, please not "can_set_role".
That's bad in at least two ways:
* it's quite unclear what "set" means in this context ... maybe the
function is testing whether you're allowed to alter properties of
the role, or do "ALTER ROLE ... SET parameter = value"? It only makes
sense if you already know that the specific command SET ROLE is being
thought of.
* it's unclear which argument is which end of the relationship.
Something like "can_set_role_to()" would help with the second problem,
but I'm not sure it does much for the first one.
On the whole I think the existing name is fine.
regards, tom lane