diff --git a/CHANGELOG.md b/CHANGELOG.md index c91a64dd..f3f8a2ed 100644 --- a/CHANGELOG.md +++ b/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.1 - 2024-03-08 diff --git a/grails-app/controllers/io/xh/hoist/admin/RoleAdminController.groovy b/grails-app/controllers/io/xh/hoist/admin/RoleAdminController.groovy index d5409622..a3e4237d 100644 --- a/grails-app/controllers/io/xh/hoist/admin/RoleAdminController.groovy +++ b/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]) } diff --git a/grails-app/services/io/xh/hoist/ldap/LdapService.groovy b/grails-app/services/io/xh/hoist/ldap/LdapService.groovy index f8374c47..2e467951 100644 --- a/grails-app/services/io/xh/hoist/ldap/LdapService.groovy +++ b/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 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> lookupGroupMembers(Set dns) { - dns.collectEntries { dn -> [dn, task { lookupGroupMembers(dn) }] } - .collectEntries { [it.key, it.value.get()] } + List findGroups(String sNamePart) { + searchMany("(sAMAccountName=*$sNamePart*)", LdapGroup, true) } - Map lookupGroups(Set 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 lookupGroups(Set dns, boolean strictMode = false) { + dns.collectEntries { dn -> [dn, task { lookupGroupInternal(dn, strictMode) }] } .collectEntries { [it.key, it.value.get()] } } - List 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> lookupGroupMembers(Set 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 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 searchOne(String baseFilter, Class objType) { + private T searchOne(String baseFilter, Class objType, boolean strictMode) { for (server in config.servers) { - List matches = doQuery(server, baseFilter, objType) + List matches = doQuery(server, baseFilter, objType, strictMode) if (matches) return matches.first() } return null } - private List searchMany(String baseFilter, Class objType) { + private List searchMany(String baseFilter, Class objType, boolean strictMode) { List ret = [] for (server in config.servers) { - List matches = doQuery(server, baseFilter, objType) + List matches = doQuery(server, baseFilter, objType, strictMode) if (matches) ret.addAll(matches) } return ret } - private List doQuery(Map server, String baseFilter, Class objType) { + private List doQuery(Map server, String baseFilter, Class 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 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) } - } 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 + } catch (Exception e) { + if (strictMode) throw e + logError("Failure querying", [host: host, filter: filter], e) + ret = null + } } + return ret } private Map getConfig() { diff --git a/grails-app/services/io/xh/hoist/role/provided/DefaultRoleAdminService.groovy b/grails-app/services/io/xh/hoist/role/provided/DefaultRoleAdminService.groovy index 1e2d2df7..24570063 100644 --- a/grails-app/services/io/xh/hoist/role/provided/DefaultRoleAdminService.groovy +++ b/grails-app/services/io/xh/hoist/role/provided/DefaultRoleAdminService.groovy @@ -41,9 +41,15 @@ class DefaultRoleAdminService extends BaseService { if (defaultRoleService.directoryGroupsSupported) { Set groups = roles.collectMany(new HashSet()) { it.directoryGroups } if (groups) { - Map groupsLookup = defaultRoleService.loadUsersForDirectoryGroups(groups) - usersForGroups = groupsLookup.findAll { it.value instanceof Set } - errorsForGroups = groupsLookup.findAll { !(it.value instanceof Set) } + try { + Map 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] } + } } } diff --git a/grails-app/services/io/xh/hoist/role/provided/DefaultRoleUpdateService.groovy b/grails-app/services/io/xh/hoist/role/provided/DefaultRoleUpdateService.groovy index 742773df..9b1c17e9 100644 --- a/grails-app/services/io/xh/hoist/role/provided/DefaultRoleUpdateService.groovy +++ b/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 } diff --git a/src/main/groovy/io/xh/hoist/role/provided/DefaultRoleService.groovy b/src/main/groovy/io/xh/hoist/role/provided/DefaultRoleService.groovy index b2c8aa5a..88659d02 100644 --- a/src/main/groovy/io/xh/hoist/role/provided/DefaultRoleService.groovy +++ b/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> _allRoleAssignments = emptyMap() protected ConcurrentMap> _roleAssignmentsByUser = new ConcurrentHashMap<>() + protected Map _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 of assigned users * OR * b) String describing lookup error. */ - protected Map doLoadUsersForDirectoryGroups(Set groups) { + protected Map doLoadUsersForDirectoryGroups(Set 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()} } @@ -268,19 +275,11 @@ class DefaultRoleService extends BaseRoleService { //--------------------------- // Implementation/Framework //--------------------------- - final Map loadUsersForDirectoryGroups(Set 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 loadUsersForDirectoryGroups(Set directoryGroups, boolean strictMode) { + doLoadUsersForDirectoryGroups(directoryGroups, strictMode) } - protected void refreshCaches() { + void refreshRoleAssignments() { withDebug('Refreshing role caches') { _allRoleAssignments = unmodifiableMap(generateRoleAssignments()) _roleAssignmentsByUser = new ConcurrentHashMap() @@ -290,16 +289,27 @@ class DefaultRoleService extends BaseRoleService { @ReadOnly protected Map> generateRoleAssignments() { List roles = Role.list() - Map usersForDirectoryGroups = [:] if (directoryGroupsSupported) { Set 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 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) } } @@ -313,7 +323,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 +357,9 @@ class DefaultRoleService extends BaseRoleService { } void clearCaches() { + _allRoleAssignments = emptyMap() + _roleAssignmentsByUser = new ConcurrentHashMap() + _usersForDirectoryGroups = emptyMap() timer.forceRun() super.clearCaches() }