Skip to content

Commit

Permalink
Add check to prevent deserialization issues when the context token is…
Browse files Browse the repository at this point in the history 
… not an AccessTokenResponse.

- also adds a test for the refresh token on first login scenario.

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
  • Loading branch information
@sguilhen @pedroigor
sguilhen authored and pedroigor committed Apr 30, 2024
1 parent 24d9a22 commit 02e2ebf
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 20 deletions.
30 changes: 14 additions & 16 deletions services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
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;
Expand Down Expand Up @@ -119,7 +120,6 @@
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;

Expand Down Expand Up @@ -1074,7 +1074,7 @@ private void setDiffAttrToConsumer(String actualValue, String newValue, Consumer

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);
Expand All @@ -1096,11 +1096,11 @@ private void migrateFederatedIdentityId(BrokeredIdentityContext context, UserMod

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);

Expand All @@ -1110,18 +1110,16 @@ private void updateToken(BrokeredIdentityContext context, UserModel federatedUse
}

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());
}
}

Expand Down
29 changes: 29 additions & 0 deletions ...main/java/org/keycloak/testsuite/broker/oidc/TestKeycloakOidcIdentityProviderFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -45,6 +53,27 @@ public String getId() {
@Override
public KeycloakOIDCIdentityProvider create(KeycloakSession session, IdentityProviderModel model) {
return new KeycloakOIDCIdentityProvider(session, new OIDCIdentityProviderConfig(model)) {

private static final Set<String> 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();
Expand Down
94 changes: 90 additions & 4 deletions ...an/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcFirstBrokerLoginTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,40 @@
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;
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;
Expand All @@ -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)));
}

/**
Expand Down Expand Up @@ -595,7 +681,7 @@ public void testDynamicUserProfileReview_requiredReadOnlyAttributeNotRenderedAnd
waitForPage(driver, "update account information", false);
updateAccountInformationPage.assertCurrent();

org.junit.Assert.assertFalse(updateAccountInformationPage.isDepartmentPresent());
assertFalse(updateAccountInformationPage.isDepartmentPresent());

updateAccountInformationPage.updateAccountInformation( "requiredReadOnlyAttributeNotRenderedAndNotBlockingRegistration", "requiredReadOnlyAttributeNotRenderedAndNotBlockingRegistration@email", "FirstAA", "LastAA");
}
Expand Down Expand Up @@ -710,7 +796,7 @@ public void testDynamicUserProfileReview_attributeRequiredButNotSelectedByScopeI
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");
Expand Down

0 comments on commit 02e2ebf

Please sign in to comment.