Bug 22253 - Koha throws an exception when updating a borrower with an insecure password
Summary: Koha throws an exception when updating a borrower with an insecure password
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-31 18:57 UTC by Nick Clemens
Modified: 2019-06-27 09:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22253: Check we actually need to update the password (1.86 KB, patch)
2019-01-31 19:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22253: Check we actually need to update the password (1.90 KB, patch)
2019-02-01 17:37 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22253: Check we actually need to update the password (1.96 KB, patch)
2019-02-02 09:53 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-01-31 18:57:16 UTC
To recreate:
1 - Set RequireStrongPassword to 'Don't require'
2 - Have or set a patron with a simple password like 'oops'
3 - Set RequireStrongPassword to 'Require'
4 - Attempt to edit another part of that patron's record (from the full edit page)
5 - Koha throws an exception
Comment 1 Tomás Cohen Arazi 2019-01-31 19:34:31 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2019-02-01 17:37:12 UTC
Created attachment 84640 [details] [review]
Bug 22253: Check we actually need to update the password

This patch makes memberentry.pl check if password needs to be updated
before attempting to call set_password. Above this there's a check that
won't raise any errors if no password is passed, or the default string (****) is received.

So we could reach that line of code with no password, but the code
wouldn't check that.

To test:
- In master, edit any patron without changing the password
=> FAIL: It raises an exception
- Apply this patch
- Edit the patron withtout changing the password
=> SUCCESS: Edit successful
- Edit the patron, changing the password
- Try to login with the new password
=> SUCCESS: The password got changed correctly
- Sigh off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Josef Moravec 2019-02-02 09:53:00 UTC
Created attachment 84657 [details] [review]
Bug 22253: Check we actually need to update the password

This patch makes memberentry.pl check if password needs to be updated
before attempting to call set_password. Above this there's a check that
won't raise any errors if no password is passed, or the default string (****) is received.

So we could reach that line of code with no password, but the code
wouldn't check that.

To test:
- In master, edit any patron without changing the password
=> FAIL: It raises an exception
- Apply this patch
- Edit the patron withtout changing the password
=> SUCCESS: Edit successful
- Edit the patron, changing the password
- Try to login with the new password
=> SUCCESS: The password got changed correctly
- Sigh off :-D

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 4 Nick Clemens 2019-02-04 17:47:55 UTC
Awesome work all!

Pushed to master for 19.05
Comment 5 Martin Renvoize 2019-02-11 13:17:26 UTC
Bug not in 18.11.x series.