[PATCH] remove is_member_of_role() from header, add can_set_role()
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t45014_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t45014_1 && git checkout t45014_1Patchset v1 (message #1) is on t45014_1
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.
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.cOther 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.cOther 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