Bug 40824

Summary: Add option to prevent re-use of passwords
Product: Koha Reporter: Jacob O'Mara <jacob.omara>
Component: AuthenticationAssignee: Jacob O'Mara <jacob.omara>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: new feature    
Priority: P5 - low CC: dcook, dpavlin
Version: unspecified   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 40824: Create borrower_password_history table
Bug 40824: Add password history check syspref
Bug 40824: Atomic update for borrower password history
Bug 40824: Create borrower password history object classes
Bug 40824: Add "password has been used before" exception
Bug 40824: Check password history for clashes when saving password
Bug 40824: Show error for password history clash in memberpassword.tt
Bug 40824: Show error for password history clash in memberentrygen.tt
Bug 40824: Show error for password history clash in opac-passwd.tt
Bug 40824: Show error for password history clash in opac-password-recovery.tt
Bug 40824: Show error for password history clash in opac-reset-password.tt
Bug 40824: Add category-level password history count support
Bug 40824: Use category-specific password history count
Bug 40824: Add password history count field to category admin
Bug 40824: Show category-specific password history count in error messages
Bug 40824: Add tests for category-level password history
Bug 40824: DBIC - DO NOT PUSH

Description Jacob O'Mara 2025-09-17 08:58:46 UTC
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.
Comment 1 Jacob O'Mara 2025-09-17 09:47:30 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
Comment 2 Jacob O'Mara 2025-09-17 09:47:32 UTC
Created attachment 186512 [details] [review]
Bug 40824: Add password history check syspref
Comment 3 Jacob O'Mara 2025-09-17 09:47:34 UTC
Created attachment 186513 [details] [review]
Bug 40824: Atomic update for borrower password history
Comment 4 Jacob O'Mara 2025-09-17 09:47:36 UTC
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
Comment 5 Jacob O'Mara 2025-09-17 09:47:38 UTC
Created attachment 186515 [details] [review]
Bug 40824: Add "password has been used before" exception
Comment 6 Jacob O'Mara 2025-09-17 09:47:40 UTC
Created attachment 186516 [details] [review]
Bug 40824: Check password history for clashes when saving password
Comment 7 Jacob O'Mara 2025-09-17 09:47:42 UTC
Created attachment 186517 [details] [review]
Bug 40824: Show error for password history clash in memberpassword.tt
Comment 8 Jacob O'Mara 2025-09-17 09:47:44 UTC
Created attachment 186518 [details] [review]
Bug 40824: Show error for password history clash in memberentrygen.tt
Comment 9 Jacob O'Mara 2025-09-17 09:47:46 UTC
Created attachment 186519 [details] [review]
Bug 40824: Show error for password history clash in opac-passwd.tt
Comment 10 Jacob O'Mara 2025-09-17 09:47:51 UTC
Created attachment 186520 [details] [review]
Bug 40824: Show error for password history clash in opac-password-recovery.tt
Comment 11 Jacob O'Mara 2025-09-17 09:47:55 UTC
Created attachment 186521 [details] [review]
Bug 40824: Show error for password history clash in opac-reset-password.tt
Comment 12 Jacob O'Mara 2025-09-17 09:47:57 UTC
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
Comment 13 Jacob O'Mara 2025-09-17 09:47:59 UTC
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
Comment 14 Jacob O'Mara 2025-09-17 09:48:02 UTC
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
Comment 15 Jacob O'Mara 2025-09-17 09:48:04 UTC
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
Comment 16 Jacob O'Mara 2025-09-17 09:48:06 UTC
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
Comment 17 Jacob O'Mara 2025-09-17 09:48:08 UTC
Created attachment 186527 [details] [review]
Bug 40824: DBIC - DO NOT PUSH
Comment 18 David Cook 2025-09-18 00:07:59 UTC
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...
Comment 19 David Cook 2025-09-18 00:09:53 UTC
Overall, it's a pretty cool feature.
Comment 20 David Cook 2025-09-18 00:14:23 UTC
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.
Comment 21 David Cook 2025-09-18 00:14:57 UTC
(That all said, still a very cool feature.)