Re: Role Attribute Bitmask Catalog Representation

Stephen,

Thanks for the feedback.

> > diff --git a/src/backend/access/transam/xlogfuncs.c
> b/src/backend/access/transam/xlogfuncs.c
> > --- 56,62 ----
> >
> > backupidstr = text_to_cstring(backupid);
> >
> > ! if (!superuser() && !check_role_attribute(GetUserId(),
> ROLE_ATTR_REPLICATION))
> > ereport(ERROR,
> > (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> > errmsg("must be superuser or replication role to run a
> backup")));
>
> The point of has_role_attribute() was to avoid the need to explicitly
> say "!superuser()" everywhere. The idea with check_role_attribute() is
> that we want to present the user (in places like pg_roles) with the
> values that are *actually* set.
>
> In other words, the above should just be:
>
> if (!has_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
>

Yes, I understand. My original thought here was that I was replacing
'has_rolreplication' which didn't perform any superuser check and that I
was trying to adhere to minimal changes. But I agree this would be the
appropriate solution. Fixed.

>
> > + /*
> > + * check_role_attribute
> > + * Check if the role with the specified id has been assigned a
> specific
> > + * role attribute. This function does not allow any superuser
> bypass.
>
> I don't think we need to say that it doesn't "allow" superuser bypass.
> Instead, I'd comment that has_role_attribute() should be used for
> permissions checks while check_role_attribute is for checking what the
> value in the rolattr bitmap is and isn't for doing permissions checks
> directly.
>

Ok. Understood. Fixed.

> diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
> > *************** pg_role_aclcheck(Oid role_oid, Oid rolei
> > *** 4602,4607 ****
> > --- 4603,4773 ----
> > return ACLCHECK_NO_PRIV;
> > }
> >
> > + /*
> > + * pg_has_role_attribute_id_attr
>
> I'm trying to figure out what the point of the trailing "_attr" in the
> function name is..? Doesn't seem necessary to have that for these
> functions and it'd look a bit cleaner without it, imv.
>

So, I was trying to follow what I perceived as the following convention for
these functions: pg_<function_name>_<arg1>_<arg2>_<argN>. So, what "_attr"
represents is the second argument of the function which is the attribute to
check. I could agree that might be unnecessary, but that was my thought
process on it. At any rate, I've removed it.

> ! #define ROLE_ATTR_ALL 255 /* or (1 << N_ROLE_ATTRIBUTES) - 1 */
>
> I'd say "equals" or something rather than "or" since that kind of
> implies that it's an laternative, but we can't do that as discussed in
> the comment (which I like).
>

Agreed. Fixed.

> > ! /* role attribute check routines */
> > ! extern bool has_role_attribute(Oid roleid, RoleAttr attribute);
> > ! extern bool check_role_attribute(Oid roleid, RoleAttr attribute);
>
> With all the 'has_role_attribute(GetUserId(), ROLEATTR_BLAH)' cases, I'd
> suggest doing the same as 'superuser()' and also provide a simpler
> version: 'have_role_attribute(ROLEATTR_BLAH)' which handles doing the
> GetUserId() itself. That'd simplify quite a few of the above calls.
>

Good point. Added.

Attached is an updated patch.

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com