Bugzilla – Attachment 159507 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: ILS-DI can return the wrong patron for AuthenticatePatron
Bug-34893-ILS-DI-can-return-the-wrong-patron-for-A.patch (text/plain), 8.21 KB, created by
Victor Grousset/tuxayo
on 2023-12-03 23:31:07 UTC
(
hide
)
Description:
Bug 34893: ILS-DI can return the wrong patron for AuthenticatePatron
Filename:
MIME Type:
Creator:
Victor Grousset/tuxayo
Created:
2023-12-03 23:31:07 UTC
Size:
8.21 KB
patch
obsolete
>From e004968982fe3c22cc625f1e2d60d857d808045f Mon Sep 17 00:00:00 2001 >From: Kyle M Hall <kyle@bywatersolutions.com> >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) Give three patrons a userid and a password >2) From the database cli, set all patrons's userid to null > Run this query: update borrowers set userid = null; >3) Call AuthenticatePatron with username being the 1st patron cardnumber, > and password being the password you set for that patron > http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=kohacard&password=koha >4) Note you get back a borrowernumber for a different patron. Refresh the page and the number is correct. >5) Do the same with the 2nd patron. Same issue at 1st and correct number after. >6) Apply this patch >7) Restart all the things! >8) Do the same with the 3rd patron. >9) Note you get the correct borrowernumber! :D >10) prove t/Auth.t t/db_dependent/Auth_with_ldap.t t/Auth_with_shibboleth.t t/db_dependent/Auth_with_cas.t > >Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> >--- > 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 c449c318a9..98e0d8198b 100644 >--- a/C4/Auth.pm >+++ b/C4/Auth.pm >@@ -1959,9 +1959,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; >@@ -1971,9 +1972,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); > } >@@ -1991,9 +1992,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; > } >@@ -2004,6 +2005,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 719df30583..e1cb1a97b2 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 422a59a45b..ab43f0cbf6 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 67552f3c89..38b93bce71 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 f01dd34770..e7bf28bfe3 100644 >--- a/C4/ILSDI/Services.pm >+++ b/C4/ILSDI/Services.pm >@@ -394,10 +394,8 @@ 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 ) { >- # Get the borrower >- my $patron = Koha::Patrons->find( { userid => $userid } ); > # Track the login > $patron->update_lastseen('connection'); > return { id => $patron->borrowernumber }; >-- >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