Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not remove previous refresh token for federated identity #29109

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Do not remove previous refresh token for federated identity #29109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee4b1cc
25815 do not remove previous refresh token for federated identity
geoffreyfourmis Mar 2, 2024
8278d8f
Add check to prevent deserialization issues when the context token is…
sguilhen Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 24 additions & 6 deletions services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java
View file
Open in desktop
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 @@ -72,6 +73,7 @@
import org.keycloak.protocol.saml.SamlSessionUtils;
import org.keycloak.protocol.saml.preprocessor.SamlAuthenticationPreprocessor;
import org.keycloak.representations.AccessToken;
import org.keycloak.representations.AccessTokenResponse;
import org.keycloak.services.ErrorPage;
import org.keycloak.services.ErrorPageException;
import org.keycloak.services.ErrorResponse;
Expand Down Expand Up @@ -118,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 @@ -1073,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 @@ -1095,13 +1096,30 @@ 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())) {
federatedIdentityModel.setToken(context.getToken());
// 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);

this.session.users().updateFederatedIdentity(this.realmModel, federatedUser, federatedIdentityModel);
if (newResponse.getRefreshToken() == null && previousResponse.getRefreshToken() != null) {
newResponse.setRefreshToken(previousResponse.getRefreshToken());
newResponse.setRefreshExpiresIn(previousResponse.getRefreshExpiresIn());
}

if (isDebugEnabled()) {
logger.debugf("Identity [%s] update with response from identity provider [%s].", federatedUser, context.getIdpConfig().getAlias());
federatedIdentityModel.setToken(JsonSerialization.writeValueAsString(newResponse));
} catch (IOException ioe) {
logger.debugf("Token deserialization failed for identity provider %s: %s", context.getIdpConfig().getAlias(), ioe.getMessage());
federatedIdentityModel.setToken(context.getToken());
}
} 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
Open in desktop
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
Open in desktop
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