From 02e2ebf25829508945be6d5be12630935903d031 Mon Sep 17 00:00:00 2001 From: Stefan Guilhen Date: Fri, 26 Apr 2024 10:22:38 -0300 Subject: [PATCH] Add check to prevent deserialization issues when the context token is not an AccessTokenResponse. - also adds a test for the refresh token on first login scenario. Signed-off-by: Stefan Guilhen --- .../resources/IdentityBrokerService.java | 30 +++--- ...stKeycloakOidcIdentityProviderFactory.java | 29 ++++++ .../broker/KcOidcFirstBrokerLoginTest.java | 94 ++++++++++++++++++- 3 files changed, 133 insertions(+), 20 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java index 005f6a6416..537471f446 100755 --- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java +++ b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java @@ -19,6 +19,7 @@ package org.keycloak.services.resources; import org.jboss.logging.Logger; import org.jboss.resteasy.reactive.NoCache; import org.keycloak.authentication.authenticators.broker.IdpConfirmOverrideLinkAuthenticator; +import org.keycloak.broker.provider.ExchangeTokenToIdentityProviderToken; import org.keycloak.http.HttpRequest; import org.keycloak.OAuthErrorException; import org.keycloak.authentication.AuthenticationProcessor; @@ -119,7 +120,6 @@ import java.util.Optional; import java.util.Set; import java.util.UUID; import java.util.function.Consumer; -import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -1074,7 +1074,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal private void setEmail(BrokeredIdentityContext context, UserModel federatedUser, String newEmail) { federatedUser.setEmail(newEmail); - // change email verified depending if it is trusted or not + // change email verified depending on if it is trusted or not if (context.getIdpConfig().isTrustEmail() && !Boolean.parseBoolean(context.getAuthenticationSession().getAuthNote(AbstractIdpAuthenticator.UPDATE_PROFILE_EMAIL_CHANGED))) { logger.tracef("Email verified automatically after updating user '%s' through Identity provider '%s' ", federatedUser.getUsername(), context.getIdpConfig().getAlias()); federatedUser.setEmailVerified(true); @@ -1096,11 +1096,11 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal private void updateToken(BrokeredIdentityContext context, UserModel federatedUser, FederatedIdentityModel federatedIdentityModel) { if (context.getIdpConfig().isStoreToken() && !ObjectUtil.isEqualOrBothNull(context.getToken(), federatedIdentityModel.getToken())) { - try { - // like in OIDCIdentityProvider.exchangeStoredToken() - // we shouldn't override the refresh token if it is null in the context and not null in the DB - // as for google IDP it will be lost forever - if (federatedIdentityModel.getToken() != null) { + // like in OIDCIdentityProvider.exchangeStoredToken() + // we shouldn't override the refresh token if it is null in the context and not null in the DB + // as for google IDP it will be lost forever + if (federatedIdentityModel.getToken() != null && ExchangeTokenToIdentityProviderToken.class.isInstance(context.getIdp())) { + try { AccessTokenResponse previousResponse = JsonSerialization.readValue(federatedIdentityModel.getToken(), AccessTokenResponse.class); AccessTokenResponse newResponse = JsonSerialization.readValue(context.getToken(), AccessTokenResponse.class); @@ -1110,18 +1110,16 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal } federatedIdentityModel.setToken(JsonSerialization.writeValueAsString(newResponse)); - } else { + } catch (IOException ioe) { + logger.debugf("Token deserialization failed for identity provider %s: %s", context.getIdpConfig().getAlias(), ioe.getMessage()); federatedIdentityModel.setToken(context.getToken()); } - - this.session.users().updateFederatedIdentity(this.realmModel, federatedUser, federatedIdentityModel); - - if (isDebugEnabled()) { - logger.debugf("Identity [%s] update with response from identity provider [%s].", federatedUser, context.getIdpConfig().getAlias()); - } - } catch (IOException e) { - throw new RuntimeException(e); + } else { + federatedIdentityModel.setToken(context.getToken()); } + + this.session.users().updateFederatedIdentity(this.realmModel, federatedUser, federatedIdentityModel); + logger.debugf("Identity [%s] update with response from identity provider [%s].", federatedUser, context.getIdpConfig().getAlias()); } } diff --git a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/broker/oidc/TestKeycloakOidcIdentityProviderFactory.java b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/broker/oidc/TestKeycloakOidcIdentityProviderFactory.java index dee4c7e4d1..dc864adb05 100644 --- a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/broker/oidc/TestKeycloakOidcIdentityProviderFactory.java +++ b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/broker/oidc/TestKeycloakOidcIdentityProviderFactory.java @@ -17,21 +17,29 @@ package org.keycloak.testsuite.broker.oidc; +import java.io.IOException; +import java.util.HashSet; +import java.util.Set; + import jakarta.ws.rs.core.UriBuilder; import org.keycloak.broker.oidc.KeycloakOIDCIdentityProvider; import org.keycloak.broker.oidc.KeycloakOIDCIdentityProviderFactory; import org.keycloak.broker.oidc.OIDCIdentityProviderConfig; import org.keycloak.broker.provider.AuthenticationRequest; +import org.keycloak.broker.provider.BrokeredIdentityContext; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.KeycloakSession; import org.keycloak.protocol.oidc.OIDCLoginProtocol; +import org.keycloak.representations.AccessTokenResponse; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.sessions.AuthenticationSessionModel; +import org.keycloak.util.JsonSerialization; public class TestKeycloakOidcIdentityProviderFactory extends KeycloakOIDCIdentityProviderFactory { public static final String ID = "test-keycloak-oidc"; public static final String IGNORE_MAX_AGE_PARAM = "ignore-max-age-param"; + public static final String USE_SINGLE_REFRESH_TOKEN = "use-single-refresh-token"; public static void setIgnoreMaxAgeParam(IdentityProviderRepresentation rep) { rep.getConfig().put(IGNORE_MAX_AGE_PARAM, Boolean.TRUE.toString()); @@ -45,6 +53,27 @@ public class TestKeycloakOidcIdentityProviderFactory extends KeycloakOIDCIdentit @Override public KeycloakOIDCIdentityProvider create(KeycloakSession session, IdentityProviderModel model) { return new KeycloakOIDCIdentityProvider(session, new OIDCIdentityProviderConfig(model)) { + + private static final Set usernames = new HashSet<>(); + + @Override + public BrokeredIdentityContext getFederatedIdentity(String response) { + BrokeredIdentityContext context = super.getFederatedIdentity(response); + if (Boolean.valueOf(model.getConfig().get(USE_SINGLE_REFRESH_TOKEN))) { + // refresh token will be available only in the first login. + if (!usernames.add(context.getUsername())) { + try { + AccessTokenResponse tokenResponse = JsonSerialization.readValue(context.getToken(), AccessTokenResponse.class); + tokenResponse.setRefreshToken(null); + context.setToken(JsonSerialization.writeValueAsString(tokenResponse)); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + } + return context; + } + @Override protected UriBuilder createAuthorizationUrl(AuthenticationRequest request) { AuthenticationSessionModel authSession = request.getAuthenticationSession(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcFirstBrokerLoginTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcFirstBrokerLoginTest.java index f666eee26c..b4038bc7b0 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcFirstBrokerLoginTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcFirstBrokerLoginTest.java @@ -3,11 +3,18 @@ package org.keycloak.testsuite.broker; import org.apache.commons.lang3.StringUtils; import org.jboss.arquillian.graphene.page.Page; import org.junit.Test; +import org.keycloak.admin.client.resource.IdentityProviderResource; import org.keycloak.admin.client.resource.RealmResource; +import org.keycloak.models.FederatedIdentityModel; +import org.keycloak.models.IdentityProviderSyncMode; +import org.keycloak.models.RealmModel; +import org.keycloak.models.UserModel; +import org.keycloak.representations.AccessTokenResponse; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.Assert; +import org.keycloak.testsuite.broker.oidc.TestKeycloakOidcIdentityProviderFactory; import org.keycloak.testsuite.forms.RegisterWithUserProfileTest; import org.keycloak.testsuite.forms.VerifyProfileTest; import org.keycloak.testsuite.pages.LoginUpdateProfilePage; @@ -15,14 +22,21 @@ import org.keycloak.testsuite.pages.RegisterPage; import org.keycloak.testsuite.pages.AppPage; import org.keycloak.testsuite.util.AccountHelper; import org.keycloak.testsuite.util.ClientScopeBuilder; +import org.keycloak.util.JsonSerialization; import org.openqa.selenium.By; import org.openqa.selenium.NoSuchElementException; -import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.keycloak.testsuite.admin.ApiUtil.removeUserByUsername; +import static org.keycloak.testsuite.broker.BrokerTestConstants.IDP_OIDC_ALIAS; +import static org.keycloak.testsuite.broker.BrokerTestTools.createIdentityProvider; import static org.keycloak.testsuite.broker.BrokerTestTools.waitForPage; import static org.keycloak.testsuite.forms.VerifyProfileTest.ATTRIBUTE_DEPARTMENT; import static org.keycloak.testsuite.forms.VerifyProfileTest.PERMISSIONS_ADMIN_EDITABLE; @@ -44,7 +58,79 @@ public class KcOidcFirstBrokerLoginTest extends AbstractFirstBrokerLoginTest { @Override protected BrokerConfiguration getBrokerConfiguration() { - return KcOidcBrokerConfiguration.INSTANCE; + return new KcOidcBrokerConfiguration() { + @Override + public IdentityProviderRepresentation setUpIdentityProvider(IdentityProviderSyncMode syncMode) { + IdentityProviderRepresentation idp = createIdentityProvider(IDP_OIDC_ALIAS, TestKeycloakOidcIdentityProviderFactory.ID); + applyDefaultConfiguration(idp.getConfig(), syncMode); + return idp; + } + }; + } + + /** + * Tests the scenario where a OIDC IDP sends the refresh token only on first login (e.g. Google). In this case, subsequent + * logins that end up triggering the update of the federated user should not rewrite the token (access token response) + * without updating it first with the stored refresh token. + * + * Github issue reference: #25815 + * + * @throws Exception if an error occurs while running the test. + */ + @Test + public void testRefreshTokenSentOnlyOnFirstLogin() throws Exception { + IdentityProviderResource idp = realmsResouce().realm(bc.consumerRealmName()).identityProviders().get(bc.getIDPAlias()); + IdentityProviderRepresentation representation = idp.toRepresentation(); + representation.setStoreToken(true); + // enable refresh tokens only for the first login (test broker mimics behavior of idps that operate like this). + representation.getConfig().put(TestKeycloakOidcIdentityProviderFactory.USE_SINGLE_REFRESH_TOKEN, "true"); + idp.update(representation); + + // create a test user in the provider realm. + createUser(bc.providerRealmName(), "brucewayne", BrokerTestConstants.USER_PASSWORD, "Bruce", "Wayne", "brucewayne@gotham.com"); + + oauth.clientId("broker-app"); + loginPage.open(bc.consumerRealmName()); + logInWithIdp(bc.getIDPAlias(), "brucewayne", BrokerTestConstants.USER_PASSWORD); + + // obtain the stored token from the federated identity. + String storedToken = testingClient.server(bc.consumerRealmName()).fetchString(session -> { + RealmModel realmModel = session.getContext().getRealm(); + UserModel userModel = session.users().getUserByUsername(realmModel, "brucewayne"); + FederatedIdentityModel fedIdentity = session.users().getFederatedIdentitiesStream(realmModel, userModel).findFirst().orElse(null); + return fedIdentity != null ? fedIdentity.getToken() : null; + }); + assertThat(storedToken, not(nullValue())); + + // convert the stored token into an access response for easier retrieval of both access and refresh tokens. + AccessTokenResponse tokenResponse = JsonSerialization.readValue(storedToken.substring(1, storedToken.length() - 1).replace("\\", ""), AccessTokenResponse.class); + String firstLoginAccessToken = tokenResponse.getToken(); + assertThat(firstLoginAccessToken, not(nullValue())); + String firstLoginRefreshToken = tokenResponse.getRefreshToken(); + assertThat(firstLoginRefreshToken, not(nullValue())); + + // logout and then log back in. + AccountHelper.logout(adminClient.realm(bc.consumerRealmName()), "brucewayne"); + + loginPage.open(bc.consumerRealmName()); + logInWithIdp(bc.getIDPAlias(), "brucewayne", BrokerTestConstants.USER_PASSWORD); + + // fetch the stored token - access token should have been updated, but the refresh token should remain the same. + storedToken = testingClient.server(bc.consumerRealmName()).fetchString(session -> { + RealmModel realmModel = session.getContext().getRealm(); + UserModel userModel = session.users().getUserByUsername(realmModel, "brucewayne"); + FederatedIdentityModel fedIdentity = session.users().getFederatedIdentitiesStream(realmModel, userModel).findFirst().orElse(null); + return fedIdentity != null ? fedIdentity.getToken() : null; + }); + + tokenResponse = JsonSerialization.readValue(storedToken.substring(1, storedToken.length() - 1).replace("\\", ""), AccessTokenResponse.class); + String secondLoginAccessToken = tokenResponse.getToken(); + assertThat(secondLoginAccessToken, not(nullValue())); + String secondLoginRefreshToken = tokenResponse.getRefreshToken(); + assertThat(secondLoginRefreshToken, not(nullValue())); + + assertThat(firstLoginAccessToken, not(equalTo(secondLoginAccessToken))); + assertThat(firstLoginRefreshToken, is(equalTo(secondLoginRefreshToken))); } /** @@ -595,7 +681,7 @@ public class KcOidcFirstBrokerLoginTest extends AbstractFirstBrokerLoginTest { waitForPage(driver, "update account information", false); updateAccountInformationPage.assertCurrent(); - org.junit.Assert.assertFalse(updateAccountInformationPage.isDepartmentPresent()); + assertFalse(updateAccountInformationPage.isDepartmentPresent()); updateAccountInformationPage.updateAccountInformation( "requiredReadOnlyAttributeNotRenderedAndNotBlockingRegistration", "requiredReadOnlyAttributeNotRenderedAndNotBlockingRegistration@email", "FirstAA", "LastAA"); } @@ -710,7 +796,7 @@ public class KcOidcFirstBrokerLoginTest extends AbstractFirstBrokerLoginTest { waitForPage(driver, "update account information", false); updateAccountInformationPage.assertCurrent(); - org.junit.Assert.assertFalse(updateAccountInformationPage.isDepartmentPresent()); + assertFalse(updateAccountInformationPage.isDepartmentPresent()); updateAccountInformationPage.updateAccountInformation( "attributeRequiredButNotSelectedByScopeIsNotRenderedAndNotBlockingRegistration", "attributeRequiredButNotSelectedByScopeIsNotRenderedAndNotBlockingRegistration@email", "FirstAA", "LastAA"); UserRepresentation user = VerifyProfileTest.getUserByUsername(testRealm(),"attributeRequiredButNotSelectedByScopeIsNotRenderedAndNotBlockingRegistration");