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

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.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