Fix refresh token grant failing after client secret rotation (ADD) - #4052
Merged
Conversation
refreshAccessToken() ran a second, redundant revocation-signature check that only compared against a single, hardcoded "newest secret" instead of every currently valid client secret. The primary check performed a few lines earlier (tokenValidationService.validateToken) already loops over all client secrets correctly, so any refresh token issued while a client had only one secret was wrongly rejected with "revocable signature mismatch" as soon as a second secret was added via changeMode=ADD, defeating zero-downtime secret rotation. Fixes #4047 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes a refresh-token validation bug triggered by zero-downtime client secret rotation (changeMode=ADD) by removing a redundant, single-secret revocation-signature check that incorrectly rejected refresh tokens signed with an older (but still valid) client secret.
Changes:
- Removed the redundant
throwIfInvalidRevocationHashSignature(...)check fromUaaTokenServices.refreshAccessToken(...)(multi-secret validation is already correctly handled byTokenValidationService.validateToken(...)). - Deleted the now-unused helper method.
- Added a regression test that issues a refresh token pre-rotation, adds a second secret, and verifies the original refresh token can still be redeemed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/token/TokenMvcMockTests.java |
Adds a regression test covering refresh-token redemption after ADD secret rotation. |
server/src/main/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenServices.java |
Removes the redundant single-secret revocation-signature validation that breaks multi-secret rotation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
Comment on lines
+3220
to
+3228
| // the refresh token issued before the rotation must still be redeemable with the original secret | ||
| mockMvc.perform(post("/oauth/token") | ||
| .accept(MediaType.APPLICATION_JSON_VALUE) | ||
| .with(httpBasic(clientId, SECRET)) | ||
| .param("grant_type", "refresh_token") | ||
| .param("client_id", clientId) | ||
| .param("client_secret", SECRET) | ||
| .param("refresh_token", refreshToken)) | ||
| .andExpect(status().isOk()); |
strehle
approved these changes
Aug 25, 2026
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
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
changeMode=ADD(the multi-secret, zero-downtime rotation feature),grant_type=refresh_tokencalls failed withinvalid_token: revocable signature mismatchfor any refresh token issued before the rotation — regardless of which secret was used to authenticate.UaaTokenServices.refreshAccessToken()ran a second, redundant revocation-signature check (throwIfInvalidRevocationHashSignature) immediately after the correct check (tokenValidationService.validateToken(...), which already loops over every currently valid client secret) had already passed. The redundant check compared against a single, hardcoded "newest secret" only, so it rejected tokens signed against an older still-valid secret.Test plan
refreshTokenIssuedBeforeAddClientSecretStillWorksAfterAddinTokenMvcMockTests.java: issues a password-grant refresh token while the client has one secret, adds a second secret viachangeMode=ADD, then confirms the original refresh token still redeems successfully.TokenMvcMockTestssuite and theorg.cloudfoundry.identity.uaa.oauth.*package in the server module — all passing.uaa-cli, and confirmed the fix resolves it.🤖 Generated with Claude Code