Attachment 82056 Details for Bug 21336 – Bug 21336: Introduce administrative lockout

[patch] Bug 21336: Introduce administrative lockout

Bug-21336-Introduce-administrative-lockout.patch (text/plain), 3.59 KB, created by Marcel de Rooy on 2018-11-08 07:56:45 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2018-11-08 07:56:45 UTC

Size: 3.59 KB

patch

obsolete

>From 966a150dc9a82e0051bb97bcd46ce87fe7ef00da Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Wed, 3 Oct 2018 15:36:46 +0200
>Subject: [PATCH] Bug 21336: Introduce administrative lockout
>Content-Type: text/plain; charset=utf-8
>
>As a preparation for Koha::Patron->lock, we add the concept of administrative
>lockout. The account is locked just as it would have been by too much
>failed login attempts.
>This is handled by a negative value in borrowers.login_attempts.
>
>Test plan:
>Run t/db_dependent/Auth.t
>---
> Koha/Patron.pm        | 14 ++++++++++----
> t/db_dependent/Auth.t | 18 +++++++++++++++++-
> 2 files changed, 27 insertions(+), 5 deletions(-)
>
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index 5831034..58d5e25 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -1122,18 +1122,24 @@ sub get_enrollable_clubs {
> 
> my $is_locked = $patron->account_locked
> 
>-Return true if the patron has reach the maximum number of login attempts (see pref FailedLoginAttempts).
>+Return true if the patron has reached the maximum number of login attempts
>+(see pref FailedLoginAttempts). If login_attempts is < 0, this is interpreted
>+as an administrative lockout (independent of FailedLoginAttempts; see also
>+Koha::Patron->lock).
> Otherwise return false.
>-If the pref is not set (empty string, null or 0), the feature is considered as disabled.
>+If the pref is not set (empty string, null or 0), the feature is considered as
>+disabled.
> 
> =cut
> 
> sub account_locked {
>     my ($self) = @_;
>     my $FailedLoginAttempts = C4::Context->preference('FailedLoginAttempts');
>-    return ( $FailedLoginAttempts
>+    return 1 if $FailedLoginAttempts
>           and $self->login_attempts
>-          and $self->login_attempts >= $FailedLoginAttempts )? 1 : 0;
>+          and $self->login_attempts >= $FailedLoginAttempts;
>+    return 1 if ($self->login_attempts || 0) < 0; # administrative lockout
>+    return 0;
> }
> 
> =head3 can_see_patron_infos
>diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t
>index 090ea45..3cf4651 100644
>--- a/t/db_dependent/Auth.t
>+++ b/t/db_dependent/Auth.t
>@@ -288,7 +288,7 @@ ok(C4::Auth::checkpw_hash('password', $hash1), 'password validates with first ha
> ok(C4::Auth::checkpw_hash('password', $hash2), 'password validates with second hash');
> 
> subtest 'Check value of login_attempts in checkpw' => sub {
>-    plan tests => 6;
>+    plan tests => 11;
> 
>     t::lib::Mocks::mock_preference('FailedLoginAttempts', 3);
> 
>@@ -313,6 +313,22 @@ subtest 'Check value of login_attempts in checkpw' => sub {
>     is( @test, 0, 'checkpw failed again and returns nothing now' );
>     $patron->discard_changes; # refresh
>     is( $patron->login_attempts, 3, 'Login attempts not increased anymore' );
>+
>+    # Administrative lockout cannot be undone?
>+    # Pass the right password now (or: add a nice mock).
>+    my $auth = Test::MockModule->new( 'C4::Auth' );
>+    $auth->mock( 'checkpw_hash', sub { return 1; } ); # not for production :)
>+    $patron->login_attempts(0)->store;
>+    @test = checkpw( $dbh, $patron->userid, '123', undef, 'opac', 1 );
>+    is( $test[0], 1, 'Build confidence in the mock' );
>+    $patron->login_attempts(-1)->store;
>+    is( $patron->account_locked, 1, 'Check administrative lockout' );
>+    @test = checkpw( $dbh, $patron->userid, '123', undef, 'opac', 1 );
>+    is( @test, 0, 'checkpw gave red' );
>+    $patron->discard_changes; # refresh
>+    is( $patron->login_attempts, -1, 'Still locked out' );
>+    t::lib::Mocks::mock_preference('FailedLoginAttempts', ''); # disable
>+    is( $patron->account_locked, 1, 'Check administrative lockout without pref' );
> };
> 
> $schema->storage->txn_rollback;
>-- 
>2.1.4

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