From 99bfebe051cba3a66a12345692ad6bd098ba7e10 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 11 Apr 2024 09:39:03 +0000 Subject: [PATCH] Bug 36575: Return correct patron when there is a shared userid / cardnumber This patch moves some patron fetching code in C4/Auth to use to patron returned from the validation methods and only try to fetch the patron (to check if locked, update attempts, etc) if we didn't authenticate To test: 1 - Set a user to have userid = BANANA password = Password1 2 - Set a user to have cardnumber = BANANA password = Password2 3 - Hit the patron authentication API: http://localhost:8080/api/v1/auth/password/validation with data: { "identifier": "BANANA", "password":"Password1" } and: { "identifier": "BANANA", "password":"Password2" } 4 - Note you receive the same response for both 5 - Apply patch, restart all 6 - Repeat the API and confirm you get the correct patron for the password submitted Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall Signed-off-by: Marcel de Rooy --- C4/Auth.pm | 35 +++++++----- t/db_dependent/Auth.t | 29 +++++++++- t/db_dependent/api/v1/password_validation.t | 61 ++++++++++++++++++++- 3 files changed, 107 insertions(+), 18 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index a0df6fec34..770d8bef39 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1920,23 +1920,17 @@ sub checkpw { my $shib_login = $shib ? get_login_shib() : undef; my @return; - my $patron; - if ( defined $userid ) { - $patron = Koha::Patrons->find( { userid => $userid } ); - $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron; - } my $check_internal_as_fallback = 0; my $passwd_ok = 0; + my $patron; + # Note: checkpw_* routines returns: # 1 if auth is ok # 0 if auth is nok # -1 if user bind failed (LDAP only) - if ( $patron and ( $patron->account_locked ) ) { - - # Nothing to check, account is locked - } elsif ( $ldap && defined($password) ) { + if ( $ldap && defined($password) ) { my ( $retval, $retcard, $retuserid ); ( $retval, $retcard, $retuserid, $patron ) = checkpw_ldap(@_); # EXTERNAL AUTH if ( $retval == 1 ) { @@ -1950,7 +1944,8 @@ sub checkpw { # In case of a CAS authentication, we use the ticket instead of the password my $ticket = $query->param('ticket'); $query->delete('ticket'); # remove ticket to come back to original URL - my ( $retval, $retcard, $retuserid, $cas_ticket, $patron ) = + my ( $retval, $retcard, $retuserid, $cas_ticket ); + ( $retval, $retcard, $retuserid, $cas_ticket, $patron ) = checkpw_cas( $ticket, $query, $type ); # EXTERNAL AUTH if ($retval) { @return = ( $retval, $retcard, $retuserid, $patron, $cas_ticket ); @@ -1971,7 +1966,8 @@ sub checkpw { # Then, we check if it matches a valid koha user if ($shib_login) { - my ( $retval, $retcard, $retuserid, $patron ) = + my ( $retval, $retcard, $retuserid ); + ( $retval, $retcard, $retuserid, $patron ) = C4::Auth_with_shibboleth::checkpw_shib($shib_login); # EXTERNAL AUTH if ($retval) { @return = ( $retval, $retcard, $retuserid, $patron ); @@ -1982,20 +1978,29 @@ sub checkpw { $check_internal_as_fallback = 1; } + if ( $check_internal_as_fallback ){ # INTERNAL AUTH - if ($check_internal_as_fallback) { @return = checkpw_internal( $userid, $password, $no_set_userenv ); - push( @return, $patron ); $passwd_ok = 1 if $return[0] > 0; # 1 or 2 + $patron = Koha::Patrons->find({ cardnumber => $return[1] }) if $passwd_ok; + push @return, $patron if $patron; + } + + if ( defined $userid && !$patron ) { + $patron = Koha::Patrons->find( { userid => $userid } ); + $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron; + push @return, $patron if $check_internal_as_fallback; } if ($patron) { - if ($passwd_ok) { + if( $patron->account_locked ){ + @return = (); + } elsif ($passwd_ok) { $patron->update( { login_attempts => 0 } ); if ( $patron->password_expired ) { @return = ( -2, $patron ); } - } elsif ( !$patron->account_locked ) { + } else { $patron->update( { login_attempts => $patron->login_attempts + 1 } ); } } diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t index 625898a604..35a5118826 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 => 21; +use Test::More tests => 22; use Test::Warn; use t::lib::Mocks; use t::lib::TestBuilder; @@ -490,7 +490,7 @@ subtest 'checkpw lockout tests' => sub { ( $checkpw, undef, undef ) = checkpw( $patron->userid, $password, undef, undef, 1 ); is( $checkpw, undef, 'checkpw returns undef with right password when account locked' ); ( $checkpw, undef, undef ) = checkpw( $patron->cardnumber, $password, undef, undef, 1 ); - is( $checkpw, undef, 'checkpw returns undefwith right password when logging in via cardnumber if account locked' ); + is( $checkpw, undef, 'checkpw returns undef with right password when logging in via cardnumber if account locked' ); }; @@ -1397,6 +1397,31 @@ subtest 'AutoLocation' => sub { }; +subtest 'checkpw for users with shared cardnumber / userid ' => sub { + + plan tests => 8; + + t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $patron_1 = $builder->build_object( { class => 'Koha::Patrons' } ); + $patron_1->set_password( { password => "OnePassword" } ); + my $patron_2 = $builder->build_object( { class => 'Koha::Patrons', value => { userid => $patron_1->cardnumber } } ); + $patron_2->set_password( { password => "PasswordTwo" } ); + + my ( $checkpw, $cardnumber, $userid, $patron ) = checkpw( $patron_1->cardnumber, "OnePassword", undef, undef, 1 ); + ok( $checkpw, 'checkpw returns true for right password when logging in via cardnumber' ); + is( $cardnumber, $patron_1->cardnumber, 'checkpw returns correct cardnumber' ); + is( $userid, $patron_1->userid, 'checkpw returns correct userid' ); + is( $patron->id, $patron_1->id, 'checkpw returns correct patron' ); + + ( $checkpw, $cardnumber, $userid, $patron ) = checkpw( $patron_2->userid, "PasswordTwo", undef, undef, 1 ); + ok( $checkpw, 'checkpw returns true for right password when logging in via userid' ); + is( $cardnumber, $patron_2->cardnumber, 'checkpw returns correct cardnumber' ); + is( $userid, $patron_2->userid, 'checkpw returns correct userid' ); + is( $patron->id, $patron_2->id, 'checkpw returns correct patron' ); + +}; + sub set_weak_password { my ($patron) = @_; my $password = 'password'; diff --git a/t/db_dependent/api/v1/password_validation.t b/t/db_dependent/api/v1/password_validation.t index 10527bc93d..1aef718700 100755 --- a/t/db_dependent/api/v1/password_validation.t +++ b/t/db_dependent/api/v1/password_validation.t @@ -18,7 +18,7 @@ use Modern::Perl; -use Test::More tests => 5; +use Test::More tests => 6; use Test::Mojo; use t::lib::TestBuilder; @@ -239,4 +239,63 @@ subtest 'password validation - expired password' => sub { $schema->storage->txn_rollback; }; +subtest 'password validation - users with shared cardnumber / userid' => sub { + + plan tests => 12; + + $schema->storage->txn_begin; + + my $patron_1 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { } + } + ); + my $patron_password_1 = 'thePassword123'; + $patron_1->set_password( { password => $patron_password_1, skip_validation => 1 } ); + + my $patron_2 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { userid => $patron_1->cardnumber } + } + ); + my $patron_password_2 = 'thePassword345'; + $patron_2->set_password( { password => $patron_password_2, skip_validation => 1 } ); + + my $json = { + identifier => $patron_1->cardnumber, + password => $patron_password_1, + }; + + $t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201) + ->json_is({ cardnumber => $patron_1->cardnumber, patron_id => $patron_1->borrowernumber, userid => $patron_1->userid} ); + + $json = { + identifier => $patron_2->userid, + password => $patron_password_2, + }; + + $t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201) + ->json_is({ cardnumber => $patron_2->cardnumber, patron_id => $patron_2->borrowernumber, userid => $patron_2->userid} ); + + my $json = { + userid => $patron_1->cardnumber, + password => $patron_password_1, + }; + + $t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201) + ->json_is({ cardnumber => $patron_1->cardnumber, patron_id => $patron_1->borrowernumber, userid => $patron_1->userid} ); + + $json = { + userid => $patron_2->userid, + password => $patron_password_2, + }; + + $t->post_ok( "//$userid:$password@/api/v1/auth/password/validation" => json => $json )->status_is(201) + ->json_is({ cardnumber => $patron_2->cardnumber, patron_id => $patron_2->borrowernumber, userid => $patron_2->userid} ); + + $schema->storage->txn_rollback; +}; + $schema->storage->txn_rollback; -- 2.39.5