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> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (cherry picked from commit 9ba199c2acc33873154c167e73e86a5e786084cb) Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> (cherry picked from commit f6c2bdf2163239eddef379ff34e769dfdaeac9ca) Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
This commit is contained in:
parent
04be28f856
commit
b732e60e0c
5 changed files with 21 additions and 24 deletions
14
C4/Auth.pm
14
C4/Auth.pm
|
@ -1953,9 +1953,10 @@ sub checkpw {
|
||||||
if ( $patron and ( $patron->account_locked ) ) {
|
if ( $patron and ( $patron->account_locked ) ) {
|
||||||
# Nothing to check, account is locked
|
# Nothing to check, account is locked
|
||||||
} elsif ($ldap && defined($password)) {
|
} 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 ) {
|
if ( $retval == 1 ) {
|
||||||
@return = ( $retval, $retcard, $retuserid );
|
@return = ( $retval, $retcard, $retuserid, $patron );
|
||||||
$passwd_ok = 1;
|
$passwd_ok = 1;
|
||||||
}
|
}
|
||||||
$check_internal_as_fallback = 1 if $retval == 0;
|
$check_internal_as_fallback = 1 if $retval == 0;
|
||||||
|
@ -1965,9 +1966,9 @@ sub checkpw {
|
||||||
# In case of a CAS authentication, we use the ticket instead of the password
|
# In case of a CAS authentication, we use the ticket instead of the password
|
||||||
my $ticket = $query->param('ticket');
|
my $ticket = $query->param('ticket');
|
||||||
$query->delete('ticket'); # remove ticket to come back to original URL
|
$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 ) {
|
if ( $retval ) {
|
||||||
@return = ( $retval, $retcard, $retuserid, $cas_ticket );
|
@return = ( $retval, $retcard, $retuserid, $patron, $cas_ticket );
|
||||||
} else {
|
} else {
|
||||||
@return = (0);
|
@return = (0);
|
||||||
}
|
}
|
||||||
|
@ -1985,9 +1986,9 @@ sub checkpw {
|
||||||
|
|
||||||
# Then, we check if it matches a valid koha user
|
# Then, we check if it matches a valid koha user
|
||||||
if ($shib_login) {
|
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 ) {
|
if ( $retval ) {
|
||||||
@return = ( $retval, $retcard, $retuserid );
|
@return = ( $retval, $retcard, $retuserid, $patron );
|
||||||
}
|
}
|
||||||
$passwd_ok = $retval;
|
$passwd_ok = $retval;
|
||||||
}
|
}
|
||||||
|
@ -1998,6 +1999,7 @@ sub checkpw {
|
||||||
# INTERNAL AUTH
|
# INTERNAL AUTH
|
||||||
if ( $check_internal_as_fallback ) {
|
if ( $check_internal_as_fallback ) {
|
||||||
@return = checkpw_internal( $userid, $password, $no_set_userenv);
|
@return = checkpw_internal( $userid, $password, $no_set_userenv);
|
||||||
|
push(@return, $patron);
|
||||||
$passwd_ok = 1 if $return[0] > 0; # 1 or 2
|
$passwd_ok = 1 if $return[0] > 0; # 1 or 2
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -118,17 +118,13 @@ sub checkpw_cas {
|
||||||
|
|
||||||
# Does it match one of our users ?
|
# Does it match one of our users ?
|
||||||
my $dbh = C4::Context->dbh;
|
my $dbh = C4::Context->dbh;
|
||||||
my $sth = $dbh->prepare("select cardnumber from borrowers where userid=?");
|
my $patron = Koha::Patrons->find({ userid => $userid });
|
||||||
$sth->execute($userid);
|
if ( $patron ) {
|
||||||
if ( $sth->rows ) {
|
return ( 1, $patron->cardnumber, $patron->userid, $ticket, $patron );
|
||||||
$retnumber = $sth->fetchrow;
|
|
||||||
return ( 1, $retnumber, $userid, $ticket );
|
|
||||||
}
|
}
|
||||||
$sth = $dbh->prepare("select userid from borrowers where cardnumber=?");
|
$patron = Koha::Patrons->find({ cardnumber => $userid });
|
||||||
$sth->execute($userid);
|
if ( $patron ) {
|
||||||
if ( $sth->rows ) {
|
return ( 1, $patron->cardnumber, $patron->userid, $ticket, $patron );
|
||||||
$retnumber = $sth->fetchrow;
|
|
||||||
return ( 1, $retnumber, $userid, $ticket );
|
|
||||||
}
|
}
|
||||||
|
|
||||||
# If we reach this point, then the user is a valid CAS user, but not a Koha user
|
# If we reach this point, then the user is a valid CAS user, but not a Koha user
|
||||||
|
|
|
@ -204,6 +204,7 @@ sub checkpw_ldap {
|
||||||
my (%borrower);
|
my (%borrower);
|
||||||
my ($borrowernumber,$cardnumber,$local_userid,$savedpw) = exists_local($userid);
|
my ($borrowernumber,$cardnumber,$local_userid,$savedpw) = exists_local($userid);
|
||||||
|
|
||||||
|
my $patron;
|
||||||
if (( $borrowernumber and $config{update} ) or
|
if (( $borrowernumber and $config{update} ) or
|
||||||
(!$borrowernumber and $config{replicate}) ) {
|
(!$borrowernumber and $config{replicate}) ) {
|
||||||
%borrower = ldap_entry_2_hash($userldapentry,$userid);
|
%borrower = ldap_entry_2_hash($userldapentry,$userid);
|
||||||
|
@ -220,7 +221,7 @@ sub checkpw_ldap {
|
||||||
}
|
}
|
||||||
} elsif ($config{replicate}) { # A2, C2
|
} elsif ($config{replicate}) { # A2, C2
|
||||||
my @columns = Koha::Patrons->columns;
|
my @columns = Koha::Patrons->columns;
|
||||||
my $patron = Koha::Patron->new(
|
$patron = Koha::Patron->new(
|
||||||
{
|
{
|
||||||
map { exists( $borrower{$_} ) ? ( $_ => $borrower{$_} ) : () } @columns
|
map { exists( $borrower{$_} ) ? ( $_ => $borrower{$_} ) : () } @columns
|
||||||
}
|
}
|
||||||
|
@ -275,7 +276,7 @@ sub checkpw_ldap {
|
||||||
unless (exists($borrower{$code}) && $borrower{$code} !~ m/^\s*$/ ) {
|
unless (exists($borrower{$code}) && $borrower{$code} !~ m/^\s*$/ ) {
|
||||||
next;
|
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...
|
if ( $patron ) { # Should not be needed, but we are in C4::Auth LDAP...
|
||||||
eval {
|
eval {
|
||||||
my $attribute = Koha::Patron::Attribute->new({code => $code, attribute => $borrower{$code}});
|
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).
|
# Pass LDAP entry object and local cardnumber (userid).
|
||||||
|
|
|
@ -109,7 +109,7 @@ sub checkpw_shib {
|
||||||
if ($config->{'sync'}) {
|
if ($config->{'sync'}) {
|
||||||
_sync($borrower->borrowernumber, $config, $match);
|
_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'} ) {
|
if ( $config->{'autocreate'} ) {
|
||||||
|
|
|
@ -394,12 +394,10 @@ sub AuthenticatePatron {
|
||||||
my ($cgi) = @_;
|
my ($cgi) = @_;
|
||||||
my $username = $cgi->param('username');
|
my $username = $cgi->param('username');
|
||||||
my $password = $cgi->param('password');
|
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 ) {
|
if ( $status == 1 ) {
|
||||||
# Track the login
|
# Track the login
|
||||||
C4::Auth::track_login_daily( $userid );
|
$patron->update_lastseen('connection');
|
||||||
# Get the borrower
|
|
||||||
my $patron = Koha::Patrons->find( { userid => $userid } );
|
|
||||||
return { id => $patron->borrowernumber };
|
return { id => $patron->borrowernumber };
|
||||||
}
|
}
|
||||||
elsif ( $status == -2 ){
|
elsif ( $status == -2 ){
|
||||||
|
|
Loading…
Reference in a new issue