From 10031b41cce26657bb1adb60a3400336831672a8 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 | 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