From b1e9498b6f8da41162ac0b20b14cd43a95a1288b Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 15 Dec 2023 10:54:11 -0300 Subject: [PATCH] Bug 34893: Unit tests for C4::Auth::checkpw This patch introduces some tests on the current (and new) behavior for the `checkpw` function. I needed it to better understand if an edge case was actually possible (it wasn't). Found a really minor annoyance for the internal check with expired password not returning the $patron object for consistency with the other use cases. I think this method deserves (at least) changing the return value to a sane data structure. But that's not target for backporting to stable releases. So a separate bug. Signed-off-by: Tomas Cohen Arazi (cherry picked from commit 5476b18e7ea34e08d9dd163e2c446d5b223cf032) Signed-off-by: Fridolin Somers --- C4/Auth.pm | 2 +- t/db_dependent/Auth.t | 219 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 219 insertions(+), 2 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index bcf65844a4..74b73563a4 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -2017,7 +2017,7 @@ sub checkpw { if ($passwd_ok) { $patron->update( { login_attempts => 0 } ); if ( $patron->password_expired ) { - @return = (-2); + @return = ( -2, $patron ); } } elsif ( !$patron->account_locked ) { $patron->update( { login_attempts => $patron->login_attempts + 1 } ); diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index 46b500c013..df79b09e54 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -7,7 +7,7 @@ use CGI qw ( -utf8 ); use Test::MockObject; use Test::MockModule; use List::MoreUtils qw/all any none/; -use Test::More tests => 18; +use Test::More tests => 19; use Test::Warn; use t::lib::Mocks; use t::lib::TestBuilder; @@ -1086,3 +1086,220 @@ subtest 'get_cataloguing_page_permissions() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'checkpw() return values tests' => sub { + + plan tests => 3; + + subtest 'Internal check tests' => sub { + + plan tests => 25; + + $schema->storage->txn_begin; + + my $account_locked; + my $password_expired; + + my $mock_patron = Test::MockModule->new('Koha::Patron'); + $mock_patron->mock( 'account_locked', sub { return $account_locked; } ); + $mock_patron->mock( 'password_expired', sub { return $password_expired; } ); + + # Only interested here in regular login + t::lib::Mocks::mock_config( 'useshibboleth', undef ); + $C4::Auth::cas = 0; + $C4::Auth::ldap = 0; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $password = 'thePassword123'; + $patron->set_password( { password => $password, skip_validation => 1 } ); + + my $patron_to_delete = $builder->build_object( { class => 'Koha::Patrons' } ); + my $unused_userid = $patron_to_delete->userid; + my $unused_cardnumber = $patron_to_delete->cardnumber; + $patron_to_delete->delete; + + $account_locked = 1; + my @return = checkpw( $patron->userid, $password, undef, ); + is_deeply( \@return, [], 'If the account is locked, empty list is returned' ); + + $account_locked = 0; + + my @matchpoints = qw(userid cardnumber); + foreach my $matchpoint (@matchpoints) { + + @return = checkpw( $patron->$matchpoint, $password, undef, ); + + is( $return[0], 1, "Password validation successful returns 1 ($matchpoint)" ); + is( $return[1], $patron->cardnumber, '`cardnumber` returned' ); + is( $return[2], $patron->userid, '`userid` returned' ); + is( ref( $return[3] ), 'Koha::Patron', 'Koha::Patron object reference returned' ); + is( $return[3]->id, $patron->id, 'Correct patron returned' ); + } + + @return = checkpw( $patron->userid, $password . 'hey', undef, ); + + is( scalar @return, 2, "Two results on invalid password scenario" ); + is( $return[0], 0, '0 returned on invalid password' ); + is( ref( $return[1] ), 'Koha::Patron' ); + is( $return[1]->id, $patron->id, 'Patron matched correctly' ); + + $password_expired = 1; + @return = checkpw( $patron->userid, $password, undef, ); + + is( scalar @return, 2, "Two results on expired password scenario" ); + is( $return[0], -2, '-2 returned' ); + is( ref( $return[1] ), 'Koha::Patron' ); + is( $return[1]->id, $patron->id, 'Patron matched correctly' ); + + @return = checkpw( $unused_userid, $password, undef, ); + + is( scalar @return, 2, "Two results on non-existing userid scenario" ); + is( $return[0], 0, '0 returned' ); + is( $return[1], undef, 'Undef returned, representing no match' ); + + @return = checkpw( $unused_cardnumber, $password, undef, ); + + is( scalar @return, 2, "Only one result on non-existing cardnumber scenario" ); + is( $return[0], 0, '0 returned' ); + is( $return[1], undef, 'Undef returned, representing no match' ); + + $schema->storage->txn_rollback; + }; + + subtest 'CAS check (mocked) tests' => sub { + + plan tests => 25; + + $schema->storage->txn_begin; + + my $account_locked; + my $password_expired; + + my $mock_patron = Test::MockModule->new('Koha::Patron'); + $mock_patron->mock( 'account_locked', sub { return $account_locked; } ); + $mock_patron->mock( 'password_expired', sub { return $password_expired; } ); + + # Only interested here in regular login + t::lib::Mocks::mock_config( 'useshibboleth', undef ); + $C4::Auth::cas = 1; + $C4::Auth::ldap = 0; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $password = 'thePassword123'; + $patron->set_password( { password => $password, skip_validation => 1 } ); + + my $patron_to_delete = $builder->build_object( { class => 'Koha::Patrons' } ); + my $unused_userid = $patron_to_delete->userid; + my $unused_cardnumber = $patron_to_delete->cardnumber; + $patron_to_delete->delete; + + my $ticket = '123456'; + my $query = CGI->new; + $query->param( -name => 'ticket', -value => $ticket ); + + my @cas_return = ( 1, $patron->cardnumber, $patron->userid, $ticket, Koha::Patrons->find( $patron->id ) ); + + my $cas_mock = Test::MockModule->new('C4::Auth'); + $cas_mock->mock( 'checkpw_cas', sub { return @cas_return; } ); + + $account_locked = 1; + my @return = checkpw( $patron->userid, $password, $query, ); + is_deeply( \@return, [], 'If the account is locked, empty list is returned' ); + + $account_locked = 0; + + my @matchpoints = qw(userid cardnumber); + foreach my $matchpoint (@matchpoints) { + + @return = checkpw( $patron->$matchpoint, $password, $query, ); + + is( $return[0], 1, "Password validation successful returns 1 ($matchpoint)" ); + is( $return[1], $patron->cardnumber, '`cardnumber` returned' ); + is( $return[2], $patron->userid, '`userid` returned' ); + is( ref( $return[3] ), 'Koha::Patron', 'Koha::Patron object reference returned' ); + is( $return[3]->id, $patron->id, 'Correct patron returned' ); + } + + @return = checkpw( $patron->userid, $password . 'hey', $query, ); + + is( scalar @return, 2, "Two results on invalid password scenario" ); + is( $return[0], 0, '0 returned on invalid password' ); + is( ref( $return[1] ), 'Koha::Patron' ); + is( $return[1]->id, $patron->id, 'Patron matched correctly' ); + + $password_expired = 1; + @return = checkpw( $patron->userid, $password, $query, ); + + is( scalar @return, 2, "Two results on expired password scenario" ); + is( $return[0], -2, '-2 returned' ); + is( ref( $return[1] ), 'Koha::Patron' ); + is( $return[1]->id, $patron->id, 'Patron matched correctly' ); + + @return = checkpw( $unused_userid, $password, $query, ); + + is( scalar @return, 2, "Two results on non-existing userid scenario" ); + is( $return[0], 0, '0 returned' ); + is( $return[1], undef, 'Undef returned, representing no match' ); + + @return = checkpw( $unused_cardnumber, $password, $query, ); + + is( scalar @return, 2, "Only one result on non-existing cardnumber scenario" ); + is( $return[0], 0, '0 returned' ); + is( $return[1], undef, 'Undef returned, representing no match' ); + + $schema->storage->txn_rollback; + }; + + subtest 'Shibboleth check (mocked) tests' => sub { + + plan tests => 6; + + $schema->storage->txn_begin; + + my $account_locked; + my $password_expired; + + my $mock_patron = Test::MockModule->new('Koha::Patron'); + $mock_patron->mock( 'account_locked', sub { return $account_locked; } ); + $mock_patron->mock( 'password_expired', sub { return $password_expired; } ); + + # Only interested here in regular login + t::lib::Mocks::mock_config( 'useshibboleth', 1 ); + $C4::Auth::cas = 0; + $C4::Auth::ldap = 0; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $password = 'thePassword123'; + $patron->set_password( { password => $password, skip_validation => 1 } ); + + my $patron_to_delete = $builder->build_object( { class => 'Koha::Patrons' } ); + my $unused_userid = $patron_to_delete->userid; + my $unused_cardnumber = $patron_to_delete->cardnumber; + $patron_to_delete->delete; + + my @shib_return = ( 1, $patron->cardnumber, $patron->userid, Koha::Patrons->find( $patron->id ) ); + + my $auth_mock = Test::MockModule->new('C4::Auth'); + $auth_mock->mock( 'shib_ok', 1 ); + $auth_mock->mock( 'get_login_shib', 1 ); + + my $shib_mock = Test::MockModule->new('C4::Auth_with_shibboleth'); + $shib_mock->mock( 'checkpw_shib', sub { return @shib_return; } ); + + $account_locked = 1; + my @return = checkpw( $patron->userid ); + is_deeply( \@return, [], 'If the account is locked, empty list is returned' ); + + $account_locked = 0; + + @return = checkpw(); + + is( $return[0], 1, "Password validation successful returns 1" ); + is( $return[1], $patron->cardnumber, '`cardnumber` returned' ); + is( $return[2], $patron->userid, '`userid` returned' ); + is( ref( $return[3] ), 'Koha::Patron', 'Koha::Patron object reference returned' ); + is( $return[3]->id, $patron->id, 'Correct patron returned' ); + + $schema->storage->txn_rollback; + }; +}; -- 2.39.5