Attachment 87160 Details for Bug 21336 – Bug 21336: Do not increase login_attempts after locking

[patch] Bug 21336: Do not increase login_attempts after locking

Bug-21336-Do-not-increase-loginattempts-after-lock.patch (text/plain), 5.61 KB, created by Marcel de Rooy on 2019-03-29 08:22:47 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2019-03-29 08:22:47 UTC

Size: 5.61 KB

patch

obsolete

>From 84005668a62ffacbea00cc27b58544ff9f93784f Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>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 <m.de.rooy@rijksmuseum.nl>
>---
> C4/Auth.pm                                        |  2 +-
> installer/data/mysql/atomicupdate/bug_21336c.perl |  6 +++++
> t/db_dependent/Auth.t                             | 33 ++++++++++++++++++++++-
> t/db_dependent/Koha/Patrons.t                     |  4 ++-
> 4 files changed, 42 insertions(+), 3 deletions(-)
> create mode 100644 installer/data/mysql/atomicupdate/bug_21336c.perl
>
>diff --git a/C4/Auth.pm b/C4/Auth.pm
>index c3ba5baad2..dc9c96a768 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -1845,7 +1845,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 0000000000..338f8f8fc5
>--- /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 e0b6036f41..490e4a7984 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 => 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;
>diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
>index 5c2e4fdf05..51f62db415 100644
>--- a/t/db_dependent/Koha/Patrons.t
>+++ b/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;
> };
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 21336: 79165 | 79166 | 79167 | 79168 | 79169 | 79170 | 80201 | 80202 | 80203 | 80204 | 80205 | 80206 | 80207 | 80208 | 80213 | 80214 | 80386 | 80387 | 80403 | 81112 | 81113 | 81114 | 81115 | 81116 | 81117 | 81118 | 81119 | 81270 | 81474 | 81475 | 81476 | 81477 | 81478 | 81479 | 81480 | 81481 | 81482 | 81483 | 82051 | 82052 | 82053 | 82054 | 82055 | 82056 | 82057 | 82058 | 82059 | 83202 | 83203 | 83204 | 83205 | 83206 | 83207 | 83208 | 83209 | 83256 | 87158 | 87159 | 87160 | 87161 | 87162 | 87163 | 87164 | 87165 | 87166 | 87167 | 87168 | 87176 | 87177 | 87178 | 87179 | 87180 | 87181 | 87182 | 87183 | 87184 | 87185 | 87387 | 87388 | 87389 | 87390 | 87391 | 87392 | 87393 | 87394 | 87395 | 87396 | 87397 | 87400 | 87401 | 88198 | 88199 | 88217 | 88218