From b60b85ab65a6197f6f1bce84880fcf447e6afbd6 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 1 Apr 2020 15:22:43 -0300 Subject: [PATCH] [KEYCLOAK-7450] - Match subject when validating id_token returned from external OP --- .../broker/oidc/OIDCIdentityProvider.java | 4 ++ .../oidc/endpoints/UserInfoEndpoint.java | 8 ++- .../mappers/AbstractPairwiseSubMapper.java | 2 +- .../testsuite/util/UserInfoClientUtil.java | 7 ++ .../KcOidcBrokerSubMatchIntrospectionest.java | 72 +++++++++++++++++++ .../keycloak/testsuite/oidc/UserInfoTest.java | 4 +- 6 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerSubMatchIntrospectionest.java diff --git a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java index ce043c756e..e7f6fa1ae4 100755 --- a/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java +++ b/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java @@ -365,6 +365,10 @@ public class OIDCIdentityProvider extends AbstractOAuth2IdentityProvider claims = new HashMap<>(); - claims.put("sub", userModel.getId()); claims.putAll(userInfo.getOtherClaims()); + // we always set the subject to the correct value and ignore any mapper (not directly related to subject mapping such as + // pseudo-subjects). the endpoint should always return a valid subject identifier. + // any attempt to customize the value of this field should be done through a different claim + claims.put("sub", userInfo.getSubject()); Response.ResponseBuilder responseBuilder; OIDCAdvancedConfigWrapper cfg = OIDCAdvancedConfigWrapper.fromClientModel(clientModel); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractPairwiseSubMapper.java b/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractPairwiseSubMapper.java index ef3b9bdcf1..b6afe0ea9d 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractPairwiseSubMapper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractPairwiseSubMapper.java @@ -90,7 +90,7 @@ public abstract class AbstractPairwiseSubMapper extends AbstractOIDCProtocolMapp } protected void setUserInfoTokenSubject(IDToken token, String pairwiseSub) { - token.getOtherClaims().put("sub", pairwiseSub); + token.setSubject(pairwiseSub); } @Override diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/UserInfoClientUtil.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/UserInfoClientUtil.java index fb39ec5457..5983e908c3 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/UserInfoClientUtil.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/UserInfoClientUtil.java @@ -50,6 +50,10 @@ public class UserInfoClientUtil { } public static UserInfo testSuccessfulUserInfoResponse(Response response, String expectedUsername, String expectedEmail) { + return testSuccessfulUserInfoResponse(response, null, expectedUsername, expectedEmail); + } + + public static UserInfo testSuccessfulUserInfoResponse(Response response, String userId, String expectedUsername, String expectedEmail) { Assert.assertEquals(Response.Status.OK.getStatusCode(), response.getStatus()); Assert.assertEquals(response.getHeaderString(HttpHeaders.CONTENT_TYPE), MediaType.APPLICATION_JSON); @@ -59,6 +63,9 @@ public class UserInfoClientUtil { Assert.assertNotNull(userInfo); Assert.assertNotNull(userInfo.getSubject()); + if (userId != null) { + Assert.assertEquals(userId, userInfo.getSubject()); + } Assert.assertEquals(expectedEmail, userInfo.getEmail()); Assert.assertEquals(expectedUsername, userInfo.getPreferredUsername()); return userInfo; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerSubMatchIntrospectionest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerSubMatchIntrospectionest.java new file mode 100644 index 0000000000..7d125ffbb5 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerSubMatchIntrospectionest.java @@ -0,0 +1,72 @@ +package org.keycloak.testsuite.broker; + +import static org.keycloak.testsuite.broker.BrokerTestTools.waitForPage; +import static org.keycloak.testsuite.util.ProtocolMapperUtil.createHardcodedClaim; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.Assert; +import org.junit.Ignore; +import org.junit.Test; +import org.keycloak.jose.jws.JWSInput; +import org.keycloak.jose.jws.JWSInputException; +import org.keycloak.representations.IDToken; +import org.keycloak.representations.idm.ClientRepresentation; +import org.keycloak.representations.idm.ProtocolMapperRepresentation; +import org.keycloak.testsuite.arquillian.SuiteContext; +import org.keycloak.testsuite.util.ClientBuilder; +import org.keycloak.testsuite.util.OAuthClient; + +public class KcOidcBrokerSubMatchIntrospectionest extends AbstractBrokerTest { + + @Override + protected BrokerConfiguration getBrokerConfiguration() { + return new KcOidcBrokerConfiguration() { + @Override + public List createConsumerClients(SuiteContext suiteContext) { + List clients = new ArrayList<>(super.createConsumerClients(suiteContext)); + + clients.add(ClientBuilder.create().clientId("consumer-client") + .publicClient() + .redirectUris("http://localhost:8180/auth/realms/master/app/auth/*", "https://localhost:8543/auth/realms/master/app/auth/*") + .publicClient().build()); + + return clients; + } + + @Override + public List createProviderClients(SuiteContext suiteContext) { + List clients = super.createProviderClients(suiteContext); + List mappers = new ArrayList<>(); + + mappers.add(createHardcodedClaim("sub-override", "sub", "overriden", "String", true, true)); + + clients.get(0).setProtocolMappers(mappers); + + return clients; + } + }; + } + + @Override + public void testLogInAsUserInIDP() { + driver.navigate().to(getAccountUrl(bc.consumerRealmName())); + + oauth.realm(bc.consumerRealmName()); + oauth.clientId("consumer-client"); + + log.debug("Clicking social " + bc.getIDPAlias()); + loginPage.clickSocial(bc.getIDPAlias()); + waitForPage(driver, "log in to", true); + + log.debug("Logging in"); + loginPage.login(bc.getUserLogin(), bc.getUserPassword()); + errorPage.assertCurrent(); + } + + @Ignore + @Override + public void loginWithExistingUser() { + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java index 936ace74d2..402182e949 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java @@ -37,6 +37,7 @@ import org.keycloak.jose.jws.crypto.RSAProvider; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.representations.AccessToken; import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.util.KeycloakModelUtils; import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper; import org.keycloak.protocol.oidc.OIDCLoginProtocolService; @@ -594,7 +595,8 @@ public class UserInfoTest extends AbstractKeycloakTest { .detail(Details.SIGNATURE_REQUIRED, "false") .client(expectedClientId) .assertEvent(); - UserInfoClientUtil.testSuccessfulUserInfoResponse(response, "test-user@localhost", "test-user@localhost"); + UserRepresentation user = adminClient.realm("test").users().search("test-user@localhost").get(0); + UserInfoClientUtil.testSuccessfulUserInfoResponse(response, user.getId(), "test-user@localhost", "test-user@localhost"); } private void testSuccessSignedResponse(Algorithm sigAlg) throws Exception {