Bug 18298

Summary: Allow enforcing password complexity
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: new feature    
Priority: P5 - low CC: abl, alex, gmcharlt, jdemuth, josef.moravec, katrin.fischer, kyle.m.hall, m.de.rooy, magnus, martin.renvoize, olli-antti.kivilahti, tomascohen, veron
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13664
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18308
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18314
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17006
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12617
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9920
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Add the option to enforce a strong password policy. That policy would mean that passwords should include 1 lowercase, 1 uppercase and 1 digit. This option is turned on for new installations.
Version(s) released in:
Bug Depends on:    
Bug Blocks: 21314, 21976, 18424, 19514, 19845, 19908    
Attachments: Bug 18298: Enforce password complexity
Bug 18298: Use the validate jQuery plugin
Bug 18298: Move password generation to template side
Bug 18298: minPaswordLength should not be < 3
Bug 18298: minPasswordLength should not be used as the default password length
Bug 18298: Add server-side checks and refactor stuffs
Screenshots of passwort hints on OPAC self registration
Bug 18298: Enforce password complexity
Bug 18298: Use the validate jQuery plugin
Bug 18298: Move password generation to template side
Bug 18298: minPaswordLength should not be < 3
Bug 18298: minPasswordLength should not be used as the default password length
Bug 18298: Add server-side checks and refactor stuffs
Bug 18298: Enforce password complexity
Bug 18298: Use the validate jQuery plugin
Bug 18298: Move password generation to template side
Bug 18298: minPaswordLength should not be < 3
Bug 18298: minPasswordLength should not be used as the default password length
Bug 18298: Add server-side checks and refactor stuffs
Bug 18298: Enforce password complexity
Bug 18298: Use the validate jQuery plugin
Bug 18298: Move password generation to template side
Bug 18298: minPaswordLength should not be < 3
Bug 18298: minPasswordLength should not be used as the default password length
Bug 18298: Add server-side checks and refactor stuffs
Bug 18298: minPaswordLength should not be < 3
Bug 18298: minPasswordLength should not be used as the default password length
Bug 18298: Add server-side checks and refactor stuffs
Bug 18298: Enforce password complexity
Bug 18298: Use the validate jQuery plugin
Bug 18298: Move password generation to template side
Bug 18298: minPaswordLength should not be < 3
Bug 18298: minPasswordLength should not be used as the default password length
Bug 18298: Add server-side checks and refactor stuffs
Bug 18298: (QA followup) Use Koha.Preference on the template
Bug 18298: Fix selenium tests

Description Jonathan Druart 2017-03-17 18:32:41 UTC
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.
Comment 1 Jonathan Druart 2017-03-17 18:51:12 UTC
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.
Comment 2 Jonathan Druart 2017-03-17 18:51:20 UTC
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
Comment 3 Jonathan Druart 2017-03-17 18:51:26 UTC
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.
Comment 4 Jonathan Druart 2017-03-17 18:51:32 UTC
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...
Comment 5 Jonathan Druart 2017-03-17 18:51:37 UTC
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.
Comment 6 Jonathan Druart 2017-03-17 18:51:43 UTC
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.
Comment 7 Marc Véron 2017-03-18 19:34:55 UTC
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.
Comment 8 Magnus Enger 2017-03-20 13:45:51 UTC
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?
Comment 9 Jonathan Druart 2017-03-20 13:57:58 UTC
(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.
Comment 10 Magnus Enger 2017-03-20 13:59:18 UTC
(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!
Comment 11 Jonathan Druart 2017-03-20 14:54:14 UTC
(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.
Comment 12 Marc Véron 2017-03-20 17:16:50 UTC
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>
Comment 13 Marc Véron 2017-03-20 17:17:41 UTC
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>
Comment 14 Marc Véron 2017-03-20 17:19:02 UTC
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>
Comment 15 Marc Véron 2017-03-20 17:20:07 UTC
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>
Comment 16 Marc Véron 2017-03-20 17:21:03 UTC
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>
Comment 17 Marc Véron 2017-03-20 17:21:58 UTC
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>
Comment 18 Katrin Fischer 2017-03-20 20:42:00 UTC
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?
Comment 19 Marcel de Rooy 2017-03-21 12:58:34 UTC
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?
Comment 20 Jonathan Druart 2017-03-21 13:15:46 UTC
(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.
Comment 21 Jonathan Druart 2017-03-21 13:17:22 UTC
(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.
Comment 22 Marcel de Rooy 2017-03-21 13:34:18 UTC
(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
Comment 23 Marcel de Rooy 2017-03-21 13:35:36 UTC
(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.
Comment 24 Jonathan Druart 2017-03-21 13:43:59 UTC
(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."
Comment 25 Marcel de Rooy 2017-03-21 13:56:20 UTC
(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.
Comment 26 Katrin Fischer 2017-03-22 07:20:17 UTC
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.
Comment 27 Marc Véron 2017-03-22 09:53:31 UTC
(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
Comment 28 Marcel de Rooy 2017-03-22 11:58:54 UTC
(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 ;)
Comment 29 Martin Renvoize 2017-03-30 09:02:59 UTC
> 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?
Comment 30 Jonathan Druart 2017-03-30 15:36:39 UTC
(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.
Comment 31 Alex Sassmannshausen 2017-05-02 14:53:21 UTC
(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
Comment 32 Jonathan Druart 2017-05-02 15:17:20 UTC
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>
Comment 33 Jonathan Druart 2017-05-02 15:17:26 UTC
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>
Comment 34 Jonathan Druart 2017-05-02 15:17:31 UTC
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>
Comment 35 Jonathan Druart 2017-05-02 15:17:35 UTC
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>
Comment 36 Jonathan Druart 2017-05-02 15:17:40 UTC
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>
Comment 37 Jonathan Druart 2017-05-02 15:17:45 UTC
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>
Comment 38 Olli-Antti Kivilahti 2017-06-13 11:42:41 UTC
Did you know that there is already a bug for this feature?

Bug 12617

And it allows configuring the password type per borrower category.
Comment 39 Olli-Antti Kivilahti 2017-06-13 12:00:31 UTC
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.
Comment 40 Katrin Fischer 2017-06-13 13:52:38 UTC
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.
Comment 41 Jonathan Druart 2017-06-13 14:32:34 UTC
(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.
Comment 42 Magnus Enger 2017-06-13 19:30:29 UTC
(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?
Comment 43 Jacek Ablewicz 2017-06-14 11:21:54 UTC
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?
Comment 44 Jonathan Druart 2017-06-14 14:07:02 UTC
(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.
Comment 45 Olli-Antti Kivilahti 2017-06-20 12:44:56 UTC
(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
Comment 46 Olli-Antti Kivilahti 2017-06-20 12:45:38 UTC
However we need password 1234 for our borrowers so they can use their pin-code with the self-service automats.
Comment 47 Olli-Antti Kivilahti 2017-06-20 12:46:55 UTC
(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.
Comment 48 Katrin Fischer 2017-07-09 17:43:13 UTC
Hm, I can see the point in differentiating between staff and patrons would be nice. But worried to have this one stuck.
Comment 49 Jonathan Druart 2017-08-30 15:29:15 UTC
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>
Comment 50 Jonathan Druart 2017-08-30 15:29:20 UTC
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>
Comment 51 Jonathan Druart 2017-08-30 15:29:25 UTC
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>
Comment 52 Jonathan Druart 2017-08-30 15:29:30 UTC
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>
Comment 53 Jonathan Druart 2017-08-30 15:29:34 UTC
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>
Comment 54 Jonathan Druart 2017-08-30 15:29:39 UTC
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>
Comment 55 Jonathan Druart 2017-10-13 19:19:03 UTC
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>
Comment 56 Jonathan Druart 2017-10-13 19:19:09 UTC
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>
Comment 57 Jonathan Druart 2017-10-13 19:19:15 UTC
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>
Comment 58 Jonathan Druart 2017-10-13 19:19:59 UTC
Very easy conflict with bug 18777 fixed.
Comment 59 Tomás Cohen Arazi 2017-10-13 19:56:27 UTC
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>
Comment 60 Tomás Cohen Arazi 2017-10-13 19:56:36 UTC
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>
Comment 61 Tomás Cohen Arazi 2017-10-13 19:56:44 UTC
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>
Comment 62 Tomás Cohen Arazi 2017-10-13 19:56:51 UTC
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>
Comment 63 Tomás Cohen Arazi 2017-10-13 19:56:58 UTC
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>
Comment 64 Tomás Cohen Arazi 2017-10-13 19:57:06 UTC
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>
Comment 65 Tomás Cohen Arazi 2017-10-13 19:57:12 UTC
Created attachment 68106 [details] [review]
Bug 18298: (QA followup) Use Koha.Preference on the template

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 66 Tomás Cohen Arazi 2017-10-13 19:57:43 UTC
Great job Jonathan!
Comment 67 Jonathan Druart 2017-10-16 13:01:10 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 68 Jonathan Druart 2017-10-17 15:42:08 UTC
Created attachment 68220 [details] [review]
Bug 18298: Fix selenium tests

The password has to be better than "password"...
Comment 69 Jonathan Druart 2017-10-17 15:44:16 UTC
Last patch pushed to master