Description
Jonathan Druart
2017-03-21 13:18:17 UTC
Definitely +5 :) Created attachment 68374 [details] [review] Bug 18308 - Increased default value of minPasswordValue Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Ensuring PatronSelfRegistration syspref is enabled, and PatronSelfRegistationDefaultCategory is set register a patron in the OPAC with a password of 2 characters and notice that a red message is displayed informing you that the password must be at least 3 characters 3. Run updatedatabase.pl 4. Repeat step 1 and notice that the password must be 8 characters or more now 5. Repeat step 2 and notice that the password must be 8 characters or more now Sponsored-By: Catalyst IT How about new installations ? Created attachment 68381 [details] [review] Bug 18308 - Followup adding default value to syspref.sql Sponsored-By: Catalyst IT Oops sorry Marcel, I forgot about new installs. I have attached a followup which has the changed minPasswordLength default value set to 8 in syspref.sql and so it should be run in the installation process From IRC: marcelr alexbuckley: thx for followup on 18308; only problem would be now: do all libraries really want this? devs and admins do, but users.. [11:59] alexbuckley hmm it could be included in the onboarding tool so when Koha is being installed the value can be set but have it set to 3 by default in sysprefs.sql? [12:07] alexbuckley what do you think of that for a solution marcelr? [12:08] marcelr i strongly think that it should be 8, but i know there will be resistence [12:08] marcelr onboarding could be a good compromise [12:09] marcelr can we set it to 8 and optionally lower it in onboarding ? [12:09] alexbuckley yeah thats a good idea [12:10] marcelr yes with a little security advertisement I do not think we should update existing installations. However we can raise a warning if < X. *** Bug 9920 has been marked as a duplicate of this bug. *** Created attachment 68451 [details] [review] Bug 18308 - Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Sponsored-By: Catalyst IT Hi Jonathan and Marcel Have a look at this patch I have just attached it adds a new step to the onboarding tool to optionally alter the minPasswordLength (with security warning which can be made JS warning if you want me to, at the moment it is HTML) and let me know what you think. I have removed the atomicupate for altering the default value for existing installations as per Jonathan's comment, and in thiss patch I am setting the default value for new installations to 8 as per Marcel and my irc discussion. One thing I haven't done yet is raise a warning for existing installations if the minPasswordLength value is < X Hi Alex Please add the warning. And I would suggest to rebase this one on top of 19514. This currently conflicts. Marcel Created attachment 69318 [details] [review] Bug 18308 - Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Sponsored-By: Catalyst IT Created attachment 69319 [details] [review] Bug 18308 - Added warning and dialog alert If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Created attachment 69320 [details] [review] Bug 18308 - Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Sponsored-By: Catalyst IT Created attachment 69321 [details] [review] Bug 18308 - Added warning and dialog alert If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Created attachment 69323 [details] [review] Bug 18308 - Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Created attachment 69324 [details] [review] Bug 18308 - Added warning and dialog alert If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT patch failed to install: Applying: Bug 18308 - Added step to onboarding tool to set minPasswordLength fatal: sha1 information is lacking or useless (installer/data/mysql/sysprefs.sql). error: could not build fake ancestor Patch failed at 0001 Bug 18308 - Added step to onboarding tool to set minPasswordLength Hi Dominic thanks for testing the patch, can you please try again and apply bug 19514 first. Created attachment 69459 [details] [review] Bug 18308 - Added warning and dialog alert Works great if you apply bug 19514 before If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Signed-off-by: Dominic Pichette <dominic@inlibro.com> Created attachment 69461 [details] [review] Bug 18308 - Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Created attachment 69462 [details] [review] Bug 18308 - Added warning and dialog alert Works great if you apply bug 19514 before If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Signed-off-by: Dominic Pichette <dominic@inlibro.com> Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Comment on attachment 69461 [details] [review] Bug 18308 - Added step to onboarding tool to set minPasswordLength Review of attachment 69461 [details] [review]: ----------------------------------------------------------------- I would like to see the setting of min password length before creating the superlibrarian user account, because now you first use the limit a then set the limit - it is not very logical IMO ::: installer/onboarding.pl @@ +313,1 @@ > $template_params->{all_done} = 1; # If step 5 is complete, we are done! The comment should be changed too, 5 => 6 ::: koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep6.tt @@ +2,5 @@ > +<title>Koha › Web installer › > + [% IF all_done %] > + Complete > + [% ELSE %] > + Create circulation rule Should be "Set minPasswordLength system preference" @@ +8,5 @@ > +</title> > +[% INCLUDE 'installer-doc-head-close.inc' %] > +[% INCLUDE 'validator-strings.inc' %] > +[% INCLUDE 'installer-strings.inc' %] > +<script type="text/javascript" src="[% interface %]/[% theme %]/js/onboarding.js"></script> Javascript should be at the end - see bug 17418 @@ +14,5 @@ > + > +<body id="installer" class="installer"> > + <div class="container-fluid"> > + <div class="row"> > + <div id="onboarding-step5" class="installer-main col-sm-8 col-sm-offset-2 col-md-8 col-md-offset-2"> This is actually step6 Created attachment 87124 [details] [review] Bug 18308: (follow up) Changed order of onboarding steps This patch cleans up the previous patches according to comment 23, fixes various typos, and changes the order of steps in the onboarding so that the setting of minPasswordLength occurs before the admin password is set. Test plan: 1) Drop and recreate your database, and restart_all 2) Go through installation and onboarding 3) Note that the order of steps is: 1 Create a library 2 Create a ptron category 3 Create Koha administrator patron 4 Create a new item type 5 Create a new circulation rule 6 Set minPasswordLength syspref/Complete 4) Repeat steps 1 & 2 5) Note that the order of steps is now: 1 Create a library 2 Create a patron category 3 Set minPasswordLength syspref 4 Create Koha administrator patron 5 Create a new item type 6 Create a new circulation rule/Complete 6) Verify that the minPasswordLength syspref is set to the value from onboarding 7) Verify that the patron, item type, and circulation rule made in the onboarding exist in the administration 8) Sign off! Sponsored-by Catalyst IT Created attachment 87125 [details] [review] Bug 18308: Fixed whitespace for qa tool Note: This will still fail qa tool due to 'forbidden' javascript fullpath and not use of Asset plugin. Asset plugin cannot be used for onboarding currently. Sponsored-by: Catalyst IT Created attachment 87126 [details] [review] Bug 18308: Added missing filters Sponsored-by: Catalyst IT Created attachment 87128 [details] [review] Bug 18308: Added another filter Sponsored-by: Catalyst IT Created attachment 87129 [details] [review] Bug 18308: Fixed whitespace and added filters for qa tools Note: This will still fail qa tool due to 'forbidden' javascript fullpath and not use of Asset plugin. Asset plugin cannot be used for onboarding currently. Sponsored-by: Catalyst IT Created attachment 87339 [details] [review] Bug 18308 - Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Signed-off-by: David Bourgault <david.bourgault@inlibro.com> https://bugs.koha-community.org/show_bug.cgi?id=6473 Hayley Mapley: Fixed merge conflicts Created attachment 87340 [details] [review] Bug 18308 - Added warning and dialog alert Works great if you apply bug 19514 before If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Signed-off-by: Dominic Pichette <dominic@inlibro.com> Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Tried to integrate this with the other onboarding related patches. Had to rebase and reattach the patches. Needs to be tested once again. Obsoleted patches made in changing order of steps due to merge conflicts and rebasing issues. Will continue working on this patch to ensure it can be used as a base for the other onboarding patches. Created attachment 87564 [details] [review] Bug 18308: Fixed onboarding_messages.inc by changing order of statements Sponsored-by: catalyst IT Created attachment 87572 [details] [review] Bug 18308: Fixed issues with onboardingstep6.tt Page did not load due to incorrect include and misplaced javascript tag and includes. Sponsored-by: Catalyst IT Created attachment 87573 [details] [review] Bug 18308: Changed ordering of onboarding steps This patch cleans up the previous patches according to comment 23, fixes various typos, and changes the order of steps in the onboarding so that the setting of minPasswordLength occurs before the admin password is set. Test plan: 1) Drop and recreate your database, and restart_all 2) Go through installation and onboarding 3) Note that the order of steps is: 1 Create a library 2 Create a ptron category 3 Create Koha administrator patron 4 Create a new item type 5 Create a new circulation rule 6 Set minPasswordLength syspref/Complete 4) Repeat steps 1 & 2 5) Note that the order of steps is now: 1 Create a library 2 Create a patron category 3 Set minPasswordLength syspref 4 Create Koha administrator patron 5 Create a new item type 6 Create a new circulation rule/Complete 6) Verify that the minPasswordLength syspref is set to the value from onboarding 7) Verify that the patron, item type, and circulation rule made in the onboarding exist in the administration 8) Sign off! Sponsored-by Catalyst IT Created attachment 87747 [details] [review] Bug 18308: Added missing onboardingstep8.tt Sponsered-by: Catalyt IT *** Bug 22625 has been marked as a duplicate of this bug. *** Hi Hayley, can you please fix a few things here? FAIL koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep6.tt OK filters FAIL forbidden patterns forbidden pattern: Do *not* include full path to js files, use the Asset TT plugin (bug 20538) (line 137) OK git manipulation OK js_in_body OK spelling OK tt_valid OK valid_template FAIL koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep8.tt OK filters FAIL forbidden patterns forbidden pattern: Do *not* include full path to js files, use the Asset TT plugin (bug 20538) (line 137) OK git manipulation OK js_in_body OK spelling OK tt_valid OK valid_template FAIL koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt FAIL filters missing_filter at line 136 ( <p><strong> Warning: </strong> [% passwordwarning %]</p>) OK forbidden patterns OK git manipulation OK js_in_body OK spelling OK tt_valid OK valid_template If you are using kohadevbox you can run the qa test tools from koha-shell: qa -c 6 -v 2 There is also a complaint about the commit messages: Processing additional checks * Commit title does not start with 'Bug XXXXX: ' - df4b236511 * Commit title does not start with 'Bug XXXXX: ' - a1946b6a31 This can be fixed easiest with: git rebase -i HEAD~6 and then rewording the 2 at the beginning. Let me know, if I can help more. (In reply to Katrin Fischer from comment #38) > Hi Hayley, > > can you please fix a few things here? > > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep6.tt > OK filters > FAIL forbidden patterns > forbidden pattern: Do *not* include full path to js files, use the Asset > TT plugin (bug 20538) (line 137) > OK git manipulation > OK js_in_body > OK spelling > OK tt_valid > OK valid_template > > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep8.tt > OK filters > FAIL forbidden patterns > forbidden pattern: Do *not* include full path to js files, use the Asset > TT plugin (bug 20538) (line 137) > OK git manipulation > OK js_in_body > OK spelling > OK tt_valid > OK valid_template > > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt > FAIL filters > missing_filter at line 136 ( <p><strong> Warning: </strong> [% > passwordwarning %]</p>) > OK forbidden patterns > OK git manipulation > OK js_in_body > OK spelling > OK tt_valid > OK valid_template > > If you are using kohadevbox you can run the qa test tools from koha-shell: > qa -c 6 -v 2 > > There is also a complaint about the commit messages: > > Processing additional checks > > * Commit title does not start with 'Bug XXXXX: ' - df4b236511 > * Commit title does not start with 'Bug XXXXX: ' - a1946b6a31 > > This can be fixed easiest with: git rebase -i HEAD~6 and then rewording the > 2 at the beginning. > > Let me know, if I can help more. Hi Katrin, The missing filter I can definitely add. I normally do run the QA tools before attaching, I must have missed it this time though! Thanks for picking it up. The Asset plugin issue is trickier because it does not work with onboarding due to how it retrieves Koha version from the database (or tries to before the database is created). There is a patch that will solve this and allow for Asset to be used in onboarding Bug 22451, but it is not yet pushed to Master, so the full path js is all that can be done for now I think. I will reattach once I have reworded the commits and added the missing filter. Created attachment 87959 [details] [review] Bug 18308: Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Signed-off-by: David Bourgault <david.bourgault@inlibro.com> https://bugs.koha-community.org/show_bug.cgi?id=6473 Created attachment 87960 [details] [review] Bug 18308: Added warning and dialog alert If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Signed-off-by: Dominic Pichette <dominic@inlibro.com> Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Created attachment 87961 [details] [review] Bug 18308: Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Signed-off-by: David Bourgault <david.bourgault@inlibro.com> https://bugs.koha-community.org/show_bug.cgi?id=6473 Created attachment 87962 [details] [review] Bug 18308: Added warning and dialog alert If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Signed-off-by: Dominic Pichette <dominic@inlibro.com> Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Created attachment 87963 [details] [review] Bug 18308: Fixed issues with onboarding_messages.inc and onboardingstep6.tt Sponsored-by: Catalyst IT Created attachment 87964 [details] [review] Bug 18308: Changed ordering of onboarding steps This patch cleans up the previous patches according to comment 23, fixes various typos, and changes the order of steps in the onboarding so that the setting of minPasswordLength occurs before the admin password is set. Test plan: 1) Drop and recreate your database, and restart_all 2) Go through installation and onboarding 3) Note that the order of steps is: 1 Create a library 2 Create a ptron category 3 Create Koha administrator patron 4 Create a new item type 5 Create a new circulation rule 6 Set minPasswordLength syspref/Complete 4) Repeat steps 1 & 2 5) Note that the order of steps is now: 1 Create a library 2 Create a patron category 3 Set minPasswordLength syspref 4 Create Koha administrator patron 5 Create a new item type 6 Create a new circulation rule/Complete 6) Verify that the minPasswordLength syspref is set to the value from onboarding 7) Verify that the patron, item type, and circulation rule made in the onboarding exist in the administration 8) Sign off! Sponsored-by Catalyst IT Created attachment 87966 [details] [review] Bug 18308: Added missing filter in memberentrygen.tt Test plan: Apply this patch and check that everything still works. Sponsored-by: Catalyst IT Created attachment 88010 [details] [review] Bug 18308: Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Signed-off-by: David Bourgault <david.bourgault@inlibro.com> https://bugs.koha-community.org/show_bug.cgi?id=6473 Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88011 [details] [review] Bug 18308: Added warning and dialog alert If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Signed-off-by: Dominic Pichette <dominic@inlibro.com> Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88012 [details] [review] Bug 18308: Fixed issues with onboarding_messages.inc and onboardingstep6.tt Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88013 [details] [review] Bug 18308: Changed ordering of onboarding steps This patch cleans up the previous patches according to comment 23, fixes various typos, and changes the order of steps in the onboarding so that the setting of minPasswordLength occurs before the admin password is set. Test plan: 1) Drop and recreate your database, and restart_all 2) Go through installation and onboarding 3) Note that the order of steps is: 1 Create a library 2 Create a ptron category 3 Create Koha administrator patron 4 Create a new item type 5 Create a new circulation rule 6 Set minPasswordLength syspref/Complete 4) Repeat steps 1 & 2 5) Note that the order of steps is now: 1 Create a library 2 Create a patron category 3 Set minPasswordLength syspref 4 Create Koha administrator patron 5 Create a new item type 6 Create a new circulation rule/Complete 6) Verify that the minPasswordLength syspref is set to the value from onboarding 7) Verify that the patron, item type, and circulation rule made in the onboarding exist in the administration 8) Sign off! Sponsored-by Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88014 [details] [review] Bug 18308: Added missing filter in memberentrygen.tt Test plan: Apply this patch and check that everything still works. Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Hi Hayley, can you please rebase? I think the conflict might have been caused by bug 22451 being pushed this week. (In reply to Katrin Fischer from comment #52) > Hi Hayley, can you please rebase? I think the conflict might have been > caused by bug 22451 being pushed this week. So now it is possible to use Asset in installer and onboarding templates ;) Created attachment 88242 [details] [review] Bug 18308: Changed templates to use Asset plugin rather than full path Test plan: Same as before Sponsored-by: Catalyst IT Created attachment 88243 [details] [review] Bug 18308: Removed extraneous file Sponsored-by: Catalyst IT Test plan: 0) Apply all patches 1) Drop and recreate your database, and restart_all 2) Go through installation and onboarding 3) Note that the order of steps is: 1 Create a library 2 Create a ptron category 3 Create Koha administrator patron 4 Create a new item type 5 Create a new circulation rule 6 Set minPasswordLength syspref/Complete 4) Repeat steps 1 & 2 5) Note that the order of steps is now: 1 Create a library 2 Create a patron category 3 Set minPasswordLength syspref 4 Create Koha administrator patron 5 Create a new item type 6 Create a new circulation rule/Complete 6) Verify that the minPasswordLength syspref is set to the value from onboarding 7) Verify that the patron, item type, and circulation rule made in the onboarding exist in the administration 8) Sign off! Created attachment 88306 [details] [review] Bug 18308: Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Signed-off-by: David Bourgault <david.bourgault@inlibro.com> https://bugs.koha-community.org/show_bug.cgi?id=6473 Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88307 [details] [review] Bug 18308: Added warning and dialog alert If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Signed-off-by: Dominic Pichette <dominic@inlibro.com> Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88308 [details] [review] Bug 18308: Fixed issues with onboarding_messages.inc and onboardingstep6.tt Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88309 [details] [review] Bug 18308: Changed ordering of onboarding steps This patch cleans up the previous patches according to comment 23, fixes various typos, and changes the order of steps in the onboarding so that the setting of minPasswordLength occurs before the admin password is set. Test plan: 1) Drop and recreate your database, and restart_all 2) Go through installation and onboarding 3) Note that the order of steps is: 1 Create a library 2 Create a ptron category 3 Create Koha administrator patron 4 Create a new item type 5 Create a new circulation rule 6 Set minPasswordLength syspref/Complete 4) Repeat steps 1 & 2 5) Note that the order of steps is now: 1 Create a library 2 Create a patron category 3 Set minPasswordLength syspref 4 Create Koha administrator patron 5 Create a new item type 6 Create a new circulation rule/Complete 6) Verify that the minPasswordLength syspref is set to the value from onboarding 7) Verify that the patron, item type, and circulation rule made in the onboarding exist in the administration 8) Sign off! Sponsored-by Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88310 [details] [review] Bug 18308: Added missing filter in memberentrygen.tt Test plan: Apply this patch and check that everything still works. Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88311 [details] [review] Bug 18308: Changed templates to use Asset plugin rather than full path Test plan: Same as before Sponsored-by: Catalyst IT Created attachment 88312 [details] [review] Bug 18308: Removed extraneous file Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88313 [details] [review] Bug 18308: (QA follow up) Resolving merge conflict Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88314 [details] [review] Bug 18308: Added step to onboarding tool to set minPasswordLength This patch increases the default minPasswordLength syspref value to 8 characters, for new installations. The final (6th step) of the onboarding tool is now a password length page where the default value of 8 can be altered. There is a security warning in red recommending to the user they keep the minimum length of the password at 8 characters or more. This patch also removes the atomicupdate .sql file to be run in an update to alter the default minPasswordLength value for existing Koha installations, based on tester feedback. Test plan: 1. Create a patron with a password less than 3 characters in length and notice that a red message is displayed by the input telling you that the password must be 3 characters minimum length 2. Query the database "select value from systempreferences where variable="minPasswordLength"; and notice the value is 3 3. Drop and recreate your database and restart memcached 4. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to create a circulation rule 5. Apply patch 6. Repeat step 3 7. Go through the web installer and onboarding tool. Noticing the last step of the onboarding tool is to set the minimum password length, the numerical input element has a default value of 8 and notice that it will go below 3 8. Repeat step 2 and notice the value is 8 9. In the onboarding tool change the minimum password length value to 7 and submit the form 10. Notice the completed page of the onboarding tool is displayed with the message that the minimum password length has been set. 11. Repeat step 2 and notice the value is now 7. 12. Try to create a patron in Intranet and OPAC with a password less than 7 characters and notice that a red message is displayed by the input telling you that the password must be 7 characters minimum length Sponsored-By: Catalyst IT Signed-off-by: David Bourgault <david.bourgault@inlibro.com> https://bugs.koha-community.org/show_bug.cgi?id=6473 Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88315 [details] [review] Bug 18308: Added warning and dialog alert If an institution has a minPasswordLength which is < 8 characters then a warning will be set (which is visible in the intranet-error.log) and a dialog alert div will display the warning to the user in the memberentry.pl page in the intranet. Test plan: 1. Ensure that your minPasswordLength is set to less than 8 characters. This replicates the use case that a Koha instance is existing (i.e. not a new Koha instance) and not gone through the password length step of the onboarding tool (previous patch on this bug report) or that the institituion is new but set a password length of less than 8 in the onboarding tool 2. Go to the memberentry.pl page and notice that there is no warning at the top of the page informing you that your minPasswordLength is less than 8 3. Apply this patch 4. Refresh the memberentry.pl page and notice that there is a yellow warning at the top of the page informing you that your minPasswordLength syspref is less than 8 Sponsored-By: Catalyst IT Signed-off-by: Dominic Pichette <dominic@inlibro.com> Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88316 [details] [review] Bug 18308: Fixed issues with onboarding_messages.inc and onboardingstep6.tt Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88317 [details] [review] Bug 18308: Changed ordering of onboarding steps This patch cleans up the previous patches according to comment 23, fixes various typos, and changes the order of steps in the onboarding so that the setting of minPasswordLength occurs before the admin password is set. Test plan: 1) Drop and recreate your database, and restart_all 2) Go through installation and onboarding 3) Note that the order of steps is: 1 Create a library 2 Create a ptron category 3 Create Koha administrator patron 4 Create a new item type 5 Create a new circulation rule 6 Set minPasswordLength syspref/Complete 4) Repeat steps 1 & 2 5) Note that the order of steps is now: 1 Create a library 2 Create a patron category 3 Set minPasswordLength syspref 4 Create Koha administrator patron 5 Create a new item type 6 Create a new circulation rule/Complete 6) Verify that the minPasswordLength syspref is set to the value from onboarding 7) Verify that the patron, item type, and circulation rule made in the onboarding exist in the administration 8) Sign off! Sponsored-by Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88318 [details] [review] Bug 18308: Added missing filter in memberentrygen.tt Test plan: Apply this patch and check that everything still works. Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88319 [details] [review] Bug 18308: Changed templates to use Asset plugin rather than full path Test plan: Same as before Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88320 [details] [review] Bug 18308: Removed extraneous file Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 88321 [details] [review] Bug 18308: (QA follow up) Resolving merge conflict Signed-off-by: Liz Rea <wizzyrea@gmail.com> Comment on attachment 88314 [details] [review] Bug 18308: Added step to onboarding tool to set minPasswordLength Review of attachment 88314 [details] [review]: ----------------------------------------------------------------- ::: installer/onboarding.pl @@ +292,5 @@ > > +if ( $step == 6 ) { > + if ( $op eq 'set_validate_min_password_length' ) { > + my $minPasswordLength = $input->param('pref_minPasswordLength'); > + warn $minPasswordLength; This should be removed. Comment on attachment 88315 [details] [review] Bug 18308: Added warning and dialog alert Review of attachment 88315 [details] [review]: ----------------------------------------------------------------- ::: members/memberentry.pl @@ +174,5 @@ > +#Check passwords > +my $minPasswordLength = C4::Context->preference('minPasswordLength'); > +warn $minPasswordLength; > +if ($minPasswordLength < 8) { > + warn my $warning = "Your minPasswordLength system preference is less than 8 characters this means library patrons can potentially set passwords which are vulnerable to cracking. Please set your minPasswordLength syspref to 8 or more characters"; This is not translatable and should not be warn'ed. Remove the warns and move the string from the perl script to the template and this should be good to go! Created attachment 88840 [details] [review] Bug 18308: Removed warns and moved password length warning to template Sponsored-by: Catalyst IT Created attachment 90079 [details] [review] Bug 18308: Removed warns and moved password length warning to template Sponsored-by: Catalyst IT Signed-off-by: Liz Rea <wizzyrea@gmail.com> 1. Only one occurrence of passwordlengthhint koha-tmpl/intranet-tmpl/prog/css/installer.css:#passwordlengthhint 2. Do we really need the warning when adding/editing a patron? BTW, $warning is not needed, memberentry.pl 177 my $warning = 1; 178 $template->param("passwordwarning"=>$warning); 3. Just a though, next time we add a "step" we will need to rethink the template. The shift is terrible. And hard to see the real changes. Not passing QA for 2, waiting for another QA point of view. (In reply to Jonathan Druart from comment #78) > 1. Only one occurrence of passwordlengthhint > koha-tmpl/intranet-tmpl/prog/css/installer.css:#passwordlengthhint > > 2. Do we really need the warning when adding/editing a patron? > BTW, $warning is not needed, memberentry.pl > 177 my $warning = 1; > 178 $template->param("passwordwarning"=>$warning); > > 3. Just a though, next time we add a "step" we will need to rethink the > template. The shift is terrible. And hard to see the real changes. > > Not passing QA for 2, waiting for another QA point of view. I would suggest to squash some patches, getting rid of merge marker stuff and warnings etc. In general, I would not favor putting more prefs inside onboarding. Keep it simple. This change can be made without changing onboarding. (In reply to Marcel de Rooy from comment #79) > (In reply to Jonathan Druart from comment #78) > > 1. Only one occurrence of passwordlengthhint > > koha-tmpl/intranet-tmpl/prog/css/installer.css:#passwordlengthhint > > > > 2. Do we really need the warning when adding/editing a patron? > > BTW, $warning is not needed, memberentry.pl > > 177 my $warning = 1; > > 178 $template->param("passwordwarning"=>$warning); > > > > 3. Just a though, next time we add a "step" we will need to rethink the > > template. The shift is terrible. And hard to see the real changes. > > > > Not passing QA for 2, waiting for another QA point of view. > > I would suggest to squash some patches, getting rid of merge marker stuff > and warnings etc. > In general, I would not favor putting more prefs inside onboarding. Keep it > simple. This change can be made without changing onboarding. I tend to agree with Marcel and Jonathan here. I am not sure if we should add this to the installer as it adds a lot more to read for little gain (slows down the process). I think instead, why not just set it to at least 8 by default? I think Jonathan had agreement, setting this as FQA Created attachment 103617 [details] [review] Bug 18308: Increase minPasswordLength to 8 characters for new installs To test: 1) Go to Administration -> system preferences -> ensure minPasswordLength syspref is set to 3, or some other number less than 8 2) Go to Patrons -> Add a new patron 3) Notice the alert at the top of the page warning that minPasswordLength is too low. Confirm you are still be able to save your patron with a password that is less than 8 characters. 4) Edit the minPasswordLength syspref to be 8 or higher. 5) Go back to Patrons -> Add a new patron. Confirm the alert is gone. 6) Drop and recreate database to trigger onboarding tool. 7) Go through onboarding tool until you are required to create the superlibrarian patron. Try to enter a password of less than 8 characters. Confirm it does not let you continue until you enter a password that is 8 characters or longer. Sponsored-by: Catalyst IT Felt like this was a bit of a mess before, so have written a new patch. Let me know if there's anything missing, otherwise it's ready to test. Hi Aleisha, I think we should keep 3 as a minimum if the pref is < 3, and not 8 if < 8. - $minPasswordLength = 3 if not $minPasswordLength or $minPasswordLength < 3; (In reply to Jonathan Druart from comment #84) > Hi Aleisha, > > I think we should keep 3 as a minimum if the pref is < 3, and not 8 if < 8. > > - $minPasswordLength = 3 if not $minPasswordLength or $minPasswordLength > < 3; I think we should really get rid of such a hardcoded low value. The pref should be filled and respected ("own responsibility?"). But if it is undefined, we should certainly not keep it at 3. (In reply to Marcel de Rooy from comment #85) > (In reply to Jonathan Druart from comment #84) > > Hi Aleisha, > > > > I think we should keep 3 as a minimum if the pref is < 3, and not 8 if < 8. > > > > - $minPasswordLength = 3 if not $minPasswordLength or $minPasswordLength > > < 3; > > I think we should really get rid of such a hardcoded low value. The pref > should be filled and respected ("own responsibility?"). But if it is > undefined, we should certainly not keep it at 3. That is bug 21314. (In reply to Jonathan Druart from comment #86) > (In reply to Marcel de Rooy from comment #85) > > (In reply to Jonathan Druart from comment #84) > > > Hi Aleisha, > > > > > > I think we should keep 3 as a minimum if the pref is < 3, and not 8 if < 8. > > > > > > - $minPasswordLength = 3 if not $minPasswordLength or $minPasswordLength > > > < 3; > > > > I think we should really get rid of such a hardcoded low value. The pref > > should be filled and respected ("own responsibility?"). But if it is > > undefined, we should certainly not keep it at 3. > > That is bug 21314. You have a circular problem. Comment14 from that bug referred me here. Where we adjust it, does not matter to me. We should adjust it. Here we only need to modify the default value of the pref to 8, for new installs. If this feature is about increasing the minimum password length, then I don't see why we can't include the fix for Bug 21314 in this patch as it is. They go together. If I understand correctly the problematic line is: $minPasswordLength = 8 if not $minPasswordLength or $minPasswordLength < 8; So if we kept the patch as attached, I think we will break people's workflows. If you have set to a lower password value now, Koha will suddenly force you to use a longer password. Another use case I could see is 4 digit PINs for self checks. I know some older self checks don't support alphanumeric, not sure about length. I feel like we should continue the discussion on a hardcoded value on bug 18308 and keep this one to just change the default value we ship Koha with. It will be a step forward that we can get through quickly. (In reply to Katrin Fischer from comment #90) > > I feel like we should continue the discussion on a hardcoded value on bug > 18308 and keep this one to just change the default value we ship Koha with. > It will be a step forward that we can get through quickly. This is bug 18308 :) What about keeping only the second (comment #4) patch? It's the only way not to break people's workflows. (In reply to Bernardo Gonzalez Kriegel from comment #91) > (In reply to Katrin Fischer from comment #90) > > > > I feel like we should continue the discussion on a hardcoded value on bug > > 18308 and keep this one to just change the default value we ship Koha with. > > It will be a step forward that we can get through quickly. > > This is bug 18308 :) > > What about keeping only the second (comment #4) patch? > It's the only way not to break people's workflows. Sorry, me and bug numbers.... I meant bug 21314. c#4 looks good to me. Wow... I can't believe this issue has been open for over 3 years, and it still defaults to 3. We should do a patch to sysprefs.sql to raise it to 10 (number given by a security consultant). Let's just push that out ASAP, and then worry about existing installations. Aleisha, would you be ok with using comment #4 patch for now? I would give it a sign-off right away. (In reply to Katrin Fischer from comment #94) > Aleisha, would you be ok with using comment #4 patch for now? I would give > it a sign-off right away. Yes absolutely. Will make those patch changes right now. Created attachment 104235 [details] [review] Bug 18308 - Followup adding default value to syspref.sql Sponsored-By: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Using my QA powers to move this to PQA directly. Created attachment 104236 [details] [review] Bug 18308: Increase minPasswordLength default to 8 Sponsored-By: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> To test: - Run the web installer for any language - Verify that minPasswordLength is set to 8 now (In reply to Katrin Fischer from comment #97) > Using my QA powers to move this to PQA directly. You're the best, Katrin :). Nice work everyone! Pushed to master for 20.05 Created attachment 104351 [details] [review] Bug 18308: fix failing tests # Failed test 'check_password hook tests' # at t/db_dependent/Koha/Plugins/Patron.t line 83. [The password was rejected by a plugin]# Looks like your test exited with 255 just after 5. not backported to 19.11.x |