|
Description
Katrin Fischer
2015-07-09 01:22:25 UTC
Created attachment 40993 [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. Created attachment 41295 [details] [review] [Signed-off] 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> 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?
(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. (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? (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. (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. The intranet side does not work if JS is disabled. 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 :) Maybe it could be a good enh to add something like: "Your password hasn't been changed." to the message in the OPAC. 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. Created attachment 42194 [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 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 Marcel, you should get the JS alert preventing the form to be submitted. Are you sure you have correctly refreshed the cache? 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> 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> Patches pushed to master. Thanks Jonathan! Pushed to 3.20.x will be in 3.20.5 Pushed to 3.18.x, is in 3.18.12. |