Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation follows existing error-event handling and includes appropriate integration coverage.
Pull request overview
Logs audit events when token post-processors reject token requests.
Changes:
- Records the rejection reason and
invalid_requestevent. - Adds integration coverage for invalid resource indicators.
File summaries
| File | Description |
|---|---|
services/.../TokenEndpoint.java |
Logs TokenInterceptorException failures. |
tests/.../ResourceIndicatorsTest.java |
Verifies the resulting client-login error event. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The TokenInterceptorException catch block in TokenEndpoint did not record an event detail or error before throwing, unlike the sibling RefreshTokenException catch block right below it. This meant every rejected resource-indicator request (handled by ResourceIndicatorsPostProcessor) left no admin/audit event trail at all. Mirror the RefreshTokenException catch block: record the exception's description as the event reason and log Errors.INVALID_REQUEST before throwing the CorsErrorResponseException, matching this method's existing error-event convention. Added an integration test (testClientCredentialsInvalidResourceLogsErrorEvent in ResourceIndicatorsTest) asserting the event is now recorded, and confirmed it fails without the fix and passes with it. Closes keycloak#52568 Signed-off-by: wakqasahmed <training@saerintech.com>
wakqasahmed
force-pushed
the
fix/token-interceptor-exception-event-logging
branch
from
September 13, 2026 07:31
6af10a8 to
2bb23bd
Compare
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.
Closes #52568
TokenEndpoint'sprocess()catches bothTokenInterceptorExceptionandRefreshTokenExceptionaroundgrant.process(context), but only theRefreshTokenExceptionbranch records an event detail/error before throwing. TheTokenInterceptorExceptionbranch (hit wheneverResourceIndicatorsPostProcessorrejects an invalid, mismatched, or unresolvableresourceindicator) throws straight away with no event logged, so these requests leave no trace in the admin/audit event log at all, unlike every other rejection path in this method.This PR mirrors the existing
RefreshTokenExceptionhandling: it records the exception's description as the event reason and logsErrors.INVALID_REQUESTbefore throwing theCorsErrorResponseException, matching the file's own established pattern for this kind of rejected-token-request error.Added an integration test,
testClientCredentialsInvalidResourceLogsErrorEventinResourceIndicatorsTest, that sends a client-credentials request with an invalid resource indicator and asserts the resulting error event is now recorded with the right error code and reason detail. Confirmed it fails onmain(no event recorded) and passes with this fix.AI disclosure: I used an AI coding assistant to help implement this fix and write the test, then reviewed and verified the change myself, including running the new test against a local build.
Testing
mvn -pl services -am compile— cleanmvn -pl tests/base -am install -DskipTeststhen-Dtest=ResourceIndicatorsTest#testClientCredentialsInvalidResourceLogsErrorEvent test— 1 test run, 0 failures