Improve Directory Group lookup error handling

CHANGELOG.md

@@ -36,9 +36,19 @@
   `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` 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.
+
+
 ### 💥 Breaking Changes
 
 * Requires `hoist-react >= 63.0` for client-side support of the new `track` and `submitError` endpoints.
+*
+### 🐞 Bug Fixes
+
+* Fixed bug in `DefaultRoleService.doLoadUsersForDirectoryGroups` where LDAP members with `null`
+  samAccountNames were not being filtered out, causing `NullPointerExceptions`.
 
 
 ## 18.5.1 - 2024-03-08

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

@@ -119,9 +119,6 @@ class LdapService extends BaseService {
                     conn.bind(username, password)
                     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
                 } finally {
                     conn?.unBind()
                     conn?.close()

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)
+                    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
         )
     }
@@ -168,6 +171,7 @@ class DefaultRoleService extends BaseRoleService {
      *  b) String describing lookup error.
      */
     protected Map<String, Object> doLoadUsersForDirectoryGroups(Set<String> groups) {
+        if (!groups) return emptyMap()
         if (!ldapService.enabled) {
             return groups.collectEntries{[it, 'LdapService not enabled in this application']}
         }
@@ -182,15 +186,17 @@ class DefaultRoleService extends BaseRoleService {
                 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)
             .each {name, members ->
-                ret[name] = members.collect(new HashSet()) { it.samaccountname.toLowerCase()}
+                ret[name] = members.collect(new HashSet()) { it.samaccountname?.toLowerCase() }
+                // Exclude members without a samaccountname (e.g. email-only contacts within a DL)
+                ret[name].remove(null)
             }
 
         return ret
@@ -269,18 +275,10 @@ 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] }
-        }
+        doLoadUsersForDirectoryGroups(directoryGroups)
     }
 
-    protected void refreshCaches() {
+    void refreshRoleAssignments() {
         withDebug('Refreshing role caches') {
             _allRoleAssignments = unmodifiableMap(generateRoleAssignments())
             _roleAssignmentsByUser = new ConcurrentHashMap()
@@ -290,16 +288,23 @@ 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)
+            // Wrapped here to avoid failing implementations from ever bringing down app.
+            try {
+                Map<String, Object> usersForDirectoryGroups = [:]
+                loadUsersForDirectoryGroups(groups).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)
             }
         }
 
@@ -313,7 +318,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")
@@ -347,6 +352,9 @@ class DefaultRoleService extends BaseRoleService {
     }
 
     void clearCaches() {
+        _allRoleAssignments = emptyMap()
+        _roleAssignmentsByUser = new ConcurrentHashMap()
+        _usersForDirectoryGroups = emptyMap()
         timer.forceRun()
         super.clearCaches()
     }