@@ -, +, @@ --- 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 --- a/C4/Auth.pm +++ a/C4/Auth.pm @@ -1839,7 +1839,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 }); } } --- a/installer/data/mysql/atomicupdate/bug_21336c.perl +++ a/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"; +} --- a/t/db_dependent/Auth.t +++ a/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; --- a/t/db_dependent/Koha/Patrons.t +++ a/t/db_dependent/Koha/Patrons.t @@ -1156,7 +1156,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 ) { @@ -1171,6 +1171,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; }; --