|
Description
Jacob O'Mara
2025-09-17 08:58:46 UTC
Created attachment 186511 [details] [review] Bug 40824: Create borrower_password_history table Test Plan: Run the following test files to verify the core functionality works: prove t/db_dependent/Koha/Patron.t prove t/db_dependent/Koha/Patron/Category.t prove t/db_dependent/Koha/PatronPasswordHistories.t prove t/db_dependent/Koha/PatronPasswordHistory.t - Navigate to system preferences and set “PasswordHistoryCount” to a value other than 0 we’ll call this X - Navigate to a test patron and change password. You should be unable to change the password to any of your last X used passwords. - Navigate to the opal and attempt to change the password there. The same should apply as on the staff interface. ( You should also receive a suitable error message explaining why your password change failed ) - Navigate to Patron Categories and set the password history count here for your patron’s category ( Y ) - Ensure that this new value Y is used in place of X as the number of previous passwords to check Created attachment 186512 [details] [review] Bug 40824: Add password history check syspref Created attachment 186513 [details] [review] Bug 40824: Atomic update for borrower password history Created attachment 186514 [details] [review] Bug 40824: Create borrower password history object classes These are named as PatronPasswordHistory(ies) for consistency with the patron.pm class objects Created attachment 186515 [details] [review] Bug 40824: Add "password has been used before" exception Created attachment 186516 [details] [review] Bug 40824: Check password history for clashes when saving password Created attachment 186517 [details] [review] Bug 40824: Show error for password history clash in memberpassword.tt Created attachment 186518 [details] [review] Bug 40824: Show error for password history clash in memberentrygen.tt Created attachment 186519 [details] [review] Bug 40824: Show error for password history clash in opac-passwd.tt Created attachment 186520 [details] [review] Bug 40824: Show error for password history clash in opac-password-recovery.tt Created attachment 186521 [details] [review] Bug 40824: Show error for password history clash in opac-reset-password.tt Created attachment 186522 [details] [review] Bug 40824: Add category-level password history count support Add effective_password_history_count method to Patron::Category that returns category-specific password_history_count or falls back to system preference PasswordHistoryCount Created attachment 186523 [details] [review] Bug 40824: Use category-specific password history count Update password validation logic to use the category-specific effective_password_history_count method instead of the global system preference: - Update Patron.pm to use category method - Update PatronPasswordHistories.pm validation logic - Update PatronPasswordHistory.pm storage cleanup logic Created attachment 186524 [details] [review] Bug 40824: Add password history count field to category admin Add password_history_count field to category administration: - Add form field processing in admin/categories.pl - Add input field to categories.tt template with system preference fallback hint Created attachment 186525 [details] [review] Bug 40824: Show category-specific password history count in error messages Update all password error message handlers to use the category-specific effective_password_history_count method instead of the global system preference: - members/member-password.pl - members/memberentry.pl - opac/opac-passwd.pl - opac/opac-password-recovery.pl - opac/opac-reset-password.pl Created attachment 186526 [details] [review] Bug 40824: Add tests for category-level password history Add comprehensive unit tests for the new password history functionality: - Add effective_password_history_count tests to Category.t - Create PatronPasswordHistory.t - Create PatronPasswordHistories.t - Add password history integration tests to Patron.t Created attachment 186527 [details] [review] Bug 40824: DBIC - DO NOT PUSH Currently failing QA scripts. Mostly for tidiness.
Seems like there are some misleading QA failures which suggests a bug in the QA script like:
[FAIL] installer/data/mysql/mandatory/sysprefs.sql
FAIL semicolon
semicolon found instead of comma at line 896
This one might be valid:
[FAIL] Koha/Exceptions/Password/UsedBefore.pm
SKIP pod_coverage
No POD exists
FAIL tidiness
File is not tidy, please run `perl misc/devel/tidy.pl Koha/Exceptions/Password/UsedBefore.pm`
FAIL valid
Class Koha::Exceptions::Exception appears to be a typo as it is only specified in the 'isa' param for Koha::Exceptions::Password::UsedBefore
I don't know heaps about Koha::Exceptions but that file does seem to differ from other Koha::Exceptions...
Overall, it's a pretty cool feature. I keep looking at Koha/PatronPasswordHistory.pm and the store method there...
And I feel like there might be a subtle bug here, but I'm not sure yet.
If the password hasn't been saved in the password history, we check if it's already Bcrypt hashed, and if it's not then we try to hash it.
But... it's possible for the old password to still be a MD5 hash. Looking at Koha::Patron... I think for older Koha systems your password history would be incorrect for the older passwords.
I think there's danger in using a line like this:
+ if ( $password && $password !~ /\$2a\$/ ) {
Because at some point we also want to upgrade passwords from Bcrypt to Argon, and this will probably be a bit of a pain to find.
So... I'm a bit wary of this bit of code.
(That all said, still a very cool feature.) Created attachment 189201 [details] [review] Bug 40824: Create borrower_password_history table Test Plan: Run the following test files to verify the core functionality works: prove t/db_dependent/Koha/Patron.t prove t/db_dependent/Koha/Patron/Category.t prove t/db_dependent/Koha/PatronPasswordHistories.t prove t/db_dependent/Koha/PatronPasswordHistory.t - Navigate to system preferences and set “PasswordHistoryCount” to a value other than 0 we’ll call this X - Navigate to a test patron and change password. You should be unable to change the password to any of your last X used passwords. - Navigate to the opal and attempt to change the password there. The same should apply as on the staff interface. ( You should also receive a suitable error message explaining why your password change failed ) - Navigate to Patron Categories and set the password history count here for your patron’s category ( Y ) - Ensure that this new value Y is used in place of X as the number of previous passwords to check Created attachment 189202 [details] [review] Bug 40824: Add password history check syspref Created attachment 189203 [details] [review] Bug 40824: Atomic update for borrower password history Created attachment 189204 [details] [review] Bug 40824: Create borrower password history object classes These are named as PatronPasswordHistory(ies) for consistency with the patron.pm class objects Created attachment 189205 [details] [review] Bug 40824: Add "password has been used before" exception Created attachment 189206 [details] [review] Bug 40824: Check password history for clashes when saving password Created attachment 189207 [details] [review] Bug 40824: Show error for password history clash in memberpassword.tt Created attachment 189208 [details] [review] Bug 40824: Show error for password history clash in memberentrygen.tt Created attachment 189209 [details] [review] Bug 40824: Show error for password history clash in opac-passwd.tt Created attachment 189210 [details] [review] Bug 40824: Show error for password history clash in opac-password-recovery.tt Created attachment 189211 [details] [review] Bug 40824: Show error for password history clash in opac-reset-password.tt Created attachment 189212 [details] [review] Bug 40824: Add category-level password history count support Add effective_password_history_count method to Patron::Category that returns category-specific password_history_count or falls back to system preference PasswordHistoryCount Created attachment 189213 [details] [review] Bug 40824: Use category-specific password history count Update password validation logic to use the category-specific effective_password_history_count method instead of the global system preference: - Update Patron.pm to use category method - Update PatronPasswordHistories.pm validation logic - Update PatronPasswordHistory.pm storage cleanup logic Created attachment 189214 [details] [review] Bug 40824: Add password history count field to category admin Add password_history_count field to category administration: - Add form field processing in admin/categories.pl - Add input field to categories.tt template with system preference fallback hint Created attachment 189215 [details] [review] Bug 40824: Show category-specific password history count in error messages Update all password error message handlers to use the category-specific effective_password_history_count method instead of the global system preference: - members/member-password.pl - members/memberentry.pl - opac/opac-passwd.pl - opac/opac-password-recovery.pl - opac/opac-reset-password.pl Created attachment 189216 [details] [review] Bug 40824: Add tests for category-level password history Add comprehensive unit tests for the new password history functionality: - Add effective_password_history_count tests to Category.t - Create PatronPasswordHistory.t - Create PatronPasswordHistories.t - Add password history integration tests to Patron.t Created attachment 189217 [details] [review] Bug 40824: DBIC - DO NOT COMMIT Thanks for the feedback! The above patchset should address the issues. Re: the slight bug you mentioned with md5 hashes, there's actually no need for any hashing at all, it's always passed a hashed password from the passwords table anyway, it's redundant checking. It also conveniently also solves the issue of updating to Argon as well. As such I've removed the checking for that in the tests too. The testing is now pretty bare as a result, but there's really not much to check, the object controller doesn't really have many duties! Let me know if there are any other issues! Thanks again |