Fix Device Code session propagation and OIDC claims - #1108
Conversation
| final String nonce = deviceCode.getNonce(); | ||
|
|
||
| // Retore Session | ||
| String sessionId = deviceCode.getSessionId(); |
There was a problem hiding this comment.
Diagnosis looks right - the flow really wasn't issuing an id_token. I'd drop the session restore though; I don't think it's needed, and it brings problems.
Why it breaks:
isValidTokenat 120 guards onlyrequest.setSession(). On failure: warn, then fall through toadditionalDataToReturnFromTokenEndpointat 150 anyway.- Line 121 is dead weight regardless.
OpenIDTokenIssuer:80overwrites the request session withaccessToken.getSessionId()->extraData["ssoTokenId"], i.e. line 141. - Session gone ->
validate()with null session ->ResourceOwnerSessionValidator:207->ResourceOwnerAuthenticationRequired->ServerException-> 500.finallyat 164 already deleted the device code, so no retry - the user restarts the whole authorization. - Client with
Default max ageset:ResourceOwnerSessionValidator:188->authenticationRequired(request, token)->destroyToken()at :345. A back-channel poll logs the user out of their browser. Deterministic, not a race. setSessionId()is new here, so device codes authorized by the currently deployed build have a nullsessionIdand 500 on first poll after upgrade.
The flow doesn't need a session. ResourceOwnerSessionValidator:201-206 already handles this for password/client_credentials - suggest adding the grant type:
} else if (TokenEndpoint.PASSWORD.equals(request.getParameter(GRANT_TYPE))
|| TokenEndpoint.CLIENT_CREDENTIALS.equals(request.getParameter(GRANT_TYPE))
|| TokenEndpoint.DEVICE_CODE.equals(request.getParameter(GRANT_TYPE))) {
return getResourceOwner(request.getToken(AccessToken.class));
}Everything it needs is already there. Access token is on the request by line 150 (StatefulTokenStore:566, StatelessTokenStore:305). auth_time is on the device code - DeviceCode.setAuthorized() stamps AUTH_INSTANT, GrantTypeAccessTokenGenerator:68-72 reads it. Both on master already, nothing exercises them yet.
The block then collapses to:
AccessToken accessToken = accessTokenGenerator.generateAccessToken(providerSettings, grantType,
clientId, resourceOwnerId, null, scope, validatedClaims, null,
deviceCode.getNonce(), request);
providerSettings.additionalDataToReturnFromTokenEndpoint(accessToken, request);
return accessToken;That drops the SSOTokenManager dep, the ssoTokenId set/unset dance, and the second isValidToken at 140 - that one resets session idle time, and a device poll shouldn't extend the user's browser session.
Also fixes the nonce: addExtraData(NONCE, ...) at 135 is never read. OpenIDTokenIssuer reads accessToken.getNonce(), set from the nonce arg to createAccessToken - currently null, so the id_token ships without a nonce claim.
Minor: line 127 logs the raw SSO token ID - worth dropping.
Two things this doesn't cover:
- amr/acr still don't reach the id_token on a default install -
OpenAMTokenStore.createOpenIDToken(:78-82) doesn't route onstatelessCheck. Details in the comment onStatelessTokenStore:250. I'd keep it in this PR; once the resource owner comes from the access token it's a two-line addition rather than a port. getOps()->accessToken.getSessionId()-> null, so noopsclaim and no OIDC session management on device tokens. Probably correct for this flow - a TV token shouldn't die when the user closes a tab - but worth making deliberate. If you agree,deviceCode.setSessionId()becomes unused and can go.
Could we get a test for poll-after-session-expiry? That's the unrecoverable case.
| if (authCode != null) { | ||
| authModules = authCode.getAuthModules(); | ||
| acr = authCode.getAuthenticationContextClassReference(); | ||
| } else if (deviceCode != null) { |
There was a problem hiding this comment.
Both DeviceCode branches - this one and createRefreshToken:559 - are stateless-only, and StatefulTokenStore didn't get an equivalent. I think that means amr/acr don't reach the id_token on a default install.
OpenAMTokenStore routes every method on statelessCheck.byRequest(request) except this one:
// OpenAMTokenStore:78-82
public OpenIdConnectToken createOpenIDToken(...) {
return statefulTokenStore.createOpenIDToken(resourceOwner, clientId, authorizationParty, nonce, ops, request);
}No branch - createOpenIDToken only exists on StatefulTokenStore; StatelessTokenStore implements TokenStore, not OpenIdConnectTokenStore. So the id_token is always built by the stateful store, and claims come from getAMRFromAuthModules:404 / getAuthenticationContextClassReference:432. Both are AuthorizationCode -> RefreshToken -> SSO-cookie ladders. Auth code is null on a device poll, so it comes down to the refresh token.
statelessTokensEnabled |
issueRefreshToken |
id_token amr/acr |
|---|---|---|
| false (default) | true (default) | absent - StatefulTokenStore.createRefreshToken:639 has no device branch, refresh token carries null |
| false | false | absent - :413 reads the cookie off the HTTP request; a device poll has none |
| true | true | present - the only path this PR exercises |
| true | false | absent - same :413 fallback |
Defaults are statelessTokensEnabled=false (OAuth2Provider.xml:88) and issueRefreshToken=true (:134), so the working combination is the one nobody has out of the box.
Third condition on top: getAMRFromAuthModules:415-425 only emits amr when getAMRAuthModuleMappings() is non-empty, and forgerock-oauth2-provider-amr-mappings (OAuth2Provider.xml:552) is optional with no default. Pre-existing, but it means amr needs stateless and refresh tokens and configured mappings.
Relevant to the other comment: in the default config getAMRFromAuthModules takes the RefreshToken branch at :411 and never reaches :413, so the restored session contributes nothing to amr/acr. It only buys passage through ResourceOwnerSessionValidator.
Separate point in this method - :255 is if, not else if:
} else if (deviceCode != null) { // :250
authModules = deviceCode.getAuthModules();
acr = deviceCode.getAcrValues();
}
if (currentRefreshToken != null) { // :255 - overwrites unconditionally
authModules = currentRefreshToken.getAuthModules();
acr = currentRefreshToken.getAuthenticationContextClassReference();
}GrantTypeAccessTokenGenerator:77-80 creates the refresh token first and puts it on the request, so :250 is always overwritten when refresh tokens are on. Same values today (the refresh token got them from the same DeviceCode at :559), so nothing breaks - but the access-token branch only runs with refresh off and the id_token fix only works with refresh on. The two halves never exercise together. else if would match the intent.
realm_access.roles staying stateless-only seems right - nowhere to put it on an opaque CTS token.
maximthomas
left a comment
There was a problem hiding this comment.
Hi @dairoca90
Thanks for the contribution! A few issues need to be addressed before this can be merged.
Fix Device Code session propagation and OIDC claims
Description
This PR updates the Device Code flow to improve session propagation and OpenID Connect claim handling.
Changes
Sessionso it is available in the Access Token Modifier Script. Previously, the session was not exposed to the script.acrandamrvalues in Device Code tokens.openidscope is requested, the OIDC-related information is included when a valid user session exists at the time the Device Code is generated.Result
The Device Code flow now provides the necessary session context for token modification and correctly propagates
acrandamrauthentication information when applicable.