Skip to content

Fix Device Code session propagation and OIDC claims - #1108

Open
dairoca90 wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
dairoca90:debug-16.1.2
Open

Fix Device Code session propagation and OIDC claims#1108
dairoca90 wants to merge 4 commits into
OpenIdentityPlatform:masterfrom
dairoca90:debug-16.1.2

Conversation

@dairoca90

Copy link
Copy Markdown
Contributor

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

  • Propagated the user Session so it is available in the Access Token Modifier Script. Previously, the session was not exposed to the script.
  • Added support for including the acr and amr values in Device Code tokens.
  • Updated the Device Code response so that when the openid scope is requested, the OIDC-related information is included when a valid user session exists at the time the Device Code is generated.
  • Ensured the session information is preserved throughout the Device Code flow so it can be consumed by downstream token customization logic.

Result

The Device Code flow now provides the necessary session context for token modification and correctly propagates acr and amr authentication information when applicable.

@maximthomas
maximthomas self-requested a review August 17, 2026 09:26
final String nonce = deviceCode.getNonce();

// Retore Session
String sessionId = deviceCode.getSessionId();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • isValidToken at 120 guards only request.setSession(). On failure: warn, then fall through to additionalDataToReturnFromTokenEndpoint at 150 anyway.
  • Line 121 is dead weight regardless. OpenIDTokenIssuer:80 overwrites the request session with accessToken.getSessionId() -> extraData["ssoTokenId"], i.e. line 141.
  • Session gone -> validate() with null session -> ResourceOwnerSessionValidator:207 -> ResourceOwnerAuthenticationRequired -> ServerException -> 500. finally at 164 already deleted the device code, so no retry - the user restarts the whole authorization.
  • Client with Default max age set: 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 null sessionId and 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 on statelessCheck. Details in the comment on StatelessTokenStore: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 no ops claim 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dairoca90
Thanks for the contribution! A few issues need to be addressed before this can be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants