bug 34893 passes a patron object back from authentication, however, when LDAP is enabled we define the patron from the return of checkpw_ldap When the ldap auth is a failure, we fallback to internal, however, we don't redefine the patron object In the code: 1965 if ( defined $userid ) { 1966 $patron = Koha::Patrons->find( { userid => $userid } ); 1967 $patron = Koha::Patrons->find( { cardnumber => $userid } ) unless $patron; 1968 } . . . 1980 } elsif ( $ldap && defined($password) ) { 1981 my ( $retval, $retcard, $retuserid ); 1982 ( $retval, $retcard, $retuserid, $patron ) = checkpw_ldap(@_); # EXTERNAL AUTH . . . 2026 # INTERNAL AUTH 2027 if ($check_internal_as_fallback) { 2028 @return = checkpw_internal( $userid, $password, $no_set_userenv ); 2029 push( @return, $patron ); 2030 $passwd_ok = 1 if $return[0] > 0; # 1 or 2 2031 } line 1982 undefs, line 2029 returns it In ilsdi: 397 my ($status, $cardnumber, $userid, $patron) = C4::Auth::checkpw( $username, $password ); 398 if ( $status == 1 ) { 399 # Track the login 400 $patron->update_lastseen('connection'); 401 return { id => $patron->borrowernumber }; 402 } We die on undefined patron
To recreate: 1 - Enable ILSDI: http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=web_services#web_services_ILS-DI 2 - Enable ldap (not for real) by adding/editing to koha-conf: <useldapserver>1</useldapserver><!-- see C4::Auth_with_ldap for extra configs you must add if you want to turn this on --> <ldapserver id="ldapserver"> <hostname>ldaps://nha.dog</hostname> <base>belong.to.us</base> <mapping> <userid is="SAMAccountname"></userid> </mapping> </ldapserver> 3 - Try to authenticate a patron (generate your correct credentials: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=edna&password=acosta 4 - Can't call method "update_lastseen" on an undefined value at /kohadevbox/koha/C4/ILSDI/Services.pm line 400
Created attachment 162458 [details] [review] Bug 36161: Fix ISLDI AuthenticatePatron when LDAP enabled Test Plan: 1) Enable ILSDI: http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=web_services#web_services_ILS-DI 2) Enable ldap (not for real) by adding/editing to koha-conf: <useldapserver>1</useldapserver><!-- see C4::Auth_with_ldap for extra configs you must add if you want to turn this on --> <ldapserver id="ldapserver"> <hostname>ldaps://nha.dog</hostname> <base>belong.to.us</base> <mapping> <userid is="SAMAccountname"></userid> </mapping> </ldapserver> 3) Try to authenticate a patron (generate your correct credentials: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=edna&password=acosta 4) Can't call method "update_lastseen" on an undefined value at /kohadevbox/koha/C4/ILSDI/Services.pm line 400 5) Apply this patch 6) Restart all the things! 7) Repeat step 3 8) No errors! 9) Set to a valid ldap server 10) Verify LDAP authentication still functions as usual
Created attachment 162460 [details] [review] Bug 36161: Fix ISLDI AuthenticatePatron when LDAP enabled Test Plan: 1) Enable ILSDI: http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=web_services#web_services_ILS-DI 2) Enable ldap (not for real) by adding/editing to koha-conf: <useldapserver>1</useldapserver><!-- see C4::Auth_with_ldap for extra configs you must add if you want to turn this on --> <ldapserver id="ldapserver"> <hostname>ldaps://nha.dog</hostname> <base>belong.to.us</base> <mapping> <userid is="SAMAccountname"></userid> </mapping> </ldapserver> 3) Try to authenticate a patron (generate your correct credentials: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=AuthenticatePatron&username=edna&password=acosta 4) Can't call method "update_lastseen" on an undefined value at /kohadevbox/koha/C4/ILSDI/Services.pm line 400 5) Apply this patch 6) Restart all the things! 7) Repeat step 3 8) No errors! 9) Set to a valid ldap server 10) Verify LDAP authentication still functions as usual
I'm afraid t/db_dependent/Auth.t is failing with this patch... could you check it out?
Currently the checkpw_internal gets patron by userid and check the password if a patron is found, then it gets patron by cardnumber and checks password. This patch reduces that to a single check, get patron by either field, then check password - it's going to muddy the case where a username/cardnumber is shared bug 14323 aims to disallow, but I think we need to reconcile this one and 36575 - can you compare them Kyle?
We believe bug 36575 may take care of this. Let's revisit it after that's been pushed.
Little prompt to ask if this is still valid or can perhaps now be closed instead?
(In reply to Martin Renvoize from comment #7) > Little prompt to ask if this is still valid or can perhaps now be closed > instead? ILS-DI is completely broken right now I believe (CSRF related), so not sure if this can be tested.
(In reply to Katrin Fischer from comment #8) > (In reply to Martin Renvoize from comment #7) > > Little prompt to ask if this is still valid or can perhaps now be closed > > instead? > > ILS-DI is completely broken right now I believe (CSRF related), so not sure > if this can be tested. We think bug 36575 has resolved this issue! We can always reopen if we find that is not the case.