From 630e3b23123b4b51a3a0e76f8ab5c4a42a58bb3c Mon Sep 17 00:00:00 2001 From: rmartinc Date: Fri, 23 Jun 2023 13:07:18 +0200 Subject: [PATCH] Revert emailVerified to false if email modified on force-sync non-trusted broker Closes https://github.com/keycloak/security/issues/48 --- .../resources/IdentityBrokerService.java | 14 +++++- .../testsuite/broker/KcOidcBrokerTest.java | 44 ++++++++++++++++--- 2 files changed, 50 insertions(+), 8 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 d9b998c6e6..24bcdad71c 100755 --- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java +++ b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java @@ -1018,7 +1018,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal } private void setBasicUserAttributes(BrokeredIdentityContext context, UserModel federatedUser) { - setDiffAttrToConsumer(federatedUser.getEmail(), context.getEmail(), federatedUser::setEmail); + setDiffAttrToConsumer(federatedUser.getEmail(), context.getEmail(), email -> setEmail(context, federatedUser, email)); setDiffAttrToConsumer(federatedUser.getFirstName(), context.getFirstName(), federatedUser::setFirstName); setDiffAttrToConsumer(federatedUser.getLastName(), context.getLastName(), federatedUser::setLastName); } @@ -1030,6 +1030,18 @@ 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 + 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); + } else { + logger.tracef("Email verified reset to false after updating user '%s' through Identity provider '%s' ", federatedUser.getUsername(), context.getIdpConfig().getAlias()); + federatedUser.setEmailVerified(false); + } + } + private void migrateFederatedIdentityId(BrokeredIdentityContext context, UserModel federatedUser) { FederatedIdentityModel identityModel = this.session.users().getFederatedIdentity(this.realmModel, federatedUser, context.getIdpConfig().getAlias()); FederatedIdentityModel migratedIdentityModel = new FederatedIdentityModel(identityModel, context.getId()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java index 9745bc23a4..b1a4fe79c0 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTest.java @@ -504,12 +504,22 @@ public final class KcOidcBrokerTest extends AbstractAdvancedBrokerTest { @Test public void testIdPForceSyncUserAttributes() { - checkUpdatedUserAttributesIdP(true); + checkUpdatedUserAttributesIdP(true, false); + } + + @Test + public void testIdPForceSyncTrustEmailUserAttributes() { + checkUpdatedUserAttributesIdP(true, true); } @Test public void testIdPNotForceSyncUserAttributes() { - checkUpdatedUserAttributesIdP(false); + checkUpdatedUserAttributesIdP(false, false); + } + + @Test + public void testIdPNotForceSyncTrustEmailUserAttributes() { + checkUpdatedUserAttributesIdP(false, true); } @Test @@ -593,7 +603,7 @@ public final class KcOidcBrokerTest extends AbstractAdvancedBrokerTest { assertThat("Claim value didn't change", idProvider.getConfig().get(IdentityProviderModel.CLAIM_FILTER_VALUE), Matchers.equalTo(claimFilterValue)); } - private void checkUpdatedUserAttributesIdP(boolean isForceSync) { + private void checkUpdatedUserAttributesIdP(boolean isForceSync, boolean isTrustEmail) { final String IDP_NAME = getBrokerConfiguration().getIDPAlias(); final String USERNAME = "demo-user"; final String PASSWORD = "demo-pwd"; @@ -612,7 +622,8 @@ public final class KcOidcBrokerTest extends AbstractAdvancedBrokerTest { UsersResource providerUsersResource = providerRealmResource.users(); - String providerUserID = createUser(bc.providerRealmName(), USERNAME, PASSWORD, FIRST_NAME, LAST_NAME, EMAIL); + String providerUserID = createUser(bc.providerRealmName(), USERNAME, PASSWORD, FIRST_NAME, LAST_NAME, EMAIL, + user -> user.setEmailVerified(true)); UserResource providerUserResource = providerUsersResource.get(providerUserID); try { @@ -620,8 +631,9 @@ public final class KcOidcBrokerTest extends AbstractAdvancedBrokerTest { IdentityProviderRepresentation idProvider = consumerIdentityResource.toRepresentation(); updateIdPSyncMode(idProvider, consumerIdentityResource, - isForceSync ? IdentityProviderSyncMode.FORCE : IdentityProviderSyncMode.IMPORT); + isForceSync ? IdentityProviderSyncMode.FORCE : IdentityProviderSyncMode.IMPORT, isTrustEmail); + // login to create the user in the consumer realm oauth.clientId("broker-app"); loginPage.open(bc.consumerRealmName()); @@ -646,17 +658,28 @@ public final class KcOidcBrokerTest extends AbstractAdvancedBrokerTest { UserResource consumerUserResource = consumerRealmResource.users().get(consumerUserID); checkFederatedIdentityLink(consumerUserResource, providerUserID, USERNAME); + assertThat(consumerUserResource.toRepresentation().isEmailVerified(), Matchers.equalTo(isTrustEmail)); AccountHelper.logout(adminClient.realm(bc.consumerRealmName()), USERNAME); AccountHelper.logout(adminClient.realm(bc.providerRealmName()), USERNAME); + // set email verified to true on the consumer resource + consumerUser = consumerUserResource.toRepresentation(); + consumerUser.setEmailVerified(true); + consumerUserResource.update(consumerUser); + consumerUserResource = consumerRealmResource.users().get(consumerUserID); + assertThat(consumerUserResource.toRepresentation().isEmailVerified(), Matchers.is(true)); + + // modify provider user with the new values UserRepresentation providerUser = providerUserResource.toRepresentation(); providerUser.setUsername(NEW_USERNAME); providerUser.setFirstName(NEW_FIRST_NAME); providerUser.setLastName(NEW_LAST_NAME); providerUser.setEmail(NEW_EMAIL); + providerUser.setEmailVerified(true); providerUserResource.update(providerUser); + // login again to force sync if force mode oauth.clientId("broker-app"); loginPage.open(bc.consumerRealmName()); @@ -674,7 +697,10 @@ public final class KcOidcBrokerTest extends AbstractAdvancedBrokerTest { assertThat(userRepresentation.getFirstName(), Matchers.equalTo(isForceSync ? NEW_FIRST_NAME : FIRST_NAME)); assertThat(userRepresentation.getLastName(), Matchers.equalTo(isForceSync ? NEW_LAST_NAME : LAST_NAME)); + consumerUserResource = consumerRealmResource.users().get(consumerUserID); checkFederatedIdentityLink(consumerUserResource, providerUserID, isForceSync ? NEW_USERNAME : USERNAME); + // the email verified should be reverted to false if force-sync and not trust-email + assertThat(consumerUserResource.toRepresentation().isEmailVerified(), Matchers.equalTo(!isForceSync || isTrustEmail)); } finally { providerUsersResource.delete(providerUserID); } @@ -695,21 +721,25 @@ public final class KcOidcBrokerTest extends AbstractAdvancedBrokerTest { assertThat(federatedIdentity.getUserName(), Matchers.equalTo(username)); } - private void updateIdPSyncMode(IdentityProviderRepresentation idProvider, IdentityProviderResource idProviderResource, IdentityProviderSyncMode syncMode) { + private void updateIdPSyncMode(IdentityProviderRepresentation idProvider, IdentityProviderResource idProviderResource, + IdentityProviderSyncMode syncMode, boolean trustEmail) { assertThat(idProvider, Matchers.notNullValue()); assertThat(idProviderResource, Matchers.notNullValue()); assertThat(syncMode, Matchers.notNullValue()); - if (idProvider.getConfig().get(IdentityProviderModel.SYNC_MODE).equals(syncMode.name())) { + if (idProvider.getConfig().get(IdentityProviderModel.SYNC_MODE).equals(syncMode.name()) + && idProvider.isTrustEmail() == trustEmail) { return; } idProvider.getConfig().put(IdentityProviderModel.SYNC_MODE, syncMode.name()); + idProvider.setTrustEmail(trustEmail); idProviderResource.update(idProvider); idProvider = idProviderResource.toRepresentation(); assertThat("Cannot get Identity Provider", idProvider, Matchers.notNullValue()); assertThat("Sync mode didn't change", idProvider.getConfig().get(IdentityProviderModel.SYNC_MODE), Matchers.equalTo(syncMode.name())); + assertThat("TrustEmail didn't change", idProvider.isTrustEmail(), Matchers.equalTo(trustEmail)); } private UserRepresentation getFederatedIdentity() {