Improved handling of errors during parallel/bulk LDAP lookup

CHANGELOG.md

@@ -37,13 +37,15 @@
   `params` posted in the request header.
 
 * `DefaultRoleService` has improved error handling for failed directory group lookups.
-* `LdapService` now optionally throws if a query fails rather than returning an empty result. This is not
-  expected to affect most applications, but may require some to adjust their error handling.
-
+* `LdapService` bulk lookup methods now provide a `strict` option to throw if a query fails rather than
+   quietly returning an empty result.
 
 ### 💥 Breaking Changes
 
 * Requires `hoist-react >= 63.0` for client-side support of the new `track` and `submitError` endpoints.
+* Implementations of `DefaultRoleService.doLoadUsersForDirectoryGroups`  will need to handle a new
+ `strictMode` flag provided as a second argument.
+
 
 
 ## 18.5.1 - 2024-03-08

grails-app/controllers/io/xh/hoist/admin/RoleAdminController.groovy

@@ -50,7 +50,7 @@ class RoleAdminController extends BaseController {
     }
 
     def usersForDirectoryGroup(String name) {
-        renderJSON(data: roleService.loadUsersForDirectoryGroups(singleton(name))[name])
+        renderJSON(data: roleService.loadUsersForDirectoryGroups(singleton(name), true)[name])
     }
 
 

grails-app/services/io/xh/hoist/ldap/LdapService.groovy

@@ -51,80 +51,106 @@ class LdapService extends BaseService {
 
 
     LdapPerson lookupUser(String sName) {
-        searchOne("(sAMAccountName=$sName) ", LdapPerson)
+        searchOne("(sAMAccountName=$sName) ", LdapPerson, true)
     }
 
     List<LdapPerson> lookupGroupMembers(String dn) {
-        // See class-level comment regarding this AD-specific query
-        searchMany("(|(member0f=$dn) (memberOf:1.2.840.113556.1.4.1941:=$dn))", LdapPerson)
+        lookupGroupInternal(dn, true)
     }
 
-    Map<String, List<LdapPerson>> lookupGroupMembers(Set<String> dns) {
-        dns.collectEntries { dn -> [dn, task { lookupGroupMembers(dn) }] }
-            .collectEntries { [it.key, it.value.get()] }
+    List<LdapGroup> findGroups(String sNamePart) {
+        searchMany("(sAMAccountName=*$sNamePart*)", LdapGroup, true)
     }
 
-    Map<String, LdapGroup> lookupGroups(Set<String> dns) {
-        dns.collectEntries { dn -> [dn, task { lookupGroup(dn) }] }
+    /**
+     * Lookup a number of groups in parallel.
+     * @param dns - set of distinguished names.
+     * @param strictMode - if true, this method will throw if any lookups fail,
+     *      otherwise, failed lookups will be logged, and resolved as null.
+     */
+    Map<String, LdapGroup> lookupGroups(Set<String> dns, boolean strictMode = false) {
+        dns.collectEntries { dn -> [dn, task { lookupGroupInternal(dn, strictMode) }] }
             .collectEntries { [it.key, it.value.get()] }
     }
 
-    List<LdapGroup> findGroups(String sNamePart) {
-        searchMany("(sAMAccountName=*$sNamePart*)", LdapGroup)
+    /**
+     * Lookup group members for a number of groups in parallel.
+     * @param dns - set of distinguished names.
+     * @param strictMode - if true, this method will throw if any lookups fail,
+     *      otherwise, failed lookups will be logged, and resolved as an empty list.
+     */
+    Map<String, List<LdapPerson>> lookupGroupMembers(Set<String> dns, boolean strictMode = false) {
+        dns.collectEntries { dn -> [dn, task { lookupGroupMembersInternal(dn, strictMode) }] }
+            .collectEntries { [it.key, it.value.get()]}
     }
 
+
     //----------------------
     // Implementation
     //----------------------
-    private LdapGroup lookupGroup(String dn) {
-        searchOne("(distinguishedName=$dn)", LdapGroup)
+    private LdapGroup lookupGroupInternal(String dn, boolean strictMode) {
+        searchOne("(distinguishedName=$dn)", LdapGroup, strictMode)
     }
 
-    private <T extends LdapObject> T searchOne(String baseFilter, Class<T> objType) {
+    private List<LdapPerson> lookupGroupMembersInternal(String dn, boolean strictMode) {
+        // See class-level comment regarding this AD-specific query
+        searchMany("(|(member0f=$dn) (memberOf:1.2.840.113556.1.4.1941:=$dn))", LdapPerson, strictMode)
+    }
+
+    private <T extends LdapObject> T searchOne(String baseFilter, Class<T> objType, boolean strictMode) {
         for (server in config.servers) {
-            List<T> matches = doQuery(server, baseFilter, objType)
+            List<T> matches = doQuery(server, baseFilter, objType, strictMode)
             if (matches) return matches.first()
         }
         return null
     }
 
-    private <T extends LdapObject> List<T> searchMany(String baseFilter, Class<T> objType) {
+    private <T extends LdapObject> List<T> searchMany(String baseFilter, Class<T> objType, boolean strictMode) {
         List<T> ret = []
         for (server in config.servers) {
-            List<T> matches = doQuery(server, baseFilter, objType)
+            List<T> matches = doQuery(server, baseFilter, objType, strictMode)
             if (matches) ret.addAll(matches)
         }
         return ret
     }
 
-    private <T extends LdapObject> List<T> doQuery(Map server, String baseFilter, Class<T> objType) {
+    private <T extends LdapObject> List<T> doQuery(Map server, String baseFilter, Class<T> objType, boolean strictMode) {
         if (!enabled) throw new RuntimeException('LdapService is not enabled.')
 
         boolean isPerson = objType == LdapPerson
         String host = server.host,
-            filter = "(&(objectCategory=${isPerson ? 'Person' : 'Group'})$baseFilter)"
-
-        cache.getOrCreate(server.toString() + filter) {
-            withDebug(["Querying LDAP", [host: host, filter: filter]]) {
-                LdapNetworkConnection conn
-                try {
-
-                    String baseDn = isPerson ? server.baseUserDn : server.baseGroupDn,
-                        username = configService.getString('xhLdapUsername'),
-                        password = configService.getPwd('xhLdapPassword')
-                    String[] keys = objType.keys.toArray() as String[]
-
-                    conn = new LdapNetworkConnection(host)
-                    conn.timeOut = config.timeoutMs as Long
-                    conn.bind(username, password)
-                    conn.search(baseDn, filter, SearchScope.SUBTREE, keys)
-                        .collect { objType.create(it.attributes as Collection<Attribute>) }
-                } finally {
-                    conn?.unBind()
-                    conn?.close()
-                }
-            } as List<T>
+            filter = "(&(objectCategory=${isPerson ? 'Person' : 'Group'})$baseFilter)",
+            key = server.toString() + filter
+
+        List<T> ret = cache.get(key)
+        if (ret) return ret
+
+        withDebug(["Querying LDAP", [host: host, filter: filter]]) {
+            LdapNetworkConnection conn
+            try {
+
+                String baseDn = isPerson ? server.baseUserDn : server.baseGroupDn,
+                    username = configService.getString('xhLdapUsername'),
+                    password = configService.getPwd('xhLdapPassword')
+                String[] keys = objType.keys.toArray() as String[]
+
+                conn = new LdapNetworkConnection(host)
+                conn.timeOut = config.timeoutMs as Long
+                conn.bind(username, password)
+                ret = conn.search(baseDn, filter, SearchScope.SUBTREE, keys)
+                    .collect { objType.create(it.attributes as Collection<Attribute>) }
+                cache.put(key, ret)
+            } catch (Exception e) {
+                if (strictMode) throw e
+                logError("Failure querying", [host: host, filter: filter], e)
+                ret = null
+            } finally {
+                conn?.unBind()
+                conn?.close()
+            }
         }
+        return ret
+
     }
 
     private Map getConfig() {

grails-app/services/io/xh/hoist/role/provided/DefaultRoleAdminService.groovy

@@ -42,7 +42,7 @@ class DefaultRoleAdminService extends BaseService {
             Set<String> groups = roles.collectMany(new HashSet()) { it.directoryGroups }
             if (groups) {
                 try {
-                    Map<String, Object> groupsLookup = defaultRoleService.loadUsersForDirectoryGroups(groups)
+                    Map<String, Object> groupsLookup = defaultRoleService.loadUsersForDirectoryGroups(groups, true)
                     usersForGroups = groupsLookup.findAll { it.value instanceof Set }
                     errorsForGroups = groupsLookup.findAll { !(it.value instanceof Set) }
                 } catch (Throwable e) {

src/main/groovy/io/xh/hoist/role/provided/DefaultRoleService.groovy

@@ -165,12 +165,15 @@ class DefaultRoleService extends BaseRoleService {
      * enabled LdapService in the application.  Override this method to customize directory-based
      * lookup to attach to different, or additional external datasources.
      *
+     * If strictMode is true, implementations should throw on any partial failures.  Otherwise, they
+     * should log, and make a best-faith effort to return whatever groups they can load.
+     *
      * Method Map of directory group names to either:
      *  a) Set<String> of assigned users
      *     OR
      *  b) String describing lookup error.
      */
-    protected Map<String, Object> doLoadUsersForDirectoryGroups(Set<String> groups) {
+    protected Map<String, Object> doLoadUsersForDirectoryGroups(Set<String> groups, boolean strictMode) {
         if (!groups) return emptyMap()
         if (!ldapService.enabled) {
             return groups.collectEntries{[it, 'LdapService not enabled in this application']}
@@ -181,7 +184,7 @@ class DefaultRoleService extends BaseRoleService {
 
         // 1) Determine valid groups
         ldapService
-            .lookupGroups(groups)
+            .lookupGroups(groups, strictMode)
             .each {name, group ->
                 if (group) {
                     foundGroups << name
@@ -192,7 +195,7 @@ class DefaultRoleService extends BaseRoleService {
 
         // 2) Search for members of valid groups
         ldapService
-            .lookupGroupMembers(foundGroups)
+            .lookupGroupMembers(foundGroups, strictMode)
             .each {name, members ->
                 ret[name] = members.collect(new HashSet()) { it.samaccountname.toLowerCase()}
             }
@@ -272,8 +275,8 @@ class DefaultRoleService extends BaseRoleService {
     //---------------------------
     // Implementation/Framework
     //---------------------------
-    final Map<String, Object> loadUsersForDirectoryGroups(Set<String> directoryGroups) {
-        doLoadUsersForDirectoryGroups(directoryGroups)
+    final Map<String, Object> loadUsersForDirectoryGroups(Set<String> directoryGroups, boolean strictMode) {
+        doLoadUsersForDirectoryGroups(directoryGroups, strictMode)
     }
 
     void refreshRoleAssignments() {
@@ -289,10 +292,14 @@ class DefaultRoleService extends BaseRoleService {
 
         if (directoryGroupsSupported) {
             Set<String> groups = roles.collectMany(new HashSet()) { it.directoryGroups }
-            // Wrapped here to avoid failing implementations from ever bringing down app.
+
+            // Error handling on resolution.  Can be complex (e.g. parallel LDAP calls) so be robust.
+            // If we don't have results, take any results we can get, but
+            // if we do have results, never replace them with non-complete/imperfect set.
+            boolean strictMode = _usersForDirectoryGroups as boolean
             try {
                 Map<String, Object> usersForDirectoryGroups = [:]
-                loadUsersForDirectoryGroups(groups).each { k, v ->
+                loadUsersForDirectoryGroups(groups, strictMode).each { k, v ->
                     if (v instanceof Set) {
                         usersForDirectoryGroups[k] = v
                     } else {