Summary: | Remove locked message from opac-auth.tt | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | Authentication | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | |
Severity: | major | ||
Priority: | P5 - low | CC: | dcook, dpavlin, fridolin.somers, jonathan.druart, katrin.fischer, martin.renvoize, mtompset, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26727 | ||
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: | |
Bug Depends on: | 18314 | ||
Bug Blocks: | |||
Attachments: |
Bug 21311: Remove locked message from opac-auth.tt
Bug 21311: Remove locked message from opac-auth.tt Bug 21311: Remove locked message from opac-auth.tt Bug 21311: (follow-up) Adjust wording of failed login message Bug 21311: (follow-up) Adjust wording of failed login message Bug 21311: (follow-up) Adjust wording of failed login message Bug 18591: Fix pluralization of type |
Description
Marcel de Rooy
2018-09-03 13:16:09 UTC
A good idea I think. Moving to regular Koha category. No real difference in keeping it here. 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 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. 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> 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> 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. 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 %] (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. (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. Current display: https://snag.gy/bsCa1J.jpg Waiting for a suggestion from a native English speaker. 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. (In reply to Martin Renvoize from comment #13) > >Usernames and passwords are case sensitive. Usernames are not. Thanks for your input! Oops They could be: bug 16282 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. Created attachment 81123 [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. Thx people for your feedback. Hope that the last follow-up covers it all. Created attachment 81124 [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> So the tekst is now: 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 3 failed attempts. Please contact a library staff member if you continue to have problems. (In reply to Marcel de Rooy from comment #21) > So the tekst is now: > 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 3 failed attempts. Please contact a library staff member if you > continue to have problems. s/tekst/text/ Obviously the 3 is based on the current value of FailedLoginAttempts. The last part of the line is removed when lockout is not enabled. Hmm. Should we reveal that value ? 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> (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. Created attachment 81403 [details] [review] Bug 18591: Fix pluralization of type Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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? I attempted to write something. Feel free to change it, if it is unclear, too long, or insufficient. Thx Mark! Let's see how it looks in the release notes. 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) (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 This patch got pushed btw. Changing status. 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 :) Pushed to 18.05.x for 18.05.06 Pushed to 17.11.x for 17.11.12 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. (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. (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. (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. |