Bug 19514

Summary: No Password restrictions in onboarding tool patron creation
Product: Koha Reporter: Alex Buckley <alexbuckley>
Component: Installation and upgrade (web-based installer)Assignee: Alex Buckley <alexbuckley>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: major    
Priority: P3 CC: david.bourgault, fridolin.somers, gmcharlt, jonathan.druart, josef.moravec, m.de.rooy, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18308
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 18298    
Bug Blocks: 18308    
Attachments: Bug 19514 - Password restrictions implemented into onboarding tool
Bug 19514 - Password restrictions implemented into onboarding tool
Bug 19514: Add client-side check
Bug 19514 - Password restrictions implemented into onboarding tool
Bug 19514: Add client-side check
Bug 19514 - Password restrictions implemented into onboarding tool
Bug 19514: Add client-side check

Description Alex Buckley 2017-10-23 11:24:58 UTC
When a superlibrarian user account is created in the onboarding tool there is no checking of the password other than checking the two inputted password values are matching.

This means the password length, complexity and if it contains whitespace is not checked. This means users can enter in weak passwords causing a security vulnerability which is particularly bad in this case because a superlibrarian user is being created.
Comment 1 Alex Buckley 2017-10-23 11:36:23 UTC
Created attachment 68386 [details] [review]
Bug 19514 - Password restrictions implemented into onboarding tool

Test plan:
1. Drop and recreate your database
2. Restart memcached
3. Go through the web installer
4. In the onboarding tool create a patron with a password of only 2 characters in length
5. Notice the patron is successfully created and no warning message is
displayed
6. Repeat step 1,2,3 and create a patron with a password of 3 characters
none of which are a uppercase letter or number and notice the patron is
successfully created and no warning message is displayed
7. Apply patch
8. Repeat steps 1,2,3 and create a patron with a password consisting of
2 characters, notice that after submitting the form the same form is
loaded again and there is a warning message at the top of the page
informing you the patron wasn't created
9. Repeat steps 1,2,3 and create a patron with a password consisting of
3 characters (all lower case) and submit the form, notice the same form
is reloaded and a warning message at the top of the page informs you
that the patron wasn't created because the password was weak
10. Repeat steps 1,2,3 and create a patron with a password consisting of
3 characters (one lower case letter, one upper case letter and one
number) and submit the form and notice this time the next form in the onboarding is displayed with the message at the top of the screen informing you that the patron was successfully created

Sponsored-By: Catalyst IT
Comment 2 Marcel de Rooy 2017-10-23 11:57:30 UTC
Created attachment 68387 [details] [review]
Bug 19514 - Password restrictions implemented into onboarding tool

Test plan:
1. Drop and recreate your database
2. Restart memcached
3. Go through the web installer
4. In the onboarding tool create a patron with a password of only 2 characters in length
5. Notice the patron is successfully created and no warning message is
displayed
6. Repeat step 1,2,3 and create a patron with a password of 3 characters
none of which are a uppercase letter or number and notice the patron is
successfully created and no warning message is displayed
7. Apply patch
8. Repeat steps 1,2,3 and create a patron with a password consisting of
2 characters, notice that after submitting the form the same form is
loaded again and there is a warning message at the top of the page
informing you the patron wasn't created
9. Repeat steps 1,2,3 and create a patron with a password consisting of
3 characters (all lower case) and submit the form, notice the same form
is reloaded and a warning message at the top of the page informs you
that the patron wasn't created because the password was weak
10. Repeat steps 1,2,3 and create a patron with a password consisting of
3 characters (one lower case letter, one upper case letter and one
number) and submit the form and notice this time the next form in the onboarding is displayed with the message at the top of the screen informing you that the patron was successfully created

Sponsored-By: Catalyst IT

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2017-10-23 11:59:03 UTC
As discussed on IRC: Did you plan to change the setting of minimum password length in onboarding too on another patch ?
Comment 4 Alex Buckley 2017-10-23 19:32:13 UTC
Hi Marcel 

Yes I plan to have a patch attached to bug 18308 for setting the minimum password length in the onboarding tool
Comment 5 Jonathan Druart 2017-10-25 16:32:15 UTC
Alex, it works but there is no client-side validation

Take a look at
  commit 7cc65af6ffdabbabe7ae3463f51096de375216ad
    Bug 18298: Use the validate jQuery plugin

We want this form to behave like others.
Comment 6 Alex Buckley 2017-11-05 05:31:20 UTC
Hi Jonathan

I have tried implemented the client side validation by doing the following:

1. Adding the following pragma at the start of onboardingstep3.tt:
[% USE Koha %]  

2. Adding the following to the bottom of the same file:
[% PROCESS 'password_check.inc' %] 
[% PROCESS 'add_password_check' new_password => 'newpassword' %]


3. Added the following to the validation in the ../../../js/onboarding.js
            password: 
                required: true,
                password_strong: true
                password_no_spaces: true
            },?
            password2: {
                required: true,
                password_match: true
            }
. 
I notice that I get the JS error 'TypeError: _(...).format is not a function. This error is not thrown by the memberentrygen.tt file which uses the same client side validation.

Can you give me any suggestions as to how to fix this issue
Comment 7 Jonathan Druart 2017-11-06 15:23:23 UTC
Created attachment 68982 [details] [review]
Bug 19514: Add client-side check
Comment 8 David Bourgault 2017-11-17 21:21:27 UTC
Created attachment 69208 [details] [review]
Bug 19514 - Password restrictions implemented into onboarding tool

Test plan:
1. Drop and recreate your database
2. Restart memcached
3. Go through the web installer
4. In the onboarding tool create a patron with a password of only 2 characters in length
5. Notice the patron is successfully created and no warning message is
displayed
6. Repeat step 1,2,3 and create a patron with a password of 3 characters
none of which are a uppercase letter or number and notice the patron is
successfully created and no warning message is displayed
7. Apply patch
8. Repeat steps 1,2,3 and create a patron with a password consisting of
2 characters, notice that after submitting the form the same form is
loaded again and there is a warning message at the top of the page
informing you the patron wasn't created
9. Repeat steps 1,2,3 and create a patron with a password consisting of
3 characters (all lower case) and submit the form, notice the same form
is reloaded and a warning message at the top of the page informs you
that the patron wasn't created because the password was weak
10. Repeat steps 1,2,3 and create a patron with a password consisting of
3 characters (one lower case letter, one upper case letter and one
number) and submit the form and notice this time the next form in the onboarding is displayed with the message at the top of the screen informing you that the patron was successfully created

Sponsored-By: Catalyst IT

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 9 David Bourgault 2017-11-17 21:21:33 UTC
Created attachment 69209 [details] [review]
Bug 19514: Add client-side check

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 10 David Bourgault 2017-11-17 21:24:29 UTC
Tested working on Firefox 57, Chrome 62 and Opera 49.

Did not work on Firefox 58 from the mozillateam/firefox-next PPA (ubuntu).

Marking as signed-off anyway since Firefox 58 is considered beta, and this provides both server-side and client-side validation.

QA tool green.
Comment 11 Jonathan Druart 2017-11-26 17:02:48 UTC
Lowering severity, this does not appear critical to me.
Comment 12 Josef Moravec 2017-12-14 10:38:42 UTC
Created attachment 69796 [details] [review]
Bug 19514 - Password restrictions implemented into onboarding tool

Test plan:
1. Drop and recreate your database
2. Restart memcached
3. Go through the web installer
4. In the onboarding tool create a patron with a password of only 2 characters in length
5. Notice the patron is successfully created and no warning message is
displayed
6. Repeat step 1,2,3 and create a patron with a password of 3 characters
none of which are a uppercase letter or number and notice the patron is
successfully created and no warning message is displayed
7. Apply patch
8. Repeat steps 1,2,3 and create a patron with a password consisting of
2 characters, notice that after submitting the form the same form is
loaded again and there is a warning message at the top of the page
informing you the patron wasn't created
9. Repeat steps 1,2,3 and create a patron with a password consisting of
3 characters (all lower case) and submit the form, notice the same form
is reloaded and a warning message at the top of the page informs you
that the patron wasn't created because the password was weak
10. Repeat steps 1,2,3 and create a patron with a password consisting of
3 characters (one lower case letter, one upper case letter and one
number) and submit the form and notice this time the next form in the onboarding is displayed with the message at the top of the screen informing you that the patron was successfully created

Sponsored-By: Catalyst IT

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2017-12-14 10:38:45 UTC
Created attachment 69797 [details] [review]
Bug 19514: Add client-side check

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Jonathan Druart 2017-12-14 20:16:54 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 15 Nick Clemens (kidclamp) 2018-01-15 13:18:13 UTC
Awesome work all! Pushed to stable for 17.11.02
Comment 16 Fridolin Somers 2018-01-15 15:29:50 UTC
Pushed to 17.05.x for v17.05.08
Comment 17 Fridolin Somers 2018-01-16 14:24:25 UTC
Ouch depends on Bug 18298
Comment 18 Fridolin Somers 2018-01-16 14:25:13 UTC
(In reply to Fridolin SOMERS from comment #16)
> Pushed to 17.05.x for v17.05.08

I choose to revert since depends on new 17.11 feature Bug 18298