From 9b6f3d0f1836d79e808ccac7b65b4321f846b3f9 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 Signed-off-by: Wainui Witika-Park --- C4/Auth.pm | 36 +++++++++++++++++++++--------------- t/db_dependent/Auth.t | 29 +++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 9ad264e18b..1a8b9bf798 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1910,23 +1910,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 ) { @@ -1940,7 +1934,9 @@ 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 ) = checkpw_cas( $ticket, $query, $type ); # EXTERNAL AUTH + 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 ); } else { @@ -1960,7 +1956,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 ); @@ -1971,20 +1968,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 5b09121884..15218545f1 100755 --- a/t/db_dependent/Auth.t +++ b/t/db_dependent/Auth.t @@ -10,7 +10,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; @@ -445,7 +445,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' ); }; @@ -1194,6 +1194,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'; -- 2.39.5