Bugzilla – Attachment 109527 Details for
Bug 23816
Allow to have different password strength and length settings for different patron categories
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23816: (follow-up) Fix many things
Bug-23816-follow-up-Fix-many-things.patch (text/plain), 19.02 KB, created by
Tomás Cohen Arazi (tcohen)
on 2020-09-02 12:55:13 UTC
(
hide
)
Description:
Bug 23816: (follow-up) Fix many things
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2020-09-02 12:55:13 UTC
Size:
19.02 KB
patch
obsolete
>From 54ce22013d3bc8919563dbd73a4c7c709fa35f1e Mon Sep 17 00:00:00 2001 >From: Agustin Moyano <agustinmoyano@theke.io> >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 <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> >--- > 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 400db6241b..99dac40f52 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 @@ > </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 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 <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 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.28.0
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=109527">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23816
:
103099
|
103100
|
103101
|
103102
|
103103
|
103104
|
103108
|
103109
|
103110
|
103111
|
103112
|
103113
|
103805
|
103806
|
103807
|
103808
|
103809
|
103810
|
107158
|
107159
|
107160
|
107161
|
107162
|
107163
|
108554
|
108555
|
108556
|
108557
|
108558
|
108559
|
108571
|
108572
|
109243
|
109244
|
109245
|
109249
|
109250
|
109251
|
109252
|
109253
|
109254
|
109255
|
109256
|
109257
|
109520
|
109521
|
109522
|
109523
|
109524
|
109525
|
109526
| 109527 |
109528
|
109833
|
109834