Bug 23473 - Add option to import/overwrite passwords when using the patron import tool
Summary: Add option to import/overwrite passwords when using the patron import tool
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 24375
  Show dependency treegraph
 
Reported: 2019-08-19 15:48 UTC by Liz Rea
Modified: 2020-05-20 09:47 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This adds a new checkbox to the patron import tool that will allow to overwrite patrons' passwords with the password from the import file.
Version(s) released in:
20.05.00


Attachments
Bug 23473: Allow overwrite of passwords during import (9.91 KB, patch)
2019-08-19 16:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: Allow overwrite of passwords during import (9.90 KB, patch)
2019-09-21 00:07 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23473: Allow overwrite of passwords during import (9.93 KB, patch)
2019-09-21 00:13 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23473: (follow-up) Make passwords 'secure' for tests (1.27 KB, patch)
2019-10-03 13:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: (follow-up) check if overwrite_passwords is set (2.57 KB, patch)
2019-10-29 10:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: Allow overwrite of passwords during import (9.40 KB, patch)
2019-11-05 12:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: (follow-up) Make passwords 'secure' for tests (1.27 KB, patch)
2019-11-05 12:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: (follow-up) check if overwrite_passwords is set (2.57 KB, patch)
2019-11-05 12:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: (follow-up) Catch and show password exceptions (3.89 KB, patch)
2019-11-05 12:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: Allow overwrite of passwords during import (9.41 KB, patch)
2020-01-08 15:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: (follow-up) Make passwords 'secure' for tests (1.27 KB, patch)
2020-01-08 15:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: (follow-up) check if overwrite_passwords is set (2.57 KB, patch)
2020-01-08 15:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: (follow-up) Catch and show password exceptions (3.91 KB, patch)
2020-01-08 15:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: Don't allow setting blank passwords via import (4.31 KB, patch)
2020-01-08 15:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: Don't allow staff password changes for imports (3.98 KB, patch)
2020-01-08 15:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: (follow-up) Fix QA complaints (tabs, filters, Dumper) (4.28 KB, patch)
2020-02-04 12:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23473: Allow overwrite of passwords during import (9.25 KB, patch)
2020-03-27 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23473: (follow-up) Make passwords 'secure' for tests (1.31 KB, patch)
2020-03-27 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23473: (follow-up) check if overwrite_passwords is set (2.61 KB, patch)
2020-03-27 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23473: (follow-up) Catch and show password exceptions (4.16 KB, patch)
2020-03-27 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23473: Don't allow setting blank passwords via import (4.35 KB, patch)
2020-03-27 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23473: Don't allow staff password changes for imports (4.02 KB, patch)
2020-03-27 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23473: (follow-up) Fix QA complaints (tabs, filters, Dumper) (2.05 KB, patch)
2020-03-27 10:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23473: (QA follow-up) Fix number of tests (849 bytes, patch)
2020-03-27 10:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Liz Rea 2019-08-19 15:48:51 UTC
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.
Comment 1 Nick Clemens 2019-08-19 16:21:07 UTC
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
Comment 2 Liz Rea 2019-09-21 00:07:35 UTC
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>
Comment 3 Liz Rea 2019-09-21 00:13:39 UTC
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>
Comment 4 Katrin Fischer 2019-10-03 12:53:40 UTC
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.
Comment 5 Nick Clemens 2019-10-03 13:09:19 UTC
Created attachment 93594 [details] [review]
Bug 23473: (follow-up) Make passwords 'secure' for tests
Comment 6 Katrin Fischer 2019-10-03 13:57:58 UTC
No matter if the checkbox is checked or not, the password is overwritten.
Comment 7 Nick Clemens 2019-10-29 10:29:25 UTC
Created attachment 94822 [details] [review]
Bug 23473: (follow-up) check if overwrite_passwords is set
Comment 8 Marcel de Rooy 2019-11-01 11:14:16 UTC
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.
Comment 9 Marcel de Rooy 2019-11-01 11:16:00 UTC
+            if ($overwrite_passwords){
+                $patron->set_password({ password => $borrower{password} });
+            }

set_password may throw several exceptions, we should do some checking here
Comment 10 Martin Renvoize 2019-11-05 08:24:49 UTC
*** Bug 23733 has been marked as a duplicate of this bug. ***
Comment 11 Martin Renvoize 2019-11-05 08:27:10 UTC
Escalating to Major as this appears to be a regression rather than an enhancement as highlighted by bug 23733.
Comment 12 Katrin Fischer 2019-11-05 12:00:30 UTC
(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?
Comment 13 Nick Clemens 2019-11-05 12:59:25 UTC
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>
Comment 14 Nick Clemens 2019-11-05 12:59:28 UTC
Created attachment 95050 [details] [review]
Bug 23473: (follow-up) Make passwords 'secure' for tests
Comment 15 Nick Clemens 2019-11-05 12:59:31 UTC
Created attachment 95051 [details] [review]
Bug 23473: (follow-up) check if overwrite_passwords is set
Comment 16 Nick Clemens 2019-11-05 12:59:34 UTC
Created attachment 95052 [details] [review]
Bug 23473: (follow-up) Catch and show password exceptions
Comment 17 Jonathan Druart 2019-11-06 10:42:52 UTC
1. If empty, should not we not update the password?

I get a "too_weak" error.

2. You forgot to adjust the CLI script.
Comment 18 Nick Clemens 2020-01-08 15:03:14 UTC
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>
Comment 19 Nick Clemens 2020-01-08 15:03:17 UTC
Created attachment 96999 [details] [review]
Bug 23473: (follow-up) Make passwords 'secure' for tests
Comment 20 Nick Clemens 2020-01-08 15:03:20 UTC
Created attachment 97000 [details] [review]
Bug 23473: (follow-up) check if overwrite_passwords is set
Comment 21 Nick Clemens 2020-01-08 15:03:23 UTC
Created attachment 97001 [details] [review]
Bug 23473: (follow-up) Catch and show password exceptions
Comment 22 Nick Clemens 2020-01-08 15:03:26 UTC
Created attachment 97002 [details] [review]
Bug 23473: Don't allow setting blank passwords via import
Comment 23 Nick Clemens 2020-01-08 15:03:29 UTC
Created attachment 97003 [details] [review]
Bug 23473: Don't allow staff password changes for imports
Comment 24 Nick Clemens 2020-01-08 15:04:46 UTC
(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)
Comment 25 Jonathan Druart 2020-02-04 10:58:30 UTC
 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)
Comment 26 Nick Clemens 2020-02-04 12:15:48 UTC
Created attachment 98382 [details] [review]
Bug 23473: (follow-up) Fix QA complaints (tabs, filters, Dumper)
Comment 27 Marcel de Rooy 2020-03-27 09:54:13 UTC
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.
Comment 28 Marcel de Rooy 2020-03-27 10:43:31 UTC
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>
Comment 29 Marcel de Rooy 2020-03-27 10:43:35 UTC
Created attachment 101982 [details] [review]
Bug 23473: (follow-up) Make passwords 'secure' for tests
Comment 30 Marcel de Rooy 2020-03-27 10:43:40 UTC
Created attachment 101983 [details] [review]
Bug 23473: (follow-up) check if overwrite_passwords is set
Comment 31 Marcel de Rooy 2020-03-27 10:43:44 UTC
Created attachment 101984 [details] [review]
Bug 23473: (follow-up) Catch and show password exceptions
Comment 32 Marcel de Rooy 2020-03-27 10:43:48 UTC
Created attachment 101985 [details] [review]
Bug 23473: Don't allow setting blank passwords via import
Comment 33 Marcel de Rooy 2020-03-27 10:43:52 UTC
Created attachment 101986 [details] [review]
Bug 23473: Don't allow staff password changes for imports
Comment 34 Marcel de Rooy 2020-03-27 10:43:56 UTC
Created attachment 101987 [details] [review]
Bug 23473: (follow-up) Fix QA complaints (tabs, filters, Dumper)
Comment 35 Marcel de Rooy 2020-03-27 10:44:01 UTC
Created attachment 101988 [details] [review]
Bug 23473: (QA follow-up) Fix number of tests

=> Looks like you planned 158 tests but ran 159.
Comment 36 Marcel de Rooy 2020-03-27 10:44:24 UTC
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.
Comment 37 Jonathan Druart 2020-03-27 11:04:12 UTC
Should not we make sure only superlibrarian is allowed to overwrite superlibrarian's passwords?

Or maybe only enable the feature from the command line?
Comment 38 Jonathan Druart 2020-03-27 11:27:12 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2020-03-27 11:57:54 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2020-03-27 11:59:25 UTC
(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!)
Comment 41 Martin Renvoize 2020-03-27 12:23:54 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 42 Joy Nelson 2020-04-03 23:00:14 UTC
enhancement not backported to 19.11.x