Remove default use of LDAP_MATCHING_RULE_IN_CHAIN (#441)

* Remove default use of LDAP_MATCHING_RULE_IN_CHAIN with option to enable * Handle duplicate entries in list returned by lookupGroupMembersRecursive * Improve efficiency --------- Co-authored-by: lbwexler <[email protected]>
xh · Feb 4, 2025 · bb30fde · bb30fde
1 parent 4cf8538
commit bb30fde
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,10 +2,16 @@
 
 ## 29.0-SNAPSHOT - unreleased
 
+### 💥 Breaking Changes (upgrade difficulty: 🟢 LOW - LDAP search behavior change)
+
+* `LdapService` no longer uses `LDAP_MATCHING_RULE_IN_CHAIN` by default. See change to
+  `xhLdapConfig` if you need to revert to the previous behavior (not expected in most cases).
+
 ### 🎁 New Features
 
 * Added new endpoints to support searching the contents of `JSONBlob` entries, JSON-based user
   preferences, and JSON-based app configs.
+* Added `xhLdapConfig.useMatchingRuleInChain` flag to enable use of `LDAP_MATCHING_RULE_IN_CHAIN`.
 
 ### ⚙️ Technical
 

diff --git a/grails-app/init/io/xh/hoist/BootStrap.groovy b/grails-app/init/io/xh/hoist/BootStrap.groovy
@@ -233,6 +233,7 @@ class BootStrap implements LogSupport {
                     enabled: false,
                     timeoutMs: 60000,
                     cacheExpireSecs: 300,
+                    useMatchingRuleInChain: false,
                     servers: [
                         [
                             host: '',

diff --git a/grails-app/services/io/xh/hoist/ldap/LdapService.groovy b/grails-app/services/io/xh/hoist/ldap/LdapService.groovy
@@ -21,21 +21,21 @@ import static io.xh.hoist.util.DateTimeUtils.SECONDS
  *          - timeoutMs - time to wait for any individual search to resolve.
  *          - cacheExpireSecs - length of time to cache results.  Set to -1 to disable caching.
  *          - skipTlsCertVerification - true to accept untrusted certificates when binding
- *          - servers - list of servers to be queried, each containing:
- *              - host
- *              - baseUserDn
- *              - baseGroupDn
+ *          - useMatchingRuleInChain - true to use Microsoft Active Directory's proprietary
+ *           "LDAP_MATCHING_RULE_IN_CHAIN" rule (the magic `1.2.840.113556.1.4.1941` string below).
+ *           This can be a more efficient way to resolve users in nested groups but should be used
+ *           with caution, as it can trigger a large database walk, which has a significant
+ *           performance impact that is unnecessary when queries are not expected to return deeply
+ *           nested groups.
+*           - servers - list of servers to be queried, each containing:
+*              - host
+*              - baseUserDn
+*              - baseGroupDn
  *     - 'xhLdapUsername' - dn of query user.
  *     - 'xhLdapPassword' - password for user
  *
  * This service will cache results, per server, for the configured interval.
  * This service may return partial results if any particular server fails to return results.
- *
- * Note that the implementation of `lookupGroupMembers()` is currently specific to Microsoft Active
- * Directory, due to the use of the proprietary "LDAP_MATCHING_RULE_IN_CHAIN" rule OID (the magic
- * `1.2.840.113556.1.4.1941` string below). This is an efficient way to resolve users in nested
- * groups, but would require an alternate implementation if this service is required to work with
- * more generic LDAP deployments.
  */
 class LdapService extends BaseService {
 
@@ -157,7 +157,27 @@ class LdapService extends BaseService {
 
     private List<LdapPerson> lookupGroupMembersInternal(String dn, boolean strictMode) {
         // See class-level comment regarding this AD-specific query
-        searchMany("(|(memberOf=$dn) (memberOf:1.2.840.113556.1.4.1941:=$dn))", LdapPerson, strictMode)
+        config.useMatchingRuleInChain ?
+            searchMany("(|(memberOf=$dn) (memberOf:1.2.840.113556.1.4.1941:=$dn))", LdapPerson, strictMode) :
+            lookupMembersRecursive(dn, strictMode).values().asList()
+    }
+
+    private Map<String, LdapPerson> lookupMembersRecursive(
+        String dn,
+        boolean strictMode,
+        Map<String, LdapPerson> members = new HashMap(),
+        Set<String> visited = new HashSet<String>()
+    ) {
+        if (visited.add(dn)) {
+            // Add direct users
+            searchMany("(memberOf=$dn)", LdapPerson, strictMode)
+                .each { members[it.distinguishedname] = it}
+
+            // Recursively add nested groups
+            searchMany("(memberOf=$dn)", LdapGroup, strictMode)
+                .each {lookupMembersRecursive(it.distinguishedname, strictMode, members, visited)}
+        }
+        return members
     }
 
     private <T extends LdapObject> List<T> doQuery(Map server, String baseFilter, Class<T> objType, boolean strictMode) {