Re: Role Attribute Bitmask Catalog Representation - Mailing list pgsql-hackers

> 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))

>
> + /*
> +  * 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.

> 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.

> ! #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).

> ! /* 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.