From 76101e359181b0e04004f59eb09d88b544fddbbe Mon Sep 17 00:00:00 2001 From: Daniel Gozalo Date: Wed, 2 Feb 2022 14:53:33 +0100 Subject: [PATCH] [fixes #9225] - Get scopeIds from the AuthorizationRequestContext instead of session if DYNAMIC_SCOPES are enabled Add a test to make sure ProtocolMappers run with Dynamic Scopes Change the way we create the DefaultClientSessionContext with respect to OAuth2 scopes, and standardize the way we obtain them from the parameter --- .../keycloak/protocol/oidc/TokenManager.java | 11 ++- .../oidc/endpoints/TokenEndpoint.java | 2 +- .../oidc/grants/ciba/CibaGrantType.java | 2 +- .../oidc/grants/device/DeviceGrantType.java | 4 +- .../managers/AuthenticationManager.java | 4 +- .../util/AuthorizationContextUtil.java | 68 +++++++++++++++++++ .../util/DefaultClientSessionContext.java | 16 +++-- .../oauth/OIDCProtocolMappersTest.java | 40 +++++++++++ 8 files changed, 135 insertions(+), 12 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java index b692e30ae2..8fb54569af 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java @@ -29,6 +29,7 @@ import org.keycloak.broker.oidc.OIDCIdentityProvider; import org.keycloak.broker.provider.IdentityBrokerException; import org.keycloak.cluster.ClusterProvider; import org.keycloak.common.ClientConnection; +import org.keycloak.common.Profile; import org.keycloak.common.VerificationException; import org.keycloak.common.util.Time; import org.keycloak.crypto.HashProvider; @@ -76,6 +77,7 @@ import org.keycloak.services.managers.AuthenticationSessionManager; import org.keycloak.services.managers.UserSessionCrossDCManager; import org.keycloak.services.managers.UserSessionManager; import org.keycloak.services.resources.IdentityBrokerService; +import org.keycloak.services.util.AuthorizationContextUtil; import org.keycloak.services.util.DefaultClientSessionContext; import org.keycloak.services.util.MtlsHoKTokenUtil; import org.keycloak.sessions.AuthenticationSessionModel; @@ -544,7 +546,14 @@ public class TokenManager { clientSession.setRedirectUri(authSession.getRedirectUri()); clientSession.setProtocol(authSession.getProtocol()); - Set clientScopeIds = authSession.getClientScopes(); + Set clientScopeIds; + if (Profile.isFeatureEnabled(Profile.Feature.DYNAMIC_SCOPES)) { + clientScopeIds = AuthorizationContextUtil.getClientScopesStreamFromAuthorizationRequestContextWithClient(session, authSession.getClientNote(OAuth2Constants.SCOPE)) + .map(ClientScopeModel::getId) + .collect(Collectors.toSet()); + } else { + clientScopeIds = authSession.getClientScopes(); + } Map transferredNotes = authSession.getClientNotes(); for (Map.Entry entry : transferredNotes.entrySet()) { diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java index c6a8447b58..372ecfdde7 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java @@ -424,7 +424,7 @@ public class TokenEndpoint { throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_SCOPE, "Client no longer has requested consent from user", Response.Status.BAD_REQUEST); } - ClientSessionContext clientSessionCtx = DefaultClientSessionContext.fromClientSessionAndClientScopes(clientSession, clientScopesSupplier.get(), session); + ClientSessionContext clientSessionCtx = DefaultClientSessionContext.fromClientSessionAndScopeParameter(clientSession, scopeParam, session); // Set nonce as an attribute in the ClientSessionContext. Will be used for the token generation clientSessionCtx.setAttribute(OIDCLoginProtocol.NONCE_PARAM, codeData.getNonce()); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantType.java b/services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantType.java index be7284258c..3fbfb9fc05 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantType.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantType.java @@ -212,7 +212,7 @@ public class CibaGrantType { } ClientSessionContext clientSessionCtx = DefaultClientSessionContext - .fromClientSessionAndClientScopes(userSession.getAuthenticatedClientSessionByClient(client.getId()), TokenManager.getRequestedClientScopes(scopeParam, client), session); + .fromClientSessionAndScopeParameter(userSession.getAuthenticatedClientSessionByClient(client.getId()), scopeParam, session); int authTime = Time.currentTime(); userSession.setNote(AuthenticationManager.AUTH_TIME, String.valueOf(authTime)); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java b/services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java index 0337c2d7c2..aa2f9ef05b 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantType.java @@ -292,8 +292,8 @@ public class DeviceGrantType { "Client no longer has requested consent from user", Response.Status.BAD_REQUEST); } - ClientSessionContext clientSessionCtx = DefaultClientSessionContext.fromClientSessionAndClientScopes(clientSession, - TokenManager.getRequestedClientScopes(scopeParam, client), session); + ClientSessionContext clientSessionCtx = DefaultClientSessionContext.fromClientSessionAndScopeParameter(clientSession, + scopeParam, session); // Set nonce as an attribute in the ClientSessionContext. Will be used for the token generation clientSessionCtx.setAttribute(OIDCLoginProtocol.NONCE_PARAM, deviceCodeModel.getNonce()); diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java index b69b52022a..c555e0934d 100755 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java @@ -1192,9 +1192,7 @@ public class AuthenticationManager { //if Dynamic Scopes are enabled, get the scopes from the AuthorizationRequestContext, passing the session and scopes as parameters // then concat a Stream with the ClientModel, as it's discarded in the getAuthorizationRequestContext method if (Profile.isFeatureEnabled(Profile.Feature.DYNAMIC_SCOPES)) { - return Stream.concat(AuthorizationContextUtil.getAuthorizationRequestContextFromScopes(session, authSession.getClientNote(OAuth2Constants.SCOPE)) - .getAuthorizationDetailEntries().stream(), - Collections.singletonList(new AuthorizationDetails(session.getContext().getClient())).stream()); + return AuthorizationContextUtil.getAuthorizationRequestsStreamFromScopesWithClient(session, authSession.getClientNote(OAuth2Constants.SCOPE)); } // if dynamic scopes are not enabled, we retain the old behaviour, but the ClientScopes will be wrapped in // AuthorizationRequest objects to standardize the code handling these. diff --git a/services/src/main/java/org/keycloak/services/util/AuthorizationContextUtil.java b/services/src/main/java/org/keycloak/services/util/AuthorizationContextUtil.java index 5dfc43557c..664642c904 100644 --- a/services/src/main/java/org/keycloak/services/util/AuthorizationContextUtil.java +++ b/services/src/main/java/org/keycloak/services/util/AuthorizationContextUtil.java @@ -1,13 +1,47 @@ +/* + * Copyright 2022 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.keycloak.services.util; import org.keycloak.common.Profile; +import org.keycloak.models.ClientScopeModel; import org.keycloak.models.KeycloakSession; import org.keycloak.protocol.oidc.rar.AuthorizationRequestParserProvider; import org.keycloak.protocol.oidc.rar.parsers.ClientScopeAuthorizationRequestParserProviderFactory; +import org.keycloak.rar.AuthorizationDetails; import org.keycloak.rar.AuthorizationRequestContext; +import org.keycloak.rar.AuthorizationRequestSource; +import java.util.stream.Stream; + + +/** + * @author Daniel Gozalo + * Util class to unify a way to obtain the {@link AuthorizationRequestContext}. + *

+ * As it can be obtained statically from just the OAuth2 scopes parameter, it can be easily referenced from almost anywhere. + */ public class AuthorizationContextUtil { + /** + * Base function to obtain a bare AuthorizationRequestContext with just OAuth2 Scopes + * @param session + * @param scope + * @return an {@link AuthorizationRequestContext} with scope entries + */ public static AuthorizationRequestContext getAuthorizationRequestContextFromScopes(KeycloakSession session, String scope) { if (!Profile.isFeatureEnabled(Profile.Feature.DYNAMIC_SCOPES)) { throw new RuntimeException("The Dynamic Scopes feature is not enabled and the AuthorizationRequestContext hasn't been generated"); @@ -23,4 +57,38 @@ public class AuthorizationContextUtil { return clientScopeParser.parseScopes(scope); } + /** + * An extension of {@link AuthorizationContextUtil#getAuthorizationRequestContextFromScopes} that appends the current context's client + * @param session + * @param scope + * @return an {@link AuthorizationRequestContext} with scope entries and a ClientModel + */ + public static AuthorizationRequestContext getAuthorizationRequestContextFromScopesWithClient(KeycloakSession session, String scope) { + AuthorizationRequestContext authorizationRequestContext = getAuthorizationRequestContextFromScopes(session, scope); + authorizationRequestContext.getAuthorizationDetailEntries().add(new AuthorizationDetails(session.getContext().getClient())); + return authorizationRequestContext; + } + + /** + * An extension of {@link AuthorizationContextUtil#getAuthorizationRequestContextFromScopesWithClient)} that returns the list as a Stream + * @param session + * @param scope + * @return a Stream of {@link AuthorizationDetails} containing a ClientModel + */ + public static Stream getAuthorizationRequestsStreamFromScopesWithClient(KeycloakSession session, String scope) { + AuthorizationRequestContext authorizationRequestContext = getAuthorizationRequestContextFromScopesWithClient(session, scope); + return authorizationRequestContext.getAuthorizationDetailEntries().stream(); + } + + /** + * Helper method to return a Stream of all the {@link ClientScopeModel} in the current {@link AuthorizationRequestContext} + * @param session + * @param scope + * @return see description + */ + public static Stream getClientScopesStreamFromAuthorizationRequestContextWithClient(KeycloakSession session, String scope) { + return getAuthorizationRequestContextFromScopesWithClient(session, scope).getAuthorizationDetailEntries().stream() + .filter(authorizationDetails -> authorizationDetails.getSource() == AuthorizationRequestSource.SCOPE) + .map(AuthorizationDetails::getClientScope); + } } diff --git a/services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java b/services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java index fe36f41aff..85490ab3ee 100644 --- a/services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java +++ b/services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java @@ -71,8 +71,8 @@ public class DefaultClientSessionContext implements ClientSessionContext { private Map attributes = new HashMap<>(); private DefaultClientSessionContext(AuthenticatedClientSessionModel clientSession, Set clientScopeIds, KeycloakSession session) { - this.clientSession = clientSession; this.clientScopeIds = clientScopeIds; + this.clientSession = clientSession; this.session = session; } @@ -86,7 +86,12 @@ public class DefaultClientSessionContext implements ClientSessionContext { public static DefaultClientSessionContext fromClientSessionAndScopeParameter(AuthenticatedClientSessionModel clientSession, String scopeParam, KeycloakSession session) { - Stream requestedClientScopes = TokenManager.getRequestedClientScopes(scopeParam, clientSession.getClient()); + Stream requestedClientScopes; + if (Profile.isFeatureEnabled(Profile.Feature.DYNAMIC_SCOPES)) { + requestedClientScopes = AuthorizationContextUtil.getClientScopesStreamFromAuthorizationRequestContextWithClient(session, scopeParam); + } else { + requestedClientScopes = TokenManager.getRequestedClientScopes(scopeParam, clientSession.getClient()); + } return fromClientSessionAndClientScopes(clientSession, requestedClientScopes, session); } @@ -96,7 +101,10 @@ public class DefaultClientSessionContext implements ClientSessionContext { } - public static DefaultClientSessionContext fromClientSessionAndClientScopes(AuthenticatedClientSessionModel clientSession, + // in order to standardize the way we create this object and with that data, it's better to compute the client scopes internally instead of relying on external sources + // i.e: the TokenManager.getRequestedClientScopes was being called in many places to obtain the ClientScopeModel stream. + // by changing this method to private, we'll only call it in this class, while also having a single place to put the DYNAMIC_SCOPES feature flag condition + private static DefaultClientSessionContext fromClientSessionAndClientScopes(AuthenticatedClientSessionModel clientSession, Stream clientScopes, KeycloakSession session) { Set clientScopeIds = clientScopes.map(ClientScopeModel::getId).collect(Collectors.toSet()); @@ -157,7 +165,7 @@ public class DefaultClientSessionContext implements ClientSessionContext { @Override public String getScopeString() { - if(Profile.isFeatureEnabled(Profile.Feature.DYNAMIC_SCOPES)) { + if (Profile.isFeatureEnabled(Profile.Feature.DYNAMIC_SCOPES)) { String scopeParam = buildScopesStringFromAuthorizationRequest(); logger.tracef("Generated scope param with Dynamic Scopes enabled: %1s", scopeParam); String scopeSent = clientSession.getNote(OAuth2Constants.SCOPE); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java index 0abec356ee..6c077f6ea2 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java @@ -30,6 +30,7 @@ import org.keycloak.common.Profile; import org.keycloak.common.util.UriUtils; import org.keycloak.jose.jws.JWSInput; import org.keycloak.models.AccountRoles; +import org.keycloak.models.ClientScopeModel; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.OIDCLoginProtocolFactory; import org.keycloak.protocol.oidc.mappers.AddressMapper; @@ -65,6 +66,7 @@ import javax.ws.rs.core.Response; import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -1268,6 +1270,44 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { } } + @Test + @EnableFeature(value = Profile.Feature.DYNAMIC_SCOPES, skipRestart = true) + public void executeTokenMappersOnDynamicScopes() { + ClientResource clientResource = findClientResourceByClientId(adminClient.realm("test"), "test-app"); + ClientScopeRepresentation scopeRep = new ClientScopeRepresentation(); + scopeRep.setName("dyn-scope-with-mapper"); + scopeRep.setProtocol("openid-connect"); + scopeRep.setAttributes(new HashMap() {{ + put(ClientScopeModel.IS_DYNAMIC_SCOPE, "true"); + put(ClientScopeModel.DYNAMIC_SCOPE_REGEXP, "dyn-scope-with-mapper:*"); + }}); + // create the attribute mapper + ProtocolMapperRepresentation protocolMapperRepresentation = createHardcodedClaim("dynamic-scope-hardcoded-mapper", "hardcoded-foo", "hardcoded-bar", "String", true, true); + scopeRep.setProtocolMappers(Collections.singletonList(protocolMapperRepresentation)); + + try (Response resp = adminClient.realm("test").clientScopes().create(scopeRep)) { + assertEquals(201, resp.getStatus()); + String clientScopeId = ApiUtil.getCreatedId(resp); + getCleanup().addClientScopeId(clientScopeId); + clientResource.addOptionalClientScope(clientScopeId); + } + + oauth.scope("openid dyn-scope-with-mapper:value"); + OAuthClient.AccessTokenResponse response = browserLogin("password", "test-user@localhost", "password"); + IDToken idToken = oauth.verifyIDToken(response.getIdToken()); + AccessToken accessToken = oauth.verifyToken(response.getAccessToken()); + + assertNotNull(idToken.getOtherClaims()); + assertNotNull(idToken.getOtherClaims().get("hardcoded-foo")); + assertTrue(idToken.getOtherClaims().get("hardcoded-foo") instanceof String); + assertEquals("hardcoded-bar", idToken.getOtherClaims().get("hardcoded-foo")); + + assertNotNull(accessToken.getOtherClaims()); + assertNotNull(accessToken.getOtherClaims().get("hardcoded-foo")); + assertTrue(accessToken.getOtherClaims().get("hardcoded-foo") instanceof String); + assertEquals("hardcoded-bar", accessToken.getOtherClaims().get("hardcoded-foo")); + } + private void assertRoles(List actualRoleList, String ...expectedRoles){ Assert.assertNames(actualRoleList, expectedRoles); }