Remove default use of LDAP_MATCHING_RULE_IN_CHAIN #441
Conversation
| if (!visitedGroups.add(dn)) return [] | ||
| List<LdapPerson> users = searchMany("(memberOf=$dn)", LdapPerson, strictMode) | ||
| List<LdapGroup> groups = searchMany("(memberOf=$dn)", LdapGroup, strictMode) | ||
| groups.each { LdapGroup group -> |
There was a problem hiding this comment.
Think this looks generally fine, but a bit worried about the efficiency if we were handling huge user lists (I know its unlikely). Looks like groovy's unique is n^2!
(https://softwareengineering.stackexchange.com/questions/202113/whats-the-computational-complexity-of-the-groovy-unique-method)
Probably safest to pass a collector Set in to recursive method that gathers the unique users, and just convert it to a list in the caller. No need for the recursive method to return anything
There was a problem hiding this comment.
Looks like the n^2 is still true on the version we are using:
https://github.com/apache/groovy/blob/GROOVY_3_0_23/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java#L1324
vs
version from 2013 Stack Exchange post: https://github.com/groovy/groovy-core/blob/6232fdb2872c13f94d5e9fd24ddf2d16fd61ee2d/src/main/org/codehaus/groovy/runtime/DefaultGroovyMethods.java#L1031
I was hoping it might have changed in the 12 years since, but it has not.
Client reported that our use of
LDAP_MATCHING_RULE_IN_CHAINhas a negative performance impact on their LDAP databases and is unnecessary when querying groups without deep nesting. Additionally, it is proprietary to MS and therefore not suitable for other LDAP implementations. This change recurses LDAP directories with its own code by default but provides a flag onxhLdapConfigto enable the previous implementation in cases where an AD instance uses deeply nested groups.