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 <input type="number"/> and replaces it by <input type="text" inputmode="numeric" pattern="[0-9]*"/>

https://bugs.koha-community.org/show_bug.cgi?id=23826

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

diff --git a/Koha/AuthUtils.pm b/Koha/AuthUtils.pm
index a325c9c..3bf28ef 100644 (file)
--- 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 9cf24f0..549e3b9 100644 (file)
--- 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 3edd866..f3be391 100755 (executable)
--- 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 400db62..99dac40 100644 (file)
--- 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 @@
                 </li>
                 <li>
                     <label for="min_password_length">Minimum password length:</label>
-                    <input id="min_password_length" type="number" name="min_password_length" value="[% category.min_password_length | html %]"/>
+                    <input id="min_password_length" type="text" inputmode="numeric" pattern="[0-9]*" name="min_password_length" value="[% category.min_password_length | html %]"/>
                     <span>Leave blank to use system default ([% Koha.Preference('minPasswordLength') | html %])</span>
                 </li>
                 <li class="pwd_setting_wrapper">

diff --git a/misc/admin/set_password.pl b/misc/admin/set_password.pl
index feec8f3..0915143 100755 (executable)
--- 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 (file)
index aa73e16..0000000
--- 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 <http://www.gnu.org/licenses>.
-
-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 5fe6295..fa5bc76 100644 (file)
--- 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;