Bug 18314 - Account lockout
Summary: Account lockout
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18174
Blocks: 19003 19344
  Show dependency treegraph
 
Reported: 2017-03-22 00:58 UTC by Jonathan Druart
Modified: 2017-10-01 21:25 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts (5.25 KB, patch)
2017-03-24 18:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: DBIC schema changes (2.67 KB, patch)
2017-03-24 18:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Account lockout (9.78 KB, patch)
2017-03-24 18:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Resetting the password removes the lock (1.08 KB, patch)
2017-03-24 18:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Add tests (1.91 KB, patch)
2017-03-24 18:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts (5.32 KB, patch)
2017-04-20 11:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18314: DBIC schema changes (2.73 KB, patch)
2017-04-20 11:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18314: Account lockout (9.85 KB, patch)
2017-04-20 11:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18314: Resetting the password removes the lock (1.14 KB, patch)
2017-04-20 11:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18314: Add tests (1.97 KB, patch)
2017-04-20 11:25 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts (5.32 KB, patch)
2017-04-28 20:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Account lockout (9.85 KB, patch)
2017-04-28 20:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Resetting the password removes the lock (1.14 KB, patch)
2017-04-28 20:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Add tests (1.97 KB, patch)
2017-04-28 20:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: DBIC schema changes (2.67 KB, patch)
2017-04-28 20:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Fix reset number of login attempts on login success (981 bytes, patch)
2017-04-28 20:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Add link to 'reset your password' from staff (1.12 KB, patch)
2017-04-28 20:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts (5.37 KB, patch)
2017-04-28 20:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18314: Account lockout (9.91 KB, patch)
2017-04-28 20:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18314: Resetting the password removes the lock (1.20 KB, patch)
2017-04-28 20:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18314: Add tests (2.02 KB, patch)
2017-04-28 20:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18314: DBIC schema changes (2.72 KB, patch)
2017-04-28 20:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18314: Fix reset number of login attempts on login success (1.01 KB, patch)
2017-04-28 20:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18314: Add link to 'reset your password' from staff (1.17 KB, patch)
2017-04-28 20:50 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18314 (QA Followup) Use OpacBaseURL for password reset link (1.22 KB, patch)
2017-04-28 20:50 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-03-22 00:58:40 UTC
To prevent brute force attacks on Koha accounts, staff and opac, we need to implement an account lockout process to Koha.

After a number of failed login attempts a users account would become locked. 
The user would then need to use the reset password functionality to send a reset token to their email account. After a successful password reset the lockout flag would be removed.

The number of failed login attempts before lockout will be configurable using a system preference.
Comment 1 Francesco Rivetti 2017-03-22 15:00:44 UTC
would be better to have a CAPTCHA after N login attempt/minute from a class C?

i'm concerned about "horizontal" brute force attacks, like checking password "1234" against random usernames
Comment 2 Jonathan Druart 2017-03-22 15:08:44 UTC
(In reply to Francesco Rivetti from comment #1)
> would be better to have a CAPTCHA after N login attempt/minute from a class
> C?

That will not be parted of this enhancement.

> i'm concerned about "horizontal" brute force attacks, like checking password
> "1234" against random usernames

See also bug 18298.
Comment 3 Marcel de Rooy 2017-03-22 15:59:42 UTC
Probably an Architecture patch instead of Patrons?
Comment 4 Jonathan Druart 2017-03-24 18:44:36 UTC
Created attachment 61630 [details] [review]
Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts

And of course deletedborrowers.login_attempts
Comment 5 Jonathan Druart 2017-03-24 18:44:40 UTC
Created attachment 61631 [details] [review]
Bug 18314: DBIC schema changes
Comment 6 Jonathan Druart 2017-03-24 18:44:44 UTC
Created attachment 61632 [details] [review]
Bug 18314: Account lockout

To prevent brute force attacks on Koha accounts, staff and opac, we need to
implement an account lockout process to Koha.

After a number of failed login attempts a users account would become locked.
The user would then need to use the reset password functionality to send a reset
token to their email account. After a successful password reset the lockout flag
would be removed.

The number of failed login attempts before lockout is configurable using a new
system preference 'FailedLoginAttempts'.

How does it work?
When a patron enter an invalid password, the borrowers.login_attempts value
for this patron is incremented. When this value reach the value of the
pref FailedLoginAttempts, the password comparison is not done and the
authentication is rejected.
This login_attempts field is reset when a patron correctly logs in. When
the account is locked the patron has to reset his/her password using
the OpacResetPassword feature or ask a staff member to generate a new
password.
If the pref is not set (0, or '') the feature is considered as disabled,
but the failed login attempts are stored anyway.

Test plan:
0/ Apply patch and execute the update DB entry
1/ Switch on the feature by setting FailedLoginAttempts to 3
2/ Use an invalid password to login at the staff or OPAC interface
3/ After the third consecutive failures, you will be asked to reset your
password if OpacResetPassword is set, or contact a staff member
4/ Switch on OpacResetPassword and reset your password
5/ Confirm that you are able to login
6/ Play with the different combinations

QA details: The trick happens in C4::Auth::checkpw, to make things clear
I had to create a return value (note the awesome name: @return) and
replace the 3 successives if statements with elsif. Indeed if one of
the condition is reached, it will return inside the given block.
Comment 7 Jonathan Druart 2017-03-24 18:44:47 UTC
Created attachment 61633 [details] [review]
Bug 18314: Resetting the password removes the lock

When a password is changed (updated by a staff member or using the
"recover password via email" feature, the counter/flag needs to be
reset.
Comment 8 Jonathan Druart 2017-03-24 18:44:51 UTC
Created attachment 61634 [details] [review]
Bug 18314: Add tests
Comment 9 David Cook 2017-03-26 23:01:22 UTC
So happy to see this. I have a number of other tasks to do, but if I can find the time, I'll test this.
Comment 10 Aleisha Amohia 2017-03-28 03:23:46 UTC
Hi, patch and db update apply cleanly.

I see no change in behaviour in staff client nor opac side. I set the FailedLoginAttempts to 3, but I was able to attempt logging in with the wrong password unlimited times.

Also, tests fail:

# Subtest: update_password
    1..7
    ok 1 - Koha::Patron->update_password should warn if the userid is already used by another patron
    ok 2 - Koha::Patron->update_password should not have updated the userid
    ok 3 - Koha::Patron->update_password should not have updated the userid
    # Looks like you planned 7 tests but ran 3.
not ok 8 - update_password

#   Failed test 'update_password'
#   at t/db_dependent/Koha/Patrons.t line 183.
The method update is not covered by tests!# Looks like your test exited with 255 just after 8.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 15/22 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Patrons.t (Wstat: 65280 Tests: 8 Failed: 1)
  Failed test:  8
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 22 tests but ran 8.
Files=1, Tests=8,  3 wallclock secs ( 0.03 usr  0.01 sys +  1.28 cusr  0.10 csys =  1.42 CPU)
Result: FAIL
Comment 11 Jonathan Druart 2017-03-28 12:00:49 UTC
Aleisha, tests pass and everything works for me. Could you double check please? (restart plack and memcached, check dependency, execute updateDB).
Comment 12 sandboxes@biblibre.com 2017-04-20 11:24:10 UTC
Patch tested with a sandbox, by Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 13 sandboxes@biblibre.com 2017-04-20 11:25:05 UTC
Created attachment 62453 [details] [review]
Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts

And of course deletedborrowers.login_attempts

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 14 sandboxes@biblibre.com 2017-04-20 11:25:09 UTC
Created attachment 62454 [details] [review]
Bug 18314: DBIC schema changes

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 15 sandboxes@biblibre.com 2017-04-20 11:25:13 UTC
Created attachment 62455 [details] [review]
Bug 18314: Account lockout

To prevent brute force attacks on Koha accounts, staff and opac, we need to
implement an account lockout process to Koha.

After a number of failed login attempts a users account would become locked.
The user would then need to use the reset password functionality to send a reset
token to their email account. After a successful password reset the lockout flag
would be removed.

The number of failed login attempts before lockout is configurable using a new
system preference 'FailedLoginAttempts'.

How does it work?
When a patron enter an invalid password, the borrowers.login_attempts value
for this patron is incremented. When this value reach the value of the
pref FailedLoginAttempts, the password comparison is not done and the
authentication is rejected.
This login_attempts field is reset when a patron correctly logs in. When
the account is locked the patron has to reset his/her password using
the OpacResetPassword feature or ask a staff member to generate a new
password.
If the pref is not set (0, or '') the feature is considered as disabled,
but the failed login attempts are stored anyway.

Test plan:
0/ Apply patch and execute the update DB entry
1/ Switch on the feature by setting FailedLoginAttempts to 3
2/ Use an invalid password to login at the staff or OPAC interface
3/ After the third consecutive failures, you will be asked to reset your
password if OpacResetPassword is set, or contact a staff member
4/ Switch on OpacResetPassword and reset your password
5/ Confirm that you are able to login
6/ Play with the different combinations

QA details: The trick happens in C4::Auth::checkpw, to make things clear
I had to create a return value (note the awesome name: @return) and
replace the 3 successives if statements with elsif. Indeed if one of
the condition is reached, it will return inside the given block.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 16 sandboxes@biblibre.com 2017-04-20 11:25:16 UTC
Created attachment 62456 [details] [review]
Bug 18314: Resetting the password removes the lock

When a password is changed (updated by a staff member or using the
"recover password via email" feature, the counter/flag needs to be
reset.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 17 sandboxes@biblibre.com 2017-04-20 11:25:20 UTC
Created attachment 62457 [details] [review]
Bug 18314: Add tests

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 18 Nick Clemens 2017-04-28 19:17:32 UTC
I hit a problem here:
1 - Staff/opac fail a logon
2 - Check db login_attempts = 1
3 - Succeed a login
4 - Check db login_attempts = 1

It should reset to zero, but instead accumulates until password reset.

I think it would also be good to add a link to the staff side if password reset enabled to point staff member to opac
Comment 19 Jonathan Druart 2017-04-28 20:16:41 UTC
Created attachment 62872 [details] [review]
Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts

And of course deletedborrowers.login_attempts

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 20 Jonathan Druart 2017-04-28 20:16:47 UTC
Created attachment 62873 [details] [review]
Bug 18314: Account lockout

To prevent brute force attacks on Koha accounts, staff and opac, we need to
implement an account lockout process to Koha.

After a number of failed login attempts a users account would become locked.
The user would then need to use the reset password functionality to send a reset
token to their email account. After a successful password reset the lockout flag
would be removed.

The number of failed login attempts before lockout is configurable using a new
system preference 'FailedLoginAttempts'.

How does it work?
When a patron enter an invalid password, the borrowers.login_attempts value
for this patron is incremented. When this value reach the value of the
pref FailedLoginAttempts, the password comparison is not done and the
authentication is rejected.
This login_attempts field is reset when a patron correctly logs in. When
the account is locked the patron has to reset his/her password using
the OpacResetPassword feature or ask a staff member to generate a new
password.
If the pref is not set (0, or '') the feature is considered as disabled,
but the failed login attempts are stored anyway.

Test plan:
0/ Apply patch and execute the update DB entry
1/ Switch on the feature by setting FailedLoginAttempts to 3
2/ Use an invalid password to login at the staff or OPAC interface
3/ After the third consecutive failures, you will be asked to reset your
password if OpacResetPassword is set, or contact a staff member
4/ Switch on OpacResetPassword and reset your password
5/ Confirm that you are able to login
6/ Play with the different combinations

QA details: The trick happens in C4::Auth::checkpw, to make things clear
I had to create a return value (note the awesome name: @return) and
replace the 3 successives if statements with elsif. Indeed if one of
the condition is reached, it will return inside the given block.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 21 Jonathan Druart 2017-04-28 20:16:52 UTC
Created attachment 62874 [details] [review]
Bug 18314: Resetting the password removes the lock

When a password is changed (updated by a staff member or using the
"recover password via email" feature, the counter/flag needs to be
reset.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 22 Jonathan Druart 2017-04-28 20:16:57 UTC
Created attachment 62875 [details] [review]
Bug 18314: Add tests

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 23 Jonathan Druart 2017-04-28 20:17:02 UTC
Created attachment 62876 [details] [review]
Bug 18314: DBIC schema changes
Comment 24 Jonathan Druart 2017-04-28 20:17:08 UTC
Created attachment 62877 [details] [review]
Bug 18314: Fix reset number of login attempts on login success
Comment 25 Jonathan Druart 2017-04-28 20:17:13 UTC
Created attachment 62878 [details] [review]
Bug 18314: Add link to 'reset your password' from staff
Comment 26 Jonathan Druart 2017-04-28 20:17:55 UTC
(In reply to Nick Clemens from comment #18)
> I hit a problem here:
> 1 - Staff/opac fail a logon
> 2 - Check db login_attempts = 1
> 3 - Succeed a login
> 4 - Check db login_attempts = 1
> 
> It should reset to zero, but instead accumulates until password reset.

Indeed that did not work! Sorry about that, I was sure I tested it.

> I think it would also be good to add a link to the staff side if password
> reset enabled to point staff member to opac

Done!
Comment 27 Nick Clemens 2017-04-28 20:49:46 UTC
Created attachment 62884 [details] [review]
Bug 18314: Add pref FailedLoginAttempts and columns borrowers.login_attempts

And of course deletedborrowers.login_attempts

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens 2017-04-28 20:49:50 UTC
Created attachment 62885 [details] [review]
Bug 18314: Account lockout

To prevent brute force attacks on Koha accounts, staff and opac, we need to
implement an account lockout process to Koha.

After a number of failed login attempts a users account would become locked.
The user would then need to use the reset password functionality to send a reset
token to their email account. After a successful password reset the lockout flag
would be removed.

The number of failed login attempts before lockout is configurable using a new
system preference 'FailedLoginAttempts'.

How does it work?
When a patron enter an invalid password, the borrowers.login_attempts value
for this patron is incremented. When this value reach the value of the
pref FailedLoginAttempts, the password comparison is not done and the
authentication is rejected.
This login_attempts field is reset when a patron correctly logs in. When
the account is locked the patron has to reset his/her password using
the OpacResetPassword feature or ask a staff member to generate a new
password.
If the pref is not set (0, or '') the feature is considered as disabled,
but the failed login attempts are stored anyway.

Test plan:
0/ Apply patch and execute the update DB entry
1/ Switch on the feature by setting FailedLoginAttempts to 3
2/ Use an invalid password to login at the staff or OPAC interface
3/ After the third consecutive failures, you will be asked to reset your
password if OpacResetPassword is set, or contact a staff member
4/ Switch on OpacResetPassword and reset your password
5/ Confirm that you are able to login
6/ Play with the different combinations

QA details: The trick happens in C4::Auth::checkpw, to make things clear
I had to create a return value (note the awesome name: @return) and
replace the 3 successives if statements with elsif. Indeed if one of
the condition is reached, it will return inside the given block.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens 2017-04-28 20:49:54 UTC
Created attachment 62886 [details] [review]
Bug 18314: Resetting the password removes the lock

When a password is changed (updated by a staff member or using the
"recover password via email" feature, the counter/flag needs to be
reset.

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Nick Clemens 2017-04-28 20:49:58 UTC
Created attachment 62887 [details] [review]
Bug 18314: Add tests

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Nick Clemens 2017-04-28 20:50:03 UTC
Created attachment 62888 [details] [review]
Bug 18314: DBIC schema changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 32 Nick Clemens 2017-04-28 20:50:08 UTC
Created attachment 62889 [details] [review]
Bug 18314: Fix reset number of login attempts on login success

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Nick Clemens 2017-04-28 20:50:12 UTC
Created attachment 62890 [details] [review]
Bug 18314: Add link to 'reset your password' from staff

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 34 Nick Clemens 2017-04-28 20:50:19 UTC
Created attachment 62891 [details] [review]
Bug 18314 (QA Followup) Use OpacBaseURL for password reset link
Comment 35 Kyle M Hall 2017-05-12 15:00:04 UTC
Pushed to master for 17.05, thanks Jonathan, Nick!
Comment 36 Katrin Fischer 2017-05-14 10:29:26 UTC
Like!

This won't get ported back to 16.11.x as it is an enhancement.
Comment 37 Marcel de Rooy 2017-07-29 15:06:19 UTC
This report causes t/db/SIP/Message.t to fail (quite often) since testbuilder fills login_attempts with a random number. Depends on a pref too btw.