From 06a220f671b31265487f9b05c68adf59ce7b3df6 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 1 Oct 2018 14:46:15 +0200 Subject: [PATCH] Bug 21336: Do not increase login_attempts after locking Content-Type: text/plain; charset=utf-8 If an account has been locked, there is no use to keep increasing this number. It is not true too; after the pref number has been reached, we can not really speak of login attempts anymore. The credentials are just ignored. Adding a dbrev to put existing values in line. And a simple test in Auth.t to confirm that login_attempts stop increasing. Note: It feels safe to keep the '>=' condition in account_locked. But it could obviously be changed to '=='. (Added a test for that.) Note: Adding a mock_preference in Auth.t too for GDPR_Policy. Since not all tests will pass when the pref is enabled (though disabled by default). Test plan: Run dbrev with updatedatabase.pl. Run t/db_dependent/Koha/Patrons.t Run t/db_dependent/Auth.t Signed-off-by: Marcel de Rooy --- C4/Auth.pm | 2 +- installer/data/mysql/atomicupdate/bug_21336c.perl | 6 ++++ t/db_dependent/Auth.t | 34 ++++++++++++++++++++++- t/db_dependent/Koha/Patrons.t | 4 ++- 4 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_21336c.perl diff --git a/C4/Auth.pm b/C4/Auth.pm index 7d330b5..5a86adb 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1842,7 +1842,7 @@ sub checkpw { if( $patron ) { if ( $passwd_ok ) { $patron->update({ login_attempts => 0 }); - } else { + } elsif( !$patron->account_locked ) { $patron->update({ login_attempts => $patron->login_attempts + 1 }); } } diff --git a/installer/data/mysql/atomicupdate/bug_21336c.perl b/installer/data/mysql/atomicupdate/bug_21336c.perl new file mode 100644 index 0000000..338f8f8 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_21336c.perl @@ -0,0 +1,6 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + $dbh->do( "UPDATE borrowers SET login_attempts = ? WHERE login_attempts > ?", undef, C4::Context->preference('FailedLoginAttempts'), C4::Context->preference('FailedLoginAttempts') ); + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 21336 - Reset login_attempts)\n"; +} diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index 1610b80..6679809 100644 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -10,7 +10,7 @@ use CGI qw ( -utf8 ); use Test::MockObject; use Test::MockModule; use List::MoreUtils qw/all any none/; -use Test::More tests => 22; +use Test::More tests => 23; use Test::Warn; use t::lib::Mocks; use t::lib::TestBuilder; @@ -32,6 +32,7 @@ my $dbh = C4::Context->dbh; # FIXME: SessionStorage defaults to mysql, but it seems to break transaction # handling t::lib::Mocks::mock_preference( 'SessionStorage', 'tmp' ); +t::lib::Mocks::mock_preference( 'GDPR_Policy', '' ); # Disabled $schema->storage->txn_begin; @@ -285,3 +286,34 @@ ok( ( any { 'OPACBaseURL' eq $_ } keys %{$template2->{VARS}} ), ok(C4::Auth::checkpw_hash('password', $hash1), 'password validates with first hash'); ok(C4::Auth::checkpw_hash('password', $hash2), 'password validates with second hash'); + +subtest 'Check value of login_attempts in checkpw' => sub { + plan tests => 6; + + t::lib::Mocks::mock_preference('FailedLoginAttempts', 3); + + # Only interested here in regular login + $C4::Auth::cas = 0; + $C4::Auth::ldap = 0; + $C4::Auth::shib = 0; + + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); + $patron->login_attempts(2); + $patron->password('123')->store; # yes, deliberately not hashed + + is( $patron->account_locked, 0, 'Patron not locked' ); + my @test = checkpw( $dbh, $patron->userid, '123', undef, 'opac', 1 ); + # Note: 123 will not be hashed to 123 ! + is( $test[0], 0, 'checkpw should have failed' ); + $patron->discard_changes; # refresh + is( $patron->login_attempts, 3, 'Login attempts increased' ); + is( $patron->account_locked, 1, 'Check locked status' ); + + # And another try to go over the limit: different return value! + @test = checkpw( $dbh, $patron->userid, '123', undef, 'opac', 1 ); + is( @test, 0, 'checkpw failed again and returns nothing now' ); + $patron->discard_changes; # refresh + is( $patron->login_attempts, 3, 'Login attempts not increased anymore' ); +}; + +$schema->storage->txn_rollback; diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index ea8c17e..57c690f 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1155,7 +1155,7 @@ subtest 'libraries_where_can_see_patrons + can_see_patron_infos + search_limited }; subtest 'account_locked' => sub { - plan tests => 8; + plan tests => 9; my $patron = $builder->build({ source => 'Borrower', value => { login_attempts => 0 } }); $patron = Koha::Patrons->find( $patron->{borrowernumber} ); for my $value ( undef, '', 0 ) { @@ -1170,6 +1170,8 @@ subtest 'account_locked' => sub { is( $patron->account_locked, 0, 'Patron has 2 failed attempts, account should not be considered locked yet' ); $patron->login_attempts(3)->store; is( $patron->account_locked, 1, 'Patron has 3 failed attempts, account should be considered locked yet' ); + $patron->login_attempts(4)->store; + is( $patron->account_locked, 1, 'Patron could not have 4 failed attempts, but account should still be considered locked' ); $patron->delete; }; -- 2.1.4