From e320679c4cc5cd9fc0f35f59fd22284fc2ecfb89 Mon Sep 17 00:00:00 2001
From: Mats Kindahl <mats@timescale.com>
Date: Fri, 7 Jan 2022 14:07:09 +0100
Subject: [PATCH] Remove grants on data node bootstrap

Starting with PG15, default permissions on the public schema is
restricted for any non-superuser non-owner. This causes test failures
since tables can no longer be created without explicitly adding
permissions, so we remove grant when bootstrapping the data nodes and
instead grant permissions to the users in the regression tests. This
keeps the default permissions on data nodes, but allow regression tests
to run.

Fixes #3957

Reference: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b073c3cc
---
 test/expected/trusted_extension.out           | 11 ++++++--
 test/runner.sh                                | 23 +++++++++++++--
 test/sql/trusted_extension.sql                | 12 +++++---
 test/test-defs.cmake                          |  7 +++--
 tsl/src/data_node.c                           | 12 --------
 tsl/test/expected/remote_connection_cache.out | 28 ++++++++++++-------
 tsl/test/sql/remote_connection_cache.sql      | 18 ++++++------
 7 files changed, 67 insertions(+), 44 deletions(-)

diff --git a/test/expected/trusted_extension.out b/test/expected/trusted_extension.out
index eefb810b8..7f0583f02 100644
--- a/test/expected/trusted_extension.out
+++ b/test/expected/trusted_extension.out
@@ -3,9 +3,14 @@
 -- LICENSE-APACHE for a copy of the license.
 \c :TEST_DBNAME :ROLE_SUPERUSER
 CREATE DATABASE trusted_test;
-GRANT CREATE ON DATABASE trusted_test TO test_role_1;
-\c trusted_test test_role_1
--- user shouldnt have superuser privilege
+GRANT CREATE ON DATABASE trusted_test TO :ROLE_1;
+\c trusted_test :ROLE_READ_ONLY
+\set ON_ERROR_STOP 0
+CREATE EXTENSION timescaledb;
+ERROR:  permission denied to create extension "timescaledb"
+\set ON_ERROR_STOP 1
+\c trusted_test :ROLE_1
+-- user shouldn't have superuser privilege
 SELECT rolsuper FROM pg_roles WHERE rolname=user;
  rolsuper 
 ----------
diff --git a/test/runner.sh b/test/runner.sh
index 735998658..e1a8a5b72 100755
--- a/test/runner.sh
+++ b/test/runner.sh
@@ -41,12 +41,23 @@ TEST_ROLE_2=${TEST_ROLE_2:-test_role_2}
 TEST_ROLE_2_PASS=${TEST_ROLE_2_PASS:-pass}
 TEST_ROLE_3=${TEST_ROLE_3:-test_role_3}
 TEST_ROLE_3_PASS=${TEST_ROLE_3_PASS:-pass}
+TEST_ROLE_READ_ONLY=${TEST_ROLE_READ_ONLY:-test_role_read_only}
 
 shift
 
 # Drop test database and make it less verbose in case of dropping a
-# distributed database
+# distributed database. Also revoke privileges granted when setting up
+# the template1 database. This has to be revoked since the user is
+# dropped from a different database.
 function cleanup {
+    ${PSQL} "$@" -U ${USER} -d template1 -v ECHO=none >/dev/null 2>&1 <<EOF
+REVOKE CREATE ON SCHEMA public FROM ${TEST_PGUSER};
+REVOKE CREATE ON SCHEMA public FROM ${TEST_ROLE_DEFAULT_PERM_USER};
+REVOKE CREATE ON SCHEMA public FROM ${TEST_ROLE_DEFAULT_PERM_USER_2};
+REVOKE CREATE ON SCHEMA public FROM ${TEST_ROLE_1};
+REVOKE CREATE ON SCHEMA public FROM ${TEST_ROLE_2};
+REVOKE CREATE ON SCHEMA public FROM ${TEST_ROLE_3};
+EOF
     cat <<EOF | ${PSQL} "$@" -U $TEST_ROLE_SUPERUSER -d postgres -v ECHO=none >/dev/null 2>&1
     SET client_min_messages=ERROR;
     DROP DATABASE "${TEST_DBNAME}";
@@ -59,9 +70,14 @@ trap cleanup EXIT
 # we use mkdir here because it is an atomic operation unlike existance of a lockfile
 # where creating and checking are 2 separate operations
 if mkdir ${TEST_OUTPUT_DIR}/.pg_init 2>/dev/null; then
-  cat <<EOF | ${PSQL} "$@" -U ${USER} -d template1 -v ECHO=none >/dev/null 2>&1
+  ${PSQL} "$@" -U ${USER} -d template1 -v ECHO=none >/dev/null 2>&1 <<EOF
     SET client_min_messages=ERROR;
-    GRANT CREATE ON SCHEMA public TO PUBLIC;
+    GRANT CREATE ON SCHEMA public TO ${TEST_PGUSER};
+    GRANT CREATE ON SCHEMA public TO ${TEST_ROLE_DEFAULT_PERM_USER};
+    GRANT CREATE ON SCHEMA public TO ${TEST_ROLE_DEFAULT_PERM_USER_2};
+    GRANT CREATE ON SCHEMA public TO ${TEST_ROLE_1};
+    GRANT CREATE ON SCHEMA public TO ${TEST_ROLE_2};
+    GRANT CREATE ON SCHEMA public TO ${TEST_ROLE_3};
     ALTER USER ${TEST_ROLE_SUPERUSER} WITH SUPERUSER;
     ALTER USER ${TEST_ROLE_CLUSTER_SUPERUSER} WITH SUPERUSER;
     ALTER USER ${TEST_ROLE_1} WITH CREATEDB CREATEROLE;
@@ -107,6 +123,7 @@ ${PSQL} -U ${TEST_PGUSER} \
      -v ROLE_1=${TEST_ROLE_1} \
      -v ROLE_2=${TEST_ROLE_2} \
      -v ROLE_3=${TEST_ROLE_3} \
+     -v ROLE_READ_ONLY=${TEST_ROLE_READ_ONLY} \
      -v ROLE_2_PASS=${TEST_ROLE_2_PASS} \
      -v ROLE_3_PASS=${TEST_ROLE_3_PASS} \
      -v MODULE_PATHNAME="'timescaledb-${EXT_VERSION}'" \
diff --git a/test/sql/trusted_extension.sql b/test/sql/trusted_extension.sql
index c2b3740f0..13dd81a51 100644
--- a/test/sql/trusted_extension.sql
+++ b/test/sql/trusted_extension.sql
@@ -3,12 +3,16 @@
 -- LICENSE-APACHE for a copy of the license.
 
 \c :TEST_DBNAME :ROLE_SUPERUSER
-
 CREATE DATABASE trusted_test;
-GRANT CREATE ON DATABASE trusted_test TO test_role_1;
+GRANT CREATE ON DATABASE trusted_test TO :ROLE_1;
 
-\c trusted_test test_role_1
--- user shouldnt have superuser privilege
+\c trusted_test :ROLE_READ_ONLY
+\set ON_ERROR_STOP 0
+CREATE EXTENSION timescaledb;
+\set ON_ERROR_STOP 1
+
+\c trusted_test :ROLE_1
+-- user shouldn't have superuser privilege
 SELECT rolsuper FROM pg_roles WHERE rolname=user;
 
 SET client_min_messages TO ERROR;
diff --git a/test/test-defs.cmake b/test/test-defs.cmake
index f4c936c5e..b47baf918 100644
--- a/test/test-defs.cmake
+++ b/test/test-defs.cmake
@@ -6,6 +6,7 @@ set(TEST_ROLE_DEFAULT_PERM_USER_2 default_perm_user_2)
 set(TEST_ROLE_1 test_role_1)
 set(TEST_ROLE_2 test_role_2)
 set(TEST_ROLE_3 test_role_3)
+set(TEST_ROLE_READ_ONLY test_role_read_only)
 
 # TEST_ROLE_2 has password in passfile
 set(TEST_ROLE_2_PASS pass)
@@ -56,7 +57,7 @@ set(PG_REGRESS_OPTS_BASE --host=${TEST_PGHOST}
                          --dlpath=${PROJECT_BINARY_DIR}/src)
 
 set(PG_REGRESS_OPTS_EXTRA
-    --create-role=${TEST_ROLE_SUPERUSER},${TEST_ROLE_DEFAULT_PERM_USER},${TEST_ROLE_DEFAULT_PERM_USER_2},${TEST_ROLE_CLUSTER_SUPERUSER},${TEST_ROLE_1},${TEST_ROLE_2},${TEST_ROLE_3}
+    --create-role=${TEST_ROLE_SUPERUSER},${TEST_ROLE_DEFAULT_PERM_USER},${TEST_ROLE_DEFAULT_PERM_USER_2},${TEST_ROLE_CLUSTER_SUPERUSER},${TEST_ROLE_1},${TEST_ROLE_2},${TEST_ROLE_3},${TEST_ROLE_READ_ONLY}
     --dbname=${TEST_DBNAME}
     --launcher=${PRIMARY_TEST_DIR}/runner.sh)
 
@@ -66,7 +67,7 @@ set(PG_REGRESS_SHARED_OPTS_EXTRA
     --launcher=${PRIMARY_TEST_DIR}/runner_shared.sh)
 
 set(PG_ISOLATION_REGRESS_OPTS_EXTRA
-    --create-role=${TEST_ROLE_SUPERUSER},${TEST_ROLE_DEFAULT_PERM_USER},${TEST_ROLE_DEFAULT_PERM_USER_2},${TEST_ROLE_CLUSTER_SUPERUSER},${TEST_ROLE_1},${TEST_ROLE_2},${TEST_ROLE_3}
+    --create-role=${TEST_ROLE_SUPERUSER},${TEST_ROLE_DEFAULT_PERM_USER},${TEST_ROLE_DEFAULT_PERM_USER_2},${TEST_ROLE_CLUSTER_SUPERUSER},${TEST_ROLE_1},${TEST_ROLE_2},${TEST_ROLE_3},${TEST_ROLE_READ_ONLY}
     --dbname=${TEST_DBNAME})
 
 set(PG_REGRESS_OPTS_INOUT --inputdir=${TEST_INPUT_DIR}
@@ -101,6 +102,7 @@ if(PG_REGRESS)
       TEST_ROLE_1=${TEST_ROLE_1}
       TEST_ROLE_2=${TEST_ROLE_2}
       TEST_ROLE_3=${TEST_ROLE_3}
+      TEST_ROLE_READ_ONLY=${TEST_ROLE_READ_ONLY}
       TEST_ROLE_2_PASS=${TEST_ROLE_2_PASS}
       TEST_ROLE_3_PASS=${TEST_ROLE_3_PASS}
       TEST_DBNAME=${TEST_DBNAME}
@@ -123,6 +125,7 @@ if(PG_ISOLATION_REGRESS)
       TEST_ROLE_3=${TEST_ROLE_3}
       TEST_ROLE_2_PASS=${TEST_2_PASS}
       TEST_ROLE_3_PASS=${TEST_3_PASS}
+      TEST_ROLE_READ_ONLY=${TEST_ROLE_READ_ONLY}
       TEST_DBNAME=${TEST_DBNAME}
       TEST_INPUT_DIR=${TEST_INPUT_DIR}
       TEST_OUTPUT_DIR=${TEST_OUTPUT_DIR}
diff --git a/tsl/src/data_node.c b/tsl/src/data_node.c
index 510a93ffe..2f3f4272d 100644
--- a/tsl/src/data_node.c
+++ b/tsl/src/data_node.c
@@ -513,18 +513,6 @@ data_node_bootstrap_extension(TSConnection *conn)
 								  " WITH SCHEMA %s VERSION %s CASCADE",
 								  schema_name_quoted,
 								  quote_literal_cstr(ts_extension_get_version()));
-#if PG15_GE
-		/*
-		 * Since PG15, by default, non-superuser accounts are not allowed to
-		 * create tables in public schema of databases they don't own. This
-		 * default can be changed manually. The following query ensures that the
-		 * permissions are going to be the same regardless of the used PostgreSQL
-		 * version.
-		 *
-		 * https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b073c3cc
-		 */
-		remote_connection_cmdf_ok(conn, "GRANT CREATE ON SCHEMA public TO PUBLIC");
-#endif
 		return true;
 	}
 	else
diff --git a/tsl/test/expected/remote_connection_cache.out b/tsl/test/expected/remote_connection_cache.out
index b00580e9d..92a38be73 100644
--- a/tsl/test/expected/remote_connection_cache.out
+++ b/tsl/test/expected/remote_connection_cache.out
@@ -12,16 +12,24 @@ CREATE FUNCTION _timescaledb_internal.test_alter_data_node(node_name NAME)
 RETURNS BOOL
 AS :TSL_MODULE_PATHNAME, 'ts_test_alter_data_node'
 LANGUAGE C STRICT;
-DO $d$
-    BEGIN
-        EXECUTE $$SELECT add_data_node('loopback_1', host => 'localhost',
-                database => 'db_remote_connection_cache_1',
-                port => current_setting('port')::int)$$;
-        EXECUTE $$SELECT add_data_node('loopback_2', host => 'localhost',
-                database => 'db_remote_connection_cache_2',
-                port => current_setting('port')::int)$$;
-    END;
-$d$;
+SET client_min_messages TO WARNING;
+SELECT node_name, database, node_created, extension_created
+FROM add_data_node('loopback_1', host => 'localhost', database => :'DN_DBNAME_1',
+                   port => current_setting('port')::int);
+ node_name  |           database           | node_created | extension_created 
+------------+------------------------------+--------------+-------------------
+ loopback_1 | db_remote_connection_cache_1 | t            | t
+(1 row)
+
+SELECT node_name, database, node_created, extension_created
+FROM add_data_node('loopback_2', host => 'localhost', database => :'DN_DBNAME_2',
+                   port => current_setting('port')::int);
+ node_name  |           database           | node_created | extension_created 
+------------+------------------------------+--------------+-------------------
+ loopback_2 | db_remote_connection_cache_2 | t            | t
+(1 row)
+
+SET client_min_messages TO INFO;
 SELECT _timescaledb_internal.test_remote_connection_cache();
  test_remote_connection_cache 
 ------------------------------
diff --git a/tsl/test/sql/remote_connection_cache.sql b/tsl/test/sql/remote_connection_cache.sql
index 91fac054b..4e1daab16 100644
--- a/tsl/test/sql/remote_connection_cache.sql
+++ b/tsl/test/sql/remote_connection_cache.sql
@@ -17,16 +17,14 @@ RETURNS BOOL
 AS :TSL_MODULE_PATHNAME, 'ts_test_alter_data_node'
 LANGUAGE C STRICT;
 
-DO $d$
-    BEGIN
-        EXECUTE $$SELECT add_data_node('loopback_1', host => 'localhost',
-                database => 'db_remote_connection_cache_1',
-                port => current_setting('port')::int)$$;
-        EXECUTE $$SELECT add_data_node('loopback_2', host => 'localhost',
-                database => 'db_remote_connection_cache_2',
-                port => current_setting('port')::int)$$;
-    END;
-$d$;
+SET client_min_messages TO WARNING;
+SELECT node_name, database, node_created, extension_created
+FROM add_data_node('loopback_1', host => 'localhost', database => :'DN_DBNAME_1',
+                   port => current_setting('port')::int);
+SELECT node_name, database, node_created, extension_created
+FROM add_data_node('loopback_2', host => 'localhost', database => :'DN_DBNAME_2',
+                   port => current_setting('port')::int);
+SET client_min_messages TO INFO;
 
 SELECT _timescaledb_internal.test_remote_connection_cache();