Switch TAP tests of pg_rewind to use role with only function permissions

Started by Michael Paquieralmost 7 years ago3 messages

michael@paquier.xyz

almost 7 years ago

1 attachment(s)

Hi all,

Recent commit bfc80683 has added some documentation in pg_rewind about
the fact that it is possible to do the operation with a non-superuser,
assuming that this role has sufficient grant rights to execute the
functions used by pg_rewind.

Peter Eisentraut has suggested to have some tests for this kind of
user here:
/messages/by-id/e1570ba6-4459-d9b2-1321-9449adaaef4c@2ndquadrant.com

Attached is a patch which switches all the TAP tests of pg_rewind to
do that. As of now, the tests depend on a superuser for everything,
and it seems to me that it makes little sense to make the tests more
pluggable by being able to switch the roles used on-the-fly (the
invocation of pg_rewind is stuck into RewindTest.pm) as a superuser
has no restrictions.

Any thoughts?
--
Michael

Attachments:

rewind-user-tap.patchtext/x-diff; charset=us-asciiDownload

diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 900d452d8b..618de85161 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -144,6 +144,20 @@ sub start_master
 {
 	$node_master->start;
 
+	# Create a custom role which will be used to run pg_rewind.  This has
+	# minimal permissions to make pg_rewind able to work with an online
+	# source.
+	$node_master->psql('postgres', "
+		CREATE ROLE rewind_user LOGIN;
+		GRANT EXECUTE ON function pg_catalog.pg_ls_dir(text, boolean, boolean)
+		  TO rewind_user;
+		GRANT EXECUTE ON function pg_catalog.pg_stat_file(text, boolean)
+		  TO rewind_user;
+		GRANT EXECUTE ON function pg_catalog.pg_read_binary_file(text)
+		  TO rewind_user;
+		GRANT EXECUTE ON function pg_catalog.pg_read_binary_file(text, bigint, bigint, boolean)
+		  TO rewind_user;");
+
 	#### Now run the test-specific parts to initialize the master before setting
 	# up standby
 
@@ -207,6 +221,9 @@ sub run_pg_rewind
 	my $standby_connstr = $node_standby->connstr('postgres');
 	my $tmp_folder      = TestLib::tempdir;
 
+	# Append the rewind role to the connection string.
+	$standby_connstr = "$standby_connstr user=rewind_user";
+
 	# Stop the master and be ready to perform the rewind
 	$node_master->stop;

Magnus Hagander

magnus@hagander.net

almost 7 years ago

In reply to: Michael Paquier (#1)

Re: Switch TAP tests of pg_rewind to use role with only function permissions

On Thu, Apr 11, 2019 at 6:13 AM Michael Paquier <michael@paquier.xyz> wrote:

Hi all,

Recent commit bfc80683 has added some documentation in pg_rewind about
the fact that it is possible to do the operation with a non-superuser,
assuming that this role has sufficient grant rights to execute the
functions used by pg_rewind.

Peter Eisentraut has suggested to have some tests for this kind of
user here:

/messages/by-id/e1570ba6-4459-d9b2-1321-9449adaaef4c@2ndquadrant.com

Attached is a patch which switches all the TAP tests of pg_rewind to
do that. As of now, the tests depend on a superuser for everything,
and it seems to me that it makes little sense to make the tests more
pluggable by being able to switch the roles used on-the-fly (the
invocation of pg_rewind is stuck into RewindTest.pm) as a superuser
has no restrictions.

Any thoughts?

+1.

I definitely think having tests for this is good, otherwise we'll just end
up making a change at some point that then suddenly breaks it and we won't
notice.

If we haven't already (and knowing you it wouldn't surprise me if you had
:P), we should probably look through the rest of the tests to see if we
have other similar cases. In general I think any case where "can be run by
non-superuser with specific permissions or a superuser" is the case, we
should be testing it with the "non-superuser with permissions". Because,
well, superusers will never have permission problems (and they will both
test the functionality).

I do think it's perfectly reasonable to have that hardcoded in the
RewindTest.pm module. It doesn't have to be pluggable.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

Michael Paquier

michael@paquier.xyz

almost 7 years ago

In reply to: Magnus Hagander (#2)

Re: Switch TAP tests of pg_rewind to use role with only function permissions

On Thu, Apr 11, 2019 at 09:40:36AM +0200, Magnus Hagander wrote:

If we haven't already (and knowing you it wouldn't surprise me if you had
:P), we should probably look through the rest of the tests to see if we
have other similar cases. In general I think any case where "can be run by
non-superuser with specific permissions or a superuser" is the case, we
should be testing it with the "non-superuser with permissions". Because,
well, superusers will never have permission problems (and they will both
test the functionality).

I am ready to bet that we have other problems lying around.

I do think it's perfectly reasonable to have that hardcoded in the
RewindTest.pm module. It doesn't have to be pluggable.

Thanks, I have committed the patch to do so (d4e2a84), after rewording
a bit the comments. And particularly thanks to Peter to mention that
having more tests with such properties would be nicer.
--
Michael