Bug 36161 - ILS-DI AuthenticatePatron broken when LDAP enabled and auth falls back to internal connection
Summary: ILS-DI AuthenticatePatron broken when LDAP enabled and auth falls back to int...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Kyle M Hall (khall)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 34893 36575
Blocks:
  Show dependency treegraph
 
Reported: 2024-02-23 19:54 UTC by Nick Clemens (kidclamp)
Modified: 2024-09-13 13:37 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 36161: Fix ISLDI AuthenticatePatron when LDAP enabled (8.62 KB, patch)
2024-02-26 14:43 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 36161: Fix ISLDI AuthenticatePatron when LDAP enabled (8.65 KB, patch)
2024-02-26 16:03 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-02-23 19:54:55 UTC
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
Comment 1 Nick Clemens (kidclamp) 2024-02-23 19:56:50 UTC
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
Comment 2 Kyle M Hall (khall) 2024-02-26 14:43:37 UTC
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
Comment 3 Kyle M Hall (khall) 2024-02-26 16:03:40 UTC
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
Comment 4 Martin Renvoize (ashimema) 2024-04-25 13:32:16 UTC
I'm afraid t/db_dependent/Auth.t is failing with this patch... could you check it out?
Comment 5 Nick Clemens (kidclamp) 2024-04-26 11:00:28 UTC
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?
Comment 6 Kyle M Hall (khall) 2024-04-26 14:50:12 UTC
We believe bug 36575 may take care of this. Let's revisit it after that's been pushed.
Comment 7 Martin Renvoize (ashimema) 2024-09-13 13:30:50 UTC
Little prompt to ask if this is still valid or can perhaps now be closed instead?
Comment 8 Katrin Fischer 2024-09-13 13:32:24 UTC
(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.
Comment 9 Kyle M Hall (khall) 2024-09-13 13:37:01 UTC
(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.