From bddf1adebabbe70e30f191725d3f327d5f5ea033 Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 8 Jul 2015 12:27:15 +0200 Subject: [PATCH 1/3] Improve description for object classes --- .../theme/base/admin/resources/partials/federated-ldap.html | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/forms/common-themes/src/main/resources/theme/base/admin/resources/partials/federated-ldap.html b/forms/common-themes/src/main/resources/theme/base/admin/resources/partials/federated-ldap.html index 7ebaa74a9c..51f3f98195 100755 --- a/forms/common-themes/src/main/resources/theme/base/admin/resources/partials/federated-ldap.html +++ b/forms/common-themes/src/main/resources/theme/base/admin/resources/partials/federated-ldap.html @@ -98,7 +98,8 @@
- All values of LDAP objectClass attribute for users in LDAP divided by comma + All values of LDAP objectClass attribute for users in LDAP divided by comma. For example: 'inetOrgPerson, organizationalPerson' . Newly created Keycloak users will be written to LDAP + with all those object classes and existing LDAP user records are found just if they contain all those object classes.
From 94c0a436b52ed600c3f290a2b0638a02389e6ec2 Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 8 Jul 2015 15:27:05 +0200 Subject: [PATCH 2/3] KEYCLOAK-1534 handle account management update email or username to the already existing value --- .../account/messages/messages_en.properties | 3 ++ .../services/resources/AccountService.java | 21 +++++++- .../testsuite/account/AccountTest.java | 53 +++++++++++++++++++ 3 files changed, 75 insertions(+), 2 deletions(-) diff --git a/forms/common-themes/src/main/resources/theme/base/account/messages/messages_en.properties b/forms/common-themes/src/main/resources/theme/base/account/messages/messages_en.properties index f783b49379..2c3b8c7e48 100755 --- a/forms/common-themes/src/main/resources/theme/base/account/messages/messages_en.properties +++ b/forms/common-themes/src/main/resources/theme/base/account/messages/messages_en.properties @@ -109,6 +109,9 @@ invalidPasswordExistingMessage=Invalid existing password. invalidPasswordConfirmMessage=Password confirmation doesn''t match. invalidTotpMessage=Invalid authenticator code. +usernameExistsMessage=Username already exists. +emailExistsMessage=Email already exists. + readOnlyUserMessage=You can''t update your account as it is read only. readOnlyPasswordMessage=You can''t update your password as your account is read only. 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 4ceebb399a..3febb7436c 100755 --- a/services/src/main/java/org/keycloak/services/resources/AccountService.java +++ b/services/src/main/java/org/keycloak/services/resources/AccountService.java @@ -42,6 +42,7 @@ import org.keycloak.models.Constants; import org.keycloak.models.FederatedIdentityModel; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.ModelException; import org.keycloak.models.ModelReadOnlyException; import org.keycloak.models.RealmModel; @@ -433,7 +434,14 @@ public class AccountService { try { if (realm.isEditUsernameAllowed()) { - user.setUsername(formData.getFirst("username")); + String username = formData.getFirst("username"); + + UserModel existing = session.users().getUserByUsername(username, realm); + if (existing != null && !existing.equals(user)) { + throw new ModelDuplicateException(Messages.USERNAME_EXISTS); + } + + user.setUsername(username); } user.setFirstName(formData.getFirst("firstName")); user.setLastName(formData.getFirst("lastName")); @@ -441,8 +449,14 @@ public class AccountService { String email = formData.getFirst("email"); String oldEmail = user.getEmail(); boolean emailChanged = oldEmail != null ? !oldEmail.equals(email) : email != null; + if (emailChanged) { + UserModel existing = session.users().getUserByEmail(email, realm); + if (existing != null && !existing.equals(user)) { + throw new ModelDuplicateException(Messages.EMAIL_EXISTS); + } + } - user.setEmail(formData.getFirst("email")); + user.setEmail(email); AttributeFormDataProcessor.process(formData, realm, user); @@ -457,6 +471,9 @@ public class AccountService { } catch (ModelReadOnlyException roe) { setReferrerOnPage(); return account.setError(Messages.READ_ONLY_USER).setProfileFormData(formData).createResponse(AccountPages.ACCOUNT); + } catch (ModelDuplicateException mde) { + setReferrerOnPage(); + return account.setError(mde.getMessage()).setProfileFormData(formData).createResponse(AccountPages.ACCOUNT); } } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/account/AccountTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/account/AccountTest.java index d621e466ab..1ea081f5ce 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/account/AccountTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/account/AccountTest.java @@ -82,6 +82,7 @@ public class AccountTest { UserModel user2 = manager.getSession().users().addUser(appRealm, "test-user-no-access@localhost"); user2.setEnabled(true); + user2.setEmail("test-user-no-access@localhost"); for (String r : accountApp.getDefaultRoles()) { user2.deleteRoleMapping(accountApp.getRole(r)); } @@ -395,6 +396,10 @@ public class AccountTest { events.expectAccount(EventType.UPDATE_PROFILE).assertEvent(); events.expectAccount(EventType.UPDATE_EMAIL).detail(Details.PREVIOUS_EMAIL, "test-user@localhost").detail(Details.UPDATED_EMAIL, "new@email.com").assertEvent(); + + // reset user for other tests + profilePage.updateProfile("Tom", "Brady", "test-user@localhost"); + events.clear(); } @Test @@ -429,6 +434,17 @@ public class AccountTest { events.assertEmpty(); + // Change to the username already occupied by other user + profilePage.updateProfile("test-user-no-access@localhost", "New first", "New last", "new@email.com"); + + Assert.assertEquals("Username already exists.", profilePage.getError()); + Assert.assertEquals("test-user-no-access@localhost", profilePage.getUsername()); + Assert.assertEquals("New first", profilePage.getFirstName()); + Assert.assertEquals("New last", profilePage.getLastName()); + Assert.assertEquals("new@email.com", profilePage.getEmail()); + + events.assertEmpty(); + profilePage.updateProfile("test-user-new@localhost", "New first", "New last", "new@email.com"); Assert.assertEquals("Your account has been updated.", profilePage.getSuccess()); @@ -452,6 +468,43 @@ public class AccountTest { } } + // KEYCLOAK-1534 + @Test + public void changeEmailToExisting() { + profilePage.open(); + loginPage.login("test-user@localhost", "password"); + + events.expectLogin().client("account").detail(Details.REDIRECT_URI, ACCOUNT_REDIRECT).assertEvent(); + + Assert.assertEquals("test-user@localhost", profilePage.getUsername()); + Assert.assertEquals("test-user@localhost", profilePage.getEmail()); + + // Change to the email, which some other user has + profilePage.updateProfile("New first", "New last", "test-user-no-access@localhost"); + + profilePage.assertCurrent(); + Assert.assertEquals("Email already exists.", profilePage.getError()); + Assert.assertEquals("New first", profilePage.getFirstName()); + Assert.assertEquals("New last", profilePage.getLastName()); + Assert.assertEquals("test-user-no-access@localhost", profilePage.getEmail()); + + events.assertEmpty(); + + // Change some other things, but not email + profilePage.updateProfile("New first", "New last", "test-user@localhost"); + + Assert.assertEquals("Your account has been updated.", profilePage.getSuccess()); + Assert.assertEquals("New first", profilePage.getFirstName()); + Assert.assertEquals("New last", profilePage.getLastName()); + Assert.assertEquals("test-user@localhost", profilePage.getEmail()); + + events.expectAccount(EventType.UPDATE_PROFILE).assertEvent(); + + // Change email and other things to original values + profilePage.updateProfile("Tom", "Brady", "test-user@localhost"); + events.expectAccount(EventType.UPDATE_PROFILE).assertEvent(); + } + @Test public void setupTotp() { totpPage.open(); From bde65a6c57e279082e96fc33c68c4f1cd7b3fd3b Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 8 Jul 2015 20:01:29 +0200 Subject: [PATCH 3/3] 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