refactor: allow multiple UaaTokenEnhancers#3904
Conversation
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
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.
There was a problem hiding this comment.
Yeah, we can add that if you think this is the time to do so. Added a new commit.
peterhaochen47
force-pushed
the
pr/develop/refactor-allow-multiple-token-enhancers-2
branch
from
c38902f to
28feee4
Compare
|
@duanemay any other requests for change? |
| if (!uaaTokenEnhancers.isEmpty()) { | ||
| additionalRootClaims = new HashMap<>(); | ||
| for (UaaTokenEnhancer enhancer : uaaTokenEnhancers) { | ||
| if (enhancer != null) { |
There was a problem hiding this comment.
Should we filter the list for non-null values already in the setter?
There was a problem hiding this comment.
suggestion adopted
| @DisplayName("claims are passed to subsequent enhancers") | ||
| @ParameterizedTest | ||
| @ValueSource(strings = {GRANT_TYPE_PASSWORD, GRANT_TYPE_AUTHORIZATION_CODE}) | ||
| void claimsArePassedToSubsequentEnhancers(String grantType) { |
There was a problem hiding this comment.
Would it make sense to add another test case where enhancer2 returns a value for claim1 and thereby overrides the value from enhancer1?
There was a problem hiding this comment.
suggestion adopted
peterhaochen47
force-pushed
the
pr/develop/refactor-allow-multiple-token-enhancers-2
branch
from
7cad5a8 to
bc378a5
Compare
- 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
- Instead of passing an empty map to each token enhancer, pass the accumulated claims from prior enhancers. - This allows successive enhancers to see previous changes and update or merge claims as needed.
- so do not have to perform null checks during runtime when the enhancers are invoked - this pattern exists at many other places in this codebase - also: backfill test on later enhancer overriding earlier enhancer on token claim
peterhaochen47
force-pushed
the
pr/develop/refactor-allow-multiple-token-enhancers-2
branch
from
bc378a5 to
3c7e39d
Compare
when UaaTokenEnhancer bean/beans are supplied)
incorporated feedback from earlier PR: #3899