Improve Directory Group lookup error handling #339
Conversation
| logError(errMsg, e) | ||
| return directoryGroups.collectEntries {[it, errMsg] } | ||
| } | ||
| !directoryGroups ? emptyMap() : doLoadUsersForDirectoryGroups(directoryGroups) |
There was a problem hiding this comment.
perhaps inline the "do" method -- one less method to trace through
| try { | ||
| Map<String, Object> groupsLookup = defaultRoleService.loadUsersForDirectoryGroups(groups) | ||
| usersForGroups = groupsLookup.findAll { it.value instanceof Set } | ||
| errorsForGroups = groupsLookup.findAll { !(it.value instanceof Set) } |
There was a problem hiding this comment.
To confirm - we might still get errorsForGroups populated as expected with partial lookup failures, but now only in the case of 'No AD group found' - is that correct?
Checking that a search returnign no result does not throw- that would be reasonable but didn't want to assume.
(Would suggest changing to "Directory Group not found" FWIW)
There was a problem hiding this comment.
Correct - will double-check this now to make sure. And sounds good re: change to error text
There was a problem hiding this comment.
Confirmed! Built a snapshot from this branch and tested in a client app to confirm.
| } | ||
| _usersForDirectoryGroups = usersForDirectoryGroups | ||
| } catch (Throwable e) { | ||
| logError("Error resolving users for directory groups", e) |
There was a problem hiding this comment.
Worth a comment to clarify that leaving the previous _usersForDirectoryGroups in place (and then re-processing it below) is a deliberate choice?
|
Trying to think big picture here and wanted to get this out here. In particular, don't want to trade one set of problems for another. What motivated this change was the concern that we are making a large number of individual LDAP queries to get groups right now. If any one of them fails (e.g. timeout, or just flaky) we can basically "blip out" user access for all of the users in the group that failed. And we may be just doing our 10 minute refresh, replacing a perfectly good result with a flaky result -- and that seemed wrong. So this should fix that. New concern -- now a single flaky call keeps us from ever getting needed roles in the app. Or worse yet, someone enters something as a new or modified group tag that somehow causes all role groups to fail to load. Maybe its overkill, but we could pass a flag to indicate whether we wanted "strict mode" or whether we are happy to have whatever results we can get? The latter could be useful when we start-up, or for whatever reason have no successful load of groups. |
# Conflicts: # CHANGELOG.md
|
Created a new branch here to incorporate my thoughts above. Greg reveiwed and testing out now |
No description provided.