Bug 21087

Summary: Patron's password is hashed twice when the object is saved
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: katrin.fischer, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 20287    
Bug Blocks: 36449, 21133    
Attachments: Bug 21087: Test to describe the issue
Bug 21087: Hash passwords in ->update_password
Bug 21087: Test to describe the issue
Bug 21087: Hash passwords in ->update_password
Bug 21087: Fix one wrong call
Bug 21087: Do not store the password in ->store for existing patrons
Bug 21087: Fix create_superlibrarian.pl script
Bug 21087: Test to describe the issue
Bug 21087: Hash passwords in ->update_password
Bug 21087: Fix one wrong call
Bug 21087: Do not store the password in ->store for existing patrons
Bug 21087: Fix create_superlibrarian.pl script
Bug 21087: (QA follow-up) Fix usage in create_superlibrarian.pl

Description Jonathan Druart 2018-07-18 22:22:26 UTC
When a patron is edited (reading the code at least using patron batch modification), the password is lost.

Prior to bug 20287 ModMember took the $hash of modification, and '****' was a trick to not update the password (when coming from memberentry).

The same trick has been reused for the move of ModMember to Koha::Patron->store, but then the following code will modify the password of the patron:

  my $password = 'password';
  $patron->password($password)->store;                          
  $patron->surname('xxx')->store;                               

=> User is not longer able to login with 'password'
Comment 1 Jonathan Druart 2018-07-18 22:25:55 UTC
Highlighted by Jenkins with t/db_dependent/selenium/authentication.t

    #   Failed test 'Patron with flags catalogue should be able to login'
    #   at t/db_dependent/selenium/authentication.t line 63.
Wide character in print at /usr/local/share/perl/5.20.2/Test2/Formatter/TAP.pm line 105.
    #                   'Koha › Invalid username or password Log in to Koha'
    #     doesn't match '(?^u:Koha staff client)'
Comment 2 Jonathan Druart 2018-07-18 22:55:06 UTC
Created attachment 77095 [details] [review]
Bug 21087: Test to describe the issue
Comment 3 Jonathan Druart 2018-07-18 22:55:10 UTC
Created attachment 77096 [details] [review]
Bug 21087: Hash passwords in ->update_password
Comment 4 Jonathan Druart 2018-07-18 22:55:49 UTC
Late here, I do not know if it's enough. Could someone take a look?
Comment 5 Jonathan Druart 2018-07-18 22:56:33 UTC
Test plan:
Create patron, edit, change password (staff and opac), use the password recovery as well as the self registration features.

Everything must behave as usual.
Comment 6 Tomás Cohen Arazi 2018-07-23 19:06:09 UTC
Created attachment 77200 [details] [review]
Bug 21087: Test to describe the issue

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2018-07-23 19:06:19 UTC
Created attachment 77201 [details] [review]
Bug 21087: Hash passwords in ->update_password

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Jonathan Druart 2018-07-25 13:40:34 UTC
Created attachment 77254 [details] [review]
Bug 21087: Fix one wrong call
Comment 9 Jonathan Druart 2018-07-25 13:40:39 UTC
Created attachment 77255 [details] [review]
Bug 21087: Do not store the password in ->store for existing patrons

When ->store is called on an existing patron we must not save the
password, but keep the one in DB instead.
There is a dedicated method (update_password) to call when the password
need to be updated
Comment 10 Jonathan Druart 2018-07-25 22:09:19 UTC
Created attachment 77273 [details] [review]
Bug 21087: Fix create_superlibrarian.pl script
Comment 11 Kyle M Hall 2018-07-26 13:36:38 UTC
Created attachment 77284 [details] [review]
Bug 21087: Test to describe the issue

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: John Doe <you@example.com>
Comment 12 Kyle M Hall 2018-07-26 13:37:22 UTC
Created attachment 77285 [details] [review]
Bug 21087: Hash passwords in ->update_password

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: John Doe <you@example.com>
Comment 13 Kyle M Hall 2018-07-26 13:37:26 UTC
Created attachment 77286 [details] [review]
Bug 21087: Fix one wrong call

Signed-off-by: John Doe <you@example.com>
Comment 14 Kyle M Hall 2018-07-26 13:37:30 UTC
Created attachment 77287 [details] [review]
Bug 21087: Do not store the password in ->store for existing patrons

When ->store is called on an existing patron we must not save the
password, but keep the one in DB instead.
There is a dedicated method (update_password) to call when the password
need to be updated

Signed-off-by: John Doe <you@example.com>
Comment 15 Kyle M Hall 2018-07-26 13:37:33 UTC
Created attachment 77288 [details] [review]
Bug 21087: Fix create_superlibrarian.pl script

Signed-off-by: John Doe <you@example.com>
Comment 16 Katrin Fischer 2018-07-26 16:29:38 UTC
Hi Kyle, something's odd about the sign-off line ;)
Signed-off-by: John Doe <you@example.com>
Comment 17 Tomás Cohen Arazi 2018-07-31 02:12:14 UTC
Created attachment 77327 [details] [review]
Bug 21087: (QA follow-up) Fix usage in create_superlibrarian.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2018-07-31 02:20:41 UTC
Pushed to master for 18.11, thanks all!
Comment 19 Katrin Fischer 2018-07-31 07:15:06 UTC
Now John Doe will have a sign off in release notes ;D
Comment 20 Martin Renvoize 2018-08-03 13:38:27 UTC
Caused by bug 20287 which is not in 18.05.x series. Marking RESOLVED FIXED.