From 4e1e8b88498b94f6dd2409e2ca72a85c5fc35116 Mon Sep 17 00:00:00 2001 From: lbwexler Date: Wed, 3 Apr 2024 11:29:00 -0400 Subject: [PATCH] Improved handling of errors during parallel/bulk LDAP lookup --- CHANGELOG.md | 8 +- .../xh/hoist/admin/RoleAdminController.groovy | 2 +- .../io/xh/hoist/ldap/LdapService.groovy | 104 +++++++++++------- .../provided/DefaultRoleAdminService.groovy | 2 +- .../role/provided/DefaultRoleService.groovy | 21 ++-- 5 files changed, 86 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bb9dcdd..f3f8a2ed 100644 --- a/CHANGELOG.md +++ b/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 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 c9fc904f..9af65d37 100644 --- a/grails-app/services/io/xh/hoist/ldap/LdapService.groovy +++ b/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 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> 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 T searchOne(String baseFilter, Class objType) { + 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, 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)" - - 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) } - } finally { - conn?.unBind() - conn?.close() - } - } as List + filter = "(&(objectCategory=${isPerson ? 'Person' : 'Group'})$baseFilter)", + key = server.toString() + filter + + List 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) } + 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() { 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 192a974e..24570063 100644 --- a/grails-app/services/io/xh/hoist/role/provided/DefaultRoleAdminService.groovy +++ b/grails-app/services/io/xh/hoist/role/provided/DefaultRoleAdminService.groovy @@ -42,7 +42,7 @@ class DefaultRoleAdminService extends BaseService { Set groups = roles.collectMany(new HashSet()) { it.directoryGroups } if (groups) { try { - Map groupsLookup = defaultRoleService.loadUsersForDirectoryGroups(groups) + Map groupsLookup = defaultRoleService.loadUsersForDirectoryGroups(groups, true) usersForGroups = groupsLookup.findAll { it.value instanceof Set } errorsForGroups = groupsLookup.findAll { !(it.value instanceof Set) } } catch (Throwable e) { 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 b4bb7993..88659d02 100644 --- a/src/main/groovy/io/xh/hoist/role/provided/DefaultRoleService.groovy +++ b/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 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']} @@ -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 loadUsersForDirectoryGroups(Set directoryGroups) { - doLoadUsersForDirectoryGroups(directoryGroups) + final Map loadUsersForDirectoryGroups(Set directoryGroups, boolean strictMode) { + doLoadUsersForDirectoryGroups(directoryGroups, strictMode) } void refreshRoleAssignments() { @@ -289,10 +292,14 @@ class DefaultRoleService extends BaseRoleService { if (directoryGroupsSupported) { Set 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 usersForDirectoryGroups = [:] - loadUsersForDirectoryGroups(groups).each { k, v -> + loadUsersForDirectoryGroups(groups, strictMode).each { k, v -> if (v instanceof Set) { usersForDirectoryGroups[k] = v } else {