Support matching LDAP external groups by full DN, not just bare CN#3982
Draft
duanemay wants to merge 4 commits into
Draft
Support matching LDAP external groups by full DN, not just bare CN#3982duanemay wants to merge 4 commits into
duanemay wants to merge 4 commits into
Conversation
UAA only ever reports the bare cn of an LDAP group as its "external group" name (used for the externalGroupsWhitelist and /userinfo's roles field), never the full DN. This breaks CF's external-group role-assignment feature when the CF-side mapping is configured with a group's full DN, since it can never match what UAA reports. Adds the includeExternalGroupDn plumbing (config field, accessors, uaa.yml wiring) needed for the fix, plus tests that pin down current default behavior and demonstrate the gap: enabling the flag should add the DN as an additional matchable external-group name, but the extraction logic hasn't been updated yet, so those new assertions fail. The fix lands in a follow-up commit.
…upDn is enabled UAA only ever surfaced the bare cn of an LDAP group as its "external group" name, discarding the full DN even though it's already present on every LdapAuthority. This broke CF's external-group role assignment feature when the CF-side mapping was configured using a group's full DN, since that string could never match. getAuthoritiesAsNames() now unions the DN in as an additional candidate name (never replacing cn) when includeExternalGroupDn is enabled, so existing cn-based whitelist/scope configs are unaffected by default and admins can opt in to DN-based matching. Makes the previously-failing tests from the prior commit pass.
Adds the new property to UAA-LDAP.md and UAA-APIs.rst (also correcting the existing externalGroupsWhitelist description, which incorrectly claimed DN values were already supported), a commented uaa.yml sample, and the REST Docs field descriptor for the identity providers API snippet generator.
…Whitelist, includeExternalGroupDn UAA-Configuration-Reference.md's LDAP table was missing entries for groupRoleAttribute and externalGroupsWhitelist even though both are documented elsewhere (UAA-LDAP.md, UAA-APIs.rst). Backfilling those alongside the new includeExternalGroupDn property so the table and its per-property detail sections stay internally consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UAA only ever reports the bare
cnof an LDAP group as a user's "external group" name — used forldap.externalGroupsWhitelistmatching and the/userinfoendpoint'srolesfield — even though the group's full DN is already available on everyLdapAuthorityand is silently discarded. This traces back to a hardcodedla.getAttributeValues("cn")call inLdapLoginAuthenticationManager.getAuthoritiesAsNames()that's been unchanged since 2015.This breaks CF's external-group role-assignment feature when the CF-side role mapping is configured using a group's full DN (e.g.
cn=cf_admin,ou=dev,dc=example,dc=com— what an admin gets copy-pasting a group out of Active Directory), since UAA never reports a string that matches it.Fix
Adds a new opt-in LDAP identity provider config property,
includeExternalGroupDn(defaultfalse). When enabled, the group's full DN is added as an additional candidate name alongside the existingcnvalue — never replacing it — so:cn-based whitelist entries can add DN-based entries without breaking thecn-based ones.true, even though it's purely additive to whitelist matching:externalGroupsWhitelistdefaults to "allow everything," so turning it on by default would silently add every LDAP group's DN into/userinfo'srolesfield and the ID token roles claim for every existing customer on upgrade — a real, externally-visible payload change nobody asked for. This mirrors the existing precedent ofuaa.jwt.access.include_external_groups, which is also opt-in for the same reason.This is scoped entirely to LDAP's whitelist/
/userinfo-roles pipeline. It does not touch:external_group_mappingtable /LdapGroupMappingAuthorizationManager), which already supports DN-based matching via the unrelatedgroupRoleAttributeconfigChanges
LdapIdentityProviderDefinition/LdapUtils: newincludeExternalGroupDnfield, accessors, anduaa.ymlbootstrap wiringLdapLoginAuthenticationManager:getAuthoritiesAsNames()unions in the DN when the flag is enabledUAA-LDAP.md,UAA-APIs.rst,UAA-Configuration-Reference.md,uaa.ymlsample, REST Docs field descriptor — also corrected a pre-existing doc inaccuracy that claimedexternalGroupsWhitelistalready matched on DN values, and backfilled two other undocumented LDAP properties (groupRoleAttribute,externalGroupsWhitelist) found missing from the config reference tableTest plan
LdapLoginAuthenticationManagerTests,LdapIdentityProviderDefinitionTest) covering default-off behavior, DN-only matching, and mixed cn+DN whitelistsAbstractLdapMockMvcTest) against a real embedded LDAP server, covering both theas-scopesandmap-to-scopesgroup profilesIdentityProviderEndpointDocs) verified green after adding the new field descriptor🤖 Generated with Claude Code