Bug 21087 - Patron's password is hashed twice when the object is saved
Summary: Patron's password is hashed twice when the object is saved
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 20287
Blocks: 36449 21133
  Show dependency treegraph
 
Reported: 2018-07-18 22:22 UTC by Jonathan Druart
Modified: 2024-03-28 09:19 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21087: Test to describe the issue (1.35 KB, patch)
2018-07-18 22:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21087: Hash passwords in ->update_password (12.50 KB, patch)
2018-07-18 22:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21087: Test to describe the issue (1.41 KB, patch)
2018-07-23 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21087: Hash passwords in ->update_password (12.54 KB, patch)
2018-07-23 19:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21087: Fix one wrong call (1.28 KB, patch)
2018-07-25 13:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21087: Do not store the password in ->store for existing patrons (3.01 KB, patch)
2018-07-25 13:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21087: Fix create_superlibrarian.pl script (1.15 KB, patch)
2018-07-25 22:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21087: Test to describe the issue (1.45 KB, patch)
2018-07-26 13:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21087: Hash passwords in ->update_password (12.59 KB, patch)
2018-07-26 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21087: Fix one wrong call (1.32 KB, patch)
2018-07-26 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21087: Do not store the password in ->store for existing patrons (3.05 KB, patch)
2018-07-26 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21087: Fix create_superlibrarian.pl script (1.19 KB, patch)
2018-07-26 13:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21087: (QA follow-up) Fix usage in create_superlibrarian.pl (1001 bytes, patch)
2018-07-31 02:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.