On Thu, Aug 23, 2018 at 9:12 AM Michael Paquier <mich...@paquier.xyz> wrote:
> On Tue, Aug 21, 2018 at 05:48:49PM +0100, Alexandra Ryzhevich wrote:
> > Just to check if changes broke something. I haven't find these checks in
> > other regress tests. In other way we get only positive tests. If this
> > is not needed then should I remove all the checks for
> > regress_role_nopriv role or negative regress_role_nopriv tests only?
>
> +-- should not fail because regress_role_haspriv is a member of
> pg_read_all_stats
> +SELECT pg_database_size('regression') > 0 AS canread;
> What is really disturbing about the ones testing the size functions is
> that they may be costly when running installcheck. By the way, it would
> be nice to avoid the system-wide REVOKE, which could impact any tests
> running in parallel. You could simply check for some other NULL
> values.
>
CONNECT ON DATABASE privilege is granted to public by default (so
all users by default can select pg_database_size()) and
REVOKE CONNECT ... FROM <username> command doesn't impact
user's privileges. The only way to revoke connect privilege from user
is to revoke it from public. That's why maybe it will be less harmful just
to
remove pg_database_size tests at all. But will it be better to remove
pg_tablespace_size() tests also? if possible costs are more harmful
than not testing this code path then I'll remove these tests also.
> Changed to use session_preload_libraries.
>
> +-- session_preload_libraries is of PGC_SUSET category
> +SET LOCAL session_preload_libraries TO '/tmp/';
> +-- vacuum_cost_delay is of PGC_USERSET category
> +SET LOCAL vacuum_cost_delay TO 40;
>
> This gets better, thanks. I would suggest using a less realistic value
> than "/tmp/", which could become a security hole if copy-pasted around...
>
Changed to unrealistic 'path-to-preload-libraries'.
Alexandra
From 000a5b728f0d3ec5079d6421aad60c4aae0f44b8 Mon Sep 17 00:00:00 2001
From: Alexandra Ryzhevich <aryzhev...@google.com>
Date: Fri, 24 Aug 2018 15:08:45 +0100
Subject: [PATCH 1/1] Add regress tests for default monitoring roles
---
src/test/regress/expected/rolenames.out | 86 +++++++++++++++++++++++++
src/test/regress/sql/rolenames.sql | 62 ++++++++++++++++++
2 files changed, 148 insertions(+)
diff --git a/src/test/regress/expected/rolenames.out b/src/test/regress/expected/rolenames.out
index 7daba9fc12..37084c1617 100644
--- a/src/test/regress/expected/rolenames.out
+++ b/src/test/regress/expected/rolenames.out
@@ -944,9 +944,95 @@ SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
testagg9 |
(9 rows)
+-- DEFAULT MONITORING ROLES
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+-- pg_read_all_stats
+GRANT pg_read_all_stats TO regress_role_haspriv;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be true because regress_role_haspriv is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+ haspriv
+---------
+ t
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ERROR: permission denied for tablespace pg_global
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+ haspriv
+---------
+ f
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+-- pg_read_all_settings
+GRANT pg_read_all_settings TO regress_role_haspriv;
+BEGIN;
+-- session_preload_libraries is of PGC_SUSET category
+SET LOCAL session_preload_libraries TO 'path-to-preload-libraries';
+-- vacuum_cost_delay is of PGC_USERSET category
+SET LOCAL vacuum_cost_delay TO 40;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW session_preload_libraries;
+ session_preload_libraries
+-----------------------------
+ "path-to-preload-libraries"
+(1 row)
+
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+ vacuum_cost_delay
+-------------------
+ 40ms
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+ vacuum_cost_delay
+-------------------
+ 40ms
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+ROLLBACK;
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv is not superuser and is not a member of pg_read_all_settings
+SHOW session_preload_libraries;
+ERROR: must be superuser or a member of pg_read_all_settings to examine "session_preload_libraries"
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
-- clean up
\c
DROP SCHEMA test_roles_schema;
DROP OWNED BY regress_testrol0, "Public", "current_user", regress_testrol1, regress_testrol2, regress_testrolx CASCADE;
DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2, regress_testrolx;
DROP ROLE "Public", "None", "current_user", "session_user", "user";
+DROP ROLE regress_role_haspriv, regress_role_nopriv;
diff --git a/src/test/regress/sql/rolenames.sql b/src/test/regress/sql/rolenames.sql
index 5fe8bc8bca..3e4e91632a 100644
--- a/src/test/regress/sql/rolenames.sql
+++ b/src/test/regress/sql/rolenames.sql
@@ -438,6 +438,67 @@ REVOKE ALL PRIVILEGES ON FUNCTION testagg9(int2) FROM "none"; --error
SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
+
+-- DEFAULT MONITORING ROLES
+
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+
+-- pg_read_all_stats
+
+GRANT pg_read_all_stats TO regress_role_haspriv;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be true because regress_role_haspriv is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+
+RESET SESSION AUTHORIZATION;
+
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+
+-- pg_read_all_settings
+
+GRANT pg_read_all_settings TO regress_role_haspriv;
+
+BEGIN;
+-- session_preload_libraries is of PGC_SUSET category
+SET LOCAL session_preload_libraries TO 'path-to-preload-libraries';
+-- vacuum_cost_delay is of PGC_USERSET category
+SET LOCAL vacuum_cost_delay TO 40;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW session_preload_libraries;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+
+RESET SESSION AUTHORIZATION;
+ROLLBACK;
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv is not superuser and is not a member of pg_read_all_settings
+SHOW session_preload_libraries;
+
+RESET SESSION AUTHORIZATION;
+
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
+
-- clean up
\c
@@ -445,3 +506,4 @@ DROP SCHEMA test_roles_schema;
DROP OWNED BY regress_testrol0, "Public", "current_user", regress_testrol1, regress_testrol2, regress_testrolx CASCADE;
DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2, regress_testrolx;
DROP ROLE "Public", "None", "current_user", "session_user", "user";
+DROP ROLE regress_role_haspriv, regress_role_nopriv;
--
2.19.0.rc0.228.g281dcd1b4d0-goog
