Re: Role Attribute Bitmask Catalog Representation

Adam,

* Adam Brightwell (adam(dot)brightwell(at)crunchydatasolutions(dot)com) wrote:
> Please let me know what you think, all feedback is greatly appreciated.

Thanks for this. Found time to do more of a review and have a few
comments:

> diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
> new file mode 100644
> index d30612c..93eb2e6
> *** a/src/backend/catalog/aclchk.c
> --- b/src/backend/catalog/aclchk.c
> *************** pg_extension_ownercheck(Oid ext_oid, Oid
> *** 5051,5056 ****
> --- 5031,5058 ----
> }
>
> /*
> + * Check whether the specified role has a specific role attribute.
> + */
> + bool
> + role_has_attribute(Oid roleid, RoleAttr attribute)
> + {
> + RoleAttr attributes;
> + HeapTuple tuple;
> +
> + tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
> +
> + if (!HeapTupleIsValid(tuple))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("role with OID %u does not exist", roleid)));
> +
> + attributes = ((Form_pg_authid) GETSTRUCT(tuple))->rolattr;
> + ReleaseSysCache(tuple);
> +
> + return ((attributes & attribute) > 0);
> + }

I'd put the superuser_arg() check in role_has_attribute.

> --- 5066,5089 ----
> bool
> has_createrole_privilege(Oid roleid)
> {
> /* Superusers bypass all permission checking. */
> if (superuser_arg(roleid))
> return true;
>
> ! return role_has_attribute(roleid, ROLE_ATTR_CREATEROLE);
> }

And then ditch the individual has_X_privilege() calls (I note that you
didn't appear to add back the has_rolcatupdate() function..).

> diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
> new file mode 100644
> index 1a73fd8..72c5dcc
> *** a/src/backend/commands/user.c
> --- b/src/backend/commands/user.c
> *************** have_createrole_privilege(void)
> *** 63,68 ****
> --- 63,73 ----
> return has_createrole_privilege(GetUserId());
> }
>
> + static RoleAttr
> + set_role_attribute(RoleAttr attributes, RoleAttr attribute)
> + {
> + return ((attributes & ~(0xFFFFFFFFFFFFFFFF)) | attribute);
> + }

I don't think this is doing what you think it is..? It certainly isn't
working as expected by AlterRole(). Rather than using a function for
these relatively simple operations, why not just have AlterRole() do:

if (isX >= 0)
attributes = (isX > 0) ? attributes | ROLE_ATTR_X
: attributes & ~(ROLE_ATTR_X);

Otherwise, you'd probably need to pass a flag into set_role_attribute()
to indicate if you're setting or unsetting the bit, or have an
'unset_role_attribute()' function, but all of that seems like overkill..

> *************** CreateRole(CreateRoleStmt *stmt)
> --- 86,99 ----
> char *password = NULL; /* user password */
> bool encrypt_password = Password_encryption; /* encrypt password? */
> char encrypted_password[MD5_PASSWD_LEN + 1];
> ! bool issuper = false; /* Make the user a superuser? */
> ! bool inherit = true; /* Auto inherit privileges? */

I'd probably leave the whitespace-only changes out.

> *************** AlterRoleSet(AlterRoleSetStmt *stmt)
> *** 889,895 ****
> * To mess with a superuser you gotta be superuser; else you need
> * createrole, or just want to change your own settings
> */
> ! if (((Form_pg_authid) GETSTRUCT(roletuple))->rolsuper)
> {
> if (!superuser())
> ereport(ERROR,
> --- 921,928 ----
> * To mess with a superuser you gotta be superuser; else you need
> * createrole, or just want to change your own settings
> */
> ! attributes = ((Form_pg_authid) GETSTRUCT(roletuple))->rolattr;
> ! if ((attributes & ROLE_ATTR_SUPERUSER) > 0)
> {
> if (!superuser())
> ereport(ERROR,

We don't bother with the '> 0' in any of the existing bit testing that
goes on in the backend, so I don't think it makes sense to start now.
Just do

if (attributes & ROLE_ATTR_SUPERUSER) ... etc

and be done with it.

> *************** aclitemin(PG_FUNCTION_ARGS)
> *** 577,582 ****
> --- 578,584 ----
> PG_RETURN_ACLITEM_P(aip);
> }
>
> +
> /*
> * aclitemout
> * Allocates storage for, and fills in, a new null-delimited string

More whitespace changes... :/

> *************** pg_role_aclcheck(Oid role_oid, Oid rolei
> *** 4602,4607 ****
> --- 4604,4712 ----
> return ACLCHECK_NO_PRIV;
> }
>
> + /*
> + * has_role_attribute_id
> + * Check the named role attribute on a role by given role oid.
> + */
> + Datum
> + has_role_attribute_id(PG_FUNCTION_ARGS)
> + {
> + Oid roleoid = PG_GETARG_OID(0);
> + text *attr_type_text = PG_GETARG_TEXT_P(1);
> + RoleAttr attribute;
> +
> + attribute = convert_role_attr_string(attr_type_text);
> +
> + PG_RETURN_BOOL(role_has_attribute(roleoid, attribute));
> + }

Why not have this as 'pg_has_role_id_attribute' and then have a
'pg_has_role_name_attribute' also? Seems like going with the pg_
namespace is the direction we want to go in, though I'm not inclined to
argue about it if folks prefer has_X.

> + /*
> + * get_all_role_attributes_attr
> + * Convert a RoleAttr representation of role attributes into an array of
> + * corresponding text values.
> + *
> + * The first and only argument is a RoleAttr (int64) representation of the
> + * role attributes.
> + */
> + Datum
> + get_all_role_attributes_rolattr(PG_FUNCTION_ARGS)

The function name in the comment should match the function..

> + {
> + RoleAttr attributes = PG_GETARG_INT64(0);
> + List *attribute_list = NIL;
> + ListCell *attribute;
> + Datum *temp_array;
> + ArrayType *result;
> + int num_attributes;
> + int i = 0;
> +
> + /*
> + * If no attributes are assigned, then there is no need to go through the
> + * individual checks for which are assigned. Therefore, we can short circuit
> + * and return an empty array.
> + */
> + if (attributes == ROLE_ATTR_NONE)
> + PG_RETURN_ARRAYTYPE_P(construct_empty_array(TEXTOID));
> +
> + /* Determine which attributes are assigned. */
> + if ((attributes & ROLE_ATTR_SUPERUSER) > 0)
> + attribute_list = lappend(attribute_list, "Superuser");
> + if ((attributes & ROLE_ATTR_INHERIT) > 0)
> + attribute_list = lappend(attribute_list, "Inherit");
> + if ((attributes & ROLE_ATTR_CREATEROLE) > 0)
> + attribute_list = lappend(attribute_list, "Create Role");
> + if ((attributes & ROLE_ATTR_CREATEDB) > 0)
> + attribute_list = lappend(attribute_list, "Create DB");
> + if ((attributes & ROLE_ATTR_CATUPDATE) > 0)
> + attribute_list = lappend(attribute_list, "Catalog Update");
> + if ((attributes & ROLE_ATTR_CANLOGIN) > 0)
> + attribute_list = lappend(attribute_list, "Login");
> + if ((attributes & ROLE_ATTR_REPLICATION) > 0)
> + attribute_list = lappend(attribute_list, "Replication");
> + if ((attributes & ROLE_ATTR_BYPASSRLS) > 0)
> + attribute_list = lappend(attribute_list, "Bypass RLS");
> +
> + /* Build and return result array */
> + num_attributes = list_length(attribute_list);
> + temp_array = (Datum *) palloc(num_attributes * sizeof(Datum));
> +
> + foreach(attribute, attribute_list)
> + temp_array[i++] = CStringGetTextDatum(lfirst(attribute));
> +
> + result = construct_array(temp_array, num_attributes, TEXTOID, -1, false, 'i');
> +
> + PG_RETURN_ARRAYTYPE_P(result);
> + }

Seems like you could just make temp_array be N_ROLE_ATTRIBUTES in
size, instead of building a list, counting it, and then building the
array from that..?

> *************** RoleMembershipCacheCallback(Datum arg, i
> --- 4743,4758 ----
> static bool
> has_rolinherit(Oid roleid)
> {
> ! RoleAttr attributes;
> HeapTuple utup;
>
> utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
> if (HeapTupleIsValid(utup))
> {
> ! attributes = ((Form_pg_authid) GETSTRUCT(utup))->rolattr;
> ReleaseSysCache(utup);
> }
> ! return ((attributes & ROLE_ATTR_INHERIT) > 0);
> }

Do we really need to keep has_rolinherit for any reason..? Seems
unnecessary given it's only got one call site and it's nearly the same
as a call to role_has_attribute() anyway. Ditto with
has_rolreplication().

> diff --git a/src/include/catalog/pg_authid.h b/src/include/catalog/pg_authid.h
> new file mode 100644
> index 3b63d2b..ff8f035
> *** a/src/include/catalog/pg_authid.h
> --- b/src/include/catalog/pg_authid.h
> ***************
> *** 22,27 ****
> --- 22,28 ----
> #define PG_AUTHID_H
>
> #include "catalog/genbki.h"
> + #include "nodes/parsenodes.h"
>
> /*
> * The CATALOG definition has to refer to the type of rolvaliduntil as

Thought we were getting rid of this..?

> ! #define N_ROLE_ATTRIBUTES 8 /* 1 plus the last 1<<x */
> ! #define ROLE_ATTR_NONE 0
> ! #define ROLE_ATTR_ALL 255 /* All currently available attributes. */

Or:

#define ROLE_ATTR_ALL (1<<N_ROLE_ATTRIBUTES)-1

?

> /* ----------------
> * initial contents of pg_authid
> *
> * The uppercase quantities will be replaced at initdb time with
> * user choices.
> + *
> + * PGROLATTRALL is substituted by genbki.pl to use the value defined by
> + * ROLE_ATTR_ALL.
> * ----------------
> */
> ! DATA(insert OID = 10 ( "POSTGRES" PGROLATTRALL -1 _null_ _null_));
>
> #define BOOTSTRAP_SUPERUSERID 10

Is it actually necessary to do this substitution when the value is
#define'd in the same .h file...? Might be something to check, if you
haven't already.

> diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
> new file mode 100644
> index 255415d..d59dcb4
> *** a/src/include/nodes/parsenodes.h
> --- b/src/include/nodes/parsenodes.h
> *************** typedef uint32 AclMode; /* a bitmask o
> *** 78,83 ****
> --- 78,101 ----
> /* Currently, SELECT ... FOR [KEY] UPDATE/SHARE requires UPDATE privileges */
> #define ACL_SELECT_FOR_UPDATE ACL_UPDATE
>
> + /*
> + * Role attributes are encoded so that we can OR them together in a bitmask.
> + * The present representation of RoleAttr limits us to 64 distinct rights.
> + *
> + * Caution: changing these codes breaks stored RoleAttrs, hence forces initdb.
> + */
> + typedef uint64 RoleAttr; /* a bitmask for role attribute bits */
> +
> + #define ROLE_ATTR_SUPERUSER (1<<0)
> + #define ROLE_ATTR_INHERIT (1<<1)
> + #define ROLE_ATTR_CREATEROLE (1<<2)
> + #define ROLE_ATTR_CREATEDB (1<<3)
> + #define ROLE_ATTR_CATUPDATE (1<<4)
> + #define ROLE_ATTR_CANLOGIN (1<<5)
> + #define ROLE_ATTR_REPLICATION (1<<6)
> + #define ROLE_ATTR_BYPASSRLS (1<<7)
> + #define N_ROLE_ATTRIBUTES 8
> + #define ROLE_ATTR_NONE 0

These shouldn't need to be here any more..?

Thanks!

Stephen