From b3af499e037f0aaa958798d91935852a040da178 Mon Sep 17 00:00:00 2001 From: Aleisha Amohia Date: Tue, 21 Apr 2020 08:29:19 +0000 Subject: [PATCH] Bug 18308: Increase minPasswordLength to 8 characters for new installs To test: 1) Go to Administration -> system preferences -> ensure minPasswordLength syspref is set to 3, or some other number less than 8 2) Go to Patrons -> Add a new patron 3) Notice the alert at the top of the page warning that minPasswordLength is too low. Confirm you are still be able to save your patron with a password that is less than 8 characters. 4) Edit the minPasswordLength syspref to be 8 or higher. 5) Go back to Patrons -> Add a new patron. Confirm the alert is gone. 6) Drop and recreate database to trigger onboarding tool. 7) Go through onboarding tool until you are required to create the superlibrarian patron. Try to enter a password of less than 8 characters. Confirm it does not let you continue until you enter a password that is 8 characters or longer. Sponsored-by: Catalyst IT --- C4/Auth.pm | 2 +- C4/InstallAuth.pm | 2 +- Koha/AuthUtils.pm | 2 +- Koha/Patron.pm | 2 +- installer/data/mysql/sysprefs.sql | 2 +- .../prog/en/modules/members/memberentrygen.tt | 6 ++++++ t/AuthUtils.t | 22 ++++++++++---------- t/db_dependent/Koha/Patrons.t | 24 +++++++++++----------- t/db_dependent/api/v1/patrons_password.t | 16 +++++++-------- 9 files changed, 42 insertions(+), 36 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 1f79ef6187b..bbc644ae185 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -455,7 +455,7 @@ sub get_template_and_user { my $using_https = ( defined $https and $https ne 'OFF' ) ? 1 : 0; my $minPasswordLength = C4::Context->preference('minPasswordLength'); - $minPasswordLength = 3 if not $minPasswordLength or $minPasswordLength < 3; + $minPasswordLength = 8 if not $minPasswordLength or $minPasswordLength < 8; $template->param( "BiblioDefaultView" . C4::Context->preference("BiblioDefaultView") => 1, EnhancedMessagingPreferences => C4::Context->preference('EnhancedMessagingPreferences'), diff --git a/C4/InstallAuth.pm b/C4/InstallAuth.pm index 987045df504..e2f59d2a583 100644 --- a/C4/InstallAuth.pm +++ b/C4/InstallAuth.pm @@ -151,7 +151,7 @@ sub get_template_and_user { } my $minPasswordLength = C4::Context->preference('minPasswordLength'); - $minPasswordLength = 3 if not $minPasswordLength or $minPasswordLength < 3; + $minPasswordLength = 8 if not $minPasswordLength or $minPasswordLength < 8; $template->param(minPasswordLength => $minPasswordLength,); } return ( $template, $borrowernumber, $cookie ); diff --git a/Koha/AuthUtils.pm b/Koha/AuthUtils.pm index 7c4dcd1002b..e961b110537 100644 --- a/Koha/AuthUtils.pm +++ b/Koha/AuthUtils.pm @@ -149,7 +149,7 @@ otherwise return $is_valid == 0 and $error will contain the error ('too_short' o sub is_password_valid { my ($password) = @_; my $minPasswordLength = C4::Context->preference('minPasswordLength'); - $minPasswordLength = 3 if not $minPasswordLength or $minPasswordLength < 3; + $minPasswordLength = 8 if not $minPasswordLength or $minPasswordLength < 8; if ( length($password) < $minPasswordLength ) { return ( 0, 'too_short' ); } diff --git a/Koha/Patron.pm b/Koha/Patron.pm index fe3e80aebb5..ad3231215f5 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -736,7 +736,7 @@ sub set_password { if ( !$is_valid ) { if ( $error eq 'too_short' ) { my $min_length = C4::Context->preference('minPasswordLength'); - $min_length = 3 if not $min_length or $min_length < 3; + $min_length = 8 if not $min_length or $min_length < 8; my $password_length = length($password); Koha::Exceptions::Password::TooShort->throw( diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index e39352448cb..39bcd59fdc2 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -326,7 +326,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('MaxTotalSuggestions','',NULL,'Number of total suggestions used for time limit with NumberOfSuggestionDays','Free'), ('MembershipExpiryDaysNotice','',NULL,'Send an account expiration notice that a patron\'s card is about to expire after','Integer'), ('MergeReportFields','',NULL,'Displayed fields for deleted MARC records after merge','Free'), -('minPasswordLength','3',NULL,'Specify the minimum length of a patron/staff password','free'), +('minPasswordLength','8',NULL,'Specify the minimum length of a patron/staff password','free'), ('NewItemsDefaultLocation','','','If set, all new items will have a location of the given Location Code ( Authorized Value type LOC )',''), ('NewsAuthorDisplay','none','none|opac|staff|both','Display the author name for news items.','Choice'), ('noissuescharge','5','','Define maximum amount withstanding before check outs are blocked','Integer'), diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt index 4b5eb35d126..e142c524e31 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt @@ -105,6 +105,12 @@ legend:hover { [% END %] + [% IF Koha.Preference('minPasswordLength') < 8 %] +
+

Warning: The minPasswordLength system preference is less than 8 characters. This means library patrons can set unsecure passwords. Please set the minPasswordLength system preference to 8 or higher.

+
+ [% END %] + [% IF ( nok ) %]

The following fields are wrong. Please fix them.

diff --git a/t/AuthUtils.t b/t/AuthUtils.t index aa73e1651c5..9380bad17b7 100644 --- a/t/AuthUtils.t +++ b/t/AuthUtils.t @@ -35,34 +35,34 @@ subtest 'is_password_valid' => sub { t::lib::Mocks::mock_preference('RequireStrongPassword', 0); t::lib::Mocks::mock_preference('minPasswordLength', 0); ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( '12' ); - is( $is_valid, 0, 'min password size should be 3' ); - is( $error, 'too_short', 'min password size should be 3' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( ' 123' ); + is( $is_valid, 0, 'min password size should be 8' ); + is( $error, 'too_short', 'min password size should be 8' ); + ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( ' 12345678' ); is( $is_valid, 0, 'password should not contain leading spaces' ); is( $error, 'has_whitespaces', 'password should not contain leading spaces' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( '123 ' ); + ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( '12345678 ' ); is( $is_valid, 0, 'password should not contain trailing spaces' ); is( $error, 'has_whitespaces', 'password should not contain trailing spaces' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( '123' ); - is( $is_valid, 1, 'min password size should be 3' ); + ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( '12345678' ); + is( $is_valid, 1, 'min password size should be 8' ); t::lib::Mocks::mock_preference('RequireStrongPassword', 1); - t::lib::Mocks::mock_preference('minPasswordLength', 8); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( '12345678' ); + t::lib::Mocks::mock_preference('minPasswordLength', 9); + ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( '123456789' ); is( $is_valid, 0, 'password should be strong' ); is( $error, 'too_weak', 'password should be strong' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( 'abcd1234' ); + ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( 'abcd12345' ); is( $is_valid, 0, 'strong password should contain uppercase' ); is( $error, 'too_weak', 'strong password should contain uppercase' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( 'abcD1234' ); + ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( 'abcD12345' ); is( $is_valid, 1, 'strong password should contain uppercase' ); }; subtest 'generate_password' => sub { plan tests => 1; t::lib::Mocks::mock_preference('RequireStrongPassword', 1); - t::lib::Mocks::mock_preference('minPasswordLength', 8); + t::lib::Mocks::mock_preference('minPasswordLength', 9); my $all_valid = 1; for ( 1 .. 10 ) { my $password = Koha::AuthUtils::generate_password; diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 0ca5faa7178..f9ea316b1db 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1788,30 +1788,30 @@ subtest '->set_password' => sub { t::lib::Mocks::mock_preference( 'minPasswordLength', undef ); throws_ok { $patron->set_password({ password => 'ab' }); } 'Koha::Exceptions::Password::TooShort', - 'minPasswordLength is undef, fall back to 3, fail test'; + 'minPasswordLength is undef, fall back to 8, fail test'; is( "$@", - 'Password length (2) is shorter than required (3)', + 'Password length (2) is shorter than required (8)', 'Exception parameters passed correctly' ); t::lib::Mocks::mock_preference( 'minPasswordLength', 2 ); throws_ok { $patron->set_password({ password => 'ab' }); } 'Koha::Exceptions::Password::TooShort', - 'minPasswordLength is 2, fall back to 3, fail test'; + 'minPasswordLength is 2, fall back to 8, fail test'; - t::lib::Mocks::mock_preference( 'minPasswordLength', 5 ); - throws_ok { $patron->set_password({ password => 'abcb' }); } + t::lib::Mocks::mock_preference( 'minPasswordLength', 10 ); + throws_ok { $patron->set_password({ password => 'abcdefghi' }); } 'Koha::Exceptions::Password::TooShort', - 'minPasswordLength is 5, fail test'; + 'minPasswordLength is 10, fail test'; # Trailing spaces tests - throws_ok { $patron->set_password({ password => 'abcD12d ' }); } + throws_ok { $patron->set_password({ password => 'abcDef12d ' }); } 'Koha::Exceptions::Password::WhitespaceCharacters', 'Password contains trailing spaces, exception is thrown'; # Require strong password tests t::lib::Mocks::mock_preference( 'RequireStrongPassword', 1 ); - throws_ok { $patron->set_password({ password => 'abcd a' }); } + throws_ok { $patron->set_password({ password => 'abcdef a' }); } 'Koha::Exceptions::Password::TooWeak', 'Password is too weak, exception is thrown'; @@ -1819,7 +1819,7 @@ subtest '->set_password' => sub { $patron->discard_changes; is( $patron->login_attempts, 3, 'Previous tests kept login attemps count' ); - $patron->set_password({ password => 'abcD12 34' }); + $patron->set_password({ password => 'abcD12345e' }); $patron->discard_changes; is( $patron->login_attempts, 0, 'Changing the password resets the login attempts count' ); @@ -1831,11 +1831,11 @@ subtest '->set_password' => sub { t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); $patron->login_attempts(3)->store; my $old_digest = $patron->password; - $patron->set_password({ password => 'abcd a' }); + $patron->set_password({ password => 'abcd abc' }); $patron->discard_changes; isnt( $patron->password, $old_digest, 'Password has been updated' ); - ok( checkpw_hash('abcd a', $patron->password), 'Password hash is correct' ); + ok( checkpw_hash('abcd abc', $patron->password), 'Password hash is correct' ); is( $patron->login_attempts, 0, 'Login attemps have been reset' ); my $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $patron->borrowernumber } )->count; @@ -1843,7 +1843,7 @@ subtest '->set_password' => sub { # Enable logging password changes t::lib::Mocks::mock_preference( 'BorrowersLog', 1 ); - $patron->set_password({ password => 'abcd b' }); + $patron->set_password({ password => 'abcd babc' }); $number_of_logs = $schema->resultset('ActionLog')->search( { module => 'MEMBERS', action => 'CHANGE PASS', object => $patron->borrowernumber } )->count; is( $number_of_logs, 1, 'With BorrowerLogs, Koha::Patron->set_password does log password changes' ); diff --git a/t/db_dependent/api/v1/patrons_password.t b/t/db_dependent/api/v1/patrons_password.t index 2a57516319d..f2faedacd3d 100644 --- a/t/db_dependent/api/v1/patrons_password.t +++ b/t/db_dependent/api/v1/patrons_password.t @@ -45,10 +45,10 @@ subtest 'set() (authorized user tests)' => sub { my ( $patron, $session ) = create_user_and_session({ authorized => 1 }); - t::lib::Mocks::mock_preference( 'minPasswordLength', 3 ); + t::lib::Mocks::mock_preference( 'minPasswordLength', 8 ); t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); - my $new_password = 'abc'; + my $new_password = 'abcdefgh'; my $tx = $t->ua->build_tx( POST => "/api/v1/patrons/" @@ -68,7 +68,7 @@ subtest 'set() (authorized user tests)' => sub { $tx->req->env( { REMOTE_ADDR => '127.0.0.1' } ); $t->request_ok($tx)->status_is(400)->json_is({ error => 'Passwords don\'t match' }); - t::lib::Mocks::mock_preference( 'minPasswordLength', 5 ); + t::lib::Mocks::mock_preference( 'minPasswordLength', 10 ); $tx = $t->ua->build_tx( POST => "/api/v1/patrons/" . $patron->id @@ -76,9 +76,9 @@ subtest 'set() (authorized user tests)' => sub { $tx->req->cookies( { name => 'CGISESSID', value => $session->id } ); $tx->req->env( { REMOTE_ADDR => '127.0.0.1' } ); - $t->request_ok($tx)->status_is(400)->json_is({ error => 'Password length (3) is shorter than required (5)' }); + $t->request_ok($tx)->status_is(400)->json_is({ error => 'Password length (8) is shorter than required (10)' }); - $new_password = 'abc '; + $new_password = 'abcdefg '; $tx = $t->ua->build_tx( POST => "/api/v1/patrons/" . $patron->id @@ -88,7 +88,7 @@ subtest 'set() (authorized user tests)' => sub { $tx->req->env( { REMOTE_ADDR => '127.0.0.1' } ); $t->request_ok($tx)->status_is(400)->json_is({ error => '[Password contains leading/trailing whitespace character(s)]' }); - $new_password = 'abcdefg'; + $new_password = 'abcdefghijkl'; $tx = $t->ua->build_tx( POST => "/api/v1/patrons/" . $patron->id @@ -134,10 +134,10 @@ subtest 'set_public() (unprivileged user tests)' => sub { t::lib::Mocks::mock_preference( 'RESTPublicAPI', 1 ); t::lib::Mocks::mock_preference( 'OpacPasswordChange', 0 ); - t::lib::Mocks::mock_preference( 'minPasswordLength', 3 ); + t::lib::Mocks::mock_preference( 'minPasswordLength', 8 ); t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); - my $new_password = 'abc'; + my $new_password = 'abcdefgh'; my $tx = $t->ua->build_tx( POST => "/api/v1/public/patrons/" -- 2.11.0