From 635310123e0e4cdcd159827a83b7c58ac96c13a1 Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 30 Nov 2015 22:46:34 +0100 Subject: [PATCH] KEYCLOAK-2167 EmailAsUsername not reflected during registration through broker --- .../IdpCreateUserIfUniqueAuthenticator.java | 23 +++++++--- .../broker/IdpReviewProfileAuthenticator.java | 5 ++- .../IdpReviewProfileAuthenticatorFactory.java | 3 +- .../SerializedBrokeredIdentityContext.java | 11 ++++- .../broker/AbstractFirstBrokerLoginTest.java | 42 ++++++++++++++++++ .../AbstractKeycloakIdentityProviderTest.java | 44 +------------------ .../OIDCKeyCloakServerBrokerBasicTest.java | 5 --- 7 files changed, 76 insertions(+), 57 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpCreateUserIfUniqueAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpCreateUserIfUniqueAuthenticator.java index b4ee957e43..f4da7e97d7 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpCreateUserIfUniqueAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpCreateUserIfUniqueAuthenticator.java @@ -41,13 +41,21 @@ public class IdpCreateUserIfUniqueAuthenticator extends AbstractIdpAuthenticator return; } - ExistingUserInfo duplication = checkExistingUser(context, serializedCtx, brokerContext); + String username = getUsername(context, serializedCtx, brokerContext); + if (username == null) { + logger.warnf("%s is null. Reset flow and enforce showing reviewProfile page", realm.isRegistrationEmailAsUsername() ? "Email" : "Username"); + context.getClientSession().setNote(ENFORCE_UPDATE_PROFILE, "true"); + context.resetFlow(); + return; + } + + ExistingUserInfo duplication = checkExistingUser(context, username, serializedCtx, brokerContext); if (duplication == null) { logger.debugf("No duplication detected. Creating account for user '%s' and linking with identity provider '%s' .", - brokerContext.getModelUsername(), brokerContext.getIdpConfig().getAlias()); + username, brokerContext.getIdpConfig().getAlias()); - UserModel federatedUser = session.users().addUser(realm, brokerContext.getModelUsername()); + UserModel federatedUser = session.users().addUser(realm, username); federatedUser.setEnabled(true); federatedUser.setEmail(brokerContext.getEmail()); federatedUser.setFirstName(brokerContext.getFirstName()); @@ -92,7 +100,7 @@ public class IdpCreateUserIfUniqueAuthenticator extends AbstractIdpAuthenticator } // Could be overriden to detect duplication based on other criterias (firstName, lastName, ...) - protected ExistingUserInfo checkExistingUser(AuthenticationFlowContext context, SerializedBrokeredIdentityContext serializedCtx, BrokeredIdentityContext brokerContext) { + protected ExistingUserInfo checkExistingUser(AuthenticationFlowContext context, String username, SerializedBrokeredIdentityContext serializedCtx, BrokeredIdentityContext brokerContext) { if (brokerContext.getEmail() != null) { UserModel existingUser = context.getSession().users().getUserByEmail(brokerContext.getEmail(), context.getRealm()); @@ -101,7 +109,7 @@ public class IdpCreateUserIfUniqueAuthenticator extends AbstractIdpAuthenticator } } - UserModel existingUser = context.getSession().users().getUserByUsername(brokerContext.getModelUsername(), context.getRealm()); + UserModel existingUser = context.getSession().users().getUserByUsername(username, context.getRealm()); if (existingUser != null) { return new ExistingUserInfo(existingUser.getId(), UserModel.USERNAME, existingUser.getUsername()); } @@ -109,6 +117,11 @@ public class IdpCreateUserIfUniqueAuthenticator extends AbstractIdpAuthenticator return null; } + protected String getUsername(AuthenticationFlowContext context, SerializedBrokeredIdentityContext serializedCtx, BrokeredIdentityContext brokerContext) { + RealmModel realm = context.getRealm(); + return realm.isRegistrationEmailAsUsername() ? brokerContext.getEmail() : brokerContext.getModelUsername(); + } + @Override public boolean requiresUser() { diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticator.java index 04eb77b520..a4d0ea345c 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticator.java @@ -83,7 +83,7 @@ public class IdpReviewProfileAuthenticator extends AbstractIdpAuthenticator { RealmModel realm = context.getRealm(); - List errors = Validation.validateUpdateProfileForm(true, formData); + List errors = Validation.validateUpdateProfileForm(!realm.isRegistrationEmailAsUsername(), formData); if (errors != null && !errors.isEmpty()) { Response challenge = context.form() .setErrors(errors) @@ -94,7 +94,8 @@ public class IdpReviewProfileAuthenticator extends AbstractIdpAuthenticator { return; } - userCtx.setUsername(formData.getFirst(UserModel.USERNAME)); + String username = realm.isRegistrationEmailAsUsername() ? formData.getFirst(UserModel.EMAIL) : formData.getFirst(UserModel.USERNAME); + userCtx.setUsername(username); userCtx.setFirstName(formData.getFirst(UserModel.FIRST_NAME)); userCtx.setLastName(formData.getFirst(UserModel.LAST_NAME)); diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticatorFactory.java b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticatorFactory.java index e10c924c5a..fc472d0b09 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticatorFactory.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpReviewProfileAuthenticatorFactory.java @@ -94,7 +94,8 @@ public class IdpReviewProfileAuthenticatorFactory implements AuthenticatorFactor property.setDefaultValue(updateProfileValues); property.setHelpText("Define conditions under which a user has to review and update his profile after first-time login. Value 'On' means that" + " page for reviewing profile will be displayed and user can review and update his profile. Value 'off' means that page won't be displayed." - + " Value 'missing' means that page is displayed just when some required attribute is missing (wasn't downloaded from identity provider). Value 'missing' is the default one"); + + " Value 'missing' means that page is displayed just when some required attribute is missing (wasn't downloaded from identity provider). Value 'missing' is the default one." + + " WARN: In case that user clicks 'Review profile info' on link duplications page, the update page will be always displayed. You would need to disable this authenticator to never display the page."); configProperties.add(property); } diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/broker/util/SerializedBrokeredIdentityContext.java b/services/src/main/java/org/keycloak/authentication/authenticators/broker/util/SerializedBrokeredIdentityContext.java index 8f1b026f0c..7e3990bbea 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/broker/util/SerializedBrokeredIdentityContext.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/broker/util/SerializedBrokeredIdentityContext.java @@ -37,13 +37,16 @@ public class SerializedBrokeredIdentityContext implements UpdateProfileContext { private String code; private String token; + @JsonIgnore + private boolean emailAsUsername; + private String identityProviderId; private Map contextData = new HashMap<>(); @JsonIgnore @Override public boolean isEditUsernameAllowed() { - return true; + return !emailAsUsername; } public String getId() { @@ -261,6 +264,8 @@ public class SerializedBrokeredIdentityContext implements UpdateProfileContext { ctx.setToken(context.getToken()); ctx.setIdentityProviderId(context.getIdpConfig().getAlias()); + ctx.emailAsUsername = context.getClientSession().getRealm().isRegistrationEmailAsUsername(); + IdentityProviderDataMarshaller serializer = context.getIdp().getMarshaller(); for (Map.Entry entry : context.getContextData().entrySet()) { @@ -289,7 +294,9 @@ public class SerializedBrokeredIdentityContext implements UpdateProfileContext { return null; } else { try { - return JsonSerialization.readValue(asString, SerializedBrokeredIdentityContext.class); + SerializedBrokeredIdentityContext serializedCtx = JsonSerialization.readValue(asString, SerializedBrokeredIdentityContext.class); + serializedCtx.emailAsUsername = clientSession.getRealm().isRegistrationEmailAsUsername(); + return serializedCtx; } catch (IOException ioe) { throw new RuntimeException(ioe); } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java index 7d780cd72f..36bf4c7908 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java @@ -178,6 +178,48 @@ public abstract class AbstractFirstBrokerLoginTest extends AbstractIdentityProvi } + /** + * Test user registers with IdentityProvider with emailAsUsername + */ + @Test + public void testRegistrationWithEmailAsUsername() { + // Require updatePassword after user registered with broker + brokerServerRule.update(new KeycloakRule.KeycloakSetup() { + + @Override + public void config(RealmManager manager, RealmModel adminstrationRealm, RealmModel realmWithBroker) { + setUpdateProfileFirstLogin(realmWithBroker, IdentityProviderRepresentation.UPFLM_ON); + realmWithBroker.setRegistrationEmailAsUsername(true); + } + + }, APP_REALM_ID); + + loginIDP("pedroigor"); + this.updateProfileWithUsernamePage.assertCurrent(); + + try { + this.updateProfileWithUsernamePage.update("Test", "User", "some-user@redhat.com", "some-user"); + Assert.fail("It is not expected to see username field"); + } catch (NoSuchElementException expected) { + } + + this.updateProfileWithUsernamePage.update("Test", "User", "some-user@redhat.com"); + + // assert authenticated + assertFederatedUser("some-user@redhat.com", "some-user@redhat.com", "pedroigor"); + + brokerServerRule.update(new KeycloakRule.KeycloakSetup() { + + @Override + public void config(RealmManager manager, RealmModel adminstrationRealm, RealmModel realmWithBroker) { + setUpdateProfileFirstLogin(realmWithBroker, IdentityProviderRepresentation.UPFLM_MISSING); + realmWithBroker.setRegistrationEmailAsUsername(false); + } + + }, APP_REALM_ID); + } + + /** * Tests that duplication is detected, the confirmation page is displayed, user clicks on "Review profile" and goes back to updateProfile page and resolves duplication * by create new user diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java index 1d2e5b68d4..f88cf0bbbb 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractKeycloakIdentityProviderTest.java @@ -266,48 +266,6 @@ public abstract class AbstractKeycloakIdentityProviderTest extends AbstractIdent } } - @Test - public void testSuccessfulAuthenticationWithoutUpdateProfile_newUser_emailAsUsername_emailNotProvided() { - - getRealm().setRegistrationEmailAsUsername(true); - brokerServerRule.stopSession(this.session, true); - this.session = brokerServerRule.startSession(); - - try { - IdentityProviderModel identityProviderModel = getIdentityProviderModel(); - setUpdateProfileFirstLogin(IdentityProviderRepresentation.UPFLM_OFF); - - authenticateWithIdentityProvider(identityProviderModel, "test-user-noemail", false); - - brokerServerRule.stopSession(session, true); - session = brokerServerRule.startSession(); - - // check correct user is created with username from provider as email is not available - RealmModel realm = getRealm(); - UserModel federatedUser = getFederatedUser(); - assertNotNull(federatedUser); - - doAssertFederatedUserNoEmail(federatedUser); - - Set federatedIdentities = this.session.users().getFederatedIdentities(federatedUser, realm); - - assertEquals(1, federatedIdentities.size()); - - FederatedIdentityModel federatedIdentityModel = federatedIdentities.iterator().next(); - - assertEquals(getProviderId(), federatedIdentityModel.getIdentityProvider()); - revokeGrant(); - - driver.navigate().to("http://localhost:8081/test-app/logout"); - driver.navigate().to("http://localhost:8081/test-app"); - - assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8081/auth/realms/realm-with-broker/protocol/openid-connect/auth")); - - } finally { - getRealm().setRegistrationEmailAsUsername(false); - } - } - @Test public void testDisabled() { IdentityProviderModel identityProviderModel = getIdentityProviderModel(); @@ -395,6 +353,8 @@ public abstract class AbstractKeycloakIdentityProviderTest extends AbstractIdent public void testAccountManagementLinkedIdentityAlreadyExists() { // Login as "test-user" through broker IdentityProviderModel identityProvider = getIdentityProviderModel(); + setUpdateProfileFirstLogin(IdentityProviderRepresentation.UPFLM_OFF); + assertSuccessfulAuthentication(identityProvider, "test-user", "test-user@localhost", false); // Login as pedroigor to account management diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java index 5bfdc1d75d..63b332bc30 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/OIDCKeyCloakServerBrokerBasicTest.java @@ -115,11 +115,6 @@ public class OIDCKeyCloakServerBrokerBasicTest extends AbstractKeycloakIdentityP super.testSuccessfulAuthenticationWithoutUpdateProfile_newUser_emailAsUsername(); } - @Test - public void testSuccessfulAuthenticationWithoutUpdateProfile_newUser_emailAsUsername_emailNotProvided() { - super.testSuccessfulAuthenticationWithoutUpdateProfile_newUser_emailAsUsername_emailNotProvided(); - } - @Test public void testTokenStorageAndRetrievalByApplication() { super.testTokenStorageAndRetrievalByApplication();