From 7cbc49c60ab1b1f45c92d9c108c773b4aeaf5b5e Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 29 Sep 2026 12:00:28 +0300 Subject: [PATCH] [#1143] Reject expired device codes at the token and verification endpoints The token endpoint checked isAuthorized() before the expiry time, so an approved device code past its lifetime was still exchanged for tokens until the CTS reaper removed it. Check the expiry first, answer expired_token and delete the code, as RFC 8628 section 3.5 requires. The user-code verification page never looked at the expiry either, and isIssued() is never set for device codes, so an expired code could be approved and an approved one re-approved by another session. Treat both as an unknown user code. Fixes #1143 --- .../core/DeviceCodeGrantTypeHandler.java | 18 +- .../DeviceCodeVerificationResource.java | 5 +- .../core/DeviceCodeGrantTypeHandlerTest.java | 49 ++++- .../DeviceCodeVerificationResourceTest.java | 179 ++++++++++++++++++ 4 files changed, 235 insertions(+), 16 deletions(-) create mode 100644 openam-oauth2/src/test/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResourceTest.java diff --git a/openam-oauth2/src/main/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandler.java b/openam-oauth2/src/main/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandler.java index 7f9de61997..b7bc479788 100644 --- a/openam-oauth2/src/main/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandler.java +++ b/openam-oauth2/src/main/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandler.java @@ -12,7 +12,7 @@ * information: "Portions copyright [year] [name of copyright owner]". * * Copyright 2015-2016 ForgeRock AS. - * Portions copyright 2025 3A Systems LLC. + * Portions copyright 2025-2026 3A Systems LLC. */ package org.forgerock.oauth2.core; @@ -91,7 +91,13 @@ protected AccessToken handle(OAuth2Request request, ClientRegistration client, !request.getParameter(REALM).equals(deviceCode.getRealm())) { throw new AuthorizationDeclinedException(); } - + + // RFC 8628 section 3.5: an expired device code gets expired_token even if the user has approved it + if (deviceCode.getExpiryTime() < currentTimeMillis()) { + tryDeleteDeviceCode(clientId, code, request); + throw new ExpiredTokenException(); + } + if (deviceCode.isAuthorized()) { String grantType = request.getParameter(OAuth2Constants.Params.GRANT_TYPE); Set scope = deviceCode.getScope(); @@ -109,14 +115,6 @@ protected AccessToken handle(OAuth2Request request, ClientRegistration client, return accessToken; } - - // only reachable when not authorized - the branch above returns - if (deviceCode.getExpiryTime() < currentTimeMillis()) { - tryDeleteDeviceCode(clientId, code, request); - throw new ExpiredTokenException(); - } - - try { final long lastPollTime = deviceCode.getLastPollTime(); if (lastPollTime + (providerSettings.getDeviceCodePollInterval() * 1000) > currentTimeMillis()) { diff --git a/openam-oauth2/src/main/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResource.java b/openam-oauth2/src/main/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResource.java index 5ccec6cec6..72349f006b 100644 --- a/openam-oauth2/src/main/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResource.java +++ b/openam-oauth2/src/main/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResource.java @@ -18,6 +18,7 @@ import static org.forgerock.openam.oauth2.OAuth2Constants.Params.CLIENT_ID; import static org.forgerock.openam.oauth2.OAuth2Constants.Params.SCOPE; +import static org.forgerock.openam.utils.Time.currentTimeMillis; import jakarta.inject.Inject; import jakarta.inject.Named; @@ -142,7 +143,9 @@ public Representation verify(Representation body) throws ServerException, NotFou return getTemplateRepresentation(FORM, request, "not_found"); } - if (deviceCode == null || deviceCode.isIssued()) { + // An expired code, or one that has already been approved, is treated as an unknown user code + if (deviceCode == null || deviceCode.isIssued() || deviceCode.isAuthorized() + || deviceCode.getExpiryTime() < currentTimeMillis()) { return getTemplateRepresentation(FORM, request, "not_found"); } diff --git a/openam-oauth2/src/test/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandlerTest.java b/openam-oauth2/src/test/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandlerTest.java index fcf0dd10f0..fe39deb80e 100644 --- a/openam-oauth2/src/test/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandlerTest.java +++ b/openam-oauth2/src/test/java/org/forgerock/oauth2/core/DeviceCodeGrantTypeHandlerTest.java @@ -137,6 +137,7 @@ public void shouldGenerateAccessTokenAndDeleteDeviceCodeWhenAuthorized() given(deviceCode.getClientId()).willReturn("CLIENT_ID"); given(deviceCode.getRealm()).willReturn("/REALM"); given(deviceCode.isAuthorized()).willReturn(true); + given(deviceCode.getExpiryTime()).willReturn(currentTimeMillis() + 10000); given(deviceCode.getScope()).willReturn(scope); given(deviceCode.getResourceOwnerId()).willReturn("RESOURCE_OWNER"); given(deviceCode.getNonce()).willReturn("NONCE"); @@ -171,8 +172,8 @@ public void shouldGenerateAccessTokenAndDeleteDeviceCodeWhenAuthorized() assertEquals(actualAccessToken, accessToken); } - @Test(expectedExceptions = ExpiredTokenException.class) - public void handleShouldThrowExpiredTokenExceptionWhenDeviceCodeHasExpired() + @Test + public void handleShouldThrowExpiredTokenExceptionAndDeleteDeviceCodeWhenDeviceCodeHasExpired() throws Exception { // Given @@ -194,10 +195,47 @@ public void handleShouldThrowExpiredTokenExceptionWhenDeviceCodeHasExpired() given(deviceCode.getExpiryTime()).willReturn(currentTimeMillis() - 100); // When - grantTypeHandler.handle(request, client, providerSettings); + try { + grantTypeHandler.handle(request, client, providerSettings); + fail("Expected ExpiredTokenException"); + } catch (ExpiredTokenException e) { + // Then + verify(tokenStore).deleteDeviceCode("CLIENT_ID", "DEVICE_CODE", request); + } + } - // Then - // Expect ExpiredTokenException + @Test + public void handleShouldNotIssueAccessTokenWhenAuthorizedDeviceCodeHasExpired() + throws Exception { + + // Given + OAuth2Request request = mock(OAuth2Request.class); + ClientRegistration client = mock(ClientRegistration.class); + DeviceCode deviceCode = mock(DeviceCode.class); + + given(request.getParameter(DEVICE_CODE)).willReturn("DEVICE_CODE"); + given(request.getParameter(REALM)).willReturn("/REALM"); + + given(client.getClientId()).willReturn("CLIENT_ID"); + + given(tokenStore.readDeviceCode("CLIENT_ID", "DEVICE_CODE", request)) + .willReturn(deviceCode); + + given(deviceCode.getClientId()).willReturn("CLIENT_ID"); + given(deviceCode.getRealm()).willReturn("/REALM"); + given(deviceCode.isAuthorized()).willReturn(true); + given(deviceCode.getResourceOwnerId()).willReturn("RESOURCE_OWNER"); + given(deviceCode.getExpiryTime()).willReturn(currentTimeMillis() - 100); + + // When + try { + grantTypeHandler.handle(request, client, providerSettings); + fail("Expected ExpiredTokenException"); + } catch (ExpiredTokenException e) { + // Then + verifyZeroInteractions(accessTokenGenerator); + verify(tokenStore).deleteDeviceCode("CLIENT_ID", "DEVICE_CODE", request); + } } @Test @@ -259,6 +297,7 @@ public void shouldNotDeleteDeviceCodeWhenAccessTokenGenerationFails() given(deviceCode.getClientId()).willReturn("CLIENT_ID"); given(deviceCode.getRealm()).willReturn("/REALM"); given(deviceCode.isAuthorized()).willReturn(true); + given(deviceCode.getExpiryTime()).willReturn(currentTimeMillis() + 10000); given(deviceCode.getScope()).willReturn(scope); given(deviceCode.getResourceOwnerId()).willReturn("RESOURCE_OWNER"); diff --git a/openam-oauth2/src/test/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResourceTest.java b/openam-oauth2/src/test/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResourceTest.java new file mode 100644 index 0000000000..3d1aa2d860 --- /dev/null +++ b/openam-oauth2/src/test/java/org/forgerock/oauth2/restlet/DeviceCodeVerificationResourceTest.java @@ -0,0 +1,179 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.oauth2.restlet; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.forgerock.openam.utils.Time.currentTimeMillis; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.*; + +import java.util.Collections; +import java.util.Map; + +import org.forgerock.oauth2.core.AuthorizationService; +import org.forgerock.oauth2.core.ClientRegistration; +import org.forgerock.oauth2.core.ClientRegistrationStore; +import org.forgerock.oauth2.core.CsrfProtection; +import org.forgerock.oauth2.core.DeviceCode; +import org.forgerock.oauth2.core.OAuth2ProviderSettings; +import org.forgerock.oauth2.core.OAuth2ProviderSettingsFactory; +import org.forgerock.oauth2.core.OAuth2Request; +import org.forgerock.oauth2.core.OAuth2RequestFactory; +import org.forgerock.oauth2.core.ResourceOwner; +import org.forgerock.oauth2.core.ResourceOwnerSessionValidator; +import org.forgerock.oauth2.core.TokenStore; +import org.forgerock.openam.oauth2.OAuth2Constants; +import org.forgerock.openam.oauth2.OAuth2Utils; +import org.forgerock.openam.services.baseurl.BaseURLProvider; +import org.forgerock.openam.services.baseurl.BaseURLProviderFactory; +import org.forgerock.openam.xui.XUIState; +import org.restlet.Context; +import org.restlet.Request; +import org.restlet.Response; +import org.restlet.Restlet; +import org.restlet.data.Status; +import org.restlet.ext.freemarker.TemplateRepresentation; +import org.restlet.representation.Representation; +import org.restlet.routing.Router; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +/** + * Tests for DeviceCodeVerificationResource. + */ +public class DeviceCodeVerificationResourceTest { + + private static final String USER_CODE = "USER_CODE"; + + private DeviceCodeVerificationResource resource; + private OAuth2Request o2request; + private TokenStore tokenStore; + private AuthorizationService authorizationService; + private ResourceOwnerSessionValidator resourceOwnerSessionValidator; + private DeviceCode deviceCode; + + @BeforeMethod + public void setup() throws Exception { + Request request = new Request(); + OAuth2RequestFactory requestFactory = mock(OAuth2RequestFactory.class); + o2request = mock(OAuth2Request.class); + given(requestFactory.create(request)).willReturn(o2request); + given(o2request.getParameter(OAuth2Constants.DeviceCode.USER_CODE)).willReturn(USER_CODE); + given(o2request.getParameter("realm")).willReturn("/"); + given(o2request.getRequest()).willReturn(request); + + BaseURLProviderFactory baseURLProviderFactory = mock(BaseURLProviderFactory.class); + given(baseURLProviderFactory.get(anyString())).willReturn(mock(BaseURLProvider.class)); + + tokenStore = mock(TokenStore.class); + deviceCode = mock(DeviceCode.class); + given(deviceCode.getObject()).willReturn(Collections.emptyMap()); + given(tokenStore.readDeviceCode(USER_CODE, o2request)).willReturn(deviceCode); + + OAuth2ProviderSettingsFactory providerSettingsFactory = mock(OAuth2ProviderSettingsFactory.class); + OAuth2ProviderSettings providerSettings = mock(OAuth2ProviderSettings.class); + given(providerSettingsFactory.get(o2request)).willReturn(providerSettings); + given(providerSettings.clientsCanSkipConsent()).willReturn(true); + + ClientRegistrationStore clientRegistrationStore = mock(ClientRegistrationStore.class); + ClientRegistration clientRegistration = mock(ClientRegistration.class); + given(clientRegistrationStore.get(any(), any())).willReturn(clientRegistration); + given(clientRegistration.isConsentImplied()).willReturn(true); + + resourceOwnerSessionValidator = mock(ResourceOwnerSessionValidator.class); + ResourceOwner resourceOwner = mock(ResourceOwner.class); + given(resourceOwner.getId()).willReturn("RESOURCE_OWNER"); + given(resourceOwnerSessionValidator.validate(o2request)).willReturn(resourceOwner); + + authorizationService = mock(AuthorizationService.class); + + resource = spy(new DeviceCodeVerificationResource(mock(XUIState.class), mock(Router.class), + baseURLProviderFactory, mock(OAuth2Representation.class), tokenStore, requestFactory, + authorizationService, providerSettingsFactory, mock(ExceptionHandler.class), + resourceOwnerSessionValidator, clientRegistrationStore, mock(OAuth2Utils.class), + mock(CsrfProtection.class))); + doReturn(request).when(resource).getRequest(); + doReturn(templatesFromClasspathContext()).when(resource).getContext(); + } + + @Test + public void shouldAuthorizeValidDeviceCode() throws Exception { + // Given + given(deviceCode.getExpiryTime()).willReturn(currentTimeMillis() + 10000); + + // When + Representation result = resource.verify(null); + + // Then + assertThat(errorCode(result)).isNull(); + verify(deviceCode).setResourceOwnerId("RESOURCE_OWNER"); + verify(deviceCode).setAuthorized(true); + verify(tokenStore).updateDeviceCode(deviceCode, o2request); + } + + @Test + public void shouldRejectExpiredDeviceCode() throws Exception { + // Given + given(deviceCode.getExpiryTime()).willReturn(currentTimeMillis() - 100); + + // When + Representation result = resource.verify(null); + + // Then + assertThat(errorCode(result)).isEqualTo("not_found"); + verify(deviceCode, never()).setAuthorized(anyBoolean()); + verify(tokenStore, never()).updateDeviceCode(any(), any()); + verifyZeroInteractions(resourceOwnerSessionValidator, authorizationService); + } + + @Test + public void shouldRejectDeviceCodeThatIsAlreadyAuthorized() throws Exception { + // Given + given(deviceCode.getExpiryTime()).willReturn(currentTimeMillis() + 10000); + given(deviceCode.isAuthorized()).willReturn(true); + + // When + Representation result = resource.verify(null); + + // Then + assertThat(errorCode(result)).isEqualTo("not_found"); + verify(deviceCode, never()).setResourceOwnerId(anyString()); + verify(tokenStore, never()).updateDeviceCode(any(), any()); + verifyZeroInteractions(resourceOwnerSessionValidator, authorizationService); + } + + /** + * A context whose clap:// lookups find nothing, so the templates load from the classpath. + */ + private static Context templatesFromClasspathContext() { + Context context = new Context(); + context.setClientDispatcher(new Restlet() { + @Override + public void handle(Request request, Response response) { + response.setStatus(Status.CLIENT_ERROR_NOT_FOUND); + } + }); + return context; + } + + @SuppressWarnings("unchecked") + private static String errorCode(Representation representation) { + return ((Map) ((TemplateRepresentation) representation).getDataModel()).get("errorCode"); + } +}