Bug 25947

Summary: Improve locked account message
Product: Koha Reporter: Caroline Cyr La Rose <caroline.cyr-la-rose>
Component: PatronsAssignee: Fridolin Somers <fridolin.somers>
Status: Failed QA --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: amaury.gau, dcook, fridolin.somers, gmcharlt, kyle.m.hall, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26823
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22854
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 23979    
Bug Blocks:    
Attachments: Warning messages in the patron details
Bug 25947: Improve locked account message
Screenshot of feature
Bug 25947: Improve locked account message
Bug 25947: Improve locked account message
Bug 25947: (QA follow-up) Fix filter

Description Caroline Cyr La Rose 2020-07-07 19:46:40 UTC
Created attachment 106643 [details]
Warning messages in the patron details

When using FailedLoginAttempts, a patron's account is blocked after X many failed login attempts on the OPAC. When that happens, a message 'Account has been locked.' appears on the patron account details page.

This message does not indicate why or how it was blocked.

I suggest we change it to 'Account has been locked due to too many failed login attempts'.

Furthermore, the font is dark red whereas the font for other warnings (such as wrong category or unpaid fines) is bright red and bold.

To test:
1. Change FailedLoginAttempts to a small number, like 2
2. Go to a patron's account and copy their username (while you're there, change the password and add some fines, too)
3. In the OPAC, try to log in with the username and a wrong password several times
4. Go back to the patron's account in the staff interface
5. Note that there is no indication as to why the patron's account is locked and that the font color and weight does not match the other warnings.

Also, there is no button to change the password next to the warning. When a patron has unpaid fines or is in the wrong category, there is a button to either pay/writeoff the fines or change the category next to the warning. I would add a button that says 'Change password' next to the locked account warning. This might be outside the scope of this bug, but I think it would improve usability.
Comment 1 Caroline Cyr La Rose 2020-07-07 19:51:30 UTC
Again, this might be outside the scope, but I also think the OPAC message could be improved.

Right now, it says

'You entered an incorrect username or password. Please try again! But note that passwords are case sensitive and that your account will be locked out after a fixed number of failed login attempts. Please contact a library staff member if you continue to have problems.'

I would add the number of failed attempts rather than 'after a fixed number of failed login attempts'

And I would also change the message once the account is blocked. Right now, even if FailedLoginAttempts is at 2, you can try to log in 10 times and it will never tell you that your account is now blocked.
Comment 2 David Cook 2020-07-08 03:14:23 UTC
I don't 100% understand these comments. 

I think that I agree that the Staff Client could be more descriptive about why the account is locked.

As for the OPAC, we shouldn't indicate that the account has been locked, as that indicates to an attacker that the account exists, which is not a good idea from a security perspective.
Comment 3 Fridolin Somers 2023-01-20 01:28:48 UTC
We have the same demand.
I'm on it
Comment 4 Fridolin Somers 2023-01-20 06:53:15 UTC
Created attachment 145502 [details] [review]
Bug 25947: Improve locked account message

When using system preference 'FailedLoginAttempts', a patron's account is blocked after X many failed login attempts on the OPAC.

This patch adds, like for "Patron's card has expired" message :
  * a message "Patron's account has been locked (due to X failed login attempts)" on the patron details and circulation pages
  * a link to change password
  * a link to reset login attempts to 0
Links are only displayed if libarian has permission to edit patrons.

For administrative lock, only a message is displayed :
"Patron's account has been administratively locked"

Reset login attempts to 0 feature uses members/setstatus.pl with
resetloginattempts=y like reregistration=y for an expired patron

To test:
1. Change system preference 'FailedLoginAttempts' to a small number, like 2
2. Go to a patron's account and copy their username (while you're there, change the password and add some fines, too)
3. In the OPAC, try to log in with the username and a wrong password 3 times
4. Go back to the patron's account in the staff interface
5. Note that there is a message : "Patron's account has been locked (due to 3 failed login attempts)"
6. Click on "Change password", you go to change password page
7. Click on "Reset login attempts", page reloads and you see patron in unlocked
Comment 5 Fridolin Somers 2023-01-20 07:09:36 UTC
Created attachment 145503 [details]
Screenshot of feature
Comment 6 Fridolin Somers 2023-01-20 07:12:50 UTC
You may ask to improve the text.

We may add some text explaining the value of syspref FailedLoginAttempts, but maybe message would be too long.

Reset login attempts as been asked by the library, this is a new feature.
I hope it makes sens, otherwise I may create a new bug report.
Comment 7 ByWater Sandboxes 2023-01-23 16:14:47 UTC
Created attachment 145589 [details] [review]
Bug 25947: Improve locked account message

When using system preference 'FailedLoginAttempts', a patron's account is blocked after X many failed login attempts on the OPAC.

This patch adds, like for "Patron's card has expired" message :
  * a message "Patron's account has been locked (due to X failed login attempts)" on the patron details and circulation pages
  * a link to change password
  * a link to reset login attempts to 0
Links are only displayed if libarian has permission to edit patrons.

For administrative lock, only a message is displayed :
"Patron's account has been administratively locked"

Reset login attempts to 0 feature uses members/setstatus.pl with
resetloginattempts=y like reregistration=y for an expired patron

To test:
1. Change system preference 'FailedLoginAttempts' to a small number, like 2
2. Go to a patron's account and copy their username (while you're there, change the password and add some fines, too)
3. In the OPAC, try to log in with the username and a wrong password 3 times
4. Go back to the patron's account in the staff interface
5. Note that there is a message : "Patron's account has been locked (due to 3 failed login attempts)"
6. Click on "Change password", you go to change password page
7. Click on "Reset login attempts", page reloads and you see patron in unlocked

Signed-off-by: Amaury GAU <amaury.gau@bulac.fr>
Comment 8 solene.ngamga 2023-01-31 14:32:59 UTC
Created attachment 145878 [details] [review]
Bug 25947: Improve locked account message

When using system preference 'FailedLoginAttempts', a patron's account is blocked after X many failed login attempts on the OPAC.

This patch adds, like for "Patron's card has expired" message :
  * a message "Patron's account has been locked (due to X failed login attempts)" on the patron details and circulation pages
  * a link to change password
  * a link to reset login attempts to 0
Links are only displayed if libarian has permission to edit patrons.

For administrative lock, only a message is displayed :
"Patron's account has been administratively locked"

Reset login attempts to 0 feature uses members/setstatus.pl with
resetloginattempts=y like reregistration=y for an expired patron

To test:
1. Change system preference 'FailedLoginAttempts' to a small number, like 2
2. Go to a patron's account and copy their username (while you're there, change the password and add some fines, too)
3. In the OPAC, try to log in with the username and a wrong password 3 times
4. Go back to the patron's account in the staff interface
5. Note that there is a message : "Patron's account has been locked (due to 3 failed login attempts)"
6. Click on "Change password", you go to change password page
7. Click on "Reset login attempts", page reloads and you see patron in unlocked

Signed-off-by: Amaury GAU <amaury.gau@bulac.fr>
Signed-off-by: Solene Ngamga <solene.ngamga@inLibro.com>
Comment 9 Marcel de Rooy 2023-03-03 09:19:58 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/includes/patron_messages.inc
   FAIL   filters
                wrong_html_filter at line 57 (                        <a href="/cgi-bin/koha/members/member-password.pl?member=[% patron.borrowernumber | html %]">Change password</a>)
Comment 10 Marcel de Rooy 2023-03-03 09:22:23 UTC
Looking here
Comment 11 Marcel de Rooy 2023-03-03 09:23:32 UTC
(In reply to Fridolin Somers from comment #6)
> Reset login attempts as been asked by the library, this is a new feature.
> I hope it makes sens, otherwise I may create a new bug report.

+my $resetloginattempts = $input->param('resetloginattempts') || '';

 my $dbh = C4::Context->dbh;
 my $dateexpiry;
@@ -54,6 +55,8 @@ if ( $logged_in_user->can_see_patron_infos($patron) ) {
     if ( $reregistration eq 'y' ) {
         # re-reregistration function to automatic calcul of date expiry
         $dateexpiry = $patron->renew_account;
+    } elsif ( $resetloginattempts eq 'y' ) {
+        $patron->login_attempts(0)->store;

This is not expected here. Improve a message?
Comment 12 Marcel de Rooy 2023-03-03 09:24:05 UTC
Created attachment 147683 [details] [review]
Bug 25947: (QA follow-up) Fix filter
Comment 13 Marcel de Rooy 2023-03-03 09:26:14 UTC
+                        or <a href="/cgi-bin/koha/members/setstatus.pl?borrowernumber=[% patron.borrowernumber | uri %]&amp;destination=circ&amp;resetloginattempts=y">Reset login attempts</a>

You are doing this in an include file. Feels wrong.
Comment 14 Marcel de Rooy 2023-03-03 09:44:59 UTC
The setstatus.pl script certainly needs attention too:

Reading comments like:

#script to set or lift debarred status
No longer true?

# Ideally we should display a warning on the interface if the logged in user is
# not allowed to modify this patron.
# But a librarian is not supposed to hack the system
No longer true?

The code contains:
my ( $loggedinuserid ) = checkauth($input, 0, { borrowers => 'edit_borrowers' }, 'intranet');
...
$logged_in_user->can_see_patron_infos
Includes:             permission => 'borrowers',
            subpermission => 'view_borrower_infos_from_any_libraries',

=> Feels like it is enough. But personally I would rather see a specific permission for things like password, and locked status?

Instead of using this script with a GET operation, it feels better to use our REST API and do a PUT/PATCH patron operation?
Comment 15 Marcel de Rooy 2023-03-03 09:46:21 UTC
Conclusion: I think that you'd better split this into two reports. The reset operation needs more attention too.
Comment 16 David Cook 2023-03-05 23:46:01 UTC
(In reply to solene.ngamga from comment #8)

> 3. In the OPAC, try to log in with the username and a wrong password 3 times
> 4. Go back to the patron's account in the staff interface
> 5. Note that there is a message : "Patron's account has been locked (due to
> 3 failed login attempts)"

We really shouldn't be doing this. It's best practice not to tell an unauthenticated user that an account has been locked due to failed login attempts. It leaks information about system settings and user accounts.
Comment 17 David Cook 2023-03-05 23:46:46 UTC
(In reply to David Cook from comment #16)
> We really shouldn't be doing this. It's best practice not to tell an
> unauthenticated user that an account has been locked due to failed login
> attempts. It leaks information about system settings and user accounts.

If we're going to do something insecure, we should wrap it in a system preference, default it to disabled, and have explanatory text on the system preference page saying it's an insecure setting.
Comment 18 Katrin Fischer 2023-03-06 09:39:03 UTC
(In reply to David Cook from comment #17)
> (In reply to David Cook from comment #16)
> > We really shouldn't be doing this. It's best practice not to tell an
> > unauthenticated user that an account has been locked due to failed login
> > attempts. It leaks information about system settings and user accounts.
> 
> If we're going to do something insecure, we should wrap it in a system
> preference, default it to disabled, and have explanatory text on the system
> preference page saying it's an insecure setting.

I thin this is a misunderstanding. The patch is not about the message that the user sees, but about the message that staff will see.

+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/patron_messages.inc

The user receives a neutral message still. This is to help staff to see the reason why someone has been locked out.
Comment 19 David Cook 2023-03-06 23:11:08 UTC
(In reply to Katrin Fischer from comment #18)
> I thin this is a misunderstanding. The patch is not about the message that
> the user sees, but about the message that staff will see.
> 
> +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/patron_messages.inc
> 
> The user receives a neutral message still. This is to help staff to see the
> reason why someone has been locked out.

Yep, looks I read that comment totally wrong! 

Maybe next time I'm surprised reading something I should slow down and just re-read the comments... 😅.
Comment 20 Marcel de Rooy 2023-04-17 13:02:17 UTC
(In reply to Fridolin Somers from comment #6)
> Reset login attempts as been asked by the library, this is a new feature.
> I hope it makes sens, otherwise I may create a new bug report.

As noted above, please remove that part here. It needs more attention too. See earlier comments. Thanks.