From 2b59db71a83fee2d9279cab9a7e7215c4f5bd7db Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 23 May 2017 23:16:57 +0200 Subject: [PATCH] KEYCLOAK-3316 Remove the IDToken if scope=openid is not used --- .../keycloak/protocol/oidc/TokenManager.java | 13 +- .../oidc/endpoints/TokenEndpoint.java | 39 +++-- .../testsuite/oauth/AccessTokenTest.java | 5 +- .../oauth/OIDCProtocolMappersTest.java | 45 ++++-- .../oidc/OIDCWellKnownProviderTest.java | 3 +- .../testsuite/oidc/ScopeParameterTest.java | 142 ++++++++++++++++++ 6 files changed, 216 insertions(+), 31 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/ScopeParameterTest.java 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 9782b48717..07aec65e6f 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java @@ -250,11 +250,16 @@ public class TokenManager { validation.clientSession.setTimestamp(currentTime); validation.userSession.setLastSessionRefresh(currentTime); - AccessTokenResponse res = responseBuilder(realm, authorizedClient, event, session, validation.userSession, validation.clientSession) + AccessTokenResponseBuilder responseBuilder = responseBuilder(realm, authorizedClient, event, session, validation.userSession, validation.clientSession) .accessToken(validation.newToken) - .generateIDToken() - .generateRefreshToken() - .build(); + .generateRefreshToken(); + + String scopeParam = validation.clientSession.getNote(OAuth2Constants.SCOPE); + if (TokenUtil.isOIDCRequest(scopeParam)) { + responseBuilder.generateIDToken(); + } + + AccessTokenResponse res = responseBuilder.build(); return new RefreshResult(res, TokenUtil.TOKEN_TYPE_OFFLINE.equals(refreshToken.getType())); } 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 83570efd75..6aa13e23a8 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 @@ -53,6 +53,7 @@ import org.keycloak.services.managers.ClientSessionCode; import org.keycloak.services.managers.RealmManager; import org.keycloak.services.resources.Cors; import org.keycloak.sessions.AuthenticationSessionModel; +import org.keycloak.util.TokenUtil; import javax.ws.rs.OPTIONS; import javax.ws.rs.POST; @@ -340,10 +341,16 @@ public class TokenEndpoint { AccessToken token = tokenManager.createClientAccessToken(session, parseResult.getCode().getRequestedRoles(), realm, client, user, userSession, clientSession); - AccessTokenResponse res = tokenManager.responseBuilder(realm, client, event, session, userSession, clientSession) + TokenManager.AccessTokenResponseBuilder responseBuilder = tokenManager.responseBuilder(realm, client, event, session, userSession, clientSession) .accessToken(token) - .generateIDToken() - .generateRefreshToken().build(); + .generateRefreshToken(); + + String scopeParam = clientSession.getNote(OAuth2Constants.SCOPE); + if (TokenUtil.isOIDCRequest(scopeParam)) { + responseBuilder.generateIDToken(); + } + + AccessTokenResponse res = responseBuilder.build(); event.success(); @@ -450,11 +457,16 @@ public class TokenEndpoint { UserSessionModel userSession = processor.getUserSession(); updateUserSessionFromClientAuth(userSession); - AccessTokenResponse res = tokenManager.responseBuilder(realm, client, event, session, userSession, clientSession) + TokenManager.AccessTokenResponseBuilder responseBuilder = tokenManager.responseBuilder(realm, client, event, session, userSession, clientSession) .generateAccessToken() - .generateRefreshToken() - .generateIDToken() - .build(); + .generateRefreshToken(); + + String scopeParam = clientSession.getNote(OAuth2Constants.SCOPE); + if (TokenUtil.isOIDCRequest(scopeParam)) { + responseBuilder.generateIDToken(); + } + + AccessTokenResponse res = responseBuilder.build(); event.success(); @@ -515,11 +527,16 @@ public class TokenEndpoint { updateUserSessionFromClientAuth(userSession); - AccessTokenResponse res = tokenManager.responseBuilder(realm, client, event, session, userSession, clientSession) + TokenManager.AccessTokenResponseBuilder responseBuilder = tokenManager.responseBuilder(realm, client, event, session, userSession, clientSession) .generateAccessToken() - .generateRefreshToken() - .generateIDToken() - .build(); + .generateRefreshToken(); + + String scopeParam = clientSession.getNote(OAuth2Constants.SCOPE); + if (TokenUtil.isOIDCRequest(scopeParam)) { + responseBuilder.generateIDToken(); + } + + AccessTokenResponse res = responseBuilder.build(); event.success(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java index 5209ef5be6..6a9aa655e5 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java @@ -94,6 +94,8 @@ import static org.keycloak.testsuite.admin.ApiUtil.findUserByUsername; import static org.keycloak.testsuite.admin.ApiUtil.findUserByUsernameId; import static org.keycloak.testsuite.util.OAuthClient.AUTH_SERVER_ROOT; import static org.keycloak.testsuite.util.ProtocolMapperUtil.createRoleNameMapper; + +import org.keycloak.util.TokenUtil; import org.openqa.selenium.By; /** @@ -994,7 +996,8 @@ public class AccessTokenTest extends AbstractKeycloakTest { Form form = new Form(); form.param(OAuth2Constants.GRANT_TYPE, OAuth2Constants.PASSWORD) .param("username", username) - .param("password", password); + .param("password", password) + .param(OAuth2Constants.SCOPE, OAuth2Constants.SCOPE_OPENID); return grantTarget.request() .header(HttpHeaders.AUTHORIZATION, header) .post(Entity.form(form)); 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 0aa91f928c..b8dd51fd0f 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 @@ -24,6 +24,7 @@ import org.junit.Test; import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.admin.client.resource.ProtocolMappersResource; import org.keycloak.admin.client.resource.UserResource; +import org.keycloak.common.util.UriUtils; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.mappers.AddressMapper; import org.keycloak.representations.AccessToken; @@ -148,7 +149,7 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { } { - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "test-user@localhost", "password"); + OAuthClient.AccessTokenResponse response = browserLogin("password", "test-user@localhost", "password"); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); assertNotNull(idToken.getAddress()); @@ -197,6 +198,8 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { assertTrue(accessToken.getRealmAccess().getRoles().contains("realm-user")); Assert.assertFalse(accessToken.getResourceAccess("test-app").getRoles().contains("customer-user")); assertTrue(accessToken.getResourceAccess("app").getRoles().contains("hardcoded")); + + oauth.openLogout(); } // undo mappers @@ -224,13 +227,15 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { { - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "test-user@localhost", "password"); + OAuthClient.AccessTokenResponse response = browserLogin("password", "test-user@localhost", "password"); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); assertNull(idToken.getAddress()); assertNull(idToken.getOtherClaims().get("home_phone")); assertNull(idToken.getOtherClaims().get("hard")); assertNull(idToken.getOtherClaims().get("nested")); assertNull(idToken.getOtherClaims().get("department")); + + oauth.openLogout(); } @@ -248,7 +253,7 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { protocolMappers.createMapper(Arrays.asList(realmMapper, clientMapper)); // Login user - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "test-user@localhost", "password"); + OAuthClient.AccessTokenResponse response = browserLogin("password", "test-user@localhost", "password"); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); // Verify attribute is filled @@ -257,11 +262,11 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { String realmRoleMappings = (String) roleMappings.get("realm"); String testAppMappings = (String) roleMappings.get("test-app"); assertRolesString(realmRoleMappings, - "pref.user", // from direct assignment in user definition - "pref.offline_access" // from direct assignment in user definition + "pref.user", // from direct assignment in user definition + "pref.offline_access" // from direct assignment in user definition ); assertRolesString(testAppMappings, - "customer-user" // from direct assignment in user definition + "customer-user" // from direct assignment in user definition ); // Revert @@ -282,7 +287,7 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { protocolMappers.createMapper(Arrays.asList(realmMapper, clientMapper)); // Login user - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "test-user@localhost", "password"); + OAuthClient.AccessTokenResponse response = browserLogin("password", "test-user@localhost", "password"); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); // Verify attribute is filled @@ -294,11 +299,11 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { List realmRoleMappings = (List) roleMappings.get("realm"); List testAppMappings = (List) roleMappings.get("test-app"); assertRoles(realmRoleMappings, - "pref.user", // from direct assignment in user definition - "pref.offline_access" // from direct assignment in user definition + "pref.user", // from direct assignment in user definition + "pref.offline_access" // from direct assignment in user definition ); assertRoles(testAppMappings, - "customer-user" // from direct assignment in user definition + "customer-user" // from direct assignment in user definition ); // Revert @@ -316,7 +321,7 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { protocolMappers.createMapper(Arrays.asList(realmMapper, clientMapper)); // Login user - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "rich.roles@redhat.com", "password"); + OAuthClient.AccessTokenResponse response = browserLogin("password", "rich.roles@redhat.com", "password"); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); // Verify attribute is filled @@ -354,9 +359,16 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Login user ClientManager.realm(adminClient.realm("test")).clientId(clientId).directAccessGrant(true); oauth.clientId(clientId); - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("secret", "rich.roles@redhat.com", "password"); + + String oldRedirectUri = oauth.getRedirectUri(); + oauth.redirectUri(UriUtils.getOrigin(oldRedirectUri) + "/test-app-authz"); + + OAuthClient.AccessTokenResponse response = browserLogin("secret", "rich.roles@redhat.com", "password"); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); + // revert redirect_uri + oauth.redirectUri(oldRedirectUri); + // Verify attribute is filled Map roleMappings = (Map)idToken.getOtherClaims().get("roles-custom"); Assert.assertThat(roleMappings.keySet(), containsInAnyOrder("realm", clientId)); @@ -387,7 +399,7 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Login user ClientManager.realm(adminClient.realm("test")).clientId(clientId).directAccessGrant(true); oauth.clientId(clientId); - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "rich.roles@redhat.com", "password"); + OAuthClient.AccessTokenResponse response = browserLogin("password", "rich.roles@redhat.com", "password"); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); // Verify attribute is filled @@ -419,7 +431,7 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Login user ClientManager.realm(adminClient.realm("test")).clientId(clientId).directAccessGrant(true); oauth.clientId(clientId); - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "rich.roles@redhat.com", "password"); + OAuthClient.AccessTokenResponse response = browserLogin("password", "rich.roles@redhat.com", "password"); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); // Verify attribute is filled @@ -468,4 +480,9 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { return rep; } + private OAuthClient.AccessTokenResponse browserLogin(String clientSecret, String username, String password) { + OAuthClient.AuthorizationEndpointResponse authzEndpointResponse = oauth.doLogin(username, password); + return oauth.doAccessTokenRequest(authzEndpointResponse.getCode(), clientSecret); + } + } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCWellKnownProviderTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCWellKnownProviderTest.java index 572d5d2772..0203eb13f2 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCWellKnownProviderTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCWellKnownProviderTest.java @@ -121,7 +121,8 @@ public class OIDCWellKnownProviderTest extends AbstractKeycloakTest { @Test public void testIssuerMatches() throws Exception { - OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "test-user@localhost", "password"); + OAuthClient.AuthorizationEndpointResponse authzResp = oauth.doLogin("test-user@localhost", "password"); + OAuthClient.AccessTokenResponse response = oauth.doAccessTokenRequest(authzResp.getCode(), "password"); Assert.assertEquals(200, response.getStatusCode()); IDToken idToken = oauth.verifyIDToken(response.getIdToken()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/ScopeParameterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/ScopeParameterTest.java new file mode 100644 index 0000000000..6096ab2f0d --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/ScopeParameterTest.java @@ -0,0 +1,142 @@ +/* + * Copyright 2016 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.testsuite.oidc; + +import java.util.List; + +import javax.ws.rs.core.UriBuilder; + +import org.jboss.arquillian.graphene.page.Page; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.keycloak.OAuth2Constants; +import org.keycloak.events.Details; +import org.keycloak.representations.AccessToken; +import org.keycloak.representations.idm.EventRepresentation; +import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; +import org.keycloak.testsuite.ActionURIUtils; +import org.keycloak.testsuite.Assert; +import org.keycloak.testsuite.AssertEvents; +import org.keycloak.testsuite.admin.AbstractAdminTest; +import org.keycloak.testsuite.pages.AccountUpdateProfilePage; +import org.keycloak.testsuite.pages.AppPage; +import org.keycloak.testsuite.pages.ErrorPage; +import org.keycloak.testsuite.pages.LoginPage; +import org.keycloak.testsuite.pages.OAuthGrantPage; +import org.keycloak.testsuite.util.ClientManager; +import org.keycloak.testsuite.util.OAuthClient; + +import static org.junit.Assert.assertEquals; + +/** + * @author Marek Posolda + */ +public class ScopeParameterTest extends AbstractTestRealmKeycloakTest { + + @Rule + public AssertEvents events = new AssertEvents(this); + + @Page + protected AppPage appPage; + + @Page + protected LoginPage loginPage; + + @Page + protected AccountUpdateProfilePage profilePage; + + @Page + protected OAuthGrantPage grantPage; + + @Page + protected ErrorPage errorPage; + + + @Override + public void configureTestRealm(RealmRepresentation testRealm) { + } + + @Before + public void clientConfiguration() { + ClientManager.realm(adminClient.realm("test")).clientId("test-app").directAccessGrant(true); + /* + * Configure the default client ID. Seems like OAuthClient is keeping the state of clientID + * For example: If some test case configure oauth.clientId("sample-public-client"), other tests + * will faile and the clientID will always be "sample-public-client + * @see AccessTokenTest#testAuthorizationNegotiateHeaderIgnored() + */ + oauth.clientId("test-app"); + oauth.maxAge(null); + } + + @Override + public void addTestRealms(List testRealms) { + RealmRepresentation realm = AbstractAdminTest.loadJson(getClass().getResourceAsStream("/testrealm.json"), RealmRepresentation.class); + testRealms.add(realm); + } + + + // If scope=openid is missing, IDToken won't be present + @Test + public void testMissingScopeOpenid() { + String loginFormUrl = oauth.getLoginFormUrl(); + loginFormUrl = ActionURIUtils.removeQueryParamFromURI(loginFormUrl, OAuth2Constants.SCOPE); + + driver.navigate().to(loginFormUrl); + oauth.fillLoginForm("test-user@localhost", "password"); + EventRepresentation loginEvent = events.expectLogin().assertEvent(); + + String code = new OAuthClient.AuthorizationEndpointResponse(oauth).getCode(); + OAuthClient.AccessTokenResponse response = oauth.doAccessTokenRequest(code, "password"); + + // IDToken is not there + Assert.assertEquals(200, response.getStatusCode()); + Assert.assertNull(response.getIdToken()); + Assert.assertNotNull(response.getRefreshToken()); + + AccessToken token = oauth.verifyToken(response.getAccessToken()); + Assert.assertEquals(token.getSubject(), loginEvent.getUserId()); + + // Refresh and assert idToken still not present + response = oauth.doRefreshTokenRequest(response.getRefreshToken(), "password"); + Assert.assertEquals(200, response.getStatusCode()); + Assert.assertNull(response.getIdToken()); + + token = oauth.verifyToken(response.getAccessToken()); + Assert.assertEquals(token.getSubject(), loginEvent.getUserId()); + } + + + // If scope=openid is missing, IDToken won't be present + @Test + public void testMissingScopeOpenidInResourceOwnerPasswordCredentialRequest() throws Exception { + OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("password", "test-user@localhost", "password"); + + assertEquals(200, response.getStatusCode()); + + // idToken not present + Assert.assertNull(response.getIdToken()); + + Assert.assertNotNull(response.getRefreshToken()); + AccessToken accessToken = oauth.verifyToken(response.getAccessToken()); + Assert.assertEquals(accessToken.getPreferredUsername(), "test-user@localhost"); + + } +}