Scenario: A library would like to import/overwrite all patrons with their passwords from another centralised system. Currently they cannot do this and overwrite the passwords, because Koha assumes that the passwords are coming in already hashed and thus should not be overwritten. It would be good to optionally allow the UI patron import tool and command line importer to overwrite user passwords. Personal note: I want to make it clear that I am personally aware of the user experience ramifications of unexpectedly changing a password on a user - it's not a nice thing to do to users. I know that there could be situations where the user has changed the password, or requested such a change, and then that user's explicit choice gets overwritten by this process. This bug is only meant to allow functionality that libraries are currently, or have in the past, done. This feature should 100% be optional, by tickbox in the UI and flag on the command line importer.
Created attachment 92352 [details] [review] Bug 23473: Allow overwrite of passwords during import To test: 1 - Have some patrons in your system 2 - Export some of their info via reports SELECT cardnumber, userid, surname, firstname, password, branchcode, categorycode 3 - Edit the file from above, changing all the password lines 4 - Import the file with overwrite 5 - Confirm passwords have not changed (run the report again and confirm the hashes are the same) 6 - Apply patch 7 - Restart all the things 8 - Check the new box on import screen to overwrite passwrods 9 - Import file again 10 - Confirm passwords have changed 11 - Signin using new password to verify the hash is the password as supplied 12 - Repeat via commandline import supplying --overwrite_passwords option 13 - Verify works as expected 14 - Prove -v t/db_dependent/Koha/Patrons/Import.t
Created attachment 93073 [details] [review] Bug 23473: Allow overwrite of passwords during import To test: 1 - Have some patrons in your system 2 - Export some of their info via reports SELECT cardnumber, userid, surname, firstname, password, branchcode, categorycode 3 - Edit the file from above, changing all the password lines 4 - Import the file with overwrite 5 - Confirm passwords have not changed (run the report again and confirm the hashes are the same) 6 - Apply patch 7 - Restart all the things 8 - Check the new box on import screen to overwrite passwrods 9 - Import file again 10 - Confirm passwords have changed 11 - Signin using new password to verify the hash is the password as supplied 12 - Repeat via commandline import supplying --overwrite_passwords option 13 - Verify works as expected 14 - Prove -v t/db_dependent/Koha/Patrons/Import.t Signed-off-by: Ron Marion <ron.marion@goddard.edu>
Created attachment 93074 [details] [review] Bug 23473: Allow overwrite of passwords during import To test: 1 - Have some patrons in your system 2 - Export some of their info via reports SELECT cardnumber, userid, surname, firstname, password, branchcode, categorycode 3 - Edit the file from above, changing all the password lines 4 - Import the file with overwrite 5 - Confirm passwords have not changed (run the report again and confirm the hashes are the same) 6 - Apply patch 7 - Restart all the things 8 - Check the new box on import screen to overwrite passwrods 9 - Import file again 10 - Confirm passwords have changed 11 - Signin using new password to verify the hash is the password as supplied 12 - Repeat via commandline import supplying --overwrite_passwords option 13 - Verify works as expected 14 - Prove -v t/db_dependent/Koha/Patrons/Import.t Sponsored-by: ByWater Solutions Signed-off-by: Ron Marion <ron.marion@goddard.edu>
I am sorry, tests are not looking good on this one for me: Processing additional checks OK! kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/Patrons/Import.t t/db_dependent/Koha/Patrons/Import.t .. 9/158 Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_2> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_2> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_2> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018. # Looks like you planned 4 tests but ran 2. # Failed test 'test_import_with_password_overwrite' # at t/db_dependent/Koha/Patrons/Import.t line 468. [Password is too weak]# Looks like your test exited with 255 just after 151. t/db_dependent/Koha/Patrons/Import.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 8/158 subtests Test Summary Report ------------------- t/db_dependent/Koha/Patrons/Import.t (Wstat: 65280 Tests: 151 Failed: 1) Failed test: 151 Non-zero exit status: 255 Parse errors: Bad plan. You planned 158 tests but ran 151.
Created attachment 93594 [details] [review] Bug 23473: (follow-up) Make passwords 'secure' for tests
No matter if the checkbox is checked or not, the password is overwritten.
Created attachment 94822 [details] [review] Bug 23473: (follow-up) check if overwrite_passwords is set
diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 8e501fcfb1..787713524e 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -250,7 +250,6 @@ sub store { # Make a copy of the plain text password for later use $self->plain_text_password( $self->password ); - Please remove that one from your patch. No need to.
+ if ($overwrite_passwords){ + $patron->set_password({ password => $borrower{password} }); + } set_password may throw several exceptions, we should do some checking here
*** Bug 23733 has been marked as a duplicate of this bug. ***
Escalating to Major as this appears to be a regression rather than an enhancement as highlighted by bug 23733.
(In reply to Martin Renvoize from comment #11) > Escalating to Major as this appears to be a regression rather than an > enhancement as highlighted by bug 23733. I just restested bug 23733 and all seems well - should we downgrade again?
Created attachment 95049 [details] [review] Bug 23473: Allow overwrite of passwords during import To test: 1 - Have some patrons in your system 2 - Export some of their info via reports SELECT cardnumber, userid, surname, firstname, password, branchcode, categorycode 3 - Edit the file from above, changing all the password lines 4 - Import the file with overwrite 5 - Confirm passwords have not changed (run the report again and confirm the hashes are the same) 6 - Apply patch 7 - Restart all the things 8 - Check the new box on import screen to overwrite passwrods 9 - Import file again 10 - Confirm passwords have changed 11 - Signin using new password to verify the hash is the password as supplied 12 - Repeat via commandline import supplying --overwrite_passwords option 13 - Verify works as expected 14 - Prove -v t/db_dependent/Koha/Patrons/Import.t Sponsored-by: ByWater Solutions Signed-off-by: Ron Marion <ron.marion@goddard.edu>
Created attachment 95050 [details] [review] Bug 23473: (follow-up) Make passwords 'secure' for tests
Created attachment 95051 [details] [review] Bug 23473: (follow-up) check if overwrite_passwords is set
Created attachment 95052 [details] [review] Bug 23473: (follow-up) Catch and show password exceptions
1. If empty, should not we not update the password? I get a "too_weak" error. 2. You forgot to adjust the CLI script.
Created attachment 96998 [details] [review] Bug 23473: Allow overwrite of passwords during import To test: 1 - Have some patrons in your system 2 - Export some of their info via reports SELECT cardnumber, userid, surname, firstname, password, branchcode, categorycode 3 - Edit the file from above, changing all the password lines 4 - Import the file with overwrite 5 - Confirm passwords have not changed (run the report again and confirm the hashes are the same) 6 - Apply patch 7 - Restart all the things 8 - Check the new box on import screen to overwrite passwrods 9 - Import file again 10 - Confirm passwords have changed 11 - Signin using new password to verify the hash is the password as supplied 12 - Repeat via commandline import supplying --overwrite_passwords option 13 - Verify works as expected 14 - Prove -v t/db_dependent/Koha/Patrons/Import.t Sponsored-by: ByWater Solutions Signed-off-by: Ron Marion <ron.marion@goddard.edu>
Created attachment 96999 [details] [review] Bug 23473: (follow-up) Make passwords 'secure' for tests
Created attachment 97000 [details] [review] Bug 23473: (follow-up) check if overwrite_passwords is set
Created attachment 97001 [details] [review] Bug 23473: (follow-up) Catch and show password exceptions
Created attachment 97002 [details] [review] Bug 23473: Don't allow setting blank passwords via import
Created attachment 97003 [details] [review] Bug 23473: Don't allow staff password changes for imports
(In reply to Jonathan Druart from comment #17) > 1. If empty, should not we not update the password? > > I get a "too_weak" error. Okay, cannot blank passwords now > > 2. You forgot to adjust the CLI script. I don't think I did: Bug 23473: Allow overwrite of passwords during import a/misc/import_patrons.pl (+3 lines)
FAIL Koha/Patrons/Import.pm FAIL forbidden patterns forbidden pattern: tab char (line 297) forbidden pattern: tab char (line 298) forbidden pattern: tab char (line 299) forbidden pattern: tab char (line 300) forbidden pattern: tab char (line 301) forbidden pattern: tab char (line 302) forbidden pattern: tab char (line 303) forbidden pattern: tab char (line 304) forbidden pattern: tab char (line 305) forbidden pattern: tab char (line 306) forbidden pattern: tab char (line 307) forbidden pattern: tab char (line 308) forbidden pattern: tab char (line 309) forbidden pattern: tab char (line 310) forbidden pattern: tab char (line 311) forbidden pattern: tab char (line 312) FAIL koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt FAIL filters missing_filter at line 120 ( Password is too short for patron with borrowernumber [% e.borrowernumber | html %]. Minimum length is [% e.min_length %], length is [% e.length %]) missing_filter at line 120 ( Password is too short for patron with borrowernumber [% e.borrowernumber | html %]. Minimum length is [% e.min_length %], length is [% e.length %]) OK misc/import_patrons.pl FAIL t/db_dependent/Koha/Patrons/Import.t FAIL forbidden patterns forbidden pattern: Data::Dumper::Dumper (line 494)
Created attachment 98382 [details] [review] Bug 23473: (follow-up) Fix QA complaints (tabs, filters, Dumper)
t/db_dependent/Koha/Patrons/Import.t .. 2/159 Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3> line 2.
Created attachment 101981 [details] [review] Bug 23473: Allow overwrite of passwords during import To test: 1 - Have some patrons in your system 2 - Export some of their info via reports SELECT cardnumber, userid, surname, firstname, password, branchcode, categorycode 3 - Edit the file from above, changing all the password lines 4 - Import the file with overwrite 5 - Confirm passwords have not changed (run the report again and confirm the hashes are the same) 6 - Apply patch 7 - Restart all the things 8 - Check the new box on import screen to overwrite passwrods 9 - Import file again 10 - Confirm passwords have changed 11 - Signin using new password to verify the hash is the password as supplied 12 - Repeat via commandline import supplying --overwrite_passwords option 13 - Verify works as expected 14 - Prove -v t/db_dependent/Koha/Patrons/Import.t Sponsored-by: ByWater Solutions Signed-off-by: Ron Marion <ron.marion@goddard.edu>
Created attachment 101982 [details] [review] Bug 23473: (follow-up) Make passwords 'secure' for tests
Created attachment 101983 [details] [review] Bug 23473: (follow-up) check if overwrite_passwords is set
Created attachment 101984 [details] [review] Bug 23473: (follow-up) Catch and show password exceptions
Created attachment 101985 [details] [review] Bug 23473: Don't allow setting blank passwords via import
Created attachment 101986 [details] [review] Bug 23473: Don't allow staff password changes for imports
Created attachment 101987 [details] [review] Bug 23473: (follow-up) Fix QA complaints (tabs, filters, Dumper)
Created attachment 101988 [details] [review] Bug 23473: (QA follow-up) Fix number of tests => Looks like you planned 158 tests but ran 159.
Several smaller things. No blockers on themselves. But struggled a bit before I got everything as it should.. misc/import_patrons: overwrite_passwords not in usage Might be helpful to include a warning if someone includes passwords in the csv file but forgets the switch? Are staff lines silently ignored ? Although we skip staff, you can promote a normal user to staff when changing his categorycode. Do we want that? We can even add flags with value 1 to overwrite users to superlib. I would say that we should not do that. (This is outside the scope of this report.) Styling on tools import could be improved for password check box? @RM: I am passing QA reluctantly on the patch set, but have the feeling that patron import still needs further attention. Including some security issues. Especially overwriting a regular user with import privs with superlibrarian privs and after that taking over control is not nice.
Should not we make sure only superlibrarian is allowed to overwrite superlibrarian's passwords? Or maybe only enable the feature from the command line?
(In reply to Jonathan Druart from comment #37) > Should not we make sure only superlibrarian is allowed to overwrite > superlibrarian's passwords? > > Or maybe only enable the feature from the command line? See bug 23473 for follow-up patches.
(In reply to Jonathan Druart from comment #37) > Should not we make sure only superlibrarian is allowed to overwrite > superlibrarian's passwords? > > Or maybe only enable the feature from the command line? See bug 23473 for follow-up patches.(In reply to Jonathan Druart from comment #37) > Should not we make sure only superlibrarian is allowed to overwrite > superlibrarian's passwords? > > Or maybe only enable the feature from the command line? See bug 24375 for follow-up patches.
(In reply to Jonathan Druart from comment #37) > Should not we make sure only superlibrarian is allowed to overwrite > superlibrarian's passwords? > > Or maybe only enable the feature from the command line? See bug 24375 for follow-up patches. (Sorry for the noise!)
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x