Thank you for pointing to some problems of the patch. I've attached a
modified version below.
On Thu, Aug 2, 2018 at 8:38 PM Michael Paquier <mich...@paquier.xyz> wrote:
> On Thu, Aug 02, 2018 at 06:25:14PM +0100, Alexandra Ryzhevich wrote:
> > I have noticed that there is no regression tests for default monitoring
> > roles such as pg_read_all_stats.
>
> A bug has been recently fixed for that, see 0c8910a0, so having more
> coverage would be welcome, now your patch has a couple of problems.
> 25fff40 is the original commit which has introduced pg_read_all_stats.
>
> Your patch has a couple of problems by the way:
> - database creation is costly, those should not be part of the main
> regression test suite.
> - Any roles created should use "regress_" as prefix.
> - You should look also at negative tests which trigger "must be
> superuser or a member of pg_read_all_settings", like say a "SHOW
> stats_temp_directory" with a non-privileged user.
> --
> Michael
>
From 40145d9431bb26b2c172b3529eceb0595703b7e0 Mon Sep 17 00:00:00 2001
From: Alexandra Ryzhevich <aryzhev...@google.com>
Date: Fri, 3 Aug 2018 12:34:53 +0100
Subject: [PATCH 1/1] Add regress test for pg_read_all_stats role
---
src/test/regress/expected/defroles.out | 102 +++++++++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/defroles.sql | 71 +++++++++++++++++
4 files changed, 175 insertions(+), 1 deletion(-)
create mode 100644 src/test/regress/expected/defroles.out
create mode 100644 src/test/regress/sql/defroles.sql
diff --git a/src/test/regress/expected/defroles.out b/src/test/regress/expected/defroles.out
new file mode 100644
index 0000000000..7cb44ae8bb
--- /dev/null
+++ b/src/test/regress/expected/defroles.out
@@ -0,0 +1,102 @@
+-- DEFAULT MONITORING ROLES
+DROP ROLE IF EXISTS regress_role_haspriv;
+NOTICE: role "regress_role_haspriv" does not exist, skipping
+DROP ROLE IF EXISTS regress_role_nopriv;
+NOTICE: role "regress_role_nopriv" does not exist, skipping
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+----
+-- pg_read_all_stats
+----
+REVOKE CONNECT ON DATABASE regression FROM public;
+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_database_size('regression') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- 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" LIKE '<insufficient privilege>';
+ haspriv
+---------
+ t
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CONNECT permission on this db
+SELECT pg_database_size('regression') > 0 AS canread;
+ERROR: permission denied for database regression
+-- 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" LIKE '<insufficient privilege>';
+ haspriv
+---------
+ f
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+GRANT CONNECT ON DATABASE regression TO public;
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+----
+-- pg_read_all_settings
+----
+GRANT pg_read_all_settings TO regress_role_haspriv;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW stats_temp_directory;
+ stats_temp_directory
+----------------------
+ pg_stat_tmp
+(1 row)
+
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW archive_timeout;
+ archive_timeout
+-----------------
+ 0
+(1 row)
+
+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 stats_temp_directory;
+ERROR: must be superuser or a member of pg_read_all_settings to examine "stats_temp_directory"
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW archive_timeout;
+ archive_timeout
+-----------------
+ 0
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
+----
+-- Clean up
+----
+DROP ROLE regress_role_haspriv;
+DROP ROLE regress_role_nopriv;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 16f979c8d9..d6cf7b8226 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -89,7 +89,7 @@ test: brin gin gist spgist privileges init_privs security_label collate matview
# ----------
# Another group of parallel tests
# ----------
-test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext
+test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext defroles
# rules cannot run concurrently with any test that creates a view
test: rules psql_crosstab amutils
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 42632be675..b12aa09904 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -135,6 +135,7 @@ test: sysviews
test: tsrf
test: tidscan
test: stats_ext
+test: defroles
test: rules
test: psql_crosstab
test: select_parallel
diff --git a/src/test/regress/sql/defroles.sql b/src/test/regress/sql/defroles.sql
new file mode 100644
index 0000000000..d75d097981
--- /dev/null
+++ b/src/test/regress/sql/defroles.sql
@@ -0,0 +1,71 @@
+-- DEFAULT MONITORING ROLES
+
+DROP ROLE IF EXISTS regress_role_haspriv;
+DROP ROLE IF EXISTS regress_role_nopriv;
+
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+
+
+----
+-- pg_read_all_stats
+----
+
+REVOKE CONNECT ON DATABASE regression FROM public;
+
+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_database_size('regression') > 0 AS canread;
+-- 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" LIKE '<insufficient privilege>';
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CONNECT permission on this db
+SELECT pg_database_size('regression') > 0 AS canread;
+-- 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" LIKE '<insufficient privilege>';
+
+RESET SESSION AUTHORIZATION;
+
+GRANT CONNECT ON DATABASE regression TO public;
+
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+
+----
+-- pg_read_all_settings
+----
+
+GRANT pg_read_all_settings TO regress_role_haspriv;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW stats_temp_directory;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW archive_timeout;
+
+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 stats_temp_directory;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW archive_timeout;
+
+RESET SESSION AUTHORIZATION;
+
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
+
+----
+-- Clean up
+----
+
+DROP ROLE regress_role_haspriv;
+DROP ROLE regress_role_nopriv;
--
2.18.0.597.ga71716f1ad-goog
