Some libraries would want to add the option to Koha to enforce a strong password policy. That policy would mean that passwords should include both upper and lower case characters one or more numbers.
Created attachment 61227 [details] [review] Bug 18298: Enforce password complexity This patchset prevents users to enter too leak password, controlled by a new syspref RequireStrongPassword. If set the staff and patrons will have to enter a strong password. The strongness cannot be modified, it has been arbitrarily set (by the author of this enhancement) to at least 1 lowercase, 1 uppercase and 1 digit. This can be inforce by increasing the value of the existing minPasswordLength pref. I decided to turn this feature on, it cannot hurt! For existing installs it will have to be turned on manually. Writing these patches I found a lot of inconsistencies all around the password checks and decided to refactor everything to make things consistent and more robust. Now the password validity is check at only one place (subroutine covered by tests). Test plan: We have several places where a password can be change/created: a. Editing a patron (members/memberentry.pl) b. Changing the password of a patron (members/member-password.pl) c. Changing your own password at the opac (opac/opac-passwd.pl). OpacPasswordChange needs to be set d. Reseting your own password at the opac (opac/opac-password-recovery.pl). OpacResetPassword needs to be set, see "Forgot your password?" link when you are not logged in e. Self registration feature, PatronSelfRegistration needs to be set. You will also need to add 'password' to PatronSelfRegistrationBorrowerMandatoryField. Note that '****' is considered by Koha internally that the password is not changed (existing behavior). To fully test this patch you will need to test the different combinations of RequireStrongPassword and minPasswordLength.
Created attachment 61228 [details] [review] Bug 18298: Use the validate jQuery plugin To validate password fields we need to use the validate jQuery plugin. To make things reusable this patch adds a new include file 'password_check.inc' at the intranet and opac sides, it creates 3 new validation methods: - password_strong => make sure the passwords are strong enough according to the values of the RequireStrongPassword and minPasswordLength prefs - password_no_spaces => prevent passwords to be entered with leading or trailing spaces - password_match => make sure both password fields match
Created attachment 61229 [details] [review] Bug 18298: Move password generation to template side This patch removes a really ugly way to generate a password: the whole template was sent and parsed to retrieve the "#defaultnewpassfield" node. To avoid the password to be sent plain text it is certainly better to generate it client-side. The same kind of passwords will be generated: 0-9a-zA-Z The while loop prevents to get an invalid generated password.
Created attachment 61230 [details] [review] Bug 18298: minPaswordLength should not be < 3 Indeed if RequireStrongPassword is set we need at least 3 characters to match 1 upper, 1 lower and 1 digit. We could make things more complicated to allow minPasswordLength < 3 but, really, 3 is already too low...
Created attachment 61231 [details] [review] Bug 18298: minPasswordLength should not be used as the default password length The length of the passwords generated for a patron should not be as long as the value of minPasswordLength. It is the minimum required size of a password, not the maximum! So let's fix it to 8 if the minPasswordLength if < 8, that sounds reasonable and less risky for patrons.
Created attachment 61232 [details] [review] Bug 18298: Add server-side checks and refactor stuffs Now that we have a check client-side, nothing prevents us from a smart guy to bypass it and force an invalid password. This patch adds two new subroutines to Koha::AuthUtils to check the validity of passwords and generate a password server-side. It is used only once (self-registration) but could be useful later. Moreover the 3 different cases of password rejection (too leak, too short, contains leading or trailing whitespaces) were not tested everywhere. Now they are! This patch makes things consistent everywhere and clean up some code.
Created attachment 61237 [details] Screenshots of passwort hints on OPAC self registration Tested following plan in comment #1, works great. One small thing: Hints for wrong password do not display properly on OPAC self registration, see screenshots.
Sadly, some libraries need to enforce weak passwords = PINs (4 digit "passwords"). The reason for this is hardwear that only has a numerical keyboard, and where patrons have to enter a pin in order to do things like self service checkouts. Will this patch make it possible to enforce a password of 4 digits?
(In reply to Magnus Enger from comment #8) > Sadly, some libraries need to enforce weak passwords = PINs (4 digit > "passwords"). The reason for this is hardwear that only has a numerical > keyboard, and where patrons have to enter a pin in order to do things like > self service checkouts. Will this patch make it possible to enforce a > password of 4 digits? Nope, the strongness is not configurable. But since the patches refactor all the password code it would be much more easy to improve it.
(In reply to Jonathan Druart from comment #9) > Nope, the strongness is not configurable. > But since the patches refactor all the password code it would be much more > easy to improve it. Ah, good to hear!
(In reply to Marc Véron from comment #7) > Created attachment 61237 [details] > Screenshots of passwort hints on OPAC self registration > > Tested following plan in comment #1, works great. > > One small thing: Hints for wrong password do not display properly on OPAC > self registration, see screenshots. That sounds like an existing issue, I'll deal with it on another bug report.
Created attachment 61331 [details] [review] Bug 18298: Enforce password complexity This patchset prevents users to enter too leak password, controlled by a new syspref RequireStrongPassword. If set the staff and patrons will have to enter a strong password. The strongness cannot be modified, it has been arbitrarily set (by the author of this enhancement) to at least 1 lowercase, 1 uppercase and 1 digit. This can be inforce by increasing the value of the existing minPasswordLength pref. I decided to turn this feature on, it cannot hurt! For existing installs it will have to be turned on manually. Writing these patches I found a lot of inconsistencies all around the password checks and decided to refactor everything to make things consistent and more robust. Now the password validity is check at only one place (subroutine covered by tests). Test plan: We have several places where a password can be change/created: a. Editing a patron (members/memberentry.pl) b. Changing the password of a patron (members/member-password.pl) c. Changing your own password at the opac (opac/opac-passwd.pl). OpacPasswordChange needs to be set d. Reseting your own password at the opac (opac/opac-password-recovery.pl). OpacResetPassword needs to be set, see "Forgot your password?" link when you are not logged in e. Self registration feature, PatronSelfRegistration needs to be set. You will also need to add 'password' to PatronSelfRegistrationBorrowerMandatoryField. Note that '****' is considered by Koha internally that the password is not changed (existing behavior). To fully test this patch you will need to test the different combinations of RequireStrongPassword and minPasswordLength. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61332 [details] [review] Bug 18298: Use the validate jQuery plugin To validate password fields we need to use the validate jQuery plugin. To make things reusable this patch adds a new include file 'password_check.inc' at the intranet and opac sides, it creates 3 new validation methods: - password_strong => make sure the passwords are strong enough according to the values of the RequireStrongPassword and minPasswordLength prefs - password_no_spaces => prevent passwords to be entered with leading or trailing spaces - password_match => make sure both password fields match Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61333 [details] [review] Bug 18298: Move password generation to template side This patch removes a really ugly way to generate a password: the whole template was sent and parsed to retrieve the "#defaultnewpassfield" node. To avoid the password to be sent plain text it is certainly better to generate it client-side. The same kind of passwords will be generated: 0-9a-zA-Z The while loop prevents to get an invalid generated password. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61334 [details] [review] Bug 18298: minPaswordLength should not be < 3 Indeed if RequireStrongPassword is set we need at least 3 characters to match 1 upper, 1 lower and 1 digit. We could make things more complicated to allow minPasswordLength < 3 but, really, 3 is already too low... Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61335 [details] [review] Bug 18298: minPasswordLength should not be used as the default password length The length of the passwords generated for a patron should not be as long as the value of minPasswordLength. It is the minimum required size of a password, not the maximum! So let's fix it to 8 if the minPasswordLength if < 8, that sounds reasonable and less risky for patrons. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61336 [details] [review] Bug 18298: Add server-side checks and refactor stuffs Now that we have a check client-side, nothing prevents us from a smart guy to bypass it and force an invalid password. This patch adds two new subroutines to Koha::AuthUtils to check the validity of passwords and generate a password server-side. It is used only once (self-registration) but could be useful later. Moreover the 3 different cases of password rejection (too leak, too short, contains leading or trailing whitespaces) were not tested everywhere. Now they are! This patch makes things consistent everywhere and clean up some code. Signed-off-by: Marc Véron <veron@veron.ch>
We got the same problem - a library with self check hardware that doesn't allow for alpha-numeric passwords. I think turning it on by default for new installations is a good idea. If we turn it off, everything will be like before?
In order to call a password "strong", we should definitely not allow a password length less than 8 characters (not to talk about 12-14). And we should enforce a special character too. (Enforcing uc,lc,digits is definitely an improvement btw!) When you enable RequireStrongPassword, you should just raise minPasswordLength. You cannot enable it and have strong passwords of 3 chars. Impossible! Also: The default for minPasswordLength should really be 8. If people want to make it three characters after install, which they should not do, we could argue that this is their own responsibility. Or we could just not allow it. I saw several constructs like: my $minpw = C4::Context->preference('minPasswordLength'); $minpw = 3 if not $minpw or $minpw < 3; We could call a function in C4/Auth to get the password length and not check the pref everywhere. And increase 3 of course. "To avoid the password to be sent plain text it is certainly better to generate it client-side." And then send it back to the server plain text? Or should we just say: use https and we trust that transmission? "Now that we have a check client-side, nothing prevents us from a smart guy to bypass it and force an invalid password." And this is an issue. How do you want to resolve that one?
(In reply to Marcel de Rooy from comment #19) > In order to call a password "strong", we should definitely not allow a > password length less than 8 characters (not to talk about 12-14). And we > should enforce a special character too. (Enforcing uc,lc,digits is > definitely an improvement btw!) > When you enable RequireStrongPassword, you should just raise > minPasswordLength. You cannot enable it and have strong passwords of 3 > chars. Impossible! I am in discussion with the sponsor about the special character. In any cases that will be dealt on another bug report. > I saw several constructs like: > my $minpw = C4::Context->preference('minPasswordLength'); > $minpw = 3 if not $minpw or $minpw < 3; > We could call a function in C4/Auth to get the password length and not check > the pref everywhere. And increase 3 of course. See the whole patch set, this is fixed in the last patch. > "To avoid the password to be sent plain text it is certainly better to > generate it client-side." > And then send it back to the server plain text? > Or should we just say: use https and we trust that transmission? Yes indeed, it is still passing plain text unless using https. > "Now that we have a check client-side, nothing prevents us from a smart guy > to > bypass it and force an invalid password." > And this is an issue. How do you want to resolve that one? Hum? I added server-side checks everywhere.
(In reply to Marcel de Rooy from comment #19) > Also: The default for minPasswordLength should really be 8. If people want > to make it three characters after install, which they should not do, we > could argue that this is their own responsibility. Or we could just not > allow it. This should be discussed on its own bug report.
(In reply to Jonathan Druart from comment #21) > (In reply to Marcel de Rooy from comment #19) > > Also: The default for minPasswordLength should really be 8. If people want > > to make it three characters after install, which they should not do, we > > could argue that this is their own responsibility. Or we could just not > > allow it. > > This should be discussed on its own bug report. Agreed
(In reply to Jonathan Druart from comment #20) > I am in discussion with the sponsor about the special character. In any > cases that will be dealt on another bug report. Well, I have quite a strong opinion on that one.. > > I saw several constructs like: > > my $minpw = C4::Context->preference('minPasswordLength'); > > $minpw = 3 if not $minpw or $minpw < 3; > > We could call a function in C4/Auth to get the password length and not check > > the pref everywhere. And increase 3 of course. > > See the whole patch set, this is fixed in the last patch. OK Sorry, I didnt see. > > "Now that we have a check client-side, nothing prevents us from a smart guy > > to > > bypass it and force an invalid password." > > And this is an issue. How do you want to resolve that one? > > Hum? I added server-side checks everywhere. If so, the commit message is confusing.
(In reply to Marcel de Rooy from comment #23) > (In reply to Jonathan Druart from comment #20) > > I am in discussion with the sponsor about the special character. In any > > cases that will be dealt on another bug report. > Well, I have quite a strong opinion on that one.. In any cases I would prefer to deal with it on another bug report. Actually my concern is that some people could find it is a too strong requirement. Indeed you can have a very strong password without any special characters. It could lead to user frustration. And libraries will turn it off. > > > "Now that we have a check client-side, nothing prevents us from a smart guy > > > to > > > bypass it and force an invalid password." > > > And this is an issue. How do you want to resolve that one? > > > > Hum? I added server-side checks everywhere. > If so, the commit message is confusing. It says: "Now that we have a check client-side, nothing prevents us from a smart guy to bypass it and force an invalid password. This patch adds two new subroutines to Koha::AuthUtils to check the validity of passwords and generate a password server-side."
(In reply to Jonathan Druart from comment #24) > In any cases I would prefer to deal with it on another bug report. > Actually my concern is that some people could find it is a too strong > requirement. > Indeed you can have a very strong password without any special characters. > It could lead to user frustration. > And libraries will turn it off. Dont give up too quickly :) Yes, a3bXyYzQ looks stronger than Myname@1 But b4_Xz#43Y may be stronger again. User experience and security may often generate some friction. Security also means education of the users.
Maybe we could just have different strength settings? Instead of "RequireStrongPassword" we could name this PasswordStrength and then have the current and alphanumeric+digits as first two options.
(In reply to Katrin Fischer from comment #26) > Maybe we could just have different strength settings? Instead of > "RequireStrongPassword" we could name this PasswordStrength and then have > the current and alphanumeric+digits as first two options. +1 Marc
(In reply to Katrin Fischer from comment #26) > Maybe we could just have different strength settings? Instead of > "RequireStrongPassword" we could name this PasswordStrength and then have > the current and alphanumeric+digits as first two options. Yes, I like that idea too. Would be a good excuse to postpone the special char to another report ;)
> Maybe we could just have different strength settings? Instead of > "RequireStrongPassword" we could name this PasswordStrength and then have > the current and alphanumeric+digits as first two options. Are you ok to provide this last followup Jonathan?
(In reply to Martin Renvoize from comment #29) > > Maybe we could just have different strength settings? Instead of > > "RequireStrongPassword" we could name this PasswordStrength and then have > > the current and alphanumeric+digits as first two options. > > Are you ok to provide this last followup Jonathan? To me it's outside the scope of this bug report. I consider this as an improvement of this new feature.
(In reply to Jonathan Druart from comment #30) > (In reply to Martin Renvoize from comment #29) > > > Maybe we could just have different strength settings? Instead of > > > "RequireStrongPassword" we could name this PasswordStrength and then have > > > the current and alphanumeric+digits as first two options. > > > > Are you ok to provide this last followup Jonathan? > > To me it's outside the scope of this bug report. I consider this as an > improvement of this new feature. Hi Jonathan, I was hoping to sign this off once more to confirm it is ready for inclusion, but it doesn't currently apply. Best wishes, Alex
Created attachment 62976 [details] [review] Bug 18298: Enforce password complexity This patchset prevents users to enter too leak password, controlled by a new syspref RequireStrongPassword. If set the staff and patrons will have to enter a strong password. The strongness cannot be modified, it has been arbitrarily set (by the author of this enhancement) to at least 1 lowercase, 1 uppercase and 1 digit. This can be inforce by increasing the value of the existing minPasswordLength pref. I decided to turn this feature on, it cannot hurt! For existing installs it will have to be turned on manually. Writing these patches I found a lot of inconsistencies all around the password checks and decided to refactor everything to make things consistent and more robust. Now the password validity is check at only one place (subroutine covered by tests). Test plan: We have several places where a password can be change/created: a. Editing a patron (members/memberentry.pl) b. Changing the password of a patron (members/member-password.pl) c. Changing your own password at the opac (opac/opac-passwd.pl). OpacPasswordChange needs to be set d. Reseting your own password at the opac (opac/opac-password-recovery.pl). OpacResetPassword needs to be set, see "Forgot your password?" link when you are not logged in e. Self registration feature, PatronSelfRegistration needs to be set. You will also need to add 'password' to PatronSelfRegistrationBorrowerMandatoryField. Note that '****' is considered by Koha internally that the password is not changed (existing behavior). To fully test this patch you will need to test the different combinations of RequireStrongPassword and minPasswordLength. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 62977 [details] [review] Bug 18298: Use the validate jQuery plugin To validate password fields we need to use the validate jQuery plugin. To make things reusable this patch adds a new include file 'password_check.inc' at the intranet and opac sides, it creates 3 new validation methods: - password_strong => make sure the passwords are strong enough according to the values of the RequireStrongPassword and minPasswordLength prefs - password_no_spaces => prevent passwords to be entered with leading or trailing spaces - password_match => make sure both password fields match Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 62978 [details] [review] Bug 18298: Move password generation to template side This patch removes a really ugly way to generate a password: the whole template was sent and parsed to retrieve the "#defaultnewpassfield" node. To avoid the password to be sent plain text it is certainly better to generate it client-side. The same kind of passwords will be generated: 0-9a-zA-Z The while loop prevents to get an invalid generated password. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 62979 [details] [review] Bug 18298: minPaswordLength should not be < 3 Indeed if RequireStrongPassword is set we need at least 3 characters to match 1 upper, 1 lower and 1 digit. We could make things more complicated to allow minPasswordLength < 3 but, really, 3 is already too low... Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 62980 [details] [review] Bug 18298: minPasswordLength should not be used as the default password length The length of the passwords generated for a patron should not be as long as the value of minPasswordLength. It is the minimum required size of a password, not the maximum! So let's fix it to 8 if the minPasswordLength if < 8, that sounds reasonable and less risky for patrons. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 62981 [details] [review] Bug 18298: Add server-side checks and refactor stuffs Now that we have a check client-side, nothing prevents us from a smart guy to bypass it and force an invalid password. This patch adds two new subroutines to Koha::AuthUtils to check the validity of passwords and generate a password server-side. It is used only once (self-registration) but could be useful later. Moreover the 3 different cases of password rejection (too leak, too short, contains leading or trailing whitespaces) were not tested everywhere. Now they are! This patch makes things consistent everywhere and clean up some code. Signed-off-by: Marc Véron <veron@veron.ch>
Did you know that there is already a bug for this feature? Bug 12617 And it allows configuring the password type per borrower category.
The important thing is that one doesn't add any more sysprefs. There are too many as is. Add a borrower category parameter, 'passwordpolicy' and configure the password policy per borrower category. Maybe even the password minSize can be set there. Much more flexible.
Why would one want different policies per patron category? I don't feel this level of granularity would be necessary here - I'd prefer a global setting.
(In reply to Olli-Antti Kivilahti from comment #38) > Did you know that there is already a bug for this feature? How do you want us to know about it? Status is "New", patches have been submitted 2 years ago and do no longer applied, changes are not covered by tests, etc.
(In reply to Katrin Fischer from comment #40) > Why would one want different policies per patron category? I don't feel this > level of granularity would be necessary here - I'd prefer a global setting. You could demand really difficult passwords for staff?
The concept of "upper and lower case characters" implemented by this patchset may be a bit limited - for many non-english-speaking countries, and especially for the countries which don't use latin-derived alphabets. Replacing [a-z] with \p{Ll} and [A-Z] with \p{Lu} will probably work just fine in the perl regexps, but would it work in the javascript / jQuery validation related code parts as well?
(In reply to Jacek Ablewicz from comment #43) > The concept of "upper and lower case characters" implemented by this > patchset may be a bit limited - for many non-english-speaking countries, and > especially for the countries which don't use latin-derived alphabets. > > Replacing [a-z] with \p{Ll} and [A-Z] with \p{Lu} will probably work just > fine in the perl regexps, but would it work in the javascript / jQuery > validation related code parts as well? Yes you are totally right. That's how I started to implement this feature, but I switched back to the naive way when I did not found a JS equivalent.
(In reply to Magnus Enger from comment #42) > (In reply to Katrin Fischer from comment #40) > > Why would one want different policies per patron category? I don't feel this > > level of granularity would be necessary here - I'd prefer a global setting. > > You could demand really difficult passwords for staff? This is what we do. No more 1234
However we need password 1234 for our borrowers so they can use their pin-code with the self-service automats.
(In reply to Jonathan Druart from comment #41) > (In reply to Olli-Antti Kivilahti from comment #38) > > Did you know that there is already a bug for this feature? > > How do you want us to know about it? > Status is "New", patches have been submitted 2 years ago and do no longer > applied, changes are not covered by tests, etc. Now you know. Shouldn't be a biggie to take the DB and GUI changes and rebase to work with this feature. There are way too many sysprefs already.
Hm, I can see the point in differentiating between staff and patrons would be nice. But worried to have this one stuck.
Created attachment 66621 [details] [review] Bug 18298: Enforce password complexity This patchset prevents users to enter too leak password, controlled by a new syspref RequireStrongPassword. If set the staff and patrons will have to enter a strong password. The strongness cannot be modified, it has been arbitrarily set (by the author of this enhancement) to at least 1 lowercase, 1 uppercase and 1 digit. This can be inforce by increasing the value of the existing minPasswordLength pref. I decided to turn this feature on, it cannot hurt! For existing installs it will have to be turned on manually. Writing these patches I found a lot of inconsistencies all around the password checks and decided to refactor everything to make things consistent and more robust. Now the password validity is check at only one place (subroutine covered by tests). Test plan: We have several places where a password can be change/created: a. Editing a patron (members/memberentry.pl) b. Changing the password of a patron (members/member-password.pl) c. Changing your own password at the opac (opac/opac-passwd.pl). OpacPasswordChange needs to be set d. Reseting your own password at the opac (opac/opac-password-recovery.pl). OpacResetPassword needs to be set, see "Forgot your password?" link when you are not logged in e. Self registration feature, PatronSelfRegistration needs to be set. You will also need to add 'password' to PatronSelfRegistrationBorrowerMandatoryField. Note that '****' is considered by Koha internally that the password is not changed (existing behavior). To fully test this patch you will need to test the different combinations of RequireStrongPassword and minPasswordLength. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66622 [details] [review] Bug 18298: Use the validate jQuery plugin To validate password fields we need to use the validate jQuery plugin. To make things reusable this patch adds a new include file 'password_check.inc' at the intranet and opac sides, it creates 3 new validation methods: - password_strong => make sure the passwords are strong enough according to the values of the RequireStrongPassword and minPasswordLength prefs - password_no_spaces => prevent passwords to be entered with leading or trailing spaces - password_match => make sure both password fields match Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66623 [details] [review] Bug 18298: Move password generation to template side This patch removes a really ugly way to generate a password: the whole template was sent and parsed to retrieve the "#defaultnewpassfield" node. To avoid the password to be sent plain text it is certainly better to generate it client-side. The same kind of passwords will be generated: 0-9a-zA-Z The while loop prevents to get an invalid generated password. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66624 [details] [review] Bug 18298: minPaswordLength should not be < 3 Indeed if RequireStrongPassword is set we need at least 3 characters to match 1 upper, 1 lower and 1 digit. We could make things more complicated to allow minPasswordLength < 3 but, really, 3 is already too low... Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66625 [details] [review] Bug 18298: minPasswordLength should not be used as the default password length The length of the passwords generated for a patron should not be as long as the value of minPasswordLength. It is the minimum required size of a password, not the maximum! So let's fix it to 8 if the minPasswordLength if < 8, that sounds reasonable and less risky for patrons. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 66626 [details] [review] Bug 18298: Add server-side checks and refactor stuffs Now that we have a check client-side, nothing prevents us from a smart guy to bypass it and force an invalid password. This patch adds two new subroutines to Koha::AuthUtils to check the validity of passwords and generate a password server-side. It is used only once (self-registration) but could be useful later. Moreover the 3 different cases of password rejection (too leak, too short, contains leading or trailing whitespaces) were not tested everywhere. Now they are! This patch makes things consistent everywhere and clean up some code. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 68096 [details] [review] Bug 18298: minPaswordLength should not be < 3 Indeed if RequireStrongPassword is set we need at least 3 characters to match 1 upper, 1 lower and 1 digit. We could make things more complicated to allow minPasswordLength < 3 but, really, 3 is already too low... Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 68097 [details] [review] Bug 18298: minPasswordLength should not be used as the default password length The length of the passwords generated for a patron should not be as long as the value of minPasswordLength. It is the minimum required size of a password, not the maximum! So let's fix it to 8 if the minPasswordLength if < 8, that sounds reasonable and less risky for patrons. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 68098 [details] [review] Bug 18298: Add server-side checks and refactor stuffs Now that we have a check client-side, nothing prevents us from a smart guy to bypass it and force an invalid password. This patch adds two new subroutines to Koha::AuthUtils to check the validity of passwords and generate a password server-side. It is used only once (self-registration) but could be useful later. Moreover the 3 different cases of password rejection (too leak, too short, contains leading or trailing whitespaces) were not tested everywhere. Now they are! This patch makes things consistent everywhere and clean up some code. Signed-off-by: Marc Véron <veron@veron.ch>
Very easy conflict with bug 18777 fixed.
Created attachment 68100 [details] [review] Bug 18298: Enforce password complexity This patchset prevents users to enter too leak password, controlled by a new syspref RequireStrongPassword. If set the staff and patrons will have to enter a strong password. The strongness cannot be modified, it has been arbitrarily set (by the author of this enhancement) to at least 1 lowercase, 1 uppercase and 1 digit. This can be inforce by increasing the value of the existing minPasswordLength pref. I decided to turn this feature on, it cannot hurt! For existing installs it will have to be turned on manually. Writing these patches I found a lot of inconsistencies all around the password checks and decided to refactor everything to make things consistent and more robust. Now the password validity is check at only one place (subroutine covered by tests). Test plan: We have several places where a password can be change/created: a. Editing a patron (members/memberentry.pl) b. Changing the password of a patron (members/member-password.pl) c. Changing your own password at the opac (opac/opac-passwd.pl). OpacPasswordChange needs to be set d. Reseting your own password at the opac (opac/opac-password-recovery.pl). OpacResetPassword needs to be set, see "Forgot your password?" link when you are not logged in e. Self registration feature, PatronSelfRegistration needs to be set. You will also need to add 'password' to PatronSelfRegistrationBorrowerMandatoryField. Note that '****' is considered by Koha internally that the password is not changed (existing behavior). To fully test this patch you will need to test the different combinations of RequireStrongPassword and minPasswordLength. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68101 [details] [review] Bug 18298: Use the validate jQuery plugin To validate password fields we need to use the validate jQuery plugin. To make things reusable this patch adds a new include file 'password_check.inc' at the intranet and opac sides, it creates 3 new validation methods: - password_strong => make sure the passwords are strong enough according to the values of the RequireStrongPassword and minPasswordLength prefs - password_no_spaces => prevent passwords to be entered with leading or trailing spaces - password_match => make sure both password fields match Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68102 [details] [review] Bug 18298: Move password generation to template side This patch removes a really ugly way to generate a password: the whole template was sent and parsed to retrieve the "#defaultnewpassfield" node. To avoid the password to be sent plain text it is certainly better to generate it client-side. The same kind of passwords will be generated: 0-9a-zA-Z The while loop prevents to get an invalid generated password. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68103 [details] [review] Bug 18298: minPaswordLength should not be < 3 Indeed if RequireStrongPassword is set we need at least 3 characters to match 1 upper, 1 lower and 1 digit. We could make things more complicated to allow minPasswordLength < 3 but, really, 3 is already too low... Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68104 [details] [review] Bug 18298: minPasswordLength should not be used as the default password length The length of the passwords generated for a patron should not be as long as the value of minPasswordLength. It is the minimum required size of a password, not the maximum! So let's fix it to 8 if the minPasswordLength if < 8, that sounds reasonable and less risky for patrons. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68105 [details] [review] Bug 18298: Add server-side checks and refactor stuffs Now that we have a check client-side, nothing prevents us from a smart guy to bypass it and force an invalid password. This patch adds two new subroutines to Koha::AuthUtils to check the validity of passwords and generate a password server-side. It is used only once (self-registration) but could be useful later. Moreover the 3 different cases of password rejection (too leak, too short, contains leading or trailing whitespaces) were not tested everywhere. Now they are! This patch makes things consistent everywhere and clean up some code. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68106 [details] [review] Bug 18298: (QA followup) Use Koha.Preference on the template Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Great job Jonathan!
Pushed to master for 17.11, thanks to everybody involved!
Created attachment 68220 [details] [review] Bug 18298: Fix selenium tests The password has to be better than "password"...
Last patch pushed to master