From: | Jose Luis Tallon <jltallon(at)adv-solutions(dot)net> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Default Roles |
Date: | 2016-03-30 11:14:37 |
Message-ID: | 20160330111437.1315.72685.pgcf@coridan.postgresql.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
* Applies cleanly to current master (3063e7a84026ced2aadd2262f75eebbe6240f85b)
* ./configure && make -j4 ok
DOCUMENTATION
* Documentation ok, both code (code comments) and docs.
* Documentation covers signalling backends/backup/monitor as well as the obvious modification to the role sections
CODE
* Checks on roles are fairly comprehensive: restrict reserved rolenames (creation/modification), prohibit granting to reserved roles
* The patch is surprisingly non-intrusive/self-contained considering the functionality.
TOOLS
* Covers pg_upgrade -- "/* 9.5 and below should not have roles starting with pg_ */"
* Covers pg_dumpall (do not export creation of system-reserved roles)
* Includes support in psql (\dgS) + accompanying documentation
REGRESSION TESTS
* Includes regression tests; Seem quite complete (including GRANT/REVOKE on reserved roles)
Suggestion for committer: add regression tests for each reserved role? (just for completeness' sake)
* make installcheck-world passes; build on amd64 / gcc4.9.2 (Debian 4.9.2-10)
- btree_gin tests fail / no contrib installed; Assumed ok
* Nitpick: tests mention the still nonexistant pg_monitor / pg_backup reserved roles ; Might as well use some obviously reserved-but-absurd rolename instead
Comment for future enhancement: might make sense to split role checking/access control functionality into a separate module, as opposed to having to include pg_authid.h everywhere
I'm Thinking about Michael and Heikki's upcoming authentication revamp re. SCRAM/multiple authenticators: authentication != authorization (apropos "has_privs_of_role()" )
Testing:
- pg_signal_backend Ok
Looking forward to seeing the other proposed default roles in!
The new status of this patch is: Ready for Committer
From | Date | Subject | |
---|---|---|---|
Next Message | Greg Stark | 2016-03-30 11:22:40 | Re: Using quicksort for every external sort run |
Previous Message | Andreas Kretschmer | 2016-03-30 10:45:54 | Re: pg_largeobject |