Bugzilla – Attachment 156123 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), 7.99 KB, created by
Kyle M Hall (khall)
on 2023-09-22 18:29:27 UTC
(
hide
)
Description:
Bug 34893: ILS-DI can return the wrong patron for AuthenticatePatron
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2023-09-22 18:29:27 UTC
Size:
7.99 KB
patch
obsolete
>From 9218f46c49688a6c14da31695b919a0b7f0873bd 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) 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! >9) prove t/Auth.t t/db_dependent/Auth_with_ldap.t t/Auth_with_shibboleth.t t/db_dependent/Auth_with_cas.t >--- > 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
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