Repository navigation
Conversation
…ired Signed-off-by: Inf1mum <vadim.lazuk@mail.ru>
There was a problem hiding this comment.
🟡 Changes recommended
Failures before the try block can skip cleanup, leaving the testing time service installed and potentially the client timeout altered, which can break later time-based tests.
1 open issue
What changed in this PR
This PR addresses #54606 by returning a standard OAuth error when a device authorization request encounters an expired client session, instead of failing with HTTP 500.
Changes:
- Guard against a missing client session and return HTTP 400 with
invalid_grant/ “Session not active.” - Record a session-expired event while preserving device-code consumption order.
- Add regression coverage for a client session expiring before the user’s SSO session.
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuth2DeviceAuthorizationGrantTest.java | Adds the expired-client-session regression test. |
| services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java | Handles a missing client session with an OAuth error. |
Review effort: Balanced
| // Consent | ||
| grantPage.accept(); | ||
|
|
||
| try { |
There was a problem hiding this comment.
No failures of the kind described have been observed on this PR's CI: the red checks are the two infra flakes documented at PR level (comment) — Test OLM installation failed with a 504 gateway timeout during the install step (run 41929270839), and Admin UI E2E (chromium) hit a flaky step reported by Playwright as 1 flaky (run 41929270979) — neither is related to this test or its cleanup ordering.
|
The two failed checks are unrelated to this PR:
The Java suites are fully green, including |
|
Friendly follow-up — the two failed checks ( |
|
Both failures look like infra flakes unrelated to the change: (1) |
d0d3336 to
22255ce
Compare
Fixes #54606
Problem
When a client uses the device authorization grant and its own client session idle timeout is shorter than the user's SSO session, the token request that follows an approved device code fails with HTTP 500 and an uncaught
NullPointerException:In
DeviceGrantType.process, the user session is first looked up withgetUserSessionIfClientExists(...), which returns null when the client session is gone (for example because the client session idle timeout has elapsed), so the code falls back togetUserSession(...), which still finds the user session. The client session is then read withgetAuthenticatedClientSessionByClient(client.getId())without a null check and dereferenced.Fix
Add a null check for the client session in
DeviceGrantType.processand, when it is missing, return the standardinvalid_grant/ "Session not active" error (HTTP 400), consistent with the existing check a few lines below that handles an invalid user session (AuthenticationManager.isSessionValid). The event is recorded with the existingSESSION_EXPIREDerror and aSession not activereason detail, mirroring the user-session-expired path.The order of operations is unchanged, in particular the device code is still removed before the session checks, so the device restarts the whole flow (as described in the issue).
Test
Added
OAuth2DeviceAuthorizationGrantTest.testExpiredClientSession, modeled on the neighboring cases of the same class: the public device client is given a client session idle timeout of 5 seconds, the device code is approved in the browser, then the token request is polled after the client session has expired but while the user session is still alive. The test expects HTTP 400 withinvalid_grant/ "Session not active" (previously this hit the 500 NPE).Notes