Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam Brightwell wrote: Alvaro and Stephen, I propose this patch on top of Adam's v5. Also included is a full patch against master. I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in 'check_role_attribute' and the documentation updates requested by Stephen. Pushed with a couple of small changes (Catalog.pm complained about the lack of a CATALOG() line in the new acldefs.h file; you had pg_role_all_attributes as returning bool in the table, but it returns text[]; and I added index entries for the new functions.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Alvarao, Pushed with a couple of small changes (Catalog.pm complained about the lack of a CATALOG() line in the new acldefs.h file; you had pg_role_all_attributes as returning bool in the table, but it returns text[]; and I added index entries for the new functions.) That's fantastic! Thanks, I appreciate it! -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Hey Alvaro, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in 'check_role_attribute' and the documentation updates requested by Stephen. Not sure if you were looking for me to comment on this or not, but I did look over the updated docs and the added Asserts and they look good to me. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Stephen Frost wrote: Hey Alvaro, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in 'check_role_attribute' and the documentation updates requested by Stephen. Not sure if you were looking for me to comment on this or not, but I did look over the updated docs and the added Asserts and they look good to me. Okay, great. I will be looking into committing this patch shortly -- unless you want to do it yourself? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Stephen Frost wrote: * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in 'check_role_attribute' and the documentation updates requested by Stephen. Not sure if you were looking for me to comment on this or not, but I did look over the updated docs and the added Asserts and they look good to me. Okay, great. I will be looking into committing this patch shortly -- unless you want to do it yourself? Please feel free to go ahead. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Alvaro and Stephen, I propose this patch on top of Adam's v5. Also included is a full patch against master. I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in 'check_role_attribute' and the documentation updates requested by Stephen. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml new file mode 100644 index 9ceb96b..9470916 *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** *** 1391,1479 /row row ! entrystructfieldrolsuper/structfield/entry ! entrytypebool/type/entry entryRole has superuser privileges/entry /row row ! entrystructfieldrolinherit/structfield/entry ! entrytypebool/type/entry ! entryRole automatically inherits privileges of roles it is a !member of/entry /row row ! entrystructfieldrolcreaterole/structfield/entry ! entrytypebool/type/entry entryRole can create more roles/entry /row row ! entrystructfieldrolcreatedb/structfield/entry ! entrytypebool/type/entry entryRole can create databases/entry /row row ! entrystructfieldrolcatupdate/structfield/entry ! entrytypebool/type/entry entry Role can update system catalogs directly. (Even a superuser cannot do this unless this column is true) /entry /row row ! entrystructfieldrolcanlogin/structfield/entry ! entrytypebool/type/entry entry Role can log in. That is, this role can be given as the initial session authorization identifier /entry /row row ! entrystructfieldrolreplication/structfield/entry ! entrytypebool/type/entry entry Role is a replication role. That is, this role can initiate streaming replication (see xref linkend=streaming-replication) and set/unset the system backup mode using functionpg_start_backup/ and functionpg_stop_backup/ /entry /row row ! entrystructfieldrolconnlimit/structfield/entry ! entrytypeint4/type/entry ! entry !For roles that can log in, this sets maximum number of concurrent !connections this role can make. -1 means no limit. ! /entry ! /row ! ! row ! entrystructfieldrolpassword/structfield/entry ! entrytypetext/type/entry entry !Password (possibly encrypted); null if none. If the password !is encrypted, this column will begin with the string literalmd5/ !followed by a 32-character hexadecimal MD5 hash. The MD5 hash !will be of the user's password concatenated to their user name. !For example, if user literaljoe/ has password literalxyzzy/, !productnamePostgreSQL/ will store the md5 hash of !literalxyzzyjoe/. A password that does not follow that !format is assumed to be unencrypted. /entry /row - row - entrystructfieldrolvaliduntil/structfield/entry - entrytypetimestamptz/type/entry - entryPassword expiry time (only used for password authentication); -null if no expiration/entry - /row /tbody /tgroup /table --- 1391,1524 /row row ! entrystructfieldrolattr/structfield/entry ! entrytypebigint/type/entry ! entry !Role attributes; see xref linkend=catalog-rolattr-bitmap-table and !xref linkend=sql-createrole for details ! /entry ! /row ! ! row ! entrystructfieldrolconnlimit/structfield/entry ! entrytypeint4/type/entry ! entry !For roles that can log in, this sets maximum number of concurrent !connections this role can make. -1 means no limit. ! /entry ! /row ! ! row ! entrystructfieldrolpassword/structfield/entry ! entrytypetext/type/entry ! entry !Password (possibly encrypted); null if none. If the password !is encrypted, this column will begin with the string literalmd5/ !followed by a 32-character hexadecimal MD5 hash. The MD5 hash !will be of the user's password concatenated to their user name. !For example, if user literaljoe/ has password literalxyzzy/, !productnamePostgreSQL/ will store the md5 hash of !literalxyzzyjoe/. A password that does not follow that !format is assumed to be unencrypted. ! /entry ! /row ! ! row ! entrystructfieldrolvaliduntil/structfield/entry ! entrytypetimestamptz/type/entry ! entryPassword expiry time (only used for password authentication); !null if no
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Stephen Frost wrote: Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: FWIW I've been giving this patch a look and and adjusting some coding details here and there. Do you intend to commit it yourself? You're not listed as reviewer or committer for it in the commitfest app, FWIW. Oh, great, thanks! And, yeah, I'm not as good as I should be about updating the commitfest app. As for committing it, I was thinking I would but you're certainly welcome to if you're interested. I would like this to be committed soonish as it will then allow Adam to rebase the patch which adds the various role attributes discussed previously on top of the representation changes. I suspect he's done some work in that direction already, but I keep asking for changes to this patch which would then ripple down into the other. Sure, I will go over it a bit more and merge changes from Adam to the docs as they come through, and commit soon. One thing I don't very much like is that check_role_attribute() receives a RoleAttr but nowhere it checks that only one bit is set in 'attribute'. From the coding, this routine would return true if just one of those bits is set, which is probably not what is intended. Now I realize that current callers all pass a bitmask with a single bit set, but I think it'd be better to have some protection against that, for possible future coding mistakes. That's certainly a good point. I'm inclined to suggest that there be an Assert() check in check_role_attribute(), or were you thinking of something else..? Yeah, an Assert() is what I had in mind. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
The fact that the ROLE_ATTR_* definitions are in pg_authid.h means that there are now a lot of files that need to include that one. I think the includes relative to ACLs and roles is rather messy now, and this patch makes it a bit worse. I think we should create a new header file (maybe acltypes.h or acldefs.h), which only contains the AclMode and RoleAttr typedefs and their associated defines; that one would be included from parsenodes.h, acl.h and pg_authid.h. Everything else would be in acl.h. So code that currently checks permissions using only acl.h do not need any extra includes. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Alvaro Herrera wrote:
The fact that the ROLE_ATTR_* definitions are in pg_authid.h means that
there are now a lot of files that need to include that one. I think the
includes relative to ACLs and roles is rather messy now, and this patch
makes it a bit worse.
I think we should create a new header file (maybe acltypes.h or
acldefs.h), which only contains the AclMode and RoleAttr typedefs and
their associated defines; that one would be included from parsenodes.h,
acl.h and pg_authid.h. Everything else would be in acl.h. So code that
currently checks permissions using only acl.h do not need any extra
includes.
I propose this patch on top of Adam's v5. Also included is a full patch
against master.
Unrelated: when changing from unified to context format, I saw
filterdiff fail to produce a complete patch, skipping some hunks in its
output. My first impression was that I had dropped some hunks in git,
so I wasted some time comparing v5 and v6 by hand before remembering
that Michael Paquier had mentioned this problem previously.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 8475985..3181a79 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -22,7 +22,6 @@
#include access/xlog_internal.h
#include access/xlogutils.h
#include catalog/catalog.h
-#include catalog/pg_authid.h
#include catalog/pg_type.h
#include funcapi.h
#include miscadmin.h
diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index a403c64..a6de2ff 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -28,7 +28,7 @@ all: $(BKIFILES) schemapg.h
# indexing.h had better be last, and toasting.h just before it.
POSTGRES_BKI_SRCS = $(addprefix $(top_srcdir)/src/include/catalog/,\
- pg_proc.h pg_type.h pg_attribute.h pg_class.h \
+ acldefs.h pg_proc.h pg_type.h pg_attribute.h pg_class.h \
pg_attrdef.h pg_constraint.h pg_inherits.h pg_index.h pg_operator.h \
pg_opfamily.h pg_opclass.h pg_am.h pg_amop.h pg_amproc.h \
pg_language.h pg_largeobject_metadata.h pg_largeobject.h pg_aggregate.h \
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 4663429..21d282c 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -5038,18 +5038,18 @@ pg_extension_ownercheck(Oid ext_oid, Oid roleid)
* roleid - the oid of the role to check.
* attribute - the attribute to check.
*
- * Note: Use this function for role attribute permission checking as it accounts
- * for superuser status. It will always return true for roles with superuser
- * privileges unless the attribute being checked is CATUPDATE (superusers are not
- * allowed to bypass CATUPDATE permissions).
+ * Note: Use this function for role attribute permission checking as it
+ * accounts for superuser status. It will always return true for roles with
+ * superuser privileges unless the attribute being checked is CATUPDATE
+ * (superusers are not allowed to bypass CATUPDATE permissions).
*
- * 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.)
+ * 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.)
*/
bool
has_role_attribute(Oid roleid, RoleAttr attribute)
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 2929b66..415ac17 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -91,7 +91,7 @@ my $BOOTSTRAP_SUPERUSERID =
my $PG_CATALOG_NAMESPACE =
find_defined_symbol('pg_namespace.h', 'PG_CATALOG_NAMESPACE');
my $ROLE_ATTR_ALL =
- find_defined_symbol('pg_authid.h', 'ROLE_ATTR_ALL');
+ find_defined_symbol('acldefs.h', 'ROLE_ATTR_ALL');
# Read all the input header files into internal data structures
my $catalogs = Catalog::Catalogs(@input_files);
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 5bde610..564f77a 100644
--- a/src/backend/commands/user.c
+++
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Alvaro Herrera wrote: I think we should create a new header file (maybe acltypes.h or acldefs.h), which only contains the AclMode and RoleAttr typedefs and their associated defines; that one would be included from parsenodes.h, acl.h and pg_authid.h. Everything else would be in acl.h. So code that currently checks permissions using only acl.h do not need any extra includes. I propose this patch on top of Adam's v5. Also included is a full patch against master. Thanks! I've just read through your changes to Adam's v5 and they all look reasonable to me. I agree that having acldefs.h with the #define's is nicer and cleaner and reduces the amount of including needed for pg_authid. I also like that it removes those ACL_X definitions from parsenodes.h. Thanks also for the whiteline/line-wrap improvements and user.c cleanup, nice that we don't need all of those individual variables now that we're using a bitmask. Unrelated: when changing from unified to context format, I saw filterdiff fail to produce a complete patch, skipping some hunks in its output. My first impression was that I had dropped some hunks in git, so I wasted some time comparing v5 and v6 by hand before remembering that Michael Paquier had mentioned this problem previously. Ugh, that's definitely frustrating.. Will keep it in mind. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I have attached an updated patch with initial documentation changes for review. Awesome, thanks. I'm going to continue looking at this in more detail, but wanted to mention a few things I noticed in the documentation changes: diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml --- 1391,1493 /row row ! entrystructfieldrolattr/structfield/entry ! entrytypebigint/type/entry ! entry !Role attributes; see xref linkend=sql-createrole for details ! /entry ! /row Shouldn't this refer to the table below (which I like..)? Or perhaps to both the table and sql-createrole? Have you had a chance to actually build the docs and look at the results to see how things look? I should have time later tomorrow to do that and look over the results also, but figured I'd ask. ! table id=catalog-rolattr-bitmap-table !titlestructfieldrolattr/ bitmap positions/title I would call this 'Attributes in rolattr' or similar rather than 'bitmap positions'. !tgroup cols=3 ! thead ! row ! entryPosition/entry ! entryAttribute/entry ! entryDescription/entry ! /row ! /thead There should be a column for 'Option' or something- basically, a clear tie-back to what CREATE ROLE refers to these as. I'm thinking that reordering the columns would help here, consider: Attribute (using the 'Superuser', 'Inherit', etc 'nice' names) CREATE ROLE Clause (note: that's how CREATE ROLE describes the terms) Description Position ! tbody ! row ! entryliteral0/literal/entry ! entrySuperuser/entry entryRole has superuser privileges/entry /row row ! entryliteral1/literal/entry ! entryInherit/entry ! entryRole automatically inherits privileges of roles it is a member of/entry /row This doesn't follow our general convention to wrap lines in the SGML at 80 chars (same as the C code) and, really, if you fix that it looks like these lines shouldn't even be different at all (see above with the 'Role has superuser privileges' entry/entry line). Same is true for a few of the other cases. row ! entryliteral4/literal/entry ! entryCatalog Update/entry entry !Role can update system catalogs directly. (Even a superuser cannot do this unless this column is true) /entry /row I'm really not sure what to do with this one. I don't like leaving it as-is, but I suppose it's probably the right thing for this patch to do. Perhaps another patch should be proposed which improves the documentation around rolcatupdate and its relationship to superuser. diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml new file mode 100644 index ef69b94..74bc702 *** a/doc/src/sgml/func.sgml --- b/doc/src/sgml/func.sgml +para + functionpg_has_role_attribute/function checks the attribute permissions + given to a role. It will always return literaltrue/literal for roles + with superuser privileges unless the attribute being checked is + literalCATUPDATE/literal (superuser cannot bypass + literalCATUPDATE/literal permissions). The role can be specified by name + and by OID. The attribute is specified by a text string which must evaluate + to one of the following role attributes: + literalSUPERUSER/literal, + literalINHERIT/literal, + literalCREATEROLE/literal, + literalCREATEDB/literal, + literalCATUPDATE/literal, + literalCANLOGIN/literal, + literalREPLICATION/literal, or + literalBYPASSRLS/literal. This should probably refer to CREATE ROLE also as being where the meaning of these strings is defined. Otherwise, the docs look pretty good to me. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
FWIW I've been giving this patch a look and and adjusting some coding details here and there. Do you intend to commit it yourself? You're not listed as reviewer or committer for it in the commitfest app, FWIW. One thing I don't very much like is that check_role_attribute() receives a RoleAttr but nowhere it checks that only one bit is set in 'attribute'. From the coding, this routine would return true if just one of those bits is set, which is probably not what is intended. Now I realize that current callers all pass a bitmask with a single bit set, but I think it'd be better to have some protection against that, for possible future coding mistakes. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: FWIW I've been giving this patch a look and and adjusting some coding details here and there. Do you intend to commit it yourself? You're not listed as reviewer or committer for it in the commitfest app, FWIW. Oh, great, thanks! And, yeah, I'm not as good as I should be about updating the commitfest app. As for committing it, I was thinking I would but you're certainly welcome to if you're interested. I would like this to be committed soonish as it will then allow Adam to rebase the patch which adds the various role attributes discussed previously on top of the representation changes. I suspect he's done some work in that direction already, but I keep asking for changes to this patch which would then ripple down into the other. One thing I don't very much like is that check_role_attribute() receives a RoleAttr but nowhere it checks that only one bit is set in 'attribute'. From the coding, this routine would return true if just one of those bits is set, which is probably not what is intended. Now I realize that current callers all pass a bitmask with a single bit set, but I think it'd be better to have some protection against that, for possible future coding mistakes. That's certainly a good point. I'm inclined to suggest that there be an Assert() check in check_role_attribute(), or were you thinking of something else..? Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
All, 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. I have attached an updated patch with initial documentation changes for review. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml new file mode 100644 index 9ceb96b..b0b4fca *** a/doc/src/sgml/catalogs.sgml --- b/doc/src/sgml/catalogs.sgml *** *** 1391,1441 /row row ! entrystructfieldrolsuper/structfield/entry ! entrytypebool/type/entry entryRole has superuser privileges/entry /row row ! entrystructfieldrolinherit/structfield/entry ! entrytypebool/type/entry ! entryRole automatically inherits privileges of roles it is a !member of/entry /row row ! entrystructfieldrolcreaterole/structfield/entry ! entrytypebool/type/entry entryRole can create more roles/entry /row row ! entrystructfieldrolcreatedb/structfield/entry ! entrytypebool/type/entry entryRole can create databases/entry /row row ! entrystructfieldrolcatupdate/structfield/entry ! entrytypebool/type/entry entry !Role can update system catalogs directly. (Even a superuser cannot do !this unless this column is true) /entry /row row ! entrystructfieldrolcanlogin/structfield/entry ! entrytypebool/type/entry entry !Role can log in. That is, this role can be given as the initial !session authorization identifier /entry /row row ! entrystructfieldrolreplication/structfield/entry ! entrytypebool/type/entry entry Role is a replication role. That is, this role can initiate streaming replication (see xref linkend=streaming-replication) and set/unset --- 1391,1493 /row row ! entrystructfieldrolattr/structfield/entry ! entrytypebigint/type/entry ! entry !Role attributes; see xref linkend=sql-createrole for details ! /entry ! /row ! ! row ! entrystructfieldrolconnlimit/structfield/entry ! entrytypeint4/type/entry ! entry !For roles that can log in, this sets maximum number of concurrent !connections this role can make. -1 means no limit. ! /entry ! /row ! ! row ! entrystructfieldrolpassword/structfield/entry ! entrytypetext/type/entry ! entry !Password (possibly encrypted); null if none. If the password !is encrypted, this column will begin with the string literalmd5/ !followed by a 32-character hexadecimal MD5 hash. The MD5 hash !will be of the user's password concatenated to their user name. !For example, if user literaljoe/ has password literalxyzzy/, !productnamePostgreSQL/ will store the md5 hash of !literalxyzzyjoe/. A password that does not follow that !format is assumed to be unencrypted. ! /entry ! /row ! ! row ! entrystructfieldrolvaliduntil/structfield/entry ! entrytypetimestamptz/type/entry ! entryPassword expiry time (only used for password authentication); !null if no expiration/entry ! /row ! /tbody !/tgroup ! /table ! ! table id=catalog-rolattr-bitmap-table !titlestructfieldrolattr/ bitmap positions/title ! !tgroup cols=3 ! thead ! row ! entryPosition/entry ! entryAttribute/entry ! entryDescription/entry ! /row ! /thead ! ! tbody ! row ! entryliteral0/literal/entry ! entrySuperuser/entry entryRole has superuser privileges/entry /row row ! entryliteral1/literal/entry ! entryInherit/entry ! entryRole automatically inherits privileges of roles it is a member of/entry /row row ! entryliteral2/literal/entry ! entryCreate Role/entry entryRole can create more roles/entry /row row ! entryliteral3/literal/entry ! entryCreate DB/entry entryRole can create databases/entry /row row ! entryliteral4/literal/entry ! entryCatalog Update/entry entry !Role can update system catalogs directly. (Even a superuser cannot do this unless this column is true) /entry /row row ! entryliteral5/literal/entry ! entryCan Login/entry entry !Role can log in. That is, this role can be given as the initial session authorization identifier /entry
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Michael, This work will certainly continue to be pursued. If a simple move is possible/acceptable, then I think that would be the best option. I can handle that as it would appear that I am capable of moving it, if that is acceptable. Yes please. Actually I could have done it, just found the option to do so. Let's see what shows up with your work. I have moved it to commitfest 2014-12 and marked as Waiting on Author if that is acceptable. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
On Tue, Dec 9, 2014 at 12:10 AM, Adam Brightwell adam.brightw...@crunchydatasolutions.com wrote: Michael, This work will certainly continue to be pursued. If a simple move is possible/acceptable, then I think that would be the best option. I can handle that as it would appear that I am capable of moving it, if that is acceptable. Yes please. Actually I could have done it, just found the option to do so. Let's see what shows up with your work. I have moved it to commitfest 2014-12 and marked as Waiting on Author if that is acceptable. Thanks! I guess that's fine. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
On Sun, Dec 7, 2014 at 1:50 AM, Stephen Frost sfr...@snowman.net wrote: * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: 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. Hah, looked just like changes to the system_views, sorry for the confusion. :) 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. This patch (https://commitfest.postgresql.org/action/patch_view?id=1616) has not been updated in the commitfest app for two months, making its progress hard to track. However, even if some progress has been made things are still not complete (documentation, etc.). Opinions about marking that as returned with feedback for the current commit fest and create a new entry for the next CF if this work is going to be pursued? I guess that it would be fine simply move it to the next CF, but it seems I cannot do that myself in the CF app. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Michael, This patch (https://commitfest.postgresql.org/action/patch_view?id=1616) has not been updated in the commitfest app for two months, making its progress hard to track. I believe that the mentioned patch should be considered 'on hold' or 'dependent' upon the acceptance of the work that is being done in this thread. Also, the changes proposed by this thread have already been added to the next commitfest (https://commitfest.postgresql.org/action/patch_view?id=1651 ). However, even if some progress has been made things are still not complete (documentation, etc.). Opinions about marking that as returned with feedback for the current commit fest and create a new entry for the next CF if this work is going to be pursued? I guess that it would be fine simply move it to the next CF, but it seems I cannot do that myself in the CF app. This work will certainly continue to be pursued. If a simple move is possible/acceptable, then I think that would be the best option. I can handle that as it would appear that I am capable of moving it, if that is acceptable. Thanks, Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
On Mon, Dec 8, 2014 at 12:34 PM, Adam Brightwell adam.brightw...@crunchydatasolutions.com wrote: Michael, This patch (https://commitfest.postgresql.org/action/patch_view?id=1616) has not been updated in the commitfest app for two months, making its progress hard to track. I believe that the mentioned patch should be considered 'on hold' or 'dependent' upon the acceptance of the work that is being done in this thread. Also, the changes proposed by this thread have already been added to the next commitfest (https://commitfest.postgresql.org/action/patch_view?id=1651). However, even if some progress has been made things are still not complete (documentation, etc.). Opinions about marking that as returned with feedback for the current commit fest and create a new entry for the next CF if this work is going to be pursued? I guess that it would be fine simply move it to the next CF, but it seems I cannot do that myself in the CF app. This work will certainly continue to be pursued. If a simple move is possible/acceptable, then I think that would be the best option. I can handle that as it would appear that I am capable of moving it, if that is acceptable. Yes please. Actually I could have done it, just found the option to do so. Let's see what shows up with your work. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam, * Adam Brightwell (adam.brightw...@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 signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
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.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: 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. Hah, looked just like changes to the system_views, sorry for the confusion. :) 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. Great! Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
All,
I have attached an updated patch that incorporates the feedback and
recommendations provided.
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
new file mode 100644
index 133143d..ccdff09
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
***
*** 22,32
--- 22,34
#include access/xlog_internal.h
#include access/xlogutils.h
#include catalog/catalog.h
+ #include catalog/pg_authid.h
#include catalog/pg_type.h
#include funcapi.h
#include miscadmin.h
#include replication/walreceiver.h
#include storage/smgr.h
+ #include utils/acl.h
#include utils/builtins.h
#include utils/numeric.h
#include utils/guc.h
*** pg_start_backup(PG_FUNCTION_ARGS)
*** 54,60
backupidstr = text_to_cstring(backupid);
! if (!superuser() !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser or replication role to run a backup)));
--- 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)));
*** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,88
{
XLogRecPtr stoppoint;
! if (!superuser() !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg(must be superuser or replication role to run a backup;
--- 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;
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d30612c..0b23ba0
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*** aclcheck_error_type(AclResult aclerr, Oi
*** 3423,3448
}
- /* Check if given user has rolcatupdate privilege according to pg_authid */
- static bool
- has_rolcatupdate(Oid roleid)
- {
- bool rolcatupdate;
- 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)));
-
- rolcatupdate = ((Form_pg_authid) GETSTRUCT(tuple))-rolcatupdate;
-
- ReleaseSysCache(tuple);
-
- return rolcatupdate;
- }
-
/*
* Relay for the various pg_*_mask routines depending on object kind
*/
--- 3423,3428
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3630,3636
if ((mask (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
IsSystemClass(table_oid, classForm)
classForm-relkind != RELKIND_VIEW
! !has_rolcatupdate(roleid)
!allowSystemTableMods)
{
#ifdef ACLDEBUG
--- 3610,3616
if ((mask (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
IsSystemClass(table_oid, classForm)
classForm-relkind != RELKIND_VIEW
! !has_role_attribute(roleid, ROLE_ATTR_CATUPDATE)
!allowSystemTableMods)
{
#ifdef ACLDEBUG
*** 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.)
*/
bool
! has_createrole_privilege(Oid roleid)
{
! bool result = false;
! HeapTuple utup;
!
! /* Superusers bypass all permission checking. */
! if (superuser_arg(roleid))
return true;
! utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
! if (HeapTupleIsValid(utup))
! {
! result = ((Form_pg_authid) GETSTRUCT(utup))-rolcreaterole;
! ReleaseSysCache(utup);
! }
! return result;
}
bool
! has_bypassrls_privilege(Oid roleid)
{
! bool result = false;
! HeapTuple utup;
! /* Superusers bypass all permission checking. */
! if (superuser_arg(roleid))
! return true;
! utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
! if (HeapTupleIsValid(utup))
! {
! result = ((Form_pg_authid) GETSTRUCT(utup))-rolbypassrls;
!
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam,
* Adam Brightwell (adam.brightw...@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
boolencrypt_password = Password_encryption; /* encrypt
password? */
charencrypted_password[MD5_PASSWD_LEN + 1];
boolissuper = false;/* Make the user a superuser? */
! boolinherit = true; /* Auto inherit privileges? */
boolcreaterole = false; /* Can this user create
roles? */
boolcreatedb = false; /* Can the user create
databases? */
boolcanlogin = false; /* Can this user login?
*/
boolisreplication = false; /* Is this a replication role?
*/
boolbypassrls = 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
boolencrypt_password = Password_encryption; /* encrypt
password? */
charencrypted_password[MD5_PASSWD_LEN + 1];
boolissuper = false;/* Make the user a superuser? */
! boolinherit = true; /* Auto inherit privileges? */
boolcreaterole = false; /* Can this user create
roles? */
boolcreatedb = false; /* Can the user create
databases? */
boolcanlogin = false; /* Can this user login?
*/
boolisreplication = false; /* Is this a replication role?
*/
boolbypassrls = false; /* Is this a row
security enabled role? */
+ RoleAttrattributes = 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
Re: [HACKERS] 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.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
new file mode 100644
index 133143d..8475985
*** a/src/backend/access/transam/xlogfuncs.c
--- b/src/backend/access/transam/xlogfuncs.c
***
*** 22,32
--- 22,34
#include access/xlog_internal.h
#include access/xlogutils.h
#include catalog/catalog.h
+ #include catalog/pg_authid.h
#include catalog/pg_type.h
#include funcapi.h
#include miscadmin.h
#include replication/walreceiver.h
#include storage/smgr.h
+ #include utils/acl.h
#include utils/builtins.h
#include utils/numeric.h
#include utils/guc.h
*** pg_start_backup(PG_FUNCTION_ARGS)
*** 54,60
backupidstr = text_to_cstring(backupid);
! if (!superuser() !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser or replication role to run a backup)));
--- 56,62
backupidstr = text_to_cstring(backupid);
! if (!have_role_attribute(ROLE_ATTR_REPLICATION))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg(must be superuser or replication role to run a backup)));
*** pg_stop_backup(PG_FUNCTION_ARGS)
*** 82,88
{
XLogRecPtr stoppoint;
! if (!superuser() !has_rolreplication(GetUserId()))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg(must be superuser or replication role to run a backup;
--- 84,90
{
XLogRecPtr stoppoint;
! if (!have_role_attribute(ROLE_ATTR_REPLICATION))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
(errmsg(must be superuser or replication role to run a
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam, * Adam Brightwell (adam.brightw...@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. http://www.postgresql.org/message-id/cakrt6cqovt2kiykg2gff7h9k8+jvu1149zlb0extkkk7taq...@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 signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam,
* Adam Brightwell (adam.brightw...@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)
+ {
+ RoleAttrattributes;
+ 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 ~(0x)) | 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 */
boolencrypt_password = Password_encryption; /* encrypt
password? */
charencrypted_password[MD5_PASSWD_LEN + 1];
! boolissuper = false;/* Make the user a
superuser? */
! boolinherit = 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
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
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.
http://www.postgresql.org/message-id/cakrt6cqovt2kiykg2gff7h9k8+jvu1149zlb0extkkk7taq...@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 ~(0x)) | 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
1x */
! #define ROLE_ATTR_NONE 0
! #define ROLE_ATTR_ALL 255 /* All
currently available attributes. */
Or:
#define ROLE_ATTR_ALL (1N_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 (10)
+ #define ROLE_ATTR_INHERIT (11)
+ #define ROLE_ATTR_CREATEROLE(12)
+ #define ROLE_ATTR_CREATEDB (13)
+ #define ROLE_ATTR_CATUPDATE (14)
+ #define ROLE_ATTR_CANLOGIN (15)
+ #define ROLE_ATTR_REPLICATION (16)
+ #define ROLE_ATTR_BYPASSRLS (17)
+ #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.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
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.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
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
*** aclcheck_error_type(AclResult aclerr, Oi
*** 3423,3448
}
- /* Check if given user has rolcatupdate privilege according to pg_authid */
- static bool
- has_rolcatupdate(Oid roleid)
- {
- bool rolcatupdate;
- 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)));
-
- rolcatupdate = ((Form_pg_authid) GETSTRUCT(tuple))-rolcatupdate;
-
- ReleaseSysCache(tuple);
-
- return rolcatupdate;
- }
-
/*
* Relay for the various pg_*_mask routines depending on object kind
*/
--- 3423,3428
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3630,3636
if ((mask (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
IsSystemClass(table_oid, classForm)
classForm-relkind != RELKIND_VIEW
! !has_rolcatupdate(roleid)
!allowSystemTableMods)
{
#ifdef ACLDEBUG
--- 3610,3616
if ((mask (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
IsSystemClass(table_oid, classForm)
classForm-relkind != RELKIND_VIEW
! !role_has_attribute(roleid, ROLE_ATTR_CATUPDATE)
!allowSystemTableMods)
{
#ifdef ACLDEBUG
*** 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);
+ }
+
+ /*
* 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
*** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5064,5102
bool
has_createrole_privilege(Oid roleid)
{
- bool result = false;
- HeapTuple utup;
-
/* Superusers bypass all permission checking. */
if (superuser_arg(roleid))
return true;
! utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
! if (HeapTupleIsValid(utup))
! {
! result = ((Form_pg_authid) GETSTRUCT(utup))-rolcreaterole;
! ReleaseSysCache(utup);
! }
! return result;
}
bool
has_bypassrls_privilege(Oid roleid)
{
- bool result = false;
- HeapTuple utup;
-
/* Superusers bypass all permission checking. */
if (superuser_arg(roleid))
return true;
! utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
! if (HeapTupleIsValid(utup))
! {
! result = ((Form_pg_authid) GETSTRUCT(utup))-rolbypassrls;
! ReleaseSysCache(utup);
! }
! return result;
}
/*
--- 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);
}
+ /*
+ * Check whether specified role has BYPASSRLS privilege.
+ */
bool
has_bypassrls_privilege(Oid roleid)
{
/* Superusers bypass all permission checking. */
if (superuser_arg(roleid))
return true;
! return role_has_attribute(roleid, ROLE_ATTR_BYPASSRLS);
}
/*
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
new file mode 100644
index ca89879..2929b66
*** a/src/backend/catalog/genbki.pl
--- b/src/backend/catalog/genbki.pl
*** my $BOOTSTRAP_SUPERUSERID =
*** 90,95
---
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam, * Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: I am simply breaking this out into its own thread from the discussion on additional role attributes ( http://www.postgresql.org/message-id/20141015052259.gg28...@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 signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
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: http://www.postgresql.org/message-id/CAKRt6CTgJdeGFqXevrp-DizaeHmg8gNVqu8n5T=ix3javpw...@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 ( http://www.postgresql.org/message-id/ca+u5nmjgvdz6jx_ybjk99nj7mwfgfvemxtdc44lvhq64gkn...@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.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
* Adam Brightwell (adam.brightw...@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: http://www.postgresql.org/message-id/CAKRt6CTgJdeGFqXevrp-DizaeHmg8gNVqu8n5T=ix3javpw...@mail.gmail.com Yeah, done now. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
David, A few related threads/discussions/posts: * http://www.postgresql.org/message-id/ 20141016115914.GQ28859@.snowman * http://www.postgresql.org/message-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.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
* Adam Brightwell (adam.brightw...@crunchydatasolutions.com) wrote: An array representation was also suggested by Simon ( http://www.postgresql.org/message-id/ca+u5nmjgvdz6jx_ybjk99nj7mwfgfvemxtdc44lvhq64gkn...@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 signature.asc Description: Digital signature
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
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.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] 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
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Adam Brightwell wrote A few related threads/discussions/posts: * http://www.postgresql.org/message-id/ 20141016115914.GQ28859@.snowman * http://www.postgresql.org/message-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
