Directory group error handling plus (#347)

CHANGELOG.md

@@ -36,9 +36,16 @@
   `JSON` object posted in the `body`. This is pattern is preferred over using a `fetchJSON` request with
   `params` posted in the request header.
 
+* `DefaultRoleService` has improved error handling for failed directory group lookups.
+* `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.2 - 2024-04-03
 

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,83 +51,107 @@ 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)
+        lookupGroupMembersInternal(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 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) {
+    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)"
+            filter = "(&(objectCategory=${isPerson ? 'Person' : 'Group'})$baseFilter)",
+            key = server.toString() + filter
 
-        cache.getOrCreate(server.toString() + filter) {
-            withDebug(["Querying LDAP", [host: host, filter: filter]]) {
-                LdapNetworkConnection conn
-                try {
+        List<T> ret = cache.get(key)
+        if (ret != null) return ret
 
-                    String baseDn = isPerson ? server.baseUserDn : server.baseGroupDn,
-                        username = configService.getString('xhLdapUsername'),
-                        password = configService.getPwd('xhLdapPassword')
-                    String[] keys = objType.keys.toArray() as String[]
+        withDebug(["Querying LDAP", [host: host, filter: filter]]) {
+            try (LdapNetworkConnection conn = new LdapNetworkConnection(host)) {
+                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.timeOut = config.timeoutMs as Long
+
+                boolean didBind = false
+                try {
                     conn.bind(username, password)
-                    conn.search(baseDn, filter, SearchScope.SUBTREE, keys)
+                    didBind = true
+                    ret = conn.search(baseDn, filter, SearchScope.SUBTREE, keys)
                         .collect { objType.create(it.attributes as Collection<Attribute>) }
-                } catch (Exception e) {
-                    logError("Failure querying", [host: host, filter: filter], e)
-                    return null
+                    cache.put(key, ret)
                 } finally {
-                    conn?.unBind()
-                    conn?.close()
+                    // Calling unBind on an unbound connection will throw an exception
+                    if (didBind) conn.unBind()
                 }
-            } as List<T>
+            } catch (Exception e) {
+                if (strictMode) throw e
+                logError("Failure querying", [host: host, filter: filter], e)
+                ret = null
+            }
         }
+        return ret
     }
 
     private Map getConfig() {

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

@@ -41,9 +41,15 @@ class DefaultRoleAdminService extends BaseService {
         if (defaultRoleService.directoryGroupsSupported) {
             Set<String> groups = roles.collectMany(new HashSet()) { it.directoryGroups }
             if (groups) {
-                Map<String, Object> groupsLookup = defaultRoleService.loadUsersForDirectoryGroups(groups)
-                usersForGroups = groupsLookup.findAll { it.value instanceof Set }
-                errorsForGroups = groupsLookup.findAll { !(it.value instanceof Set) }
+                try {
+                    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) {
+                    def errMsg = 'Error resolving Directory Groups'
+                    logError(errMsg, e)
+                    errorsForGroups = groups.collectEntries {[it, errMsg] }
+                }
             }
         }
 

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

@@ -50,7 +50,7 @@ class DefaultRoleUpdateService extends BaseService {
         roleToDelete.delete(flush: true)
 
         trackService.track(msg: "Deleted role: '$id'", category: 'Audit')
-        defaultRoleService.clearCaches()
+        defaultRoleService.refreshRoleAssignments()
     }
 
 
@@ -99,7 +99,7 @@ class DefaultRoleUpdateService extends BaseService {
             if (role) {
                 role.addToMembers(type: USER, name: user.username, createdBy: 'defaultRoleUpdateService')
                 role.save(flush: true)
-                defaultRoleService.clearCaches()
+                defaultRoleService.refreshRoleAssignments()
             } else {
                 logWarn("Failed to find role $roleName to assign to $user", "role will not be assigned")
             }
@@ -163,7 +163,7 @@ class DefaultRoleUpdateService extends BaseService {
             )
         }
 
-        defaultRoleService.clearCaches()
+        defaultRoleService.refreshRoleAssignments()
         return role
     }
 

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

@@ -58,6 +58,8 @@ import static io.xh.hoist.util.InstanceConfigUtils.getInstanceConfig
  * Role.
  *
  * Applications wishing to extend this feature should override doLoadUsersForDirectoryGroups().
+ * If doLoadUsersForDirectoryGroups() throws an exception, this service will use the last successful
+ * lookup result and log an error. Clearing this service's caches will also clear the cached lookup.
  *
  * Certain aspects of this service and its Admin Console UI are soft-configurable via a JSON
  * `xhRoleModuleConfig`. This service will create this config entry if not found on startup.
@@ -81,6 +83,7 @@ class DefaultRoleService extends BaseRoleService {
     private Timer timer
     protected Map<String, Set<String>> _allRoleAssignments = emptyMap()
     protected ConcurrentMap<String, Set<String>> _roleAssignmentsByUser = new ConcurrentHashMap<>()
+    protected Map<String, Object> _usersForDirectoryGroups = emptyMap()
 
     static clearCachesConfigs = ['xhRoleModuleConfig']
 
@@ -89,7 +92,7 @@ class DefaultRoleService extends BaseRoleService {
 
         timer = createTimer(
             interval: { config.refreshIntervalSecs as int * DateTimeUtils.SECONDS },
-            runFn: this.&refreshCaches,
+            runFn: this.&refreshRoleAssignments,
             runImmediatelyAndBlock: true
         )
     }
@@ -162,12 +165,16 @@ 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']}
         }
@@ -177,18 +184,18 @@ class DefaultRoleService extends BaseRoleService {
 
         // 1) Determine valid groups
         ldapService
-            .lookupGroups(groups)
+            .lookupGroups(groups, strictMode)
             .each {name, group ->
                 if (group) {
                     foundGroups << name
                 } else {
-                    ret[name] = 'No AD group found'
+                    ret[name] = 'Directory Group not found'
                 }
             }
 
         // 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() }
                 // Exclude members without a samaccountname (e.g. email-only contacts within a DL)
@@ -270,19 +277,11 @@ class DefaultRoleService extends BaseRoleService {
     //---------------------------
     // Implementation/Framework
     //---------------------------
-    final Map<String, Object> loadUsersForDirectoryGroups(Set<String> directoryGroups) {
-       if (!directoryGroups) return emptyMap()
-        // Wrapped here to avoid failing implementations from ever bringing down app.
-        try {
-            return doLoadUsersForDirectoryGroups(directoryGroups)
-        } catch (Throwable e) {
-            def errMsg = 'Error resolving Directory Groups'
-            logError(errMsg, e)
-            return directoryGroups.collectEntries {[it, errMsg] }
-        }
+    final Map<String, Object> loadUsersForDirectoryGroups(Set<String> directoryGroups, boolean strictMode) {
+        doLoadUsersForDirectoryGroups(directoryGroups, strictMode)
     }
 
-    protected void refreshCaches() {
+    void refreshRoleAssignments() {
         withDebug('Refreshing role caches') {
             _allRoleAssignments = unmodifiableMap(generateRoleAssignments())
             _roleAssignmentsByUser = new ConcurrentHashMap()
@@ -292,16 +291,27 @@ class DefaultRoleService extends BaseRoleService {
     @ReadOnly
     protected Map<String, Set<String>> generateRoleAssignments() {
         List<Role> roles = Role.list()
-        Map<String, Object> usersForDirectoryGroups = [:]
 
         if (directoryGroupsSupported) {
             Set<String> groups = roles.collectMany(new HashSet()) { it.directoryGroups }
-            loadUsersForDirectoryGroups(groups).each { k, v ->
-                if (v instanceof Set) {
-                    usersForDirectoryGroups[k] = v
-                } else {
-                    logError("Error resolving users for directory group", k, v)
+
+            // 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, strictMode).each { k, v ->
+                    if (v instanceof Set) {
+                        usersForDirectoryGroups[k] = v
+                    } else {
+                        logError("Error resolving users for directory group", k, v)
+                    }
                 }
+                _usersForDirectoryGroups = usersForDirectoryGroups
+            } catch (Throwable e) {
+                // Leave existing _usersForDirectoryGroups cache in place, log error, and continue.
+                logError("Error resolving users for directory groups", e)
             }
         }
 
@@ -315,7 +325,7 @@ class DefaultRoleService extends BaseRoleService {
                 if (directoryGroupsSupported) groups.addAll(effRole.directoryGroups)
             }
             groups.each { group ->
-                usersForDirectoryGroups[group]?.each { users << it.toLowerCase() }
+                _usersForDirectoryGroups[group]?.each { users << it.toLowerCase() }
             }
 
             logTrace("Generated assignments for ${role.name}", "${users.size()} effective users")
@@ -349,6 +359,9 @@ class DefaultRoleService extends BaseRoleService {
     }
 
     void clearCaches() {
+        _allRoleAssignments = emptyMap()
+        _roleAssignmentsByUser = new ConcurrentHashMap()
+        _usersForDirectoryGroups = emptyMap()
         timer.forceRun()
         super.clearCaches()
     }