From bcc0bbe3192e7d37fcb6b18a7558d3f8e8ec6593 Mon Sep 17 00:00:00 2001 From: Jacob O'Mara Date: Thu, 10 Apr 2025 12:26:55 +0100 Subject: [PATCH] Bug 40824: Create borrower_password_history table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- installer/data/mysql/kohastructure.sql | 20 ++ t/db_dependent/Koha/Patron.t | 2 +- t/db_dependent/Koha/PatronPasswordHistories.t | 263 ++++++++++++++++++ t/db_dependent/Koha/PatronPasswordHistory.t | 203 ++++++++++++++ 4 files changed, 487 insertions(+), 1 deletion(-) create mode 100755 t/db_dependent/Koha/PatronPasswordHistories.t create mode 100755 t/db_dependent/Koha/PatronPasswordHistory.t diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index e5bb429ecec..b59e4b090d2 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1816,6 +1816,7 @@ CREATE TABLE `categories` ( `can_be_guarantee` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'if patrons of this category can be guarantees', `reset_password` tinyint(1) DEFAULT NULL COMMENT 'if patrons of this category can do the password reset flow,', `change_password` tinyint(1) DEFAULT NULL COMMENT 'if patrons of this category can change their passwords in the OAPC', + `password_history_count` smallint(6) DEFAULT NULL COMMENT 'Number of previous passwords to check against when changing password for this patron type', `min_password_length` smallint(6) DEFAULT NULL COMMENT 'set minimum password length for patrons in this category', `require_strong_password` tinyint(1) DEFAULT NULL COMMENT 'set required password strength for patrons in this category', `force_password_reset_when_set_by_staff` tinyint(1) DEFAULT NULL COMMENT 'if patrons of this category are required to reset password after being created by a staff member', @@ -6867,6 +6868,25 @@ CREATE TABLE `zebraqueue` ( /*!40101 SET character_set_client = @saved_cs_client */; /*!40103 SET TIME_ZONE=@OLD_TIME_ZONE */; +-- +-- Table structure for table `borrower_password_history` +-- + +DROP TABLE IF EXISTS `borrower_password_history`; +/*!40101 SET @saved_cs_client = @@character_set_client */; +/*!40101 SET character_set_client = utf8 */; +CREATE TABLE IF NOT EXISTS borrower_password_history ( + id int(11) NOT NULL AUTO_INCREMENT, + borrowernumber int(11) NOT NULL, + password varchar(128) NOT NULL, + created_on timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (id), + KEY borrowernumber (borrowernumber), + CONSTRAINT borrower_password_history_ibfk_1 FOREIGN KEY (borrowernumber) REFERENCES borrowers(borrowernumber) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; +/*!40101 SET character_set_client = @saved_cs_client */; + + /*!40101 SET SQL_MODE=@OLD_SQL_MODE */; /*!40014 SET FOREIGN_KEY_CHECKS=@OLD_FOREIGN_KEY_CHECKS */; /*!40014 SET UNIQUE_CHECKS=@OLD_UNIQUE_CHECKS */; diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index 8a6769cde00..5af4dae347e 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 45; +use Test::More tests => 47; use Test::NoWarnings; use Test::Exception; use Test::Warn; diff --git a/t/db_dependent/Koha/PatronPasswordHistories.t b/t/db_dependent/Koha/PatronPasswordHistories.t new file mode 100755 index 00000000000..dff9a1470d6 --- /dev/null +++ b/t/db_dependent/Koha/PatronPasswordHistories.t @@ -0,0 +1,263 @@ +#!/usr/bin/perl + +# This file is part of Koha +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Test::More tests => 4; +use Test::NoWarnings; + +use t::lib::TestBuilder; +use t::lib::Mocks; + +use Koha::Database; +use Koha::PatronPasswordHistory; +use Koha::PatronPasswordHistories; +use Koha::AuthUtils; + +my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new; + +subtest 'cleanup_old_password_history' => sub { + plan tests => 6; + + $schema->storage->txn_begin; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + + # Create 5 password history entries + my @passwords = (); + for my $i ( 1 .. 5 ) { + my $password_history = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => Koha::AuthUtils::hash_password("password_$i") + } + ); + $password_history->store; + push @passwords, $password_history; + sleep(1); # Ensure different timestamps + } + + my $count = Koha::PatronPasswordHistories->search( { borrowernumber => $patron->borrowernumber } )->count; + is( $count, 5, 'Initially have 5 password history entries' ); + + # Keep 3 most recent + my $deleted = Koha::PatronPasswordHistories->cleanup_old_password_history( $patron->borrowernumber, 3 ); + is( $deleted, 2, 'Deleted 2 old entries' ); + + $count = Koha::PatronPasswordHistories->search( { borrowernumber => $patron->borrowernumber } )->count; + is( $count, 3, 'Now have 3 password history entries' ); + + # Keep 1 most recent + $deleted = Koha::PatronPasswordHistories->cleanup_old_password_history( $patron->borrowernumber, 1 ); + is( $deleted, 2, 'Deleted 2 more entries' ); + + $count = Koha::PatronPasswordHistories->search( { borrowernumber => $patron->borrowernumber } )->count; + is( $count, 1, 'Now have 1 password history entry' ); + + # Delete all (count = 0) + $deleted = Koha::PatronPasswordHistories->cleanup_old_password_history( $patron->borrowernumber, 0 ); + is( $deleted, 1, 'Deleted remaining entry' ); + + $schema->storage->txn_rollback; +}; + +subtest 'has_used_password with category settings' => sub { + plan tests => 10; + + $schema->storage->txn_begin; + + my $category = $builder->build_object( + { + class => 'Koha::Patron::Categories', + value => { password_history_count => 3 } + } + ); + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + categorycode => $category->categorycode, + password => Koha::AuthUtils::hash_password('current_password') + } + } + ); + + # Create password history + my $old_password1 = 'old_password_1'; + my $old_password2 = 'old_password_2'; + + my $history1 = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => Koha::AuthUtils::hash_password($old_password1) + } + ); + $history1->store; + sleep(1); + + my $history2 = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => Koha::AuthUtils::hash_password($old_password2) + } + ); + $history2->store; + + # Test checking against current password + my $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => 'current_password', + current_password => $patron->password + } + ); + ok( $is_used, 'Current password detected as already used' ); + + # Test checking against history + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => $old_password1, + current_password => $patron->password + } + ); + ok( $is_used, 'Old password from history detected as already used' ); + + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => $old_password2, + current_password => $patron->password + } + ); + ok( $is_used, 'Another old password from history detected as already used' ); + + # Test new password + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => 'brand_new_password', + current_password => $patron->password + } + ); + ok( !$is_used, 'New password not detected as used' ); + + # Test with category history_count = 1 (only checks current) + $category->password_history_count(1)->store; + + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => $old_password1, + current_password => $patron->password + } + ); + ok( !$is_used, 'With history_count=1, old passwords not checked against history' ); + + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => 'current_password', + current_password => $patron->password + } + ); + ok( $is_used, 'With history_count=1, current password still checked' ); + + # Test with category history_count = 0 (no checking) + $category->password_history_count(0)->store; + + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => 'current_password', + current_password => $patron->password + } + ); + ok( !$is_used, 'With history_count=0, no password checking performed' ); + + # Test fallback to system preference + $category->password_history_count(undef)->store; + t::lib::Mocks::mock_preference( 'PasswordHistoryCount', 2 ); + + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => $old_password2, + current_password => $patron->password + } + ); + ok( $is_used, 'Falls back to system preference when category setting is null' ); + + # Test without current password parameter + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $patron->borrowernumber, + password => $old_password1 + } + ); + ok( $is_used, 'Can check password history without current_password parameter' ); + + # Test with patron that has no password history + my $new_patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + categorycode => $category->categorycode, + password => Koha::AuthUtils::hash_password('new_patron_password') + } + } + ); + + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => $new_patron->borrowernumber, + password => 'some_password', + current_password => $new_patron->password + } + ); + ok( !$is_used, 'Returns false for patron with no password history' ); + + $schema->storage->txn_rollback; +}; + +subtest 'edge cases and validation' => sub { + plan tests => 4; + + $schema->storage->txn_begin; + + # Test with missing parameters + my $is_used = Koha::PatronPasswordHistories->has_used_password( {} ); + ok( !$is_used, 'Returns false with no parameters' ); + + $is_used = Koha::PatronPasswordHistories->has_used_password( { borrowernumber => 99999 } ); + ok( !$is_used, 'Returns false with missing password' ); + + $is_used = Koha::PatronPasswordHistories->has_used_password( { password => 'test' } ); + ok( !$is_used, 'Returns false with missing borrowernumber' ); + + # Test with non-existent patron + $is_used = Koha::PatronPasswordHistories->has_used_password( + { + borrowernumber => 99999, + password => 'test_password' + } + ); + ok( !$is_used, 'Returns false for non-existent patron' ); + + $schema->storage->txn_rollback; +}; diff --git a/t/db_dependent/Koha/PatronPasswordHistory.t b/t/db_dependent/Koha/PatronPasswordHistory.t new file mode 100755 index 00000000000..809618db176 --- /dev/null +++ b/t/db_dependent/Koha/PatronPasswordHistory.t @@ -0,0 +1,203 @@ +#!/usr/bin/perl + +# This file is part of Koha +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see . + +use Modern::Perl; + +use Test::More tests => 4; +use Test::NoWarnings; + +use t::lib::TestBuilder; +use t::lib::Mocks; + +use Koha::Database; +use Koha::PatronPasswordHistory; +use Koha::PatronPasswordHistories; + +my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new; + +subtest 'basic functionality' => sub { + plan tests => 3; + + $schema->storage->txn_begin; + + my $category = $builder->build_object( + { + class => 'Koha::Patron::Categories', + value => { password_history_count => 3 } + } + ); + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { categorycode => $category->categorycode } + } + ); + + my $password_history = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => 'plaintext_password' + } + ); + + ok( $password_history, 'PatronPasswordHistory object created' ); + isa_ok( $password_history, 'Koha::PatronPasswordHistory' ); + + $password_history->store; + ok( $password_history->in_storage, 'PatronPasswordHistory stored successfully' ); + + $schema->storage->txn_rollback; +}; + +subtest 'store password' => sub { + plan tests => 1; + + $schema->storage->txn_begin; + + my $category = $builder->build_object( + { + class => 'Koha::Patron::Categories', + value => { password_history_count => 3 } + } + ); + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { categorycode => $category->categorycode } + } + ); + + my $plain_password = 'test_password_123'; + my $hashed_password = Koha::AuthUtils::hash_password($plain_password); + my $password_history = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => $hashed_password + } + ); + + $password_history->store; + + is( $password_history->password, $hashed_password, 'Password is stored as provided' ); + + $schema->storage->txn_rollback; +}; + +subtest 'cleanup of old history entries' => sub { + plan tests => 5; + + $schema->storage->txn_begin; + + my $category = $builder->build_object( + { + class => 'Koha::Patron::Categories', + value => { password_history_count => 3 } + } + ); + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { categorycode => $category->categorycode } + } + ); + + # Create multiple password history entries + for my $i ( 1 .. 5 ) { + my $password_history = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => "password_$i" + } + ); + $password_history->store; + sleep(1); # Ensure different created_on timestamps + } + + my $count = Koha::PatronPasswordHistories->search( { borrowernumber => $patron->borrowernumber } )->count; + + # With password_history_count = 3, we should keep 2 historical entries (3 - 1 for current) + is( $count, 2, 'Old password history entries cleaned up correctly (kept 2 for history_count 3)' ); + + # Test with different category setting - add more entries to test the limit + $category->password_history_count(4)->store; + + # Add more password entries to exceed the limit + for my $i ( 6 .. 10 ) { + my $password_history = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => "password_$i" + } + ); + $password_history->store; + sleep(1); + } + + $count = Koha::PatronPasswordHistories->search( { borrowernumber => $patron->borrowernumber } )->count; + + # With password_history_count = 4, we should keep 3 historical entries (4 - 1 for current) + is( $count, 3, 'Cleanup respects updated category password_history_count and does not exceed limit' ); + + # Test with history_count = 1 (should keep 1 historical entry) + $category->password_history_count(1)->store; + + my $password_history2 = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => "password_7" + } + ); + $password_history2->store; + + $count = Koha::PatronPasswordHistories->search( { borrowernumber => $patron->borrowernumber } )->count; + + is( $count, 1, 'With history_count=1, keeps exactly 1 entry' ); + + # Test with history_count = 0 (should delete all) + $category->password_history_count(0)->store; + + my $password_history3 = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => "password_8" + } + ); + $password_history3->store; + + $count = Koha::PatronPasswordHistories->search( { borrowernumber => $patron->borrowernumber } )->count; + + is( $count, 0, 'With history_count=0, deletes all entries' ); + + # Test fallback to system preference + $category->password_history_count(undef)->store; + t::lib::Mocks::mock_preference( 'PasswordHistoryCount', 2 ); + + my $password_history4 = Koha::PatronPasswordHistory->new( + { + borrowernumber => $patron->borrowernumber, + password => "password_9" + } + ); + $password_history4->store; + + $count = Koha::PatronPasswordHistories->search( { borrowernumber => $patron->borrowernumber } )->count; + + is( $count, 1, 'Falls back to system preference when category setting is null' ); + + $schema->storage->txn_rollback; +}; -- 2.39.5