refactor: allow multiple UaaTokenEnhancers#3904
refactor: allow multiple UaaTokenEnhancers#3904peterhaochen47 wants to merge 1 commit intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates UaaTokenServices to support configuring multiple UaaTokenEnhancer instances (instead of a single enhancer), while keeping the legacy setUaaTokenEnhancer API for backward compatibility (now deprecated).
Changes:
- Add
setUaaTokenEnhancers(List<UaaTokenEnhancer>)and migrate token-claim enrichment to iterate over multiple enhancers. - Deprecate
setUaaTokenEnhancer(UaaTokenEnhancer)and adapt it to delegate to the new list-based setter. - Add/extend tests to verify that claims from multiple enhancers are applied to generated tokens.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenServices.java |
Introduces list-based enhancer injection and merges enhancer-produced claims into access tokens; deprecates the single-enhancer setter. |
uaa/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenServicesTests.java |
Adds coverage ensuring multiple enhancers’ claims appear in issued JWT access tokens. |
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/DeprecatedUaaTokenServicesTests.java |
Adds coverage ensuring the deprecated token services path also supports multiple enhancers. |
| if (enhancer != null) { | ||
| Map<String, Object> claims = enhancer.enhance(emptyMap(), authentication); | ||
| if (claims != null) { | ||
| additionalRootClaims.putAll(claims); |
There was a problem hiding this comment.
I am not sure merging the claim values from different token enhancers is a good idea; that would result in unpredictable/unintended final outcome. It seems more rigorous that an enhancer has deterministic power of the final outcome of the claim it touches, and if there is a conflict, the later enhancer wins.
If we do wanna let two enhancers touch on the same claim (not a use case now), we could pass in the previously added claims resulted from prior enhancers to the current enhancer, like an assembly line, so the current enhancer knows what has already been added to what claims (and merge if needed):
enhancer.enhance(additionalRootClaims(), authentication);
But we don't need this for now; we could always add this when we need to. But once we add this, we can't remove it without a breaking change. So I'm holding off on that for now.
There was a problem hiding this comment.
I like this idea, so each successive enhancer (ordered by @order) can see previous changes and update as needed.
- previously, only one is allowed - add setUaaTokenEnhancers function to allow adding multiple UaaTokenEnhancers (this func will be implicitly called when UaaTokenEnhancer bean/beans are supplied) - refactor setUaaTokenEnhancer function (but keep same general business logic as before) for backward compatibility; mark as deprecated
c38902f to
28feee4
Compare
when UaaTokenEnhancer bean/beans are supplied)
incorporated feedback from earlier PR: #3899