WIP: changes from fredvd and datakurre improvements.

[rnixx]

Keep track of changes and improvements made by @fredvd and @datakurre.

[fredvd]

@rnixx I didn't do many fixes here, most of the work is done by Asko. Our main reason to test his work and use it (been in production for a year now on 2 sites) is the ability to do wildcard user search in Pone. iirc Asko also did a performance optimisations that are in here, but our setups are way smaller, (1200-1500 AD users).

-> Skip matches that don't have our required login attribute. - befc47b

The problem here is that your LDAP/AD might contain objects that are returned from a substring search on all attributes, but those objects not having a (required) login attribute. In our case we had an Active Directory where 'Contacts' were stored that didn't have a sAMAccountName (windows login name) which was mapped to the login attribute. An argument can be made that we should have filtered straight away on the correct object types to be returned, but this commit skips over objects that don't have at least the required mapped attributes.

-> Check if interface is active when calling a method - 789f07e

This fixes not being able to deactivate the plugins in the ZMI in PAS. Unfortunately this also caused some side effects in the testing which we were able to fix after a long debug session at the ploneconf sprint @jensens already merged this into pas.plugins.ldap

sprint branch https://github.com/collective/pas.plugins.ldap/commit/60ba06479a0d1c86e222376833dc2e0ca52905c8 merge by Jens later https://github.com/collective/pas.plugins.ldap/commit/e25165f1619d5c655aa58eb9ef733593aed3c1b5

-> Fix memcached setting str to unicode - 7718dea -> It should be unicode, not integer - d0155d3

Also have been merged already, simple YAFOWIL type mismatch

merged at https://github.com/collective/pas.plugins.ldap/commit/67cfa51dc4398bdc0e5f0a13af2c3206696dc3c5

Avoid KeyError in getPropertiesForUser when self.users is None. - 7831b42

@mauritsvanrees knows more about this one, I think this as an issue he ran into at another customer for which he did this fix.

@rnixx Does this help?

[rnixx]

@fredvd Thanks for the summary! This helps a lot. This makes it easier for us to integrate all the improvements back to mainline step by step (even if you cannot expect it to happen fast ;)).

[mauritsvanrees]

@fredvd wrote:

Avoid KeyError in getPropertiesForUser when self.users is None. - 7831b42 @mauritsvanrees knows more about this one, I think this as an issue he ran into at another customer for which he did this fix.

I don't remember. A problem solved is a problem forgotten. :-) Either it was for a customer where I saw this, or this fixes a test failure.

[gotcha]

@rnixx @jensens Were @datakurre optimizations merged or worked on ?

[datakurre]

As a disclaimer, I have not been able to work on this for some time and because we were unable to get where we wanted even after all optimizations, we are changing our approach this summer to syncing users and groups (and looking into http://www.simplecloud.info/ for PAS) and optimizing searches in Plone PAS. (Our authentication would happen with separate plugin using Open ID Connect.)

[rnixx]

@datakurre

we were unable to get where we wanted even after all optimizations

how big was the difference from what you were expecting? Actually the behavior where LDAP nodes in the underlying node.ext.ldap are kept in memory should be definable in future versions and your query normalization approach is actually a good thing. Regardless of your actual plans pas.plugins.ldap performance shall be improved ;)

[gotcha]

@rnixx What would be the approach if we want to merge some of the optimizations in this pull request ?

[rnixx]

@gotcha I'd start cherrypicking from https://github.com/bluedynamics/node.ext.ldap/pull/37 and then continue investigating this PR.

[gotcha]

@datakurre Would you mind mentioning the main problem you saw that you feel cannot been solved ?

[datakurre]

@gotcha Probably it is our use case of 100k users and 2k groups, which some of them have >1k members... and I have not figured out how to fix PAS and Plone's use of PAS to support paging with LDAP all the way to the frontend without always fetching all the results from LDAP or cache all the time.

So, I just tried to optimize queries and their caching.

After I had removed unnecessary encoding/decoding, fixed property sheet to not fetch all the attributes, canonized queries to remove redundant queries and combined some queries, I got issues with caching: pure python memcached client was not performant enough and c-optimized clients had issues with the few huge LDAP responses we had (caused by large groups). Once everything seemed to work, we got reports of random false "Insufficient permissions" errors that went away with a reload. At that point I had no longer time left to resolve those errors (nothing in logs and not obvious thread safety issues with cahce) and we had to revert back to LDAPUserFolder (and live without LDAP based groups). I was left with a feeling that pas.plugins.ldap stack is too complex.

Well, later we turned pas.plugins.ldap back on for groups and now we have two LDAP plugins with too different LDAP backend enabled. LDAPUserFolder for authentication and user attributes, and pas.plugins.ldap for groups. No more reports of that "Insufficient permissions" error.

But even with all the optimizations we could not use recursive groups plugin, because of the performance (and our users miss that).

[gotcha]

@datakurre Thanks a lot for this very detailed answer ! Are you running that setup in 5.0 or 5.1 ?

[datakurre]

Something between the last 5.1a and 5.1b1 (before ZODB upgrade).