On Sat, Nov 21, 2015 at 2:29 AM, Stephen Frost <sfr...@snowman.net> wrote: > * Michael Paquier (michael.paqu...@gmail.com) wrote: > Even so, in the interest of having more fine-grained permission > controls, I've gone ahead and added a pg_switch_xlog default role. > Note that this means that pg_switch_xlog() can be called by both > pg_switch_xlog roles and pg_backup roles. I'd be very much against > removing the ability to call pg_switch_xlog from the pg_backup role as > that really is a capability which is needed by users running backups and > it'd just add unnecessary complexity to require users setting up backup > tools to grant two different roles to get the backup to work.
There is going to be many opinions regarding the granularity of this control, each one of us having a different opinion at the end. I don't think this should be a stopper for this patch, hence I am fine with the judgement you think is good. We could still more finely tune those default roles later in the dev cycle of 9.6 (10.0?). >> For the code paths where a backend would be actually running, >> you could use the following: >> SET client_min_messages TO 'error'; >> -- This should return "1" and not an ERROR nor a WARNING if PID does not exist. >> select count(pg_terminate_backend(0)); >> But that's ugly depending on your taste. > > I really dislike that. > >> Do you think that it makes sense to add tests as well in the second >> patch to check that restrictions pg_* are in place? Except that I >> don't have much more to say about patches 1 and 2 which are rather >> straight-forward. > > Ah, yes. I've now moved those hunks to the second patch where they > really should have been. > >> Regarding patch 3, the documentation of psql should mention the new >> subommands \dgS and \dgS+. That's a one-liner. > > Ah, right. I've updated the psql SGML documentation for \duS and \dgS. > The '\h' output had already been updated. Was there something else > which you meant above that I've missed? Note that these fixes went into > the second patch. Thanks, this looks good to me. >> +GRANT pg_backup TO regressuser1; >> +SET SESSION AUTHORIZATION regressuser1; >> +SELECT pg_start_backup('abc'); -- fail >> +ERROR: WAL level not sufficient for making an online backup >> +HINT: wal_level must be set to "archive", "hot_standby", or >> "logical" at server start. >> make install would wal on a server with something else than wal_level >> = minimal. Just checking that errors kick correctly looks fine to me >> here. > > I've removed those checks as they could get annoying on the buildfarm or > for people doing make installcheck, as you say, but I'm not really > thrilled that we're only testing the failure paths. I guess that's better than nothing. >> +GRANT pg_backup TO pg_monitor; -- error >> +ERROR: role "pg_monitor" is reserved >> +DETAIL: Roles starting with "pg_" are reserved. >> +GRANT testrol0 TO pg_backup; -- error >> +ERROR: role "pg_backup" is reserved >> We would gain with verbose error messages here, like, "cannot GRANT >> somerole to a system role", and "cannot GRANT system role to >> somerole". > > Alright, I've changed these to have better detail messages. @@ -183,6 +190,13 @@ pg_current_xlog_location(PG_FUNCTION_ARGS) { XLogRecPtr current_recptr; + if (!has_privs_of_role(GetUserId(), DEFAULT_ROLE_BACKUPID) && + !has_privs_of_role(GetUserId(), DEFAULT_ROLE_MONITORID) && + !has_privs_of_role(GetUserId(), DEFAULT_ROLE_SWITCH_XLOGID)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser or member of pg_monitor, pg_backup, or pg_switch_xlog to switch transaction log files"))); I don't think you mean to refer to the switch of segments files here. Same comment for pg_current_xlog_insert_location, pg_last_xlog_receive_location and pg_last_xlog_replay_location. + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("must be superuser or member of pg_file_settings to see all config file settings"))); Should avoid abbreviations => "all configuration file settings". <varlistentry> - <term><literal>\dg[+] [ <link linkend="APP-PSQL-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term> + <term><literal>\dgS[+] [ <link linkend="APP-PSQL-patterns"><replaceable class="parameter">pattern</replaceable></link> ]</literal></term> <listitem> I'm picky here, but that should be "\dg[S+]". Same for \du[S+]. The rest looks fine. Regards, -- Michael