Add TAP test for auth_delay extension
diff --git a/contrib/auth_delay/Makefile b/contrib/auth_delay/Makefile
index 4b86ec37f0..b65097789a 100644
--- a/contrib/auth_delay/Makefile
+++ b/contrib/auth_delay/Makefile
@@ -3,6 +3,8 @@
MODULES = auth_delay
PGFILEDESC = "auth_delay - delay authentication failure reports"+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/auth_delay/t/001_auth_delay.pl b/contrib/auth_delay/t/001_auth_delay.pl
new file mode 100644
index 0000000000..644026e4f2
--- /dev/null
+++ b/contrib/auth_delay/t/001_auth_delay.pl
@@ -0,0 +1,87 @@
+
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(gettimeofday tv_interval);
+
+# Delete pg_hba.conf from the given node, add a new entry to it
+# and then execute a reload to refresh it.
+sub reset_pg_hba
+{
+ my $node = shift;
+ my $hba_method = shift;
+
+ unlink($node->data_dir . '/pg_hba.conf');
+ # just for testing purposes, use a continuation line
+ $node->append_conf('pg_hba.conf', "local all all $hba_method");
+ $node->reload;
+ return;
+}
+
+# Test access for a single role, with password
+sub test_login
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+ my $node = shift;
+ my $role = shift;
+ my $password = shift;
+ my $expected_res = shift;
+ my $status_string = 'failed';
+
+ $status_string = 'success' if ($expected_res eq 0);
+
+ my $connstr = "user=$role";
+ my $testname =
+ "authentication $status_string for role $role with password $password";
+
+ $ENV{"PGPASSWORD"} = $password;
+
+ if ($expected_res eq 0)
+ {
+ $node->connect_ok($connstr, $testname);
+ }
+ else
+ {
+ # No checks of the error message, only the status code.
+ $node->connect_fails($connstr, $testname);
+ }
+}
+
+note "setting up PostgreSQL instance";
+
+my $delay_milliseconds = 500;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+ qq{shared_preload_libraries = 'auth_delay'
+ auth_delay.milliseconds = '$delay_milliseconds'});
+$node->start;
+
+$node->safe_psql(
+ 'postgres',
+ "CREATE ROLE user_role LOGIN PASSWORD 'pass';");
+reset_pg_hba($node, 'password');
+
+note "running tests";
+
+# check enter wrong password
+my $t0 = [gettimeofday];
+test_login($node, 'user_role', "wrongpass", 2);
+my $elapsed = tv_interval($t0, [gettimeofday]);
+ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
+
+# check enter correct password
+my $t0 = [gettimeofday];
+test_login($node, 'user_role', "pass", 0);
+my $elapsed = tv_interval($t0, [gettimeofday]);
+ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
+
+done_testing();
+Hi Hackers,
I just wrote a test for `auth_delay` extension.
It's a test which confirms whether there is a delay for a second when
you enter the wrong password.
I sent an email using mutt, but I have a problem and sent it again.
---
Regards,
Dong Wook Lee.
2022년 6월 7일 (화) 오후 6:32, Dong Wook Lee <sh95119@gmail.com>님이 작성:
Hi Hackers,
I just wrote a test for `auth_delay` extension.
It's a test which confirms whether there is a delay for a second when
you enter the wrong password.
I sent an email using mutt, but I have a problem and sent it again.---
Regards,
Dong Wook Lee.
Hi,
I have written a test for the auth_delay extension before,
but if it is okay, can you review it?
---
Regards,
DongWook Lee.
On Sat, Jun 18, 2022 at 11:06:02AM +0900, Dong Wook Lee wrote:
I have written a test for the auth_delay extension before,
but if it is okay, can you review it?
+# check enter wrong password
+my $t0 = [gettimeofday];
+test_login($node, 'user_role', "wrongpass", 2);
+my $elapsed = tv_interval($t0, [gettimeofday]);
+ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
+
+# check enter correct password
+my $t0 = [gettimeofday];
+test_login($node, 'user_role', "pass", 0);
+my $elapsed = tv_interval($t0, [gettimeofday]);
+ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds");On a slow machine, I suspect that the second test is going to be
unstable as it would fail if the login attempt (that succeeds) takes
more than $delay_milliseconds. You could increase more
delay_milliseconds to leverage that, but it would make the first test
slower for nothing on faster machines in the case where the
authentication attempt has failed. I guess that you could leverage
that by using a large value for delay_milliseconds in the second test,
because we are never going to wait. For the first test, you could on
the contrary use a much lower value, still on slow machines it may not
test what the code path of auth_delay you are willing to test.
As a whole, I am not sure that this is really worth spending cycles on
when running check-world or similar, and the code of the extension is
trivial.
--
Michael
On 22/06/18 12:07오후, Michael Paquier wrote:
On Sat, Jun 18, 2022 at 11:06:02AM +0900, Dong Wook Lee wrote:
I have written a test for the auth_delay extension before,
but if it is okay, can you review it?+# check enter wrong password +my $t0 = [gettimeofday]; +test_login($node, 'user_role', "wrongpass", 2); +my $elapsed = tv_interval($t0, [gettimeofday]); +ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds"); + +# check enter correct password +my $t0 = [gettimeofday]; +test_login($node, 'user_role', "pass", 0); +my $elapsed = tv_interval($t0, [gettimeofday]); +ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds");On a slow machine, I suspect that the second test is going to be
unstable as it would fail if the login attempt (that succeeds) takes
more than $delay_milliseconds. You could increase more
delay_milliseconds to leverage that, but it would make the first test
slower for nothing on faster machines in the case where the
authentication attempt has failed. I guess that you could leverage
that by using a large value for delay_milliseconds in the second test,
because we are never going to wait. For the first test, you could on
the contrary use a much lower value, still on slow machines it may not
test what the code path of auth_delay you are willing to test.
Thank you for your valuable advice I didn't think about the slow system.
Therefore, in the case of the second test, the time was extended a little.
As a whole, I am not sure that this is really worth spending cycles on
when running check-world or similar, and the code of the extension is
trivial.
Even though it is trivial, I think it would be better if there was a test.
Show quoted text
--
Michael
Attachments:
0002_add_test_auth_delay.patchtext/x-diff; charset=us-asciiDownload
diff --git a/contrib/auth_delay/Makefile b/contrib/auth_delay/Makefile
index 4b86ec37f0..b65097789a 100644
--- a/contrib/auth_delay/Makefile
+++ b/contrib/auth_delay/Makefile
@@ -3,6 +3,8 @@
MODULES = auth_delay
PGFILEDESC = "auth_delay - delay authentication failure reports"
+TAP_TESTS = 1
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/auth_delay/t/001_auth_delay.pl b/contrib/auth_delay/t/001_auth_delay.pl
new file mode 100644
index 0000000000..644026e4f2
--- /dev/null
+++ b/contrib/auth_delay/t/001_auth_delay.pl
@@ -0,0 +1,87 @@
+
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(gettimeofday tv_interval);
+
+# Delete pg_hba.conf from the given node, add a new entry to it
+# and then execute a reload to refresh it.
+sub reset_pg_hba
+{
+ my $node = shift;
+ my $hba_method = shift;
+
+ unlink($node->data_dir . '/pg_hba.conf');
+ # just for testing purposes, use a continuation line
+ $node->append_conf('pg_hba.conf', "local all all $hba_method");
+ $node->reload;
+ return;
+}
+
+# Test access for a single role, with password
+sub test_login
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+ my $node = shift;
+ my $role = shift;
+ my $password = shift;
+ my $expected_res = shift;
+ my $status_string = 'failed';
+
+ $status_string = 'success' if ($expected_res eq 0);
+
+ my $connstr = "user=$role";
+ my $testname =
+ "authentication $status_string for role $role with password $password";
+
+ $ENV{"PGPASSWORD"} = $password;
+
+ if ($expected_res eq 0)
+ {
+ $node->connect_ok($connstr, $testname);
+ }
+ else
+ {
+ # No checks of the error message, only the status code.
+ $node->connect_fails($connstr, $testname);
+ }
+}
+
+note "setting up PostgreSQL instance";
+
+my $delay_milliseconds = 500;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+ qq{shared_preload_libraries = 'auth_delay'
+ auth_delay.milliseconds = '$delay_milliseconds'});
+$node->start;
+
+$node->safe_psql(
+ 'postgres',
+ "CREATE ROLE user_role LOGIN PASSWORD 'pass';");
+reset_pg_hba($node, 'password');
+
+note "running tests";
+
+# check enter wrong password
+my $t0 = [gettimeofday];
+test_login($node, 'user_role', "wrongpass", 2);
+my $elapsed = tv_interval($t0, [gettimeofday]);
+ok($elapsed >= $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
+
+# check enter correct password
+my $t0 = [gettimeofday];
+test_login($node, 'user_role', "pass", 0);
+my $elapsed = tv_interval($t0, [gettimeofday]);
+ok($elapsed < $delay_milliseconds / 1000, "auth_delay $elapsed seconds");
+
+done_testing();
+
Dong Wook Lee <sh95119@gmail.com> writes:
On 22/06/18 12:07오후, Michael Paquier wrote:
As a whole, I am not sure that this is really worth spending cycles on
when running check-world or similar, and the code of the extension is
trivial.
Even though it is trivial, I think it would be better if there was a test.
I looked at this and concur with Michael's evaluation. A new TAP module
is quite an expensive thing, since it incurs (at least) an initdb run.
In this case, the need to delay a long time to ensure that the test
doesn't fail on slow systems makes that even worse. I don't think
I want to incur these costs every time I run check-world in order to
test a pg_usleep() call, which is what this module boils down to.
If we had some sort of "attic" of tests that aren't run by either
check-world or most buildfarm members, perhaps this would be worth
putting there. But we don't.
One idea could be to install the test but leave the TAP_TESTS line in
the Makefile commented out. Then, somebody who was actively working on
the module could enable the test easily enough (without even modifying
that file: just do "make check TAP_TESTS=1"), but otherwise we don't
pay for it. However, I'm not sure how well that plan will translate
to the upcoming meson build system.
If we don't do it like that, I'd vote for rejecting the patch.
regards, tom lane