From a1614e92ab4b20511821522e80848a90f6870215 Mon Sep 17 00:00:00 2001 From: Jacob O'Mara 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 | 77 ++++++++++++++++++++------------- Koha/PatronPasswordHistory.pm | 18 +++++--- 3 files changed, 60 insertions(+), 37 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 6af253a13b1..b47defb8aa5 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1162,7 +1162,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 82609e4d016..876ff2cb73e 100644 --- a/Koha/PatronPasswordHistories.pm +++ b/Koha/PatronPasswordHistories.pm @@ -21,6 +21,7 @@ use Koha::Database; use Koha::PatronPasswordHistory; use C4::Context; use Koha::AuthUtils; +use Koha::Patrons; use base qw(Koha::Objects); @@ -42,15 +43,17 @@ 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; } @@ -59,18 +62,20 @@ sub cleanup_old_password_history { { 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; @@ -83,6 +88,7 @@ Return the DBIC resultset type for this class =cut sub _type { + # The table name in DB is borrower_password_history return 'BorrowerPasswordHistory'; } @@ -112,58 +118,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; + 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 + 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 816b5737bf3..5d3f0bb8213 100644 --- a/Koha/PatronPasswordHistory.pm +++ b/Koha/PatronPasswordHistory.pm @@ -40,6 +40,7 @@ Return the DBIC resultset type for this object =cut sub _type { + # The table name in DB is borrower_password_history return 'BorrowerPasswordHistory'; } @@ -65,7 +66,7 @@ sub store { my ($self) = @_; my $borrowernumber = $self->borrowernumber; - my $password = $self->password; + my $password = $self->password; unless ($password) { return; @@ -78,20 +79,23 @@ sub store { # 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 + ? $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