From 3e217b292eac233b3057878420ad9f14dd843521 Mon Sep 17 00:00:00 2001 From: Agustin Moyano Date: Thu, 27 Aug 2020 12:58:56 -0300 Subject: [PATCH] Bug 23816: (follow-up) Fix many things This patch: * reverts changes on misc/admin/set_password.pl * makes category param mandatory for AuthUtils::is_valid_password and AuthUtils::generate_password * changes onboarding.pl to set category param in AuthUtils::is_valid_password * Completes t/db_dependent/AuthUtils.t and drops t/AuthUtils.t * Removes offending and replaces it by https://bugs.koha-community.org/show_bug.cgi?id=23826 Signed-off-by: Katrin Fischer --- Koha/AuthUtils.pm | 13 +- Koha/Exceptions/Password.pm | 4 + installer/onboarding.pl | 12 +- .../prog/en/modules/admin/categories.tt | 2 +- misc/admin/set_password.pl | 13 +- t/AuthUtils.t | 73 ----------- t/db_dependent/AuthUtils.t | 140 ++++++++++++++------- 7 files changed, 123 insertions(+), 134 deletions(-) delete mode 100644 t/AuthUtils.t diff --git a/Koha/AuthUtils.pm b/Koha/AuthUtils.pm index a325c9cbdd..3bf28ef73a 100644 --- a/Koha/AuthUtils.pm +++ b/Koha/AuthUtils.pm @@ -23,6 +23,7 @@ use Encode qw( encode is_utf8 ); use Fcntl qw/O_RDONLY/; # O_RDONLY is used in generate_salt use List::MoreUtils qw/ any /; use String::Random qw( random_string ); +use Koha::Exceptions::Password; use C4::Context; @@ -148,12 +149,15 @@ otherwise return $is_valid == 0 and $error will contain the error ('too_short' o sub is_password_valid { my ($password, $category) = @_; - my $minPasswordLength = $category?$category->effective_min_password_length:C4::Context->preference('minPasswordLength'); + if(!$category) { + Koha::Exceptions::Password::NoCategoryProvided->throw(); + } + my $minPasswordLength = $category->effective_min_password_length; $minPasswordLength = 3 if not $minPasswordLength or $minPasswordLength < 3; if ( length($password) < $minPasswordLength ) { return ( 0, 'too_short' ); } - elsif ( $category?$category->effective_require_strong_password:C4::Context->preference('RequireStrongPassword') ) { + elsif ( $category->effective_require_strong_password ) { return ( 0, 'too_weak' ) if $password !~ m|(?=.*\d)(?=.*[a-z])(?=.*[A-Z]).{$minPasswordLength,}|; } @@ -171,7 +175,10 @@ Generate a password according to category's minimum password length and strength sub generate_password { my ($category) = @_; - my $minPasswordLength = $category?$category->effective_min_password_length:C4::Context->preference('minPasswordLength'); + if(!$category) { + Koha::Exceptions::Password::NoCategoryProvided->throw(); + } + my $minPasswordLength = $category->effective_min_password_length; $minPasswordLength = 8 if not $minPasswordLength or $minPasswordLength < 8; my ( $password, $is_valid ); diff --git a/Koha/Exceptions/Password.pm b/Koha/Exceptions/Password.pm index 9cf24f0db0..549e3b9519 100644 --- a/Koha/Exceptions/Password.pm +++ b/Koha/Exceptions/Password.pm @@ -43,6 +43,10 @@ use Exception::Class ( isa => 'Koha::Exceptions::Password', description => 'The password was rejected by a plugin' }, + 'Koha::Exceptions::Password::NoCategoryProvided' => { + isa => 'Koha::Exceptions::Password', + description => 'You must provide a patron\'s category to validate password\'s strength and length' + } ); sub full_message { diff --git a/installer/onboarding.pl b/installer/onboarding.pl index 7a76342311..e23fe44eab 100755 --- a/installer/onboarding.pl +++ b/installer/onboarding.pl @@ -144,8 +144,14 @@ if ( $step == 3 ) { my $secondpassword = $input->param('password2') || ''; my $cardnumber = $input->param('cardnumber'); my $userid = $input->param('userid'); + my $categorycode = $input->param('categorycode_entry'); + my $patron_category = + Koha::Patron::Categories->find( $categorycode ); + + my ( $is_valid, $passworderror ) = + Koha::AuthUtils::is_password_valid( $firstpassword, + $patron_category ); - my ( $is_valid, $passworderror) = Koha::AuthUtils::is_password_valid( $firstpassword ); if ( my $error_code = checkcardnumber($cardnumber) ) { if ( $error_code == 1 ) { @@ -171,7 +177,7 @@ if ( $step == 3 ) { firstname => scalar $input->param('firstname'), cardnumber => scalar $input->param('cardnumber'), branchcode => scalar $input->param('libraries'), - categorycode => scalar $input->param('categorycode_entry'), + categorycode => $categorycode, userid => scalar $input->param('userid'), privacy => "default", address => "", @@ -179,8 +185,6 @@ if ( $step == 3 ) { flags => 1, # Will be superlibrarian }; - my $patron_category = - Koha::Patron::Categories->find( $patron_data->{categorycode} ); $patron_data->{dateexpiry} = $patron_category->get_expiry_date( $patron_data->{dateenrolled} ); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt index 091936f38b..ef1ca35234 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt @@ -231,7 +231,7 @@
  • - + Leave blank to use system default ([% Koha.Preference('minPasswordLength') | html %])
  • diff --git a/misc/admin/set_password.pl b/misc/admin/set_password.pl index feec8f330f..0915143503 100755 --- a/misc/admin/set_password.pl +++ b/misc/admin/set_password.pl @@ -44,6 +44,11 @@ unless ( $userid or $patron_id or $cardnumber ) { pod2usage("cardnumber is mandatory") unless $cardnumber; } +unless ($password) { + my $generator = String::Random->new( rand_gen => \&alt_rand ); + $password = $generator->randregex('[A-Za-z][A-Za-z0-9_]{6}.[A-Za-z][A-Za-z0-9_]{6}\d'); +} + my $filter; if ( $userid ) { @@ -65,14 +70,6 @@ unless ( $patrons->count > 0 ) { } my $patron = $patrons->next; - -unless ($password) { - my $generator = String::Random->new( rand_gen => \&alt_rand ); - my $n = $patron->category->effective_min_password_length; - $n = $n<6?6:$n; - $password = $generator->randregex('[A-Za-z][A-Za-z0-9_]{6}.[A-Za-z][A-Za-z0-9_]{'.$n.'}\d'); -} - $patron->set_password({ password => $password, skip_validation => 1 }); print $patron->userid . " " . $password . "\n"; diff --git a/t/AuthUtils.t b/t/AuthUtils.t deleted file mode 100644 index aa73e1651c..0000000000 --- a/t/AuthUtils.t +++ /dev/null @@ -1,73 +0,0 @@ -# This file is part of Koha. -# -# Copyright (C) 2013 Equinox Software, Inc. -# Copyright 2017 Koha Development Team -# -# 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 => 3; - -use t::lib::Mocks; -use Koha::AuthUtils qw/hash_password/; - -my $hash1 = hash_password('password'); -my $hash2 = hash_password('password'); - -ok($hash1 ne $hash2, 'random salts used when generating password hash'); - -subtest 'is_password_valid' => sub { - plan tests => 12; - - my ( $is_valid, $error ); - - 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, '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( $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' ); - - t::lib::Mocks::mock_preference('RequireStrongPassword', 1); - t::lib::Mocks::mock_preference('minPasswordLength', 8); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( '12345678' ); - 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( $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( $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); - my $all_valid = 1; - for ( 1 .. 10 ) { - my $password = Koha::AuthUtils::generate_password; - my ( $is_valid, undef ) = Koha::AuthUtils::is_password_valid( $password ); - $all_valid = 0 unless $is_valid; - } - is ( $all_valid, 1, 'generate_password should generate valid passwords' ); -}; diff --git a/t/db_dependent/AuthUtils.t b/t/db_dependent/AuthUtils.t index 5fe62957ae..fa5bc76038 100644 --- a/t/db_dependent/AuthUtils.t +++ b/t/db_dependent/AuthUtils.t @@ -18,6 +18,7 @@ use Modern::Perl; use Test::More tests => 2; +use Test::Exception; use Test::MockModule; use t::lib::Mocks; use t::lib::TestBuilder; @@ -28,87 +29,136 @@ my $builder = t::lib::TestBuilder->new; $schema->storage->txn_begin; -my $category1 = $builder->build_object({class=>'Koha::Patron::Categories', value=>{min_password_length => 15, require_strong_password => 1}}); -my $category2 = $builder->build_object({class=>'Koha::Patron::Categories', value=>{min_password_length => 5, require_strong_password => undef}}); -my $category3 = $builder->build_object({class=>'Koha::Patron::Categories', value=>{min_password_length => undef, require_strong_password => 1}}); -my $category4 = $builder->build_object({class=>'Koha::Patron::Categories', value=>{min_password_length => undef, require_strong_password => undef}}); - -my $p_3l_weak = '123'; -my $p_3l_strong = '1Ab'; -my $p_5l_weak = 'abcde'; -my $p_15l_weak = '0123456789abcdf'; -my $p_5l_strong = 'Abc12'; +my $category1 = $builder->build_object( + { + class => 'Koha::Patron::Categories', + value => { min_password_length => 15, require_strong_password => 1 } + } +); +my $category2 = $builder->build_object( + { + class => 'Koha::Patron::Categories', + value => { min_password_length => 5, require_strong_password => undef } + } +); +my $category3 = $builder->build_object( + { + class => 'Koha::Patron::Categories', + value => { min_password_length => undef, require_strong_password => 1 } + } +); +my $category4 = $builder->build_object( + { + class => 'Koha::Patron::Categories', + value => + { min_password_length => undef, require_strong_password => undef } + } +); + +my $p_2l = '1A'; +my $p_3l_weak = '123'; +my $p_3l_strong = '1Ab'; +my $p_5l_weak = 'abcde'; +my $p_15l_weak = '0123456789abcdf'; +my $p_5l_strong = 'Abc12'; my $p_15l_strong = '0123456789AbCdF'; subtest 'is_password_valid for category' => sub { - plan tests => 12; + plan tests => 15; my ( $is_valid, $error ); - t::lib::Mocks::mock_preference('RequireStrongPassword', 0); - t::lib::Mocks::mock_preference('minPasswordLength', 3); + t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); + t::lib::Mocks::mock_preference( 'minPasswordLength', 3 ); #Category 1 - override=>1, length=>15, strong=>1 - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $p_5l_strong, $category1 ); - is($is_valid, 0, 'min password length for this category is 15'); - is($error, 'too_short', 'min password length for this category is 15'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_5l_strong, $category1 ); + is( $is_valid, 0, 'min password length for this category is 15' ); + is( $error, 'too_short', 'min password length for this category is 15' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $p_15l_weak, $category1 ); - is($is_valid, 0, 'password should be strong for this category'); - is($error, 'too_weak', 'password should be strong for this category'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_15l_weak, $category1 ); + is( $is_valid, 0, 'password should be strong for this category' ); + is( $error, 'too_weak', 'password should be strong for this category' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $p_15l_strong, $category1 ); - is($is_valid, 1, 'password should be ok for this category'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_15l_strong, $category1 ); + is( $is_valid, 1, 'password should be ok for this category' ); #Category 2 - override=>1, length=>5, strong=>0 - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $p_3l_strong, $category2 ); - is($is_valid, 0, 'min password length for this category is 5'); - is($error, 'too_short', 'min password length for this category is 5'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_3l_strong, $category2 ); + is( $is_valid, 0, 'min password length for this category is 5' ); + is( $error, 'too_short', 'min password length for this category is 5' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $p_5l_weak, $category2 ); - is($is_valid, 1, 'password should be ok for this category'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_5l_weak, $category2 ); + is( $is_valid, 1, 'password should be ok for this category' ); #Category 3 - override=>0, length=>20, strong=>0 - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $p_3l_weak, $category3 ); - is($is_valid, 0, 'password should be strong'); - is($error, 'too_weak', 'password should be strong'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_3l_weak, $category3 ); + is( $is_valid, 0, 'password should be strong' ); + is( $error, 'too_weak', 'password should be strong' ); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $p_3l_strong, $category3 ); - is($is_valid, 1, 'password should be ok'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_3l_strong, $category3 ); + is( $is_valid, 1, 'password should be ok' ); #Category 4 - default settings - override=>undef, length=>undef, strong=>undef - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $p_3l_weak, $category4 ); - is($is_valid, 1, 'password should be ok'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_3l_weak, $category4 ); + is( $is_valid, 1, 'password should be ok' ); + + t::lib::Mocks::mock_preference( 'minPasswordLength', 0 ); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $p_2l, $category4 ); + is( $is_valid, 0, '3 is absolute minimum password' ); + is( $error, 'too_short', '3 is absolute minimum password' ); + + throws_ok { Koha::AuthUtils::is_password_valid($p_2l); } + 'Koha::Exceptions::Password::NoCategoryProvided', + 'Category should always be provided'; + }; subtest 'generate_password for category' => sub { - plan tests => 4; + plan tests => 5; my ( $is_valid, $error ); - t::lib::Mocks::mock_preference('RequireStrongPassword', 0); - t::lib::Mocks::mock_preference('minPasswordLength', 3); + t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); + t::lib::Mocks::mock_preference( 'minPasswordLength', 3 ); #Category 4 my $password = Koha::AuthUtils::generate_password($category4); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $password, $category4 ); - is($is_valid, 1, 'password should be ok'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $password, $category4 ); + is( $is_valid, 1, 'password should be ok' ); #Category 3 $password = Koha::AuthUtils::generate_password($category3); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $password, $category3 ); - is($is_valid, 1, 'password should be ok'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $password, $category3 ); + is( $is_valid, 1, 'password should be ok' ); #Category 2 $password = Koha::AuthUtils::generate_password($category2); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $password, $category2 ); - is($is_valid, 1, 'password should be ok'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $password, $category2 ); + is( $is_valid, 1, 'password should be ok' ); #Category 1 $password = Koha::AuthUtils::generate_password($category1); - ( $is_valid, $error ) = Koha::AuthUtils::is_password_valid( $password, $category1 ); - is($is_valid, 1, 'password should be ok'); + ( $is_valid, $error ) = + Koha::AuthUtils::is_password_valid( $password, $category1 ); + is( $is_valid, 1, 'password should be ok' ); + + throws_ok { Koha::AuthUtils::generate_password(); } + 'Koha::Exceptions::Password::NoCategoryProvided', + 'Category should always be provided'; }; -$schema->storage->txn_rollback; \ No newline at end of file +$schema->storage->txn_rollback; -- 2.11.0