This feature would prevent you from reusing the last X number of passwords at password change. Would be controlled at system level but also with patron category level overrides for the default.
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.)