Bugzilla – Attachment 186523 Details for
Bug 40824
Add option to prevent re-use of passwords
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40824: Use category-specific password history count
Bug-40824-Use-category-specific-password-history-c.patch (text/plain), 9.55 KB, created by
Jacob O'Mara
on 2025-09-17 09:47:59 UTC
(
hide
)
Description:
Bug 40824: Use category-specific password history count
Filename:
MIME Type:
Creator:
Jacob O'Mara
Created:
2025-09-17 09:47:59 UTC
Size:
9.55 KB
patch
obsolete
>From 10031b41cce26657bb1adb60a3400336831672a8 Mon Sep 17 00:00:00 2001 >From: Jacob O'Mara <Jacob.omara@openfifth.co.uk> >Date: Thu, 14 Aug 2025 11:49:27 +0100 >Subject: [PATCH] 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 >--- > Koha/Patron.pm | 2 +- > Koha/PatronPasswordHistories.pm | 103 +++++++++++++++++++------------- > Koha/PatronPasswordHistory.pm | 46 +++++++------- > 3 files changed, 87 insertions(+), 64 deletions(-) > >diff --git a/Koha/Patron.pm b/Koha/Patron.pm >index 4efa4a107bd..11cd2cad163 100644 >--- a/Koha/Patron.pm >+++ b/Koha/Patron.pm >@@ -1160,7 +1160,7 @@ sub set_password { > if ($self->password && $self->in_storage) { > # Get the old password before it's replaced > my $old_password = $self->get_from_storage->password; >- my $history_count = C4::Context->preference('PasswordHistoryCount') || 0; >+ my $history_count = $self->category->effective_password_history_count || 0; > > # Always store the old password, regardless of current preference > # This ensures we maintain history if the preference is increased later >diff --git a/Koha/PatronPasswordHistories.pm b/Koha/PatronPasswordHistories.pm >index ada409354d7..fd1b043819d 100644 >--- a/Koha/PatronPasswordHistories.pm >+++ b/Koha/PatronPasswordHistories.pm >@@ -23,6 +23,7 @@ use Koha::Database; > use Koha::PatronPasswordHistory; > use C4::Context; > use Koha::AuthUtils; >+use Koha::Patrons; > > use base qw(Koha::Objects); > >@@ -44,37 +45,41 @@ If count is 0 or negative, all password history for the patron will be deleted. > =cut > > sub cleanup_old_password_history { >- my ($class, $borrowernumber, $count) = @_; >+ my ( $class, $borrowernumber, $count ) = @_; > > return unless $borrowernumber; >- >+ > # If count is 0 or negative, delete all entries >- if ($count <= 0) { >- my $deleted = $class->search({ >- borrowernumber => $borrowernumber >- })->delete; >+ if ( $count <= 0 ) { >+ my $deleted = $class->search( >+ { >+ borrowernumber => $borrowernumber >+ } >+ )->delete; > return $deleted; > } >- >+ > # Get IDs of entries to keep (the most recent ones) > my @keep_ids = $class->search( > { borrowernumber => $borrowernumber }, >- { >+ { > order_by => { -desc => 'created_on' }, >- rows => $count, >- columns => ['id'] >+ rows => $count, >+ columns => ['id'] > } > )->get_column('id'); >- >+ > # Delete all entries for this borrower except the ones we're keeping > my $deleted = 0; > if (@keep_ids) { >- $deleted = $class->search({ >- borrowernumber => $borrowernumber, >- id => { -not_in => \@keep_ids } >- })->delete; >+ $deleted = $class->search( >+ { >+ borrowernumber => $borrowernumber, >+ id => { -not_in => \@keep_ids } >+ } >+ )->delete; > } >- >+ > return $deleted; > } > >@@ -85,6 +90,7 @@ Return the DBIC resultset type for this class > =cut > > sub _type { >+ > # The table name in DB is borrower_password_history > return 'BorrowerPasswordHistory'; > } >@@ -114,58 +120,71 @@ Returns true if the password has been used before, false otherwise. > =cut > > sub has_used_password { >- my ($class, $params) = @_; >+ my ( $class, $params ) = @_; > >- my $borrowernumber = $params->{borrowernumber}; >- my $password = $params->{password}; >+ my $borrowernumber = $params->{borrowernumber}; >+ my $password = $params->{password}; > my $current_password = $params->{current_password}; >- >- unless ($borrowernumber && $password) { >+ >+ unless ( $borrowernumber && $password ) { > return 0; > } >- >- my $history_count = C4::Context->preference('PasswordHistoryCount') || 0; >+ >+ # Get the patron and their category >+ my $patron = Koha::Patrons->find($borrowernumber); >+ return 0 unless $patron; >+ >+ my $history_count = >+ $patron->category->effective_password_history_count || 0; > > # If history count is 0, don't check history at all >- if ($history_count == 0) { >+ if ( $history_count == 0 ) { > return 0; > } >- >+ > # If history count is 1, only check current password >- if ($history_count == 1) { >- my $matches_current = $current_password && C4::Auth::checkpw_hash($password, $current_password); >+ if ( $history_count == 1 ) { >+ my $matches_current = $current_password >+ && C4::Auth::checkpw_hash( $password, $current_password ); > return $matches_current ? 1 : 0; > } > > # First check if password matches current one, if provided >- if ($current_password && C4::Auth::checkpw_hash($password, $current_password)) { >- return 1; # Current password counts as a used password >+ if ( $current_password >+ && C4::Auth::checkpw_hash( $password, $current_password ) ) >+ { >+ return 1; # Current password counts as a used password > } >- >+ > # Adjust history count to account for current password >- my $history_entries_to_check = $current_password ? $history_count - 1 : $history_count; >- >- return 0 if $history_entries_to_check <= 0; # No need to check history if only checking current >+ my $history_entries_to_check = >+ $current_password ? $history_count - 1 : $history_count; >+ >+ return 0 >+ if $history_entries_to_check <= >+ 0; # No need to check history if only checking current > > # Get the most recent password history entries > my $password_history = $class->search( > { borrowernumber => $borrowernumber }, >- { >+ { > order_by => { -desc => 'created_on' }, >- rows => $history_entries_to_check >+ rows => $history_entries_to_check > } > ); >- >+ > # Check each password history entry >- while (my $history_entry = $password_history->next) { >- # Use checkpw_hash to properly compare the plaintext password with stored hash >- my $matches = C4::Auth::checkpw_hash($password, $history_entry->password); >+ while ( my $history_entry = $password_history->next ) { >+ >+ # Use checkpw_hash to properly compare the plaintext password with stored hash >+ my $matches = >+ C4::Auth::checkpw_hash( $password, $history_entry->password ); > if ($matches) { >- return 1; # Password found in history >+ return 1; # Password found in history > } > } >- >- return 0; # Password not found in history >+ >+ return 0; # Password not found in history > } > > =head1 AUTHOR >diff --git a/Koha/PatronPasswordHistory.pm b/Koha/PatronPasswordHistory.pm >index 175f58de079..67405a93c5d 100644 >--- a/Koha/PatronPasswordHistory.pm >+++ b/Koha/PatronPasswordHistory.pm >@@ -42,6 +42,7 @@ Return the DBIC resultset type for this object > =cut > > sub _type { >+ > # The table name in DB is borrower_password_history > return 'BorrowerPasswordHistory'; > } >@@ -65,44 +66,47 @@ Also handles cleanup of old password history entries. > > sub store { > my ($self) = @_; >- >+ > my $borrowernumber = $self->borrowernumber; >- my $password = $self->password; >- >+ my $password = $self->password; >+ > unless ($password) { > return; > } >- >- # Note: Don't use string comparison for password hashes - this is a common pattern >- # where we're intentionally storing the old password in history >- # The old check would fail when the patron's current hashed password was exactly >- # what we were trying to store (which is the expected case when changing passwords) >- >+ >+# Note: Don't use string comparison for password hashes - this is a common pattern >+# where we're intentionally storing the old password in history >+# The old check would fail when the patron's current hashed password was exactly >+# what we were trying to store (which is the expected case when changing passwords) >+ > # Hash the password if it hasn't been hashed already > unless ( $self->in_storage ) { > if ( $password && $password !~ /\$2a\$/ ) { > $self->password( Koha::AuthUtils::hash_password($password) ); > } > } >- >+ > # Store in database > $self = $self->SUPER::store(); >- >+ >+ # Get the patron and their category >+ my $patron = Koha::Patrons->find($borrowernumber); >+ return $self unless $patron; >+ > # Get the history count preference >- my $history_count = C4::Context->preference('PasswordHistoryCount') || 0; >- >+ my $history_count = >+ $patron->category->effective_password_history_count || 0; >+ > # Clean up old history entries > # Adjust history count to account for current password >- my $history_entries_to_keep = $history_count <= 1 >- ? $history_count # If 0 or 1, keep that many >- : $history_count - 1; # Otherwise keep historical entries minus current >- >+ my $history_entries_to_keep = $history_count <= 1 >+ ? $history_count # If 0 or 1, keep that many >+ : $history_count - 1; # Otherwise keep historical entries minus current >+ > # Clean up old entries > Koha::PatronPasswordHistories->cleanup_old_password_history( >- $borrowernumber, >- $history_entries_to_keep >- ); >- >+ $borrowernumber, $history_entries_to_keep ); >+ > return $self; > } > >-- >2.39.5
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 40824
:
186511
|
186512
|
186513
|
186514
|
186515
|
186516
|
186517
|
186518
|
186519
|
186520
|
186521
|
186522
| 186523 |
186524
|
186525
|
186526
|
186527