Bug 21178 - Add Koha::Patron::set_password method
Summary: Add Koha::Patron::set_password method
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 21233
Blocks: 21431 21547 21992 22047 22059
  Show dependency treegraph
 
Reported: 2018-08-08 14:00 UTC by Tomás Cohen Arazi
Modified: 2020-01-06 20:14 UTC (History)
5 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 21178: Unit tests (3.06 KB, patch)
2018-08-08 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21178: Add Koha::Patron::set_password method (5.01 KB, patch)
2018-08-08 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21178: (follow-up) Stringify exception correctly (2.64 KB, patch)
2018-08-08 14:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21178: Unit tests (3.73 KB, patch)
2018-08-08 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21178: (QA follow-up) Rely on is_password_valid (3.02 KB, patch)
2018-08-09 16:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21178: Unit tests (3.68 KB, patch)
2018-08-16 10:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21178: Add Koha::Patron::set_password (2.95 KB, patch)
2018-08-16 10:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21178: Unit tests (3.73 KB, patch)
2018-08-16 12:58 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21178: Add Koha::Patron::set_password (3.01 KB, patch)
2018-08-16 12:58 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21178: (QA follow-up) whitespace characters fix (2.00 KB, patch)
2018-09-14 23:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21178: Unit tests (3.79 KB, patch)
2018-10-05 15:22 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21178: Add Koha::Patron::set_password (3.07 KB, patch)
2018-10-05 15:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21178: (QA follow-up) whitespace characters fix (2.06 KB, patch)
2018-10-05 15:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 21178: (QA follow-up) Add a test to verify that the hash is correct (1.36 KB, patch)
2018-10-05 15:23 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2018-08-08 14:00:53 UTC
There should be a method for setting a patron's password without the need for it to also touch the userid. Also, it should rise exceptions we can trap on error conditions on the passed password.
Comment 1 Tomás Cohen Arazi 2018-08-08 14:27:01 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2018-08-08 14:27:07 UTC
Created attachment 77576 [details] [review]
Bug 21178: Add Koha::Patron::set_password method

This patch adds the set_password method. This method implements the
password validation done in Koha::AuthUtils and raises exceptions when
the current password policy is not met.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
- Sign off! :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2018-08-08 14:27:12 UTC
Created attachment 77577 [details] [review]
Bug 21178: (follow-up) Stringify exception correctly

This patch adds code to stringify the Password::TooShort exception.

To test:
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2018-08-08 14:56:11 UTC
Created attachment 77578 [details] [review]
Bug 21178: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2018-08-08 14:57:06 UTC
I edited the tests to make them test the logaction() call.
Comment 6 Tomás Cohen Arazi 2018-08-09 16:21:46 UTC
Created attachment 77632 [details] [review]
Bug 21178: (QA follow-up) Rely on is_password_valid

This patch makes set_password still rely on Koha::AuthUtils'
is_password_valid method as the only source for truth regarding password
validity.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests still pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2018-08-16 10:18:46 UTC
Created attachment 77885 [details] [review]
Bug 21178: Unit tests
Comment 8 Tomás Cohen Arazi 2018-08-16 10:18:50 UTC
Created attachment 77886 [details] [review]
Bug 21178: Add Koha::Patron::set_password

This patch introduces the 'set_password' method for Koha::Patron
objects. The main point is to make password changing atomic
(update_password touches the userid on the DB, which should be done
carefully with better error handling, and it is done there only for
legacy backwards compatibility).

A follow-up bug will make the codebase use this instead of
update_password, and use a proper method for changing the userid if
required.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
- Sign off! :-D
Comment 9 Josef Moravec 2018-08-16 12:58:12 UTC
Created attachment 77916 [details] [review]
Bug 21178: Unit tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2018-08-16 12:58:15 UTC
Created attachment 77917 [details] [review]
Bug 21178: Add Koha::Patron::set_password

This patch introduces the 'set_password' method for Koha::Patron
objects. The main point is to make password changing atomic
(update_password touches the userid on the DB, which should be done
carefully with better error handling, and it is done there only for
legacy backwards compatibility).

A follow-up bug will make the codebase use this instead of
update_password, and use a proper method for changing the userid if
required.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
- Sign off! :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Marcel de Rooy 2018-08-24 09:10:32 UTC
tomas: whitespaces alarm :)
Comment 12 Tomás Cohen Arazi 2018-09-14 23:16:09 UTC
Created attachment 78833 [details] [review]
Bug 21178: (QA follow-up) whitespace characters fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Julian Maurice 2018-10-05 15:22:55 UTC
Created attachment 80133 [details] [review]
Bug 21178: Unit tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 14 Julian Maurice 2018-10-05 15:23:08 UTC
Created attachment 80134 [details] [review]
Bug 21178: Add Koha::Patron::set_password

This patch introduces the 'set_password' method for Koha::Patron
objects. The main point is to make password changing atomic
(update_password touches the userid on the DB, which should be done
carefully with better error handling, and it is done there only for
legacy backwards compatibility).

A follow-up bug will make the codebase use this instead of
update_password, and use a proper method for changing the userid if
required.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
- Sign off! :-D

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 15 Julian Maurice 2018-10-05 15:23:17 UTC
Created attachment 80135 [details] [review]
Bug 21178: (QA follow-up) whitespace characters fix

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 16 Julian Maurice 2018-10-05 15:23:21 UTC
Created attachment 80136 [details] [review]
Bug 21178: (QA follow-up) Add a test to verify that the hash is correct

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 17 Marcel de Rooy 2018-10-11 10:33:56 UTC
> A follow-up bug will make the codebase use this instead of
> update_password, and use a proper method for changing the userid if
> required.

Is it already written?
I wrote a number of patches around lockout status now, and have some thoughts about setting login_attempts to 0 in set_password / update_password.
(See 21336 and dependees)
In my idea an administrative lockout (login_attempts<0) should not be reset from the opac scripts. So we should need some check or parameter in set_password. See also 21533.
Comment 18 Tomás Cohen Arazi 2018-10-11 10:52:01 UTC
(In reply to Marcel de Rooy from comment #17)
> > A follow-up bug will make the codebase use this instead of
> > update_password, and use a proper method for changing the userid if
> > required.
> 
> Is it already written?

No, but I can do it

> I wrote a number of patches around lockout status now, and have some
> thoughts about setting login_attempts to 0 in set_password / update_password.
> (See 21336 and dependees)
> In my idea an administrative lockout (login_attempts<0) should not be reset
> from the opac scripts. So we should need some check or parameter in
> set_password. See also 21533.

I'd say an administrative lockout should be identifiable as such, like staff_locked => true but I'll take a look at your bugs and see how can they coexist. The main thing here was decoupling password change from userid overwrite.
Comment 19 Tomás Cohen Arazi 2018-10-13 11:37:01 UTC
Pushed for 18.11. Thanks all!
Comment 20 Martin Renvoize 2018-10-16 12:28:27 UTC
Enhancement, will not be backported to 18.05.x series.
Comment 21 Jonathan Druart 2018-12-12 16:10:48 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> A follow-up bug will make the codebase use this instead of
> update_password, and use a proper method for changing the userid if
> required.

Where is this follow-up? (Yes I saw bug 21547)
Comment 22 Tomás Cohen Arazi 2018-12-12 16:54:03 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Tomás Cohen Arazi from comment #8)
> > A follow-up bug will make the codebase use this instead of
> > update_password, and use a proper method for changing the userid if
> > required.
> 
> Where is this follow-up? (Yes I saw bug 21547)

That'd be bug 21992