Modify user_ldap checkPassword to not fetch records from ldap each time because of extremely high CPU usage!

[akhil1508]

Summary

• Currently on each checkPassword call, the function getLDAPUserByLoginName is called

• This calls fetchListOfUsers which is defined here

• So on each authentication request:

  • there is first an admin bind to LDAP(if not already existing)
  • a search request to LDAP
  • a bunch of SQL UPDATEs from calling batchApplyUserAttributes
  • a user BIND to LDAP to check if credentials are valid

• DAV mobile clients(for example DAVx5) don't support token auth or oauth yet. This means that they pass the username and password on every single sync request

• We have an NC instance serving around 100 DAV sync requests every second

• For every sync request the LDAP logs look like(admin bind + search for user):

conn=1687813954 fd=21 ACCEPT from IP=*** (IP=***)
conn=1687813954 op=0 BIND dn="***" method=128
conn=1687813954 op=0 BIND dn="***" mech=SIMPLE bind_ssf=0 ssf=0
conn=1687813954 op=0 RESULT tag=97 err=0 qtime=0.000014 etime=0.000156 text=
conn=1687813954 op=1 SRCH base="***" scope=2 deref=0 filter="***"
conn=1687813954 op=1 SRCH attr=entryuuid nsuniqueid objectguid guid ipauniqueid dn uid samaccountname memberof 
conn=1687813954 op=1 SEARCH RESULT tag=101 err=0 qtime=0.000027 etime=0.000332 nentries=1 text=

• followed by BIND(to check user credentials)

conn=1687813956 fd=51 ACCEPT from IP=*** (IP=***)
conn=1687813956 op=0 BIND dn="***" method=128
conn=1687813956 op=0 BIND dn="***" mech=SIMPLE bind_ssf=0 ssf=0
conn=1687813956 op=0 RESULT tag=97 err=0 qtime=0.000012 etime=0.004391 text=

• CPU Usage by openldap is extremely high! CPU usage by database is similarly extremely high but lower compared to LDAP

Proposed solution here

• Get username and dn from cache if existing
  • Use the loginName2UserName function that will fetch the ldap record and update attributes if it is not in cache
• Check credentials with BIND but don't process attributes again
  • $user->processAttributes here seems redundant as it is already done during the getLDAPUserByLoginName function call
• Will save ~0.5ms per auth request if user already cached
  • Doesn't sound like too big a deal but with 100 auth requests per second, this can definitely help the load issue
• Will also avoid ~12 SQL UPDATE operations on each authentication request
  • ~6 if user is not cached yet
  • UNBIND happens in the destructor of the Connection class so pretty sure it is done only after all these SQL commands are completed, which means the LDAP bind stays open till then

Please do let me know if there are any pitfalls to this and if the ldap record fetching and processing on every single authentication request happens for an unavoidable reason I am not aware of..

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• [ ] Tests (unit, integration, api and/or acceptance) are included
• [ ] Screenshots before/after for front-end changes
• [ ] Documentation (manuals or wiki) has been updated or is not required
• [ ] Backports requested where applicable (ex: critical bugfixes)