From 41b64c91aa5fd8dabc8e69aa19d22d73d259aa92 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Mon, 22 Sep 2025 18:02:45 -0300 Subject: [PATCH] Do not update email if there is no email from the IdP Closes #42390 Signed-off-by: Pedro Igor --- .../ldap/ReadonlyLDAPUserModelDelegate.java | 2 +- .../provider/AbstractIdentityProvider.java | 14 +- .../broker/KcOidcBrokerLdapReadOnlyTest.java | 139 ++++++++++++++++++ .../federation/ldap/LDAPTestContext.java | 6 +- 4 files changed, 157 insertions(+), 4 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerLdapReadOnlyTest.java diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/ReadonlyLDAPUserModelDelegate.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/ReadonlyLDAPUserModelDelegate.java index 45978e17ac0..da8b918fa2c 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/ReadonlyLDAPUserModelDelegate.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/ReadonlyLDAPUserModelDelegate.java @@ -70,7 +70,7 @@ public class ReadonlyLDAPUserModelDelegate extends UserModelDelegate { @Override public void setSingleAttribute(String name, String value) { - setAttribute(name, List.of(value)); + setAttribute(name, value != null ? List.of(value) : null); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/AbstractIdentityProvider.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/AbstractIdentityProvider.java index 3c3fad2b7e2..6035a0ea44f 100755 --- a/server-spi-private/src/main/java/org/keycloak/broker/provider/AbstractIdentityProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/broker/provider/AbstractIdentityProvider.java @@ -176,8 +176,18 @@ public abstract class AbstractIdentityProvider protected void updateEmail(UserModel user, BrokeredIdentityContext context) { AuthenticationSessionModel authSession = context.getAuthenticationSession(); + // Could be the case during external-internal token exchange - if (authSession == null) return; + if (authSession == null) { + return; + } + + String email = context.getEmail(); + + if (email == null) { + // do not set email if not provided by the IdP + return; + } boolean isNewUser = Boolean.parseBoolean(authSession.getAuthNote(BROKER_REGISTERED_NEW_USER)); @@ -189,7 +199,7 @@ public abstract class AbstractIdentityProvider } setEmailVerified(user, context); - user.setEmail(context.getEmail()); + user.setEmail(email); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerLdapReadOnlyTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerLdapReadOnlyTest.java new file mode 100644 index 00000000000..c40b2b34b18 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerLdapReadOnlyTest.java @@ -0,0 +1,139 @@ +package org.keycloak.testsuite.broker; + +import static org.junit.Assert.assertEquals; +import static org.keycloak.models.utils.ModelToRepresentation.toRepresentationWithoutConfig; + +import java.util.Map; + +import jakarta.ws.rs.core.Response; +import org.jboss.arquillian.graphene.page.Page; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Test; +import org.keycloak.admin.client.resource.UserProfileResource; +import org.keycloak.common.util.MultivaluedHashMap; +import org.keycloak.models.IdentityProviderSyncMode; +import org.keycloak.models.LDAPConstants; +import org.keycloak.models.RealmModel; +import org.keycloak.models.UserModel; +import org.keycloak.representations.idm.IdentityProviderRepresentation; +import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.representations.userprofile.config.UPConfig; +import org.keycloak.storage.UserStorageProvider.EditMode; +import org.keycloak.storage.UserStorageProviderModel; +import org.keycloak.storage.ldap.LDAPStorageProviderFactory; +import org.keycloak.storage.ldap.idm.model.LDAPObject; +import org.keycloak.testsuite.admin.ApiUtil; +import org.keycloak.testsuite.federation.ldap.LDAPTestContext; +import org.keycloak.testsuite.pages.IdpConfirmLinkPage; +import org.keycloak.testsuite.util.LDAPRule; +import org.keycloak.testsuite.util.LDAPTestUtils; + +public final class KcOidcBrokerLdapReadOnlyTest extends AbstractInitializedBaseBrokerTest { + + @ClassRule + public static LDAPRule ldapRule = new LDAPRule(); + + @Page + private IdpConfirmLinkPage confirmLinkPage; + + @Override + protected BrokerConfiguration getBrokerConfiguration() { + return new KcOidcBrokerConfiguration() { + @Override + public IdentityProviderRepresentation setUpIdentityProvider(IdentityProviderSyncMode syncMode) { + return super.setUpIdentityProvider(IdentityProviderSyncMode.FORCE); + } + }; + } + + @Before + public void onBefore() { + createLdapStorageProvider(); + addLdapUser(bc.getUserLogin(), bc.getUserEmail()); + } + + @Test + public void testDoNotUpdateEmail() { + // email as optional in both realms + UserProfileResource userProfile = adminClient.realm(bc.consumerRealmName()).users().userProfile(); + UPConfig upConfig = userProfile.getConfiguration(); + upConfig.getAttribute(UserModel.EMAIL).setRequired(null); + userProfile.update(upConfig); + userProfile = adminClient.realm(bc.providerRealmName()).users().userProfile(); + upConfig = userProfile.getConfiguration(); + upConfig.getAttribute(UserModel.EMAIL).setRequired(null); + userProfile.update(upConfig); + + // federate user and link account + oauth.clientId("broker-app"); + loginPage.open(bc.consumerRealmName()); + logInWithBroker(bc); + updateAccountInformationPage.updateAccountInformation(bc.getUserLogin(), bc.getUserEmail(), "f", "l"); + confirmLinkPage.clickLinkAccount(); + loginPage.login(bc.getUserLogin(), "Password1"); + appPage.assertCurrent(); + + // unset email on the provider realm + UserRepresentation user = adminClient.realm(bc.providerRealmName()).users().search(bc.getUserLogin()).get(0); + user.setEmail(""); + adminClient.realm(bc.providerRealmName()).users().get(user.getId()).update(user); + + // logout user on the consumer realm and login again + user = adminClient.realm(bc.consumerRealmName()).users().search(bc.getUserLogin()).get(0); + adminClient.realm(bc.consumerRealmName()).users().get(user.getId()).logout(); + oauth.clientId("broker-app"); + loginPage.open(bc.consumerRealmName()); + logInWithBroker(bc); + appPage.assertCurrent(); + + // email should remain unchanged + user = adminClient.realm(bc.consumerRealmName()).users().search(bc.getUserLogin()).get(0); + assertEquals(bc.getUserEmail(), user.getEmail()); + } + + private void createLdapStorageProvider() { + String providerName = "ldap"; + String providerId = LDAPStorageProviderFactory.PROVIDER_NAME; + + Map ldapConfig = ldapRule.getConfig(); + ldapConfig.put(LDAPConstants.SYNC_REGISTRATIONS, "false"); + ldapConfig.put(LDAPConstants.EDIT_MODE, EditMode.READ_ONLY.name()); + ldapConfig.put(UserStorageProviderModel.IMPORT_ENABLED, "true"); + MultivaluedHashMap config = toComponentConfig(ldapConfig); + + UserStorageProviderModel model = new UserStorageProviderModel(); + model.setLastSync(0); + model.setChangedSyncPeriod(-1); + model.setFullSyncPeriod(-1); + model.setName(providerName); + model.setPriority(0); + model.setProviderId(providerId); + model.setConfig(config); + + Response resp = adminClient.realm(bc.consumerRealmName()).components().add(toRepresentationWithoutConfig(model)); + getCleanup().addComponentId(ApiUtil.getCreatedId(resp)); + } + + private MultivaluedHashMap toComponentConfig(Map ldapConfig) { + MultivaluedHashMap config = new MultivaluedHashMap<>(); + for (Map.Entry entry : ldapConfig.entrySet()) { + config.add(entry.getKey(), entry.getValue()); + + } + return config; + } + + private void addLdapUser(String username, String email) { + String realmName = bc.consumerRealmName(); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session, realmName, null); + RealmModel appRealm = ctx.getRealm(); + + LDAPTestUtils.removeAllLDAPUsers(ctx.getLdapProvider(), appRealm); + LDAPObject user = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, username, "f", "l", email , new MultivaluedHashMap<>()); + LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), user, "Password1"); + }); + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPTestContext.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPTestContext.java index a7f263fe2c6..ef7714cf57a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPTestContext.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPTestContext.java @@ -38,7 +38,11 @@ public class LDAPTestContext { } public static LDAPTestContext init(KeycloakSession session, String providerName) { - RealmModel testRealm = session.realms().getRealmByName(AbstractLDAPTest.TEST_REALM_NAME); + return init(session, AbstractLDAPTest.TEST_REALM_NAME, providerName); + } + + public static LDAPTestContext init(KeycloakSession session, String realmName, String providerName) { + RealmModel testRealm = session.realms().getRealmByName(realmName); ComponentModel ldapCompModel = LDAPTestUtils.getLdapProviderModel(testRealm, providerName); UserStorageProviderModel ldapModel = new UserStorageProviderModel(ldapCompModel); LDAPStorageProvider ldapProvider = LDAPTestUtils.getLdapProvider(session, ldapModel);