Add nested group resolution to LDAP group provider with optional AD matching-rule support#28492
Conversation
OmerRaifler
force-pushed
the
feature/add-nested-ldap-group-resolution
branch
3 times, most recently
from
9c90ea8 to
d0697cb
Compare
| SearchResult groupResult = search.next(); | ||
| Attribute groupName = groupResult.getAttributes().get(groupsNameAttribute); | ||
| if (groupName == null) { | ||
| log.warn("The group object [%s] does not have group name attribute [%s]. Falling back on object full name.", groupResult, groupsNameAttribute); |
There was a problem hiding this comment.
Consider debug log level to avoid this warning being thrown for every user?
There was a problem hiding this comment.
I agree that warning on every malformed group entry can become noisy.
I changed the implementation to log the per-entry details at debug, and emit a single warn if any group is missing the configured ldap.group-name-attribute. That preserves operator visibility for misconfiguration or unexpected LDAP schema while avoiding repeated warnings for every user lookup.
| * - `ldap.group-search-member-attribute` | ||
| - Attribute from group documents used for filtering by member. For example, | ||
| `cn`. | ||
| * - `ldap.group-search-enable-nested-groups` |
There was a problem hiding this comment.
Can you sort the configuration keys alphabetically?
There was a problem hiding this comment.
Or consider renaming them so all nested properties stay grouped (😉), so 'ldap.group-search-nested-enable' and 'ldap.group-search-nested-use-matching-rule-in-chain'
There was a problem hiding this comment.
I renamed the properties 🙌
|
I'm not able to test it IRL, but the code looks good. |
OmerRaifler
force-pushed
the
feature/add-nested-ldap-group-resolution
branch
from
d0697cb to
32b3342
Compare
Thanks for the review and positive feedback! I will bump this on Slack. |
2 participants
Motivation
LDAP_MATCHING_RULE_IN_CHAINmatching rule to avoid recursive client-side lookups.Description
ldap.group-search-enable-nested-groupsandldap.group-search-use-matching-rule-in-chainand wired them intoLdapFilteringGroupProviderConfigwith getters, setters, and config annotations.LdapFilteringGroupProviderwith three resolution modes: direct-only (resolveDirectGroups), recursive client-side resolution (resolveGroupsRecursively), and single-query AD matching-rule resolution (resolveGroupsInSingleQuery), which uses the matching rule OID1.2.840.113556.1.4.1941when enabled.extractGroupsand introduced a smallLdapGrouprecord to carry both distinguished name and display name; updated search filter construction to support the matching-rule predicate.group-mapping.mdto describe the new properties and explain nested group behavior and the AD matching-rule option.TestLdapFilteringGroupProviderConfigandTestLdapGroupProviderIntegrationto cover defaults, explicit mapping, and nested group behavior (added a nestedengineeringgroup and additional config builder for nested mode).Testing
TestLdapFilteringGroupProviderConfigto verify default values and property mapping, and it passed.TestLdapGroupProviderIntegrationwhich exercises direct, recursive, and AD matching-rule flows via a test OpenLDAP server, and it passed.