Bug 21336: Do not increase login_attempts after locking

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 <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

C4/Auth.pm

@@ -1851,7 +1851,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 });
         }
     }

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";
+}

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 => 20;
+use Test::More tests => 21;
 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;
 
@@ -315,3 +316,33 @@ my $hash2 = hash_password('password');
 
 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;
+
+    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;

t/db_dependent/Koha/Patrons.t

@@ -1121,7 +1121,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 ) {
@@ -1136,6 +1136,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;
 };