From 9b4aaabab3b04ac592277271d73581b2ecfe4870 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 22 Sep 2023 14:20:59 -0400 Subject: [PATCH] Bug 34893: ILS-DI can return the wrong patron for AuthenticatePatron Imagine we have a set of users. Some of those users have a NULL userid. We then call AuthenticatePatron from ILS-DI for a patron with a NULL userid, but a valid cardnumber. We call checkpw, which returns the cardnumber and userid. We then call Koha::Patrons->find on the userid *which is null*, meaning the borrowernumber returned is not the correct one, but instead the earliest patron inserted into the database that has a NULL userid. Test Plan: 1) Create a new patron ( we assume there are existing patrons as well ) 2) From the database cli, set the patron's userid to null 3) Run this query: update borrowers set userid = null; 3) Call AuthenticatePatron with username being the patron's cardnumber, and password being the password you set for that patron 4) Note you get back a borrowernumber for a different patron 5) Apply this patch 6) Restart all the things! 7) Call AuthenticatePatron again 8) Note you get the correct borrowernumber! --- C4/Auth.pm | 14 ++++++++------ C4/Auth_with_cas.pm | 16 ++++++---------- C4/Auth_with_ldap.pm | 7 ++++--- C4/Auth_with_shibboleth.pm | 2 +- C4/ILSDI/Services.pm | 4 +--- 5 files changed, 20 insertions(+), 23 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 8906be8e228..7b0deab54be 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1952,9 +1952,10 @@ sub checkpw { if ( $patron and ( $patron->account_locked ) ) { # Nothing to check, account is locked } elsif ($ldap && defined($password)) { - my ( $retval, $retcard, $retuserid ) = checkpw_ldap(@_); # EXTERNAL AUTH + my ( $retval, $retcard, $retuserid ); + ( $retval, $retcard, $retuserid, $patron ) = checkpw_ldap(@_); # EXTERNAL AUTH if ( $retval == 1 ) { - @return = ( $retval, $retcard, $retuserid ); + @return = ( $retval, $retcard, $retuserid, $patron ); $passwd_ok = 1; } $check_internal_as_fallback = 1 if $retval == 0; @@ -1964,9 +1965,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 ) = checkpw_cas( $ticket, $query, $type ); # EXTERNAL AUTH + my ( $retval, $retcard, $retuserid, $cas_ticket, $patron ) = checkpw_cas( $ticket, $query, $type ); # EXTERNAL AUTH if ( $retval ) { - @return = ( $retval, $retcard, $retuserid, $cas_ticket ); + @return = ( $retval, $retcard, $retuserid, $patron, $cas_ticket ); } else { @return = (0); } @@ -1984,9 +1985,9 @@ sub checkpw { # Then, we check if it matches a valid koha user if ($shib_login) { - my ( $retval, $retcard, $retuserid ) = C4::Auth_with_shibboleth::checkpw_shib($shib_login); # EXTERNAL AUTH + my ( $retval, $retcard, $retuserid, $patron ) = C4::Auth_with_shibboleth::checkpw_shib($shib_login); # EXTERNAL AUTH if ( $retval ) { - @return = ( $retval, $retcard, $retuserid ); + @return = ( $retval, $retcard, $retuserid, $patron ); } $passwd_ok = $retval; } @@ -1997,6 +1998,7 @@ sub checkpw { # 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 } diff --git a/C4/Auth_with_cas.pm b/C4/Auth_with_cas.pm index 719df30583c..e1cb1a97b25 100644 --- a/C4/Auth_with_cas.pm +++ b/C4/Auth_with_cas.pm @@ -118,17 +118,13 @@ sub checkpw_cas { # Does it match one of our users ? my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("select cardnumber from borrowers where userid=?"); - $sth->execute($userid); - if ( $sth->rows ) { - $retnumber = $sth->fetchrow; - return ( 1, $retnumber, $userid, $ticket ); + my $patron = Koha::Patrons->find({ userid => $userid }); + if ( $patron ) { + return ( 1, $patron->cardnumber, $patron->userid, $ticket, $patron ); } - $sth = $dbh->prepare("select userid from borrowers where cardnumber=?"); - $sth->execute($userid); - if ( $sth->rows ) { - $retnumber = $sth->fetchrow; - return ( 1, $retnumber, $userid, $ticket ); + $patron = Koha::Patrons->find({ cardnumber => $userid }); + if ( $patron ) { + return ( 1, $patron->cardnumber, $patron->userid, $ticket, $patron ); } # If we reach this point, then the user is a valid CAS user, but not a Koha user diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm index f13dbdcf8e8..1cd02eb9056 100644 --- a/C4/Auth_with_ldap.pm +++ b/C4/Auth_with_ldap.pm @@ -204,6 +204,7 @@ sub checkpw_ldap { my (%borrower); my ($borrowernumber,$cardnumber,$local_userid,$savedpw) = exists_local($userid); + my $patron; if (( $borrowernumber and $config{update} ) or (!$borrowernumber and $config{replicate}) ) { %borrower = ldap_entry_2_hash($userldapentry,$userid); @@ -220,7 +221,7 @@ sub checkpw_ldap { } } elsif ($config{replicate}) { # A2, C2 my @columns = Koha::Patrons->columns; - my $patron = Koha::Patron->new( + $patron = Koha::Patron->new( { map { exists( $borrower{$_} ) ? ( $_ => $borrower{$_} ) : () } @columns } @@ -275,7 +276,7 @@ sub checkpw_ldap { unless (exists($borrower{$code}) && $borrower{$code} !~ m/^\s*$/ ) { next; } - my $patron = Koha::Patrons->find($borrowernumber); + $patron = Koha::Patrons->find($borrowernumber); if ( $patron ) { # Should not be needed, but we are in C4::Auth LDAP... eval { my $attribute = Koha::Patron::Attribute->new({code => $code, attribute => $borrower{$code}}); @@ -287,7 +288,7 @@ sub checkpw_ldap { } } } - return(1, $cardnumber, $userid); + return(1, $cardnumber, $userid, $patron); } # Pass LDAP entry object and local cardnumber (userid). diff --git a/C4/Auth_with_shibboleth.pm b/C4/Auth_with_shibboleth.pm index 364fd774b1f..e356641c476 100644 --- a/C4/Auth_with_shibboleth.pm +++ b/C4/Auth_with_shibboleth.pm @@ -109,7 +109,7 @@ sub checkpw_shib { if ($config->{'sync'}) { _sync($borrower->borrowernumber, $config, $match); } - return ( 1, $borrower->get_column('cardnumber'), $borrower->get_column('userid') ); + return ( 1, $borrower->get_column('cardnumber'), $borrower->get_column('userid'), $borrower ); } if ( $config->{'autocreate'} ) { diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index f6bf6bcc140..bc8a2b34d08 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -394,12 +394,10 @@ sub AuthenticatePatron { my ($cgi) = @_; my $username = $cgi->param('username'); my $password = $cgi->param('password'); - my ($status, $cardnumber, $userid) = C4::Auth::checkpw( $username, $password ); + my ($status, $cardnumber, $userid, $patron) = C4::Auth::checkpw( $username, $password ); if ( $status == 1 ) { # Track the login C4::Auth::track_login_daily( $userid ); - # Get the borrower - my $patron = Koha::Patrons->find( { userid => $userid } ); return { id => $patron->borrowernumber }; } elsif ( $status == -2 ){ -- 2.30.2