Bug 21311 - Remove locked message from opac-auth.tt
Summary: Remove locked message from opac-auth.tt
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Marcel de Rooy
QA Contact:
URL:
Keywords:
Depends on: 18314
Blocks:
  Show dependency tree / graph
 
Reported: 2018-09-03 13:16 UTC by Marcel de Rooy
Modified: 2020-10-18 17:03 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
It is good security practice to not provide details which could confirm or deny the existence of an account. Previously, the simple "This account has been locked!" confirmed its existence which would only encourage more attacks by hackers. To prevent aiding malicious attacks, the message has been changed to something that does not expressly state the account has been locked. It only mentions that accounts will be locked after a number of failed attempts, instead of saying whether it is locked or not. So while a successful attempt will seem to have an invalid username or password suggestion after the account is locked, users should be reminded that they can always reset their password or contact library staff for help.
Version(s) released in:
Circulation function:

Attachments
Bug 21311: Remove locked message from opac-auth.tt (2.66 KB, patch)
2018-10-10 14:16 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 21311: Remove locked message from opac-auth.tt (2.92 KB, patch)
2018-10-11 08:20 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 21311: Remove locked message from opac-auth.tt (2.94 KB, patch)
2018-10-15 16:55 UTC, Mark Tompsett 		Details | Diff | Splinter Review
Bug 21311: (follow-up) Adjust wording of failed login message (2.43 KB, patch)
2018-10-25 10:04 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 21311: (follow-up) Adjust wording of failed login message (2.49 KB, patch)
2018-10-25 10:05 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 21311: (follow-up) Adjust wording of failed login message (2.47 KB, patch)
2018-10-25 10:11 UTC, Marcel de Rooy 		Details | Diff | Splinter Review
Bug 18591: Fix pluralization of type (699 bytes, patch)
2018-10-28 12:13 UTC, Katrin Fischer 		Details | Diff | Splinter Review
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2018-09-03 13:16:09 UTC 
Wrong user or password generates:
You entered an incorrect username or password. Please try again! And remember, passwords are case sensitive.
But a locked account (login_attempts>=pref) generates:
This account has been locked! Please contact a library staff member.

The second message could be considered as helpful to the user, but also reveals an attacker that he found an existing account (still locked out, but maybe activated soon again).

We could merge both messages into one general one. Just a bit paranoid or a good idea ?
Comment 1 Marcel de Rooy 2018-09-03 13:18:49 UTC 
See also bug 18314
Comment 2 Fridolin Somers 2018-09-16 22:16:08 UTC 
A good idea I think.
Comment 3 Marcel de Rooy 2018-10-10 14:03:57 UTC 
Moving to regular Koha category. No real difference in keeping it here.
Comment 4 Marcel de Rooy 2018-10-10 14:16:54 UTC 
Created attachment 80352 [details] [review]
Bug 21311: Remove locked message from opac-auth.tt

We should not expose more information than needed when someone tries
to login with invalid credentials. Saying that an account is locked
reveals that the account exists (or perhaps an email address).

Trivial fix. Keeping the var too_many_login_attempts for staff.
Note: We do not remove this distinction for the staff client here (in the
assumption that a library may well have additional security measures in
place for staff client). But it could be done too (on another report).

Test plan:
Enable lockout feature.
Enter invalid credentials until account locks out (on OPAC !!)
Note that message does no longer change to 'Account is locked'.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Mark Tompsett 2018-10-10 16:06:04 UTC 
Comment on attachment 80352 [details] [review]
Bug 21311: Remove locked message from opac-auth.tt

Review of attachment 80352 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
@@ +57,5 @@
>                              [% END %]
>  
>  
> +                            [% IF invalid_username_or_password || too_many_login_attempts %]
> +                                <!-- This is what is displayed if user doesnt have permission or account is locked. (Do not expose more information than needed.) -->

This is a very good security step.

@@ -62,5 @@
> -                                This account has been locked!
> -                                [% IF Koha.Preference('OpacResetPassword') %]
> -                                    <a href="/cgi-bin/koha/opac-password-recovery.pl">You must reset your password</a>.
> -                                [% ELSE %]
> -                                    Please contact a library staff member.

I think adding the "Please contact a library staff member if you continue to have problems." would be a good idea.
Comment 6 Marcel de Rooy 2018-10-11 08:20:28 UTC 
Created attachment 80388 [details] [review]
Bug 21311: Remove locked message from opac-auth.tt

We should not expose more information than needed when someone tries
to login with invalid credentials. Saying that an account is locked
reveals that the account exists (or perhaps an email address).

Trivial fix. Keeping the var too_many_login_attempts for staff.
Note: We do not remove this distinction for the staff client here (in the
assumption that a library may well have additional security measures in
place for staff client). But it could be done too (on another report).

Test plan:
Enable lockout feature.
Enter invalid credentials until account locks out (on OPAC !!)
Note that message does no longer change to 'Account is locked'.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Mark Tompsett 2018-10-15 16:55:28 UTC 
Created attachment 80623 [details] [review]
Bug 21311: Remove locked message from opac-auth.tt

We should not expose more information than needed when someone tries
to login with invalid credentials. Saying that an account is locked
reveals that the account exists (or perhaps an email address).

Trivial fix. Keeping the var too_many_login_attempts for staff.
Note: We do not remove this distinction for the staff client here (in the
assumption that a library may well have additional security measures in
place for staff client). But it could be done too (on another report).

Test plan:
Enable lockout feature.
Enter invalid credentials until account locks out (on OPAC !!)
Note that message does no longer change to 'Account is locked'.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 8 Mark Tompsett 2018-10-15 16:58:22 UTC 
The only thing that may be in question is whether or not we change the other part of the message. A simple "Access Denied" without reasons is clearer in that it doesn't say what was wrong. The failure to mention the possibility of lock out seems wrong to me, but I'm not hung up on it. I'll let QA decide that quandary with a follow up patch.
Comment 9 Jonathan Druart 2018-10-15 17:57:55 UTC 
Maybe we could keep the link to recover the password?

-                                [% IF Koha.Preference('OpacResetPassword') %]
-                                    <a href="/cgi-bin/koha/opac-password-recovery.pl">You must reset your password</a>.
-                                [% ELSE %]
Comment 10 Mark Tompsett 2018-10-15 18:25:59 UTC 
(In reply to Jonathan Druart from comment #9)
> Maybe we could keep the link to recover the password?
> 
> -                                [% IF Koha.Preference('OpacResetPassword')
> %]
> -                                    <a
> href="/cgi-bin/koha/opac-password-recovery.pl">You must reset your
> password</a>.
> -                                [% ELSE %]

Do you want people to be able to unlock locked accounts?
Perhaps a wording change would be required so as to not let the person know whether or not the account is locked. And does the reset password reset the failed log in attempts? I thought there was another bug related to this.
Comment 11 Jonathan Druart 2018-10-16 17:34:03 UTC 
(In reply to M. Tompsett from comment #10)
> (In reply to Jonathan Druart from comment #9)
> > Maybe we could keep the link to recover the password?
> > 
> > -                                [% IF Koha.Preference('OpacResetPassword')
> > %]
> > -                                    <a
> > href="/cgi-bin/koha/opac-password-recovery.pl">You must reset your
> > password</a>.
> > -                                [% ELSE %]
> 
> Do you want people to be able to unlock locked accounts?

Yes, using the password recovery method. But actually the link appears under the login form so no need to repeat it.

However I would recommend to keep something about "or you account has been locked" if FailedLoginAttempts is set.
Comment 12 Jonathan Druart 2018-10-16 17:35:18 UTC 
Current display: https://snag.gy/bsCa1J.jpg

Waiting for a suggestion from a native English speaker.
Comment 13 Martin Renvoize (ashimema) 2018-10-16 18:19:30 UTC 
I'd go with:

>You entered an incorrect username or password, please try again.
>
>Usernames and passwords are case sensitive.
>
>Please contact a member of the library staff if you continue to have problems.
Comment 14 Jonathan Druart 2018-10-16 18:39:25 UTC 
(In reply to Martin Renvoize from comment #13)

> >Usernames and passwords are case sensitive.

Usernames are not. Thanks for your input!
Comment 15 Martin Renvoize (ashimema) 2018-10-16 19:12:14 UTC 
Oops
Comment 16 Katrin Fischer 2018-10-16 19:34:49 UTC 
They could be: bug 16282
Comment 17 Katrin Fischer 2018-10-16 19:35:48 UTC 
I think having a note about "please note that after X attempts you will be locked out" could be really helpful without revealing too much.
Comment 18 Marcel de Rooy 2018-10-25 10:04:08 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2018-10-25 10:04:55 UTC 
Thx people for your feedback. Hope that the last follow-up covers it all.
Comment 20 Marcel de Rooy 2018-10-25 10:05:55 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2018-10-25 10:07:06 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2018-10-25 10:07:27 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2018-10-25 10:08:32 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2018-10-25 10:09:00 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2018-10-25 10:11:38 UTC 
Created attachment 81125 [details] [review]
Bug 21311: (follow-up) Adjust wording of failed login message

Based on the feedback of Jonathan, Katrin, Mark and Martin (thx), adding
the general remark that if lockout is enabled, an account may be locked
out after x attempts. This does not say that the account on hand is
locked or not.

Adding a <p> tag between 'Log in'-button and 'Forgot your password' for
presentation.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2018-10-25 10:12:25 UTC 
(In reply to Marcel de Rooy from comment #24)
> Hmm. Should we reveal that value ?

Perhaps not. So have adjusted it too:

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.
Comment 27 Katrin Fischer 2018-10-28 12:13:40 UTC 
Created attachment 81403 [details] [review]
Bug 18591: Fix pluralization of type

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Katrin Fischer 2018-10-28 12:14:45 UTC 
I think this is good, but since it's a behavior change not everyone will understand right away, can we have a little explanation in the 'text to go in release notes' please?
Comment 29 Mark Tompsett 2018-10-28 14:56:45 UTC 
I attempted to write something. Feel free to change it, if it is unclear, too long, or insufficient.
Comment 30 Katrin Fischer 2018-10-28 14:59:07 UTC 
Thx Mark! Let's see how it looks in the release notes.
Comment 31 Marcel de Rooy 2018-10-29 08:57:20 UTC 
Comment on attachment 81403 [details] [review]
Bug 18591: Fix pluralization of type

Koha/Illcomments.pm ?


Nick, Katrin: This patch did not belong here. Missing it somewhere else (18591)
Comment 32 Marcel de Rooy 2018-10-29 08:57:36 UTC 
(In reply to M. Tompsett from comment #29)
> I attempted to write something. Feel free to change it, if it is unclear,
> too long, or insufficient.

Thanks Mark
Comment 33 Marcel de Rooy 2018-10-29 09:07:59 UTC 
This patch got pushed btw. Changing status.
Comment 34 Katrin Fischer 2018-10-29 10:55:40 UTC 
Rooy from comment #31)
> Comment on attachment 81403 [details] [review] [review]
> Bug 18591: Fix pluralization of type
> 
> Koha/Illcomments.pm ?
> 
> 
> Nick, Katrin: This patch did not belong here. Missing it somewhere else
> (18591)

Sorry, must have gotten the range wrong when reattaching. It was the top commit on master when I tested this, not missing :)
Comment 35 Martin Renvoize (ashimema) 2018-11-08 15:33:38 UTC 
Pushed to 18.05.x for 18.05.06
Comment 36 Fridolin Somers 2018-11-26 06:31:53 UTC 
Pushed to 17.11.x for 17.11.12
Comment 37 David Cook 2020-02-18 00:54:53 UTC 
Comment on attachment 81125 [details] [review]
Bug 21311: (follow-up) Adjust wording of failed login message

Review of attachment 81125 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
@@ +168,4 @@
>                              </fieldset>
>  
>                              <input type="submit" value="Log in" class="btn" />
> +                            <p/>

I think this must've been a typo.
Comment 38 Marcel de Rooy 2020-09-29 11:48:53 UTC 
(In reply to David Cook from comment #37)
> Comment on attachment 81125 [details] [review] [review]
> Bug 21311: (follow-up) Adjust wording of failed login message
> 
> Review of attachment 81125 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
> @@ +168,4 @@
> >                              </fieldset>
> >  
> >                              <input type="submit" value="Log in" class="btn" />
> > +                            <p/>
> 
> I think this must've been a typo.

Sorry, late response ;) Commit message says:
Adding a <p> tag between 'Log in'-button and 'Forgot your password' for
presentation.
Comment 39 David Cook 2020-09-30 08:18:37 UTC 
(In reply to Marcel de Rooy from comment #38)
> (In reply to David Cook from comment #37)
> > Comment on attachment 81125 [details] [review] [review] [review]
> > Bug 21311: (follow-up) Adjust wording of failed login message
> > 
> > Review of attachment 81125 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
> > @@ +168,4 @@
> > >                              </fieldset>
> > >  
> > >                              <input type="submit" value="Log in" class="btn" />
> > > +                            <p/>
> > 
> > I think this must've been a typo.
> 
> Sorry, late response ;) Commit message says:
> Adding a <p> tag between 'Log in'-button and 'Forgot your password' for
> presentation.

I think that <p/> might be valid XHTML but I'm quite certain that is not valid HTML/HTML5 though.
Comment 40 Katrin Fischer 2020-10-18 17:03:59 UTC 
(In reply to David Cook from comment #39)
...
> I think that <p/> might be valid XHTML but I'm quite certain that is not
> valid HTML/HTML5 though.

I've filed a separate bug 26727 for this and fixed all occurences I found.