Description
Tomás Cohen Arazi
2018-08-08 14:00:53 UTC
Created attachment 77575 [details] [review] Bug 21178: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> Created attachment 77578 [details] [review] Bug 21178: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I edited the tests to make them test the logaction() call. 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> Created attachment 77885 [details] [review] Bug 21178: Unit tests 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 Created attachment 77916 [details] [review] Bug 21178: Unit tests Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> tomas: whitespaces alarm :) Created attachment 78833 [details] [review] Bug 21178: (QA follow-up) whitespace characters fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> 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> 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> > 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.
(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. Pushed for 18.11. Thanks all! Enhancement, will not be backported to 18.05.x series. (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) (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 |