From bde65a6c57e279082e96fc33c68c4f1cd7b3fd3b Mon Sep 17 00:00:00 2001
From: mposolda <mposolda@gmail.com>
Date: Wed, 8 Jul 2015 20:01:29 +0200
Subject: [PATCH] KEYCLOAK-1533 Handle importing LDAP user with duplicate email
 during authentication

---
 .../UserAttributeLDAPFederationMapper.java    | 21 ++++++++++++-
 .../models/ModelDuplicateException.java       | 10 +++++++
 .../infinispan/ClientSessionAdapter.java      |  2 +-
 .../sessions/jpa/ClientSessionAdapter.java    |  2 +-
 .../sessions/mem/ClientSessionAdapter.java    |  2 +-
 .../sessions/mongo/ClientSessionAdapter.java  |  2 +-
 .../AbstractFormAuthenticator.java            | 30 ++++++++++++++++++-
 .../services/resources/AccountService.java    |  4 +--
 .../FederationProvidersIntegrationTest.java   | 23 ++++++++++++++
 9 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java
index 283089a0ba..438c57d729 100644
--- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java
+++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java
@@ -16,7 +16,9 @@ import org.keycloak.federation.ldap.idm.model.LDAPObject;
 import org.keycloak.federation.ldap.idm.query.Condition;
 import org.keycloak.federation.ldap.idm.query.QueryParameter;
 import org.keycloak.federation.ldap.idm.query.internal.LDAPQuery;
+import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.LDAPConstants;
+import org.keycloak.models.ModelDuplicateException;
 import org.keycloak.models.RealmModel;
 import org.keycloak.models.UserFederationMapperModel;
 import org.keycloak.models.UserFederationProvider;
@@ -74,6 +76,9 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
 
             // we have java property on UserModel
             String ldapAttrValue = ldapUser.getAttributeAsString(ldapAttrName);
+
+            checkDuplicateEmail(userModelAttrName, ldapAttrValue, realm, ldapProvider.getSession(), user);
+
             setPropertyOnUserModel(userModelProperty, user, ldapAttrValue);
         } else {
 
@@ -130,8 +135,20 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
         }
     }
 
+    // throw ModelDuplicateException if there is different user in model with same email
+    protected void checkDuplicateEmail(String userModelAttrName, String email, RealmModel realm, KeycloakSession session, UserModel user) {
+        if (UserModel.EMAIL.equalsIgnoreCase(userModelAttrName)) {
+            UserModel that = session.userStorage().getUserByEmail(email, realm);
+            if (that != null && !that.getId().equals(user.getId())) {
+                session.getTransaction().setRollbackOnly();
+                String exceptionMessage = String.format("Can't import user '%s' from LDAP because email '%s' already exists in Keycloak. Existing user with this email is '%s'", user.getUsername(), email, that.getUsername());
+                throw new ModelDuplicateException(exceptionMessage, UserModel.EMAIL);
+            }
+        }
+    }
+
     @Override
-    public UserModel proxy(UserFederationMapperModel mapperModel, LDAPFederationProvider ldapProvider, final LDAPObject ldapUser, UserModel delegate, RealmModel realm) {
+    public UserModel proxy(UserFederationMapperModel mapperModel, final LDAPFederationProvider ldapProvider, final LDAPObject ldapUser, UserModel delegate, final RealmModel realm) {
         final String userModelAttrName = mapperModel.getConfig().get(USER_MODEL_ATTRIBUTE);
         final String ldapAttrName = mapperModel.getConfig().get(LDAP_ATTRIBUTE);
         boolean isAlwaysReadValueFromLDAP = parseBooleanParameter(mapperModel, ALWAYS_READ_VALUE_FROM_LDAP);
@@ -162,6 +179,8 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
 
                 @Override
                 public void setEmail(String email) {
+                    checkDuplicateEmail(userModelAttrName, email, realm, ldapProvider.getSession(), this);
+
                     setLDAPAttribute(UserModel.EMAIL, email);
                     super.setEmail(email);
                 }
diff --git a/model/api/src/main/java/org/keycloak/models/ModelDuplicateException.java b/model/api/src/main/java/org/keycloak/models/ModelDuplicateException.java
index 1d6e69aa98..eb4776d365 100644
--- a/model/api/src/main/java/org/keycloak/models/ModelDuplicateException.java
+++ b/model/api/src/main/java/org/keycloak/models/ModelDuplicateException.java
@@ -5,6 +5,8 @@ package org.keycloak.models;
  */
 public class ModelDuplicateException extends ModelException {
 
+    private String duplicateFieldName;
+
     public ModelDuplicateException() {
     }
 
@@ -12,6 +14,11 @@ public class ModelDuplicateException extends ModelException {
         super(message);
     }
 
+    public ModelDuplicateException(String message, String duplicateFieldName) {
+        super(message);
+        this.duplicateFieldName = duplicateFieldName;
+    }
+
     public ModelDuplicateException(String message, Throwable cause) {
         super(message, cause);
     }
@@ -20,4 +27,7 @@ public class ModelDuplicateException extends ModelException {
         super(cause);
     }
 
+    public String getDuplicateFieldName() {
+        return duplicateFieldName;
+    }
 }
diff --git a/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/ClientSessionAdapter.java b/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/ClientSessionAdapter.java
index 66b75929cb..52ff887de6 100755
--- a/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/ClientSessionAdapter.java
+++ b/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/ClientSessionAdapter.java
@@ -223,7 +223,7 @@ public class ClientSessionAdapter implements ClientSessionModel {
 
     @Override
     public UserModel getAuthenticatedUser() {
-        return session.users().getUserById(entity.getAuthUserId(), realm);    }
+        return entity.getAuthUserId() == null ? null : session.users().getUserById(entity.getAuthUserId(), realm);    }
 
     @Override
     public void setAuthenticatedUser(UserModel user) {
diff --git a/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/ClientSessionAdapter.java b/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/ClientSessionAdapter.java
index e76811ea98..1de321eaeb 100755
--- a/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/ClientSessionAdapter.java
+++ b/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/ClientSessionAdapter.java
@@ -301,7 +301,7 @@ public class ClientSessionAdapter implements ClientSessionModel {
 
     @Override
     public UserModel getAuthenticatedUser() {
-        return session.users().getUserById(entity.getUserId(), realm);
+        return entity.getUserId() == null ? null : session.users().getUserById(entity.getUserId(), realm);
     }
 
     @Override
diff --git a/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/ClientSessionAdapter.java b/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/ClientSessionAdapter.java
index 458278a4fa..c54d341d11 100755
--- a/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/ClientSessionAdapter.java
+++ b/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/ClientSessionAdapter.java
@@ -189,7 +189,7 @@ public class ClientSessionAdapter implements ClientSessionModel {
 
     @Override
     public UserModel getAuthenticatedUser() {
-        return session.users().getUserById(entity.getAuthUserId(), realm);    }
+        return entity.getAuthUserId() == null ? null : session.users().getUserById(entity.getAuthUserId(), realm);    }
 
     @Override
     public void setAuthenticatedUser(UserModel user) {
diff --git a/model/sessions-mongo/src/main/java/org/keycloak/models/sessions/mongo/ClientSessionAdapter.java b/model/sessions-mongo/src/main/java/org/keycloak/models/sessions/mongo/ClientSessionAdapter.java
index 55013ed16e..7545dd09c3 100755
--- a/model/sessions-mongo/src/main/java/org/keycloak/models/sessions/mongo/ClientSessionAdapter.java
+++ b/model/sessions-mongo/src/main/java/org/keycloak/models/sessions/mongo/ClientSessionAdapter.java
@@ -205,7 +205,7 @@ public class ClientSessionAdapter extends AbstractMongoAdapter<MongoClientSessio
 
     @Override
     public UserModel getAuthenticatedUser() {
-        return session.users().getUserById(entity.getAuthUserId(), realm);
+        return entity.getAuthUserId() == null ? null : session.users().getUserById(entity.getAuthUserId(), realm);
     }
 
     @Override
diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/AbstractFormAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/AbstractFormAuthenticator.java
index 957c5407fe..98b906c44b 100755
--- a/services/src/main/java/org/keycloak/authentication/authenticators/AbstractFormAuthenticator.java
+++ b/services/src/main/java/org/keycloak/authentication/authenticators/AbstractFormAuthenticator.java
@@ -1,5 +1,6 @@
 package org.keycloak.authentication.authenticators;
 
+import org.jboss.logging.Logger;
 import org.keycloak.OAuth2Constants;
 import org.keycloak.authentication.AuthenticationProcessor;
 import org.keycloak.authentication.Authenticator;
@@ -7,6 +8,7 @@ import org.keycloak.authentication.AuthenticatorContext;
 import org.keycloak.events.Details;
 import org.keycloak.events.Errors;
 import org.keycloak.login.LoginFormsProvider;
+import org.keycloak.models.ModelDuplicateException;
 import org.keycloak.models.UserCredentialModel;
 import org.keycloak.models.UserModel;
 import org.keycloak.models.utils.KeycloakModelUtils;
@@ -27,6 +29,8 @@ import java.util.List;
  */
 public abstract class AbstractFormAuthenticator implements Authenticator {
 
+    private static final Logger logger = Logger.getLogger(AbstractFormAuthenticator.class);
+
     public static final String REGISTRATION_FORM_ACTION = "registration_form";
     public static final String EXECUTION = "execution";
     public static final String ATTEMPTED_USERNAME = "ATTEMPTED_USERNAME";
@@ -82,6 +86,14 @@ public abstract class AbstractFormAuthenticator implements Authenticator {
                 .setError(Messages.INVALID_USER).createLogin();
     }
 
+    protected Response setDuplicateUserChallenge(AuthenticatorContext context, String eventError, String loginFormError, AuthenticationProcessor.Error authenticatorError) {
+        context.getEvent().error(eventError);
+        Response challengeResponse = loginForm(context)
+                .setError(loginFormError).createLogin();
+        context.failureChallenge(authenticatorError, challengeResponse);
+        return challengeResponse;
+    }
+
     public boolean invalidUser(AuthenticatorContext context, UserModel user) {
         if (user == null) {
             context.getEvent().error(Errors.USER_NOT_FOUND);
@@ -118,7 +130,23 @@ public abstract class AbstractFormAuthenticator implements Authenticator {
         }
         context.getEvent().detail(Details.USERNAME, username);
         context.getClientSession().setNote(AbstractFormAuthenticator.ATTEMPTED_USERNAME, username);
-        UserModel user = KeycloakModelUtils.findUserByNameOrEmail(context.getSession(), context.getRealm(), username);
+
+        UserModel user = null;
+        try {
+            user = KeycloakModelUtils.findUserByNameOrEmail(context.getSession(), context.getRealm(), username);
+        } catch (ModelDuplicateException mde) {
+            logger.error(mde.getMessage(), mde);
+
+            // Could happen during federation import
+            if (mde.getDuplicateFieldName() != null && mde.getDuplicateFieldName().equals(UserModel.EMAIL)) {
+                setDuplicateUserChallenge(context, Errors.EMAIL_IN_USE, Messages.EMAIL_EXISTS, AuthenticationProcessor.Error.INVALID_USER);
+            } else {
+                setDuplicateUserChallenge(context, Errors.USERNAME_IN_USE, Messages.USERNAME_EXISTS, AuthenticationProcessor.Error.INVALID_USER);
+            }
+
+            return false;
+        }
+
         if (invalidUser(context, user)) return false;
         String rememberMe = inputData.getFirst("rememberMe");
         boolean remember = rememberMe != null && rememberMe.equalsIgnoreCase("on");
diff --git a/services/src/main/java/org/keycloak/services/resources/AccountService.java b/services/src/main/java/org/keycloak/services/resources/AccountService.java
index 3febb7436c..02762af48d 100755
--- a/services/src/main/java/org/keycloak/services/resources/AccountService.java
+++ b/services/src/main/java/org/keycloak/services/resources/AccountService.java
@@ -437,7 +437,7 @@ public class AccountService {
                 String username = formData.getFirst("username");
 
                 UserModel existing = session.users().getUserByUsername(username, realm);
-                if (existing != null && !existing.equals(user)) {
+                if (existing != null && !existing.getId().equals(user.getId())) {
                     throw new ModelDuplicateException(Messages.USERNAME_EXISTS);
                 }
 
@@ -451,7 +451,7 @@ public class AccountService {
             boolean emailChanged = oldEmail != null ? !oldEmail.equals(email) : email != null;
             if (emailChanged) {
                 UserModel existing = session.users().getUserByEmail(email, realm);
-                if (existing != null && !existing.equals(user)) {
+                if (existing != null && !existing.getId().equals(user.getId())) {
                     throw new ModelDuplicateException(Messages.EMAIL_EXISTS);
                 }
             }
diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java
index 1945701fb1..708da5b473 100755
--- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java
+++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java
@@ -416,6 +416,29 @@ public class FederationProvidersIntegrationTest {
         }
     }
 
+    @Test
+    public void testImportExistingUserFromLDAP() throws Exception {
+        // Add LDAP user with same email like existing model user
+        keycloakRule.update(new KeycloakRule.KeycloakSetup() {
+
+            @Override
+            public void config(RealmManager manager, RealmModel adminstrationRealm, RealmModel appRealm) {
+                LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel);
+                FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "mary", "Mary1", "Kelly1", "mary1@email.org", null, "123");
+                FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "mary-duplicatemail", "Mary2", "Kelly2", "mary@test.com", null, "123");
+            }
+
+        });
+
+        // Try to import the duplicated LDAP user into Keycloak
+        loginPage.open();
+        loginPage.login("mary-duplicatemail", "password");
+        Assert.assertEquals("Email already exists.", loginPage.getError());
+
+        loginPage.login("mary1@email.org", "password");
+        Assert.assertEquals("Username already exists.", loginPage.getError());
+    }
+
     @Test
     public void testReadonly() {
         KeycloakSession session = keycloakRule.startSession();