Bug 14509

Summary: Don't allow spaces at the end, beginning or as your password
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: AuthenticationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: chris, dpavlin, jonathan.druart, joonas.kylmala, liz, m.de.rooy, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10177
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14620
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15702
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 14509: Reject invalid passwords
[Signed-off] Bug 14509: Reject invalid passwords
Bug 14509: (follow-up) Reject invalid passwords
Bug 14509: Reject invalid passwords
Bug 14509: (follow-up) Reject invalid passwords

Description Katrin Fischer 2015-07-09 01:22:25 UTC
This is a follow-up bug for bug 10177 which fixed the 'change password' page to not allow your password to have spaces at the beginning or ending or to consist solely of spaces.

But there are still 2 other pages to fix:
- Patron account > Edit patron
- OPAC > Change password
Comment 1 Jonathan Druart 2015-07-14 14:37:01 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-08-01 09:22:10 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2015-08-11 14:15:11 UTC
Attachment 41295 [details] uses client side checking for the whitespaces in the staff side password changing view. Shouldn't we change it to server side as it looks like all the other fields are checked that way. I think it would be more consistent. What do you think?
Comment 4 Owen Leonard 2015-08-11 14:20:41 UTC
(In reply to Joonas Kylmälä from comment #3)
> Shouldn't we change it to server side
> as it looks like all the other fields are checked that way.

There are changes to the template and to opac-passwd.pl. To confirm, disable JavaScript and see if the server-side check is working correctly as well.
Comment 5 Joonas Kylmälä 2015-08-11 14:40:47 UTC
(In reply to Owen Leonard from comment #4)
> (In reply to Joonas Kylmälä from comment #3)
> > Shouldn't we change it to server side
> > as it looks like all the other fields are checked that way.
> 
> There are changes to the template and to opac-passwd.pl. To confirm, disable
> JavaScript and see if the server-side check is working correctly as well.

opac-passwd.pl is for the end user server side check, right? I disabled JavaScript in the staff side and there wasn't any check from the server side, it allowed to change the password (this was also mentioned in the test plan). So should we change the code so that it checks for the whitespaces in the server side when the password is changed from the staff view?
Comment 6 Jonathan Druart 2015-08-18 17:24:02 UTC
(In reply to Joonas Kylmälä from comment #5)
> (In reply to Owen Leonard from comment #4)
> > (In reply to Joonas Kylmälä from comment #3)
> > > Shouldn't we change it to server side
> > > as it looks like all the other fields are checked that way.
> > 
> > There are changes to the template and to opac-passwd.pl. To confirm, disable
> > JavaScript and see if the server-side check is working correctly as well.
> 
> opac-passwd.pl is for the end user server side check, right? I disabled
> JavaScript in the staff side and there wasn't any check from the server
> side, it allowed to change the password (this was also mentioned in the test
> plan). So should we change the code so that it checks for the whitespaces in
> the server side when the password is changed from the staff view?

If you like, you can provide it.
But I don't know why a librarian will force a password if the application denies it.
Comment 7 Joonas Kylmälä 2015-08-18 19:18:00 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Joonas Kylmälä from comment #5)
> > (In reply to Owen Leonard from comment #4)
> > > (In reply to Joonas Kylmälä from comment #3)
> > > > Shouldn't we change it to server side
> > > > as it looks like all the other fields are checked that way.
> > > 
> > > There are changes to the template and to opac-passwd.pl. To confirm, disable
> > > JavaScript and see if the server-side check is working correctly as well.
> > 
> > opac-passwd.pl is for the end user server side check, right? I disabled
> > JavaScript in the staff side and there wasn't any check from the server
> > side, it allowed to change the password (this was also mentioned in the test
> > plan). So should we change the code so that it checks for the whitespaces in
> > the server side when the password is changed from the staff view?
> 
> If you like, you can provide it.
> But I don't know why a librarian will force a password if the application
> denies it.

Librarian doesn't need to force the password, only use a browser where the JavaScript is disabled. So I give one vote for the server side check.
Comment 8 Jonathan Druart 2015-08-19 07:33:59 UTC
The intranet side does not work if JS is disabled.
Comment 9 Joonas Kylmälä 2015-08-19 07:47:51 UTC
Well it doesn't (In reply to Jonathan Druart from comment #8)
> The intranet side does not work if JS is disabled.

It works and looks actually pretty ok, from the whole process of finding the patron with the search bar in the top to changing the password. But I also think it is not so urgent to do the server side check. However, there is bug 14620, so if it goes to upstream then maybe the password validation could go there and then it could check also for the spaces :)
Comment 10 Katrin Fischer 2015-09-01 22:06:45 UTC
Maybe it could be a good enh to add something like: "Your password hasn't been changed." to the message in the OPAC.
Comment 11 Katrin Fischer 2015-09-01 22:11:58 UTC
Opac works great, but I can't trigger the message changing my password from the 'Change password' page in staff (http://localhost:8080/cgi-bin/koha/members/member-password.pl?member=1)
It seems to work alright without this patch.
Comment 12 Jonathan Druart 2015-09-02 08:45:30 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2015-09-03 12:39:24 UTC
member-password and member-entry are not consistent: member-password does not allow trailing space, but member-entry does ...

member-entry:
1234<space> becomes $2a$08$se1EC5tmOPHRh2NKoHlVVu4UWzq.SBTh1zY/qjQ94qzZPOd20HcW2
1234 becomes $2a$08$0nW80AYBaRFf5MEVoesp0uzMAGYlTZiZDlpTl4bjQNx7bhIGGS6FW

FQA
Comment 14 Jonathan Druart 2015-09-03 13:27:48 UTC
Marcel, you should get the JS alert preventing the form to be submitted.
Are you sure you have correctly refreshed the cache?
Comment 15 Marcel de Rooy 2015-09-03 14:07:12 UTC
Created attachment 42324 [details] [review]
Bug 14509: Reject invalid passwords

Bug 10177 rejects password with leading or trailing whitespaces, but
only on the member-password page.
It's not consistent to only do this check on 1 place.
This patch adds the check for the 2 other places: memberentry and at the
OPAC.

Test plan:
1/ Edit a patron and set a password with leading and/or trailing
whitespaces. You should not be allowed to do it (no server side check).
2/ Same at the OPAC ("Change you password" tab). Here there is a server
side check.

Followed test plan. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2015-09-03 14:07:22 UTC
Created attachment 42325 [details] [review]
Bug 14509: (follow-up) Reject invalid passwords

The members.js was not included in member-password.tt
And it cannot, there are specific code which cannot be used outside of
memberentry.tt

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Tomás Cohen Arazi 2015-09-28 15:15:11 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 18 Chris Cormack 2015-10-11 20:45:00 UTC
Pushed to 3.20.x will be in 3.20.5
Comment 19 Liz Rea 2015-11-02 21:00:40 UTC
Pushed to 3.18.x, is in 3.18.12.