Bugzilla – Attachment 159893 Details for
Bug 34893
ILS-DI can return the wrong patron for AuthenticatePatron
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 34893: Unit tests for C4::Auth::checkpw
Bug-34893-Unit-tests-for-C4Authcheckpw.patch (text/plain), 10.95 KB, created by
Tomás Cohen Arazi (tcohen)
on 2023-12-15 13:58:41 UTC
(
hide
)
Description:
Bug 34893: Unit tests for C4::Auth::checkpw
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2023-12-15 13:58:41 UTC
Size:
10.95 KB
patch
obsolete
>From d8806c8bf947545eec353a25ecee5423939b760e Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> >--- > 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 bcf65844a41..74b73563a4a 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 46b500c013f..df79b09e54b 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.43.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 34893
:
156121
|
156123
|
159311
|
159314
|
159315
|
159356
|
159441
|
159506
|
159507
|
159508
|
159509
|
159797
|
159798
|
159799
|
159800
|
159893
|
161097
|
161098
|
161099
|
161100
|
161101
|
161324
|
163964