Bug 21178

Summary: Add Koha::Patron::set_password method
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, julian.maurice, m.de.rooy, martin.renvoize, tomascohen
Version: master   
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: 21233    
Bug Blocks: 21431, 21547, 21992, 22047, 22059    
Attachments: Bug 21178: Unit tests
Bug 21178: Add Koha::Patron::set_password method
Bug 21178: (follow-up) Stringify exception correctly
Bug 21178: Unit tests
Bug 21178: (QA follow-up) Rely on is_password_valid
Bug 21178: Unit tests
Bug 21178: Add Koha::Patron::set_password
Bug 21178: Unit tests
Bug 21178: Add Koha::Patron::set_password
Bug 21178: (QA follow-up) whitespace characters fix
Bug 21178: Unit tests
Bug 21178: Add Koha::Patron::set_password
Bug 21178: (QA follow-up) whitespace characters fix
Bug 21178: (QA follow-up) Add a test to verify that the hash is correct

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