Role Attribute Bitmask Catalog Representation

On 2014-11-24 15:39:22 -0500, Adam Brightwell wrote:

* int64 (C) to int8 (SQL) mapping for genbki.

That definitely should be a separate patch. Which can be committed much
earlier than the rest - even if we don't actually end up needing it for
this feature, it's still good to have it.

* replace all role attributes columns in pg_authid with single int64 column
named rolattr.
* update CreateRole and AlterRole to use rolattr.
* update all has_*_privilege functions to check rolattr.
* builtin SQL function 'has_role_attribute' that takes a role oid and text
name of the attribute as input and returns a boolean.

I think if we're going to do this - and I'm not yet convinced that
that's the best route, we should add returns all permissions a user
has. Right now that's quite easily queryable, but it won't be after
moving everything into one column. You'd need to manually use all has_*_
functions... Yes, you've added them already to pg_roles, but there's
sometimes good reasons to go to pg_authid instead.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Adam Brightwell wrote

A few related threads/discussions/posts:

* http://www.postgresql.org/message-id/

20141016115914.GQ28859@.snowman

*
/messages/by-id/CA+TgmobkYXNOWKEKzX2qGPSr_nvacFGueV=

orxND-xmZvOVYvg@.gmail

* http://www.postgresql.org/message-id/

20141016115914.GQ28859@.snowman

FYI: the first and third links are the same...was there another one you
meant to provide instead?

David J.

--
View this message in context: http://postgresql.nabble.com/Role-Attribute-Bitmask-Catalog-Representation-tp5828078p5828106.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres,

Thanks for the feedback.

* int64 (C) to int8 (SQL) mapping for genbki.

That definitely should be a separate patch. Which can be committed much
earlier than the rest - even if we don't actually end up needing it for
this feature, it's still good to have it.

Agreed. I had previously submitted this as a separate patch, but I think
it got lost in the weeds. At any rate, here is the relevant post:

/messages/by-id/CAKRt6CTgJdeGFqXevrp-DizaeHmg8gNVqu8n5T=ix3JAvpwwDQ@mail.gmail.com

* replace all role attributes columns in pg_authid with single int64

column

named rolattr.
* update CreateRole and AlterRole to use rolattr.
* update all has_*_privilege functions to check rolattr.
* builtin SQL function 'has_role_attribute' that takes a role oid and

text

name of the attribute as input and returns a boolean.

I think if we're going to do this - and I'm not yet convinced that
that's the best route, we should add returns all permissions a user
has. Right now that's quite easily queryable, but it won't be after
moving everything into one column. You'd need to manually use all has_*_
functions... Yes, you've added them already to pg_roles, but there's
sometimes good reasons to go to pg_authid instead.

This is a good point. I'll start looking at this and see what I can come
up with.

An array representation was also suggested by Simon (
/messages/by-id/CA+U5nMJGVdz6jX_YBJk99Nj7mWfGfVEmxtdc44LVHq64gkN8qg@mail.gmail.com).
Obviously there are pro's and con's to either approach. I'm not married to
it, but felt that a bitmask was certainly more efficient. However, I know
that an array would be more extensible given that we could envision more
than 64 role attributes. I'm uncertain if that is a potential reality or
not, but I believe it is certainly worth considering.

-Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

* int64 (C) to int8 (SQL) mapping for genbki.

That definitely should be a separate patch. Which can be committed much
earlier than the rest - even if we don't actually end up needing it for
this feature, it's still good to have it.

Agreed. I had previously submitted this as a separate patch, but I think
it got lost in the weeds. At any rate, here is the relevant post:

/messages/by-id/CAKRt6CTgJdeGFqXevrp-DizaeHmg8gNVqu8n5T=ix3JAvpwwDQ@mail.gmail.com

Yeah, done now.

Thanks,

Stephen

David,

A few related threads/discussions/posts:

* http://www.postgresql.org/message-id/

20141016115914.GQ28859@.snowman

*

/messages/by-id/CA+TgmobkYXNOWKEKzX2qGPSr_nvacFGueV=

orxND-xmZvOVYvg@.gmail

* http://www.postgresql.org/message-id/

20141016115914.GQ28859@.snowman

FYI: the first and third links are the same...was there another one you
meant to provide instead?

Whoops. Yes there was, but if I remember correctly it was part of that
same overarching thread. The two provided, I believe are sufficient to
lead to any prior relevant discussions that influenced/motivated this patch.

-Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

An array representation was also suggested by Simon (
/messages/by-id/CA+U5nMJGVdz6jX_YBJk99Nj7mWfGfVEmxtdc44LVHq64gkN8qg@mail.gmail.com).
Obviously there are pro's and con's to either approach. I'm not married to
it, but felt that a bitmask was certainly more efficient. However, I know
that an array would be more extensible given that we could envision more
than 64 role attributes. I'm uncertain if that is a potential reality or
not, but I believe it is certainly worth considering.

I'd be pretty surprised if we actually got up to 64, and if we did we
could change it to a bytea. It wouldn't be the cleanest thing, but
using an array would change pg_authid from "same size as today" to
"quite a bit larger" and I don't really see the advantage. We use a bit
field for the GRANT-based permissions and people have to use functions
to decode those too and while it's not ideal, I don't feel like we hear
people complaining about it.

Thanks,

Stephen

All,

I think if we're going to do this - and I'm not yet convinced that

that's the best route, we should add returns all permissions a user
has. Right now that's quite easily queryable, but it won't be after
moving everything into one column. You'd need to manually use all has_*_
functions... Yes, you've added them already to pg_roles, but there's
sometimes good reasons to go to pg_authid instead.

This is a good point. I'll start looking at this and see what I can come
up with.

Giving this some thought, I'm curious what would be acceptable as an end
result, specifically related to how a query on pg_authid might look/work.
I was able to preserve the structure of results from pg_roles, however,
that same approach is obviously not possible with pg_authid. So, I'm
curious what the thoughts might be on how to best solve this while
minimizing impact (perhaps not possible) on users. Currently, my thought
is to have a builtin function called 'get_all_role_attributes' or similar,
that returns an array of each attribute in string form. My thoughts are
that it might look something like the following:

SELECT rolname, get_all_role_attributes(rolattr) AS attributes FROM
pg_authid;

| rolname | attributes |
+---------+-------------------------------------+
| user1 | {Superuser, Create Role, Create DB} |

Another approach might be that the above function return a string of comma
separated attributes, similar to what \du in psql does. IMO, I think the
array approach would be more appropriate than a string but I'm willing to
accept that neither is acceptable and would certainly be interested in
opinions.

Thanks,
Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

Giving this some thought, I'm curious what would be acceptable as an end
result, specifically related to how a query on pg_authid might look/work.
I was able to preserve the structure of results from pg_roles, however,
that same approach is obviously not possible with pg_authid. So, I'm
curious what the thoughts might be on how to best solve this while
minimizing impact (perhaps not possible) on users. Currently, my thought
is to have a builtin function called 'get_all_role_attributes' or similar,
that returns an array of each attribute in string form. My thoughts are
that it might look something like the following:

Having an array sounds pretty reasonable to me.

Another approach might be that the above function return a string of comma
separated attributes, similar to what \du in psql does. IMO, I think the
array approach would be more appropriate than a string but I'm willing to
accept that neither is acceptable and would certainly be interested in
opinions.

Users interested in having a string instead could use array_to_string().
Having to go the other way isn't as nice, imo.

Thanks!

Stephen

Stephen,

Having an array sounds pretty reasonable to me.

Ok, sounds good, I think so too.

Users interested in having a string instead could use array_to_string().

Having to go the other way isn't as nice, imo.

My thoughts exactly, but wanted to at least put it out there.

Thanks,
Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

I am simply breaking this out into its own thread from the discussion on
additional role attributes (
/messages/by-id/20141015052259.GG28859@tamriel.snowman.net
).

Makes sense to me, thanks.

Based on these above I have attached an initial WIP patch for review and
discussion that takes a swing at changing the catalog representation.

Just a quick initial look, but I don't think we want to #include
parsenodes.h into pg_authid.h. Why not put the #define ROLE_ATTR_* into
pg_authid.h instead? We have similar #define's in other catalog .h's
(PROARGMODE_*, RELKIND_*, etc).

I'm also not a huge fan of the hard-coded 255 for the default superuser.
That goes back to the other question of if we should bother having those
explicitly listed at all, but I'd suggest having a #define for 'all'
bits to be true (for currently used bits) and then a comment above the
superuser which references that #define (we can't use the #define
directly or we'd be including pg_authid.h into pg_proc.h, which isn't a
good idea either; if we really want to use the #define, genbki.pl could
be adjusted to find the #define similar to what it does for PGUID and
PGNSP).

Thanks!

Stephen

All,

I have attached a patch that addresses the current suggestions and
recommendations:

* Add 'get_all_role_attributes' SQL function - returns a text array
representation of the attributes from a value passed to it.

Example:

postgres=# SELECT rolname, get_all_role_attributes(rolattr) AS rolattr FROM
pg_authid;
rolname | rolattr

----------+-----------------------------------------------------------------------------------------------
postgres | {Superuser,Inherit,"Create Role","Create DB","Catalog
Update",Login,Replication,"Bypass RLS"}
(1 row)

* Refactor #define's from 'parsenodes.h' to 'acl.h'
* Added #define ROLE_ATTR_ALL to represent all currently available
attributes.
* Added genbki.pl substitution for PGROLEATTRALL constant.

Please let me know what you think, all feedback is greatly appreciated.

Thanks,
Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

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

Stephen,

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

comments:

Thanks for taking a look at this and for the feedback.

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

Ok. Though, this would affect how CATUPDATE is handled. Peter Eisentraut
previously raised a question about whether superuser checks should be
included with catupdate which led me to create the following post.

/messages/by-id/CAKRt6CQOvT2KiyKg2gFf7h9k8+JVU1149zLb0EXTkKk7TAQGMQ@mail.gmail.com

Certainly, we could keep has_rolcatupdate for this case and put the
superuser check in role_has_attribute, but it seems like it might be worth
taking a look at whether a superuser can bypass catupdate or not. Just a
thought.

And then ditch the individual has_X_privilege() calls (I note that you

didn't appear to add back the has_rolcatupdate() function..).

Ok. I had originally thought for this patch that I would try to minimize
these types of changes, though perhaps this is that opportunity previously
mentioned in refactoring those. However, the catupdate question still
remains.

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

Yes, this was originally my first approach. I'm not recollecting why I
decided on the other route, but agree that is preferable and simpler.

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

Agreed.

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.

Ok, easy enough.

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.

I have no reason for one over the other, though I did ask myself that
question. I did find it curious that in some cases there is "has_X" and
then in others "pg_has_X". Perhaps I'm not looking in the right places,
but I haven't found anything that helps to distinguish when one vs the
other is appropriate (even if it is a general rule of thumb).

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

Yes, this is true.

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

I really don't see any reason and as above, I can certainly do those
refactors now if that is what is desired.

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

Yes, we were, however the latter causes a syntax error with initdb. :-/

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

Yes, I had previously checked this, I get the following error from initdb.

FATAL: invalid input syntax for integer: "ROLE_ATTR_ALL"

+ #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..?

No they shouldn't, not sure how I missed that.

Thanks,
Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

Ok. Though, this would affect how CATUPDATE is handled. Peter Eisentraut
previously raised a question about whether superuser checks should be
included with catupdate which led me to create the following post.

/messages/by-id/CAKRt6CQOvT2KiyKg2gFf7h9k8+JVU1149zLb0EXTkKk7TAQGMQ@mail.gmail.com

Certainly, we could keep has_rolcatupdate for this case and put the
superuser check in role_has_attribute, but it seems like it might be worth
taking a look at whether a superuser can bypass catupdate or not. Just a
thought.

My recollection matches the documentation- rolcatupdate should be
required to update the catalogs. The fact that rolcatupdate is set by
AlterUser to match rolsuper is an interesting point and one which we
might want to reconsider, but that's beyond the scope of this patch.

Ergo, I'd suggest keeping has_rolcatupdate, but have it do the check
itself directly instead of calling down into role_has_attribute().

There's an interesting flip side to that though, which is the question
of what to do with pg_roles and psql. Based on the discussion this far,
it seems like we'd want to keep the distinction for pg_roles and psql
based on what bits have explicitly been set rather than what's actually
checked for. As such, I'd have one other function-
check_has_attribute() which *doesn't* have the superuser allow-all check
and is what is used in pg_roles and by psql. I'd expose both functions
at the SQL level.

Ok. I had originally thought for this patch that I would try to minimize
these types of changes, though perhaps this is that opportunity previously
mentioned in refactoring those. However, the catupdate question still
remains.

It makes sense to me, at least, to include removing those individual
attribute functions in this patch.

I have no reason for one over the other, though I did ask myself that
question. I did find it curious that in some cases there is "has_X" and
then in others "pg_has_X". Perhaps I'm not looking in the right places,
but I haven't found anything that helps to distinguish when one vs the
other is appropriate (even if it is a general rule of thumb).

Given that we're changing things anyway, it seems to me that the pg_
prefix makes sense.

Yes, we were, however the latter causes a syntax error with initdb. :-/

Ok, then just stuff the 255 back there and add a comment about why it's
required and mention that cute tricks to calculate the value won't work.

Thanks!

Stephen

All,

I have attached an updated patch that incorporates the feedback and
recommendations provided.

Thanks,
Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

I have attached an updated patch that incorporates the feedback and
recommendations provided.

Thanks. Comments below.

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

--- 84,90 ----
{
XLogRecPtr	stoppoint;

! 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"))));

Ditto here.

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
--- 5031,5081 ----
}

/*
! * has_role_attribute
! * Check if the role has the specified role has a specific role attribute.
! * This function will always return true for roles with superuser privileges
! * unless the attribute being checked is CATUPDATE.
*
! * roleid - the oid of the role to check.
! * attribute - the attribute to check.
*/
bool
! has_role_attribute(Oid roleid, RoleAttr attribute)
{
! /*
! * Superusers bypass all permission checking except in the case of CATUPDATE.
! */
! if (!(attribute & ROLE_ATTR_CATUPDATE) && superuser_arg(roleid))
return true;

! return check_role_attribute(roleid, attribute);
}

+ /*
+  * 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/commands/user.c b/src/backend/commands/user.c
*************** CreateRole(CreateRoleStmt *stmt)
*** 82,93 ****
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? */
bool		createrole = false;		/* Can this user create roles? */
bool		createdb = false;		/* Can the user create databases? */
bool		canlogin = false;		/* Can this user login? */
bool		isreplication = false;	/* Is this a replication role? */
bool		bypassrls = false;		/* Is this a row security enabled role? */
int			connlimit = -1; /* maximum connections allowed */
List	   *addroleto = NIL;	/* roles to make this a member of */
List	   *rolemembers = NIL;		/* roles to be members of this role */
--- 74,86 ----
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? */
bool		createrole = false;		/* Can this user create roles? */
bool		createdb = false;		/* Can the user create databases? */
bool		canlogin = false;		/* Can this user login? */
bool		isreplication = false;	/* Is this a replication role? */
bool		bypassrls = false;		/* Is this a row security enabled role? */
+ 	RoleAttr	attributes = ROLE_ATTR_NONE;	/* role attributes, initialized to none. */
int			connlimit = -1; /* maximum connections allowed */
List	   *addroleto = NIL;	/* roles to make this a member of */
List	   *rolemembers = NIL;		/* roles to be members of this role */

Please clean up the whitespace changes- there's no need for the
'inherit' line to change..

diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
*************** XLogRead(char *buf, TimeLineID tli, XLog
*** 205,211 ****
static void
check_permissions(void)
{
! 	if (!superuser() && !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser or replication role to use replication slots"))));
--- 207,213 ----
static void
check_permissions(void)
{
! 	if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser or replication role to use replication slots"))));

Another case which should be using has_role_attribute()

diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
--- 17,34 ----
#include "miscadmin.h"

#include "access/htup_details.h"
+ #include "catalog/pg_authid.h"
#include "replication/slot.h"
#include "replication/logical.h"
#include "replication/logicalfuncs.h"
+ #include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/pg_lsn.h"

static void
check_permissions(void)
{
! if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg("must be superuser or replication role to use replication slots"))));

Also here.

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.

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
new file mode 100644
index c348034..be0e1cc
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
*************** InitPostgres(const char *in_dbname, Oid
*** 762,768 ****
{
Assert(!bootstrap);

! 		if (!superuser() && !has_rolreplication(GetUserId()))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or replication role to start walsender")));
--- 762,768 ----
{
Assert(!bootstrap);

! if (!superuser() && !check_role_attribute(GetUserId(), ROLE_ATTR_REPLICATION))
ereport(FATAL,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser or replication role to start walsender")));

Also here.

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

I'm pretty happy with the rest of it.

Thanks!

Stephen

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.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

Attached is an updated patch.

Awesome, thanks!

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
*************** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5051,5102 ****
}

/*
! * Check whether specified role has CREATEROLE privilege (or is a superuser)
*
! * Note: roles do not have owners per se; instead we use this test in
! * places where an ownership-like permissions test is needed for a role.
! * Be sure to apply it to the role trying to do the operation, not the
! * role being operated on! Also note that this generally should not be
! * considered enough privilege if the target role is a superuser.
! * (We don't handle that consideration here because we want to give a
! * separate error message for such cases, so the caller has to deal with it.)
*/

The comment above is pretty big and I don't think we want to completely
remove it. Can you add it as another 'Note' in the 'has_role_attribute'
comment and reword it accordingly?

*************** AlterRole(AlterRoleStmt *stmt)
*** 508,513 ****
--- 512,518 ----
DefElem    *dvalidUntil = NULL;
DefElem    *dbypassRLS = NULL;
Oid			roleid;
+ 	RoleAttr attributes;

Whitespace issue that should be fixed- attributes should line up with
roleid.

--- 1405,1421 ----
FROM (pg_get_replication_slots() l(slot_name, plugin, slot_type, datoid, active, xmin, catalog_xmin, restart_lsn)
LEFT JOIN pg_database d ON ((l.datoid = d.oid)));
pg_roles| SELECT pg_authid.rolname,
!     pg_check_role_attribute(pg_authid.oid, 'SUPERUSER'::text) AS rolsuper,
!     pg_check_role_attribute(pg_authid.oid, 'INHERIT'::text) AS rolinherit,
!     pg_check_role_attribute(pg_authid.oid, 'CREATEROLE'::text) AS rolcreaterole,
!     pg_check_role_attribute(pg_authid.oid, 'CREATEDB'::text) AS rolcreatedb,
!     pg_check_role_attribute(pg_authid.oid, 'CATUPDATE'::text) AS rolcatupdate,
!     pg_check_role_attribute(pg_authid.oid, 'CANLOGIN'::text) AS rolcanlogin,
!     pg_check_role_attribute(pg_authid.oid, 'REPLICATION'::text) AS rolreplication,
!     pg_check_role_attribute(pg_authid.oid, 'BYPASSRLS'::text) AS rolbypassrls,
pg_authid.rolconnlimit,
'********'::text AS rolpassword,
pg_authid.rolvaliduntil,
s.setconfig AS rolconfig,
pg_authid.oid
FROM (pg_authid

It occurs to me that in this case (and a few others..), we're doing a
lot of extra work- each call to pg_check_role_attribute() is doing a
lookup on the oid just to get back to what the rolattr value on this row
is. How about a function which takes rolattr and the text
representation of the attribute and returns if the bit is set for that
rolattr value? Then you'd pass pg_authid.rolattr into the function
calls above instead of the role's oid.

I don't see any changes to the regression test files, were they
forgotten in the patch? I would think that at least the view definition
changes would require updates to the regression tests, though perhaps
nothing else.

Overall, I'm pretty happy with the patch and would suggest moving on to
writing up the documentation changes to go along with the code changes.
I'll continue to play around with it but it all seems pretty clean to
me and will allow us to easily add the additiaonl role attributes being
discussed.

Thanks!

Stephen

Stephen,

The comment above is pretty big and I don't think we want to completely

remove it. Can you add it as another 'Note' in the 'has_role_attribute'
comment and reword it accordingly?

Ok, agreed. Will address.

Whitespace issue that should be fixed- attributes should line up with

roleid.

Ok. Will address.

It occurs to me that in this case (and a few others..), we're doing a

lot of extra work- each call to pg_check_role_attribute() is doing a
lookup on the oid just to get back to what the rolattr value on this row
is. How about a function which takes rolattr and the text
representation of the attribute and returns if the bit is set for that
rolattr value? Then you'd pass pg_authid.rolattr into the function
calls above instead of the role's oid.

Makes sense, I'll put that together.

I don't see any changes to the regression test files, were they
forgotten in the patch? I would think that at least the view definition
changes would require updates to the regression tests, though perhaps
nothing else.

Hmmm... :-/ The regression tests that changed were in
'src/test/regress/expected/rules.out' and should be near the bottom of the
patch.

Overall, I'm pretty happy with the patch and would suggest moving on to
writing up the documentation changes to go along with the code changes.
I'll continue to play around with it but it all seems pretty clean to
me and will allow us to easily add the additiaonl role attributes being
discussed.

Sounds good. I'll start on those changes next.

Thanks,
Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com