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
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.
|
Hi @dairoca90, |
…for token generation
… in stateless tokens" \ -m "Map internal OpenAM authentication module names to their configured AMR values before adding them to stateless JWT tokens. Resolve authModules from the appropriate token context and use the OAuth2 provider AMR mappings to populate the amr claim instead of exposing internal authentication module names."
Add test coverage for propagating ACR and mapped AMR values from authorization codes and refresh tokens into stateless access tokens. Verify that refresh token authentication context takes precedence over the authorization code and that internal authModules values are not exposed in the resulting token.
ea92c0d to
82d2808
Compare
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
Hi @dairoca90
Thanks for the quick fix!
Please see the PR feeback below:
The device-code acr/amr plumbing works. The problem is that the PR also renames the stateless JWT claim authModules → amr for all grants — not mentioned in the descriptio — and that rename breaks two things outside the device-code flow.
issue (blocking): amr is written under a key nothing reads back, so it is lost on every stateless refresh
openam-oauth2/src/main/java/org/forgerock/openam/oauth2/StatelessTokenStore.java:581
// createRefreshToken — writes "amr"
claimsSetBuilder.claim("amr", getAMRFromAuthModules(authModules, providerSettings));// StatelessRefreshToken.java:63-64 — reads "authModules". Untouched by this PR.
return jwt.getClaimsSet().getClaim(AUTH_MODULES, String.class); // AUTH_MODULES == "authModules"On grant_type=refresh_token, readRefreshToken (:719-726) rebuilds the token, createAccessToken
(:257) takes the REFRESH_TOKEN branch, getAuthModules() returns null, and the authModules != null
guard at :272 is skipped. The reissued access token has no amr, and :580 drops it from the reissued
refresh token too. amr survives zero refreshes. At base the write and read keys matched.
Suggest writing both keys for one release rather than only changing the read side — refresh tokens
minted before the upgrade are still in the wild.
issue (blocking): in a default install the authModules claim silently disappears from all stateless tokens
openam-oauth2/src/main/java/org/forgerock/openam/oauth2/StatelessTokenStore.java:272-274 (same at :580-581)
if (authModules != null) { // guards on the raw String…
claimsSetBuilder.claim("amr", getAMRFromAuthModules(authModules, providerSettings));
} // …but writes the mapped ListgetAMRFromAuthModules returns null unless getAMRAuthModuleMappings() is non-empty. That mapping
is optional with no default — OAuth2Provider.xml:552-565 has <IsOptional/> and no <DefaultValues>,
and AgentOAuth2ProviderSettings:277 returns an empty map unconditionally. JWObject.put drops null
values, so the key is simply absent.
Net effect on a stock install: a token that carried "authModules":"DataStore" now carries neither
authModules nor amr. Affects authorization_code and refresh_token, not just device code.
Suggest falling back to the raw authModules value when the mapping is empty.
issue (blocking): one query parameter turns /oauth2/authorize into a 500 for anonymous callers
openam-oauth2/src/main/java/org/forgerock/oauth2/core/ResourceOwnerSessionValidator.java:203
} else if (/* password || client_credentials || */ DEVICE_CODE.equals(request.getParameter(GRANT_TYPE))) {
return getResourceOwner(request.getToken(AccessToken.class)); // :207 — may be null
}
// :253
return new ResourceOwner(token.getResourceOwnerId(), ...); // NPE, not an OAuth2ExceptionOAuth2Request.getParameter falls back to query params (:93-101, :162), so:
GET /oauth2/authorize?client_id=X&response_type=code&redirect_uri=…&scope=openid
&grant_type=urn:ietf:params:oauth:grant-type:device_code
with no SSO session reaches AuthorizationService:163 → validate() → this branch. Nothing sets an
AccessToken on an /authorize request, so it NPEs. Previously it redirected to login.
The token-endpoint path is fine — both stores call setToken(AccessToken.class, …) first. Suggest
gating the branch on the token actually being present.
issue (non-blocking): the script-facing amr changes type, breaking existing customer scripts
openam-oauth2/src/main/java/org/forgerock/openam/oauth2/StatelessTokenStore.java:294 (and :604)
accessTokenContext.put("amr", authModules); // base — pipe-joined String
accessTokenContext.put("amr", getAMRFromAuthModules(authModules, providerSettings)); // head — List<String> or nullThat map is bound as ScriptParams.ACCESS_TOKEN and read by access-token-modification.groovy via
getField("amr"), which returns the raw object with no coercion. A script doing amr.split("\\|")
gets a MissingMethodException at token-issuance time.
Needs a release note, and openam-scripting/src/main/groovy/access-token-modification.groovy (header
comment at line 30) still documents the old contract.
issue (non-blocking): a failed id_token leaves orphaned tokens and destroys the device code
openam-oauth2/src/main/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandler.java:108
try {
accessToken = generateAccessToken(...); // :105 — already in CTS
providerSettings.additionalDataToReturnFromTokenEndpoint(accessToken, request); // :108 — can throw
return accessToken;
} finally {
if (deviceCode.isAuthorized() || ...) {
tokenStore.deleteDeviceCode(clientId, code, request); // :118-124 — runs anyway
}
}createOpenIDToken throws ServerException on any CoreTokenException. Result for an openid-scoped
device grant during a CTS blip: access + refresh tokens live in CTS but never returned, device code
gone, client gets a 500, and every retry poll then reports invalid_grant (:90).
Widens a pre-existing window rather than creating one. Moving the call outside the try is enough.
question (blocking): was the Session propagation descoped, or dropped?
The first bullet of the description says the user Session is propagated to the Access Token
Modifier Script. I can't find it implemented. Three pieces of scaffolding, none wired up:
// StatelessTokenStore.java:208 — hoisted to method scope, still only assigned inside `if (authCode != null)`
String sessionId = null;
// StatelessTokenStore.java:894 — no call site in this class; sole reason for the new ServletUtils import
private String getAuthModulesFromSSOToken(OAuth2Request request) { ... }accessTokenContext (:287-295) gains no session entry. What DeviceCodeVerificationResource actually
propagates is one String:
deviceCode.setAuthModules(token.getProperty(ISAuthConstants.AUTH_TYPE));If it was descoped, could the description be trimmed and the dead method plus hoisted variable removed?
If it was meant to land here, the PR looks incomplete.
note (non-blocking): id_token is now issued for openid device grants unconditionally
OpenAMScopeValidator:492 issues on scope.contains(OPENID) alone — there is no check that a session
existed when the device code was generated, as the description states. Almost certainly fine in
practice; worth correcting the wording and noting the response-shape change for existing clients.
suggestion (non-blocking): two tests would pin the riskiest parts
Would you consider adding:
- A mint-then-refresh case. The tests here stub
RefreshTokendirectly, so they can't see what
createRefreshTokenwrote. Since it returnsnew StatelessRefreshToken(jwt, jwt.build())
(StatelessTokenStore.java:622), assertinggetAuthModules()on the returned object is enough to
catch the first issue above — noreadRefreshTokenround-trip needed. - An empty AMR-mapping case. That's the shipped default, and the assertion that used to guard it
(doesNotContainKey("authModules")) was removed with the rename.
The new device_code branches in both stores, the new branch in ResourceOwnerSessionValidator, and
the acr/session block in DeviceCodeVerificationResource have no coverage at all — a follow-up is fine.
nitpick (non-blocking): the new acr/session block is copy-pasted into both branches
openam-oauth2/src/main/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResource.java:174-179 and :191-196
deviceCode.setAcrValues(getAuthenticationContextClassReferenceFromRequest(request));
SSOToken token = resourceOwnerSessionValidator.getResourceOwnerSession(request);
if (token != null) {
populateAuthenticationInfo(deviceCode, token);
}Verbatim in both. Worth a private helper — the adjacent authorize block was already duplicated, so
this grows it from 3 to 7 lines.
Checked and found fine
- New
authModuleskey onDeviceCodesurvives the CTS round-trip —OAuthAdapterserialises the
wholeJsonValuemap into the blob, no field whitelist. acrrecorded at verification is the matched value, not the raw requestedacr_values;
setCurrentAcrdoes run, and unmatched values become"0", consistent withauthorization_code.- No
"amr": nullis emitted —JWObject.putdrops null values. - The three modified assertions in
StatelessTokenStoreTesthold at head.
|
Hi @maximthomas , I removed the Session-related invocations because, after reviewing the flow, they did not seem to provide a meaningful benefit and introduced the issues you described around session restoration and validation. I also reverted the changes in StatelessTokenStore to preserve the existing behavior and avoid introducing compatibility issues for existing tokens and scripts. This also means reverting the related test changes. From my perspective, this leaves an open question regarding the OAuth2 provider configuration: prioritizing backward compatibility means that StatelessTokenStore will continue using the existing authModules behavior rather than exposing the mapped amr value in the same way as the StatefulTokenStore. As a result, there may be some inconsistency between stateful and stateless tokens, and part of the AMR mapping configuration may not apply to stateless tokens as expected. Regarding moving additionalDataToReturnFromTokenEndpoint outside the try block in DeviceCodeGrantTypeHandler, I am still trying to understand the intended restructuring. Simply moving the invocation after the current block does not seem viable because the successful path currently returns the accessToken before execution can reach that code. I also want to avoid changing the existing flow in a way that could affect the current OIDC functionality. Could you clarify what structure you would recommend for handling this while preserving the current behavior? |
|
Hi @dairoca90, the answers are below:
No inconsistency — the two paths write different claims on different tokens:
So nothing is lost by the revert. Mapped
Fair — my wording was wrong. I meant the shape the other grant handlers use ( AccessToken accessToken = null;
try {
if (deviceCode.isAuthorized()) {
String grantType = request.getParameter(OAuth2Constants.Params.GRANT_TYPE);
Set<String> scope = deviceCode.getScope();
String resourceOwnerId = deviceCode.getResourceOwnerId();
String validatedClaims = providerSettings.validateRequestedClaims(
deviceCode.getStringProperty(OAuth2Constants.Custom.CLAIMS));
accessToken = generateAccessToken(providerSettings, grantType, clientId, resourceOwnerId,
scope, validatedClaims, deviceCode.getNonce(), request);
} else if (deviceCode.getExpiryTime() < currentTimeMillis()) {
throw new ExpiredTokenException();
}
} finally {
if (deviceCode.isAuthorized() || deviceCode.getExpiryTime() < currentTimeMillis()) {
try {
tokenStore.deleteDeviceCode(clientId, code, request);
} catch (OAuth2Exception e) {
logger.warn("Could not delete issued/expired device code", e);
}
}
}
if (accessToken != null) {
providerSettings.additionalDataToReturnFromTokenEndpoint(accessToken, request);
return accessToken;
}
// polling path unchanged
Two things to watch there. That replaces
Calling That said, "outside the try is enough" was wrong on its own terms: the Closing the window means each exit owning its delete: if (deviceCode.isAuthorized()) {
String grantType = request.getParameter(OAuth2Constants.Params.GRANT_TYPE);
Set<String> scope = deviceCode.getScope();
String resourceOwnerId = deviceCode.getResourceOwnerId();
String validatedClaims = providerSettings.validateRequestedClaims(
deviceCode.getStringProperty(OAuth2Constants.Custom.CLAIMS));
AccessToken accessToken = generateAccessToken(providerSettings, grantType, clientId,
resourceOwnerId, scope, validatedClaims, deviceCode.getNonce(), request);
providerSettings.additionalDataToReturnFromTokenEndpoint(accessToken, request);
tryDeleteDeviceCode(clientId, code, request); // only once the response is complete
return accessToken;
}
// only reachable when not authorized - the branch above returns
if (deviceCode.getExpiryTime() < currentTimeMillis()) {
tryDeleteDeviceCode(clientId, code, request);
throw new ExpiredTokenException();
}
private void tryDeleteDeviceCode(String clientId, String code, OAuth2Request request) {
try {
tokenStore.deleteDeviceCode(clientId, code, request);
} catch (OAuth2Exception e) {
logger.warn("Could not delete issued/expired device code", e);
}
}Here The guards differ on purpose: the first keeps |
2 participants
Description
This PR improves OpenID Connect support in the OAuth2 Device Code flow.
Changes
acrandamrclaims can be included in OIDC tokens where applicable.nonceto access token generation so it can be used during OIDC token generation.DEVICE_CODEhandling in the resource owner validation flow, allowing the resource owner to be resolved from the generated access token without restoring the user's browser session.StatelessTokenStorebehavior and compatibility for existingauthModulesclaims and token customization scripts.additionalDataToReturnFromTokenEndpointis completed before the Device Code is consumed.Result
The Device Code flow can now issue OIDC-related token data, including support for
nonce,acr, andamrwhere applicable, without requiring restoration of the user's browser session during device polling.The existing behavior of stateless access and refresh tokens remains unchanged to preserve compatibility.