diff --git a/forms/common-themes/src/main/resources/theme/admin/base/resources/partials/user-detail.html b/forms/common-themes/src/main/resources/theme/admin/base/resources/partials/user-detail.html index 892e8a8e92..f0b32d4b7e 100755 --- a/forms/common-themes/src/main/resources/theme/admin/base/resources/partials/user-detail.html +++ b/forms/common-themes/src/main/resources/theme/admin/base/resources/partials/user-detail.html @@ -26,12 +26,19 @@
* Required fields
+
+ +
+ +
+
+
+ required ng-pattern="/^[^\<\>\\\/]*$/">
diff --git a/model/api/src/main/java/org/keycloak/models/UserModel.java b/model/api/src/main/java/org/keycloak/models/UserModel.java index 762acc8f8f..b01cf93e1f 100755 --- a/model/api/src/main/java/org/keycloak/models/UserModel.java +++ b/model/api/src/main/java/org/keycloak/models/UserModel.java @@ -17,6 +17,8 @@ public interface UserModel { String getLoginName(); + void setLoginName(String loginName); + boolean isEnabled(); boolean isTotp(); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaKeycloakTransaction.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaKeycloakTransaction.java index a300b0f7e7..095b3414a1 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaKeycloakTransaction.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaKeycloakTransaction.java @@ -27,7 +27,7 @@ public class JpaKeycloakTransaction implements KeycloakTransaction { try { em.getTransaction().commit(); } catch (PersistenceException e) { - throw PersistenceExceptionConverter.convert(e.getCause()); + throw PersistenceExceptionConverter.convert(e.getCause() != null ? e.getCause() : e); } } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java index 54559ab881..d76524dede 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java @@ -454,9 +454,6 @@ public class RealmAdapter implements RealmModel { @Override public UserModel addUser(String username) { - if (getUser(username) != null) { - throw new RuntimeException("Username already exists: " + username); - } UserEntity entity = new UserEntity(); entity.setLoginName(username); entity.setRealm(realm); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java index 50daa11449..98570625c9 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java @@ -34,6 +34,11 @@ public class UserAdapter implements UserModel { return user.getLoginName(); } + @Override + public void setLoginName(String loginName) { + user.setLoginName(loginName); + } + @Override public boolean isEnabled() { return user.isEnabled(); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserEntity.java index 1ffba3ba80..f7ef212c4a 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/UserEntity.java @@ -10,12 +10,15 @@ import javax.persistence.ElementCollection; import javax.persistence.Entity; import javax.persistence.GeneratedValue; import javax.persistence.Id; +import javax.persistence.JoinColumn; import javax.persistence.ManyToOne; import javax.persistence.MapKeyColumn; import javax.persistence.NamedQueries; import javax.persistence.NamedQuery; import javax.persistence.OneToMany; import javax.persistence.OneToOne; +import javax.persistence.Table; +import javax.persistence.UniqueConstraint; import java.util.ArrayList; import java.util.Collection; @@ -35,6 +38,10 @@ import java.util.Set; @NamedQuery(name="getRealmUserByFirstLastName", query="select u from UserEntity u where u.firstName = :first and u.lastName = :last and u.realm = :realm") }) @Entity +@Table(uniqueConstraints = { + @UniqueConstraint(columnNames = { "realm", "loginName" }), + @UniqueConstraint(columnNames = { "realm", "email" }) +}) public class UserEntity { @Id @GenericGenerator(name="uuid_generator", strategy="org.keycloak.models.jpa.utils.JpaIdGenerator") @@ -52,6 +59,7 @@ public class UserEntity { @ManyToOne + @JoinColumn(name = "realm") protected RealmEntity realm; @ElementCollection diff --git a/model/mongo/src/main/java/org/keycloak/models/mongo/api/MongoIndex.java b/model/mongo/src/main/java/org/keycloak/models/mongo/api/MongoIndex.java index 48b6d09698..ed44b2b91b 100644 --- a/model/mongo/src/main/java/org/keycloak/models/mongo/api/MongoIndex.java +++ b/model/mongo/src/main/java/org/keycloak/models/mongo/api/MongoIndex.java @@ -23,4 +23,6 @@ public @interface MongoIndex { boolean unique() default false; + boolean sparse() default false; + } diff --git a/model/mongo/src/main/java/org/keycloak/models/mongo/impl/MongoStoreImpl.java b/model/mongo/src/main/java/org/keycloak/models/mongo/impl/MongoStoreImpl.java index 483128dbb0..445f77796c 100755 --- a/model/mongo/src/main/java/org/keycloak/models/mongo/impl/MongoStoreImpl.java +++ b/model/mongo/src/main/java/org/keycloak/models/mongo/impl/MongoStoreImpl.java @@ -139,7 +139,7 @@ public class MongoStoreImpl implements MongoStore { protected void createIndex(DBCollection dbCollection, MongoIndex index) { BasicDBObject fields = new BasicDBObject(); - for (String f : index.fields()) { + for (String f : index.fields()) { fields.put(f, 1); } String name = index.name(); @@ -147,10 +147,22 @@ public class MongoStoreImpl implements MongoStore { name = null; } boolean unique = index.unique(); + boolean sparse = index.sparse(); - dbCollection.ensureIndex(fields, name, unique); + BasicDBObject options = new BasicDBObject(); + if (name != null) { + options.put("name", name); + } + if (unique) { + options.put("unique", unique); + } + if (sparse) { + options.put("sparse", sparse); + } - logger.debug("Created index " + fields + (unique ? " (unique)" : "") + " on " + dbCollection.getName() + " in " + this.database.getName()); + dbCollection.ensureIndex(fields, options); + + logger.debug("Created index " + fields + "(options: " + options + ") on " + dbCollection.getName() + " in " + this.database.getName()); } @Override diff --git a/model/mongo/src/main/java/org/keycloak/models/mongo/impl/types/MongoEntityMapper.java b/model/mongo/src/main/java/org/keycloak/models/mongo/impl/types/MongoEntityMapper.java index d62deac409..1d3d665af2 100755 --- a/model/mongo/src/main/java/org/keycloak/models/mongo/impl/types/MongoEntityMapper.java +++ b/model/mongo/src/main/java/org/keycloak/models/mongo/impl/types/MongoEntityMapper.java @@ -39,8 +39,10 @@ public class MongoEntityMapper implements Mapper implements R // Add just user entity without defaultRoles protected UserAdapter addUserEntity(String username) { - if (getUser(username) != null) { - throw new IllegalArgumentException("User " + username + " already exists"); - } - UserEntity userEntity = new UserEntity(); userEntity.setLoginName(username); // Compatibility with JPA model, which has user disabled by default diff --git a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/UserAdapter.java b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/UserAdapter.java index 3ae2502717..608082e761 100755 --- a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/UserAdapter.java +++ b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/UserAdapter.java @@ -34,6 +34,12 @@ public class UserAdapter extends AbstractMongoAdapter implements Use return user.getLoginName(); } + @Override + public void setLoginName(String loginName) { + user.setLoginName(loginName); + updateUser(); + } + @Override public boolean isEnabled() { return user.isEnabled(); diff --git a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/entities/UserEntity.java b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/entities/UserEntity.java index 5aeaceeec8..752d2e926e 100755 --- a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/entities/UserEntity.java +++ b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/entities/UserEntity.java @@ -5,6 +5,8 @@ import org.keycloak.models.mongo.api.AbstractMongoIdentifiableEntity; import org.keycloak.models.mongo.api.MongoCollection; import org.keycloak.models.mongo.api.MongoEntity; import org.keycloak.models.mongo.api.MongoField; +import org.keycloak.models.mongo.api.MongoIndex; +import org.keycloak.models.mongo.api.MongoIndexes; import java.util.ArrayList; import java.util.List; @@ -14,6 +16,10 @@ import java.util.Map; * @author Marek Posolda */ @MongoCollection(collectionName = "users") +@MongoIndexes({ + @MongoIndex(name = "loginName-within-realm", fields = { "realmId", "loginName" }, unique = true), + @MongoIndex(name = "email-within-realm", fields = { "emailRealm" }, unique = true, sparse = true), +}) public class UserEntity extends AbstractMongoIdentifiableEntity implements MongoEntity { private String loginName; @@ -76,6 +82,15 @@ public class UserEntity extends AbstractMongoIdentifiableEntity implements Mongo this.email = email; } + @MongoField + // TODO This is required as Mongo doesn't support sparse indexes with compound keys (see https://jira.mongodb.org/browse/SERVER-2193) + public String getEmailRealm() { + return email != null ? realmId + "//" + email : null; + } + + public void setEmailRealm(String emailRealm) { + } + @MongoField public boolean isEmailVerified() { return emailVerified; diff --git a/model/tests/src/test/java/org/keycloak/model/test/AbstractModelTest.java b/model/tests/src/test/java/org/keycloak/model/test/AbstractModelTest.java index 8438bb3781..2f7177b215 100755 --- a/model/tests/src/test/java/org/keycloak/model/test/AbstractModelTest.java +++ b/model/tests/src/test/java/org/keycloak/model/test/AbstractModelTest.java @@ -99,6 +99,10 @@ public class AbstractModelTest { } else { identitySession.getTransaction().commit(); } + resetSession(); + } + + protected void resetSession() { identitySession.close(); identitySession = factory.createSession(); identitySession.getTransaction().begin(); diff --git a/model/tests/src/test/java/org/keycloak/model/test/AdapterTest.java b/model/tests/src/test/java/org/keycloak/model/test/AdapterTest.java index a6c23151d4..45ba227ae3 100755 --- a/model/tests/src/test/java/org/keycloak/model/test/AdapterTest.java +++ b/model/tests/src/test/java/org/keycloak/model/test/AdapterTest.java @@ -267,7 +267,7 @@ public class AdapterTest extends AbstractModelTest { UserModel user3 = realmModel.addUser("doublelast"); user3.setFirstName("Ole"); user3.setLastName("Alver Veland"); - user3.setEmail("knut@redhat.com"); + user3.setEmail("knut2@redhat.com"); } RealmManager adapter = realmManager; @@ -531,9 +531,7 @@ public class AdapterTest extends AbstractModelTest { } catch (ModelDuplicateException e) { } - identitySession.close(); - identitySession = factory.createSession(); - identitySession.getTransaction().begin(); + resetSession(); } @Test @@ -562,9 +560,7 @@ public class AdapterTest extends AbstractModelTest { } catch (ModelDuplicateException e) { } - identitySession.close(); - identitySession = factory.createSession(); - identitySession.getTransaction().begin(); + resetSession(); } @Test @@ -593,9 +589,64 @@ public class AdapterTest extends AbstractModelTest { } catch (ModelDuplicateException e) { } - identitySession.close(); - identitySession = factory.createSession(); - identitySession.getTransaction().begin(); + resetSession(); + } + + @Test + public void testUsernameCollisions() throws Exception { + realmManager.createRealm("JUGGLER1").addUser("user1"); + realmManager.createRealm("JUGGLER2").addUser("user1"); + commit(); + + // Try to create user with duplicate login name + try { + realmManager.getRealmByName("JUGGLER1").addUser("user1"); + commit(); + Assert.fail("Expected exception"); + } catch (ModelDuplicateException e) { + } + commit(true); + + // Ty to rename user to duplicate login name + realmManager.getRealmByName("JUGGLER1").addUser("user2"); + commit(); + try { + realmManager.getRealmByName("JUGGLER1").getUser("user2").setLoginName("user1"); + commit(); + Assert.fail("Expected exception"); + } catch (ModelDuplicateException e) { + } + + resetSession(); + } + + @Test + public void testEmailCollisions() throws Exception { + realmManager.createRealm("JUGGLER1").addUser("user1").setEmail("email@example.com"); + realmManager.createRealm("JUGGLER2").addUser("user1").setEmail("email@example.com"); + commit(); + + // Try to create user with duplicate email + try { + realmManager.getRealmByName("JUGGLER1").addUser("user2").setEmail("email@example.com"); + commit(); + Assert.fail("Expected exception"); + } catch (ModelDuplicateException e) { + } + + resetSession(); + + // Ty to rename user to duplicate email + realmManager.getRealmByName("JUGGLER1").addUser("user3").setEmail("email2@example.com"); + commit(); + try { + realmManager.getRealmByName("JUGGLER1").getUser("user3").setEmail("email@example.com"); + commit(); + Assert.fail("Expected exception"); + } catch (ModelDuplicateException e) { + } + + resetSession(); } } diff --git a/model/tests/src/test/resources/testcomposites.json b/model/tests/src/test/resources/testcomposites.json index 61038ea65b..9b0878447e 100755 --- a/model/tests/src/test/resources/testcomposites.json +++ b/model/tests/src/test/resources/testcomposites.json @@ -18,7 +18,7 @@ { "username" : "REALM_COMPOSITE_1_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user1@localhost", "credentials" : [ { "type" : "password", "value" : "password" } @@ -27,7 +27,7 @@ { "username" : "REALM_ROLE_1_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user2@localhost", "credentials" : [ { "type" : "password", "value" : "password" } @@ -36,7 +36,7 @@ { "username" : "REALM_APP_COMPOSITE_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user3@localhost", "credentials" : [ { "type" : "password", "value" : "password" } @@ -45,7 +45,7 @@ { "username" : "REALM_APP_ROLE_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user4@localhost", "credentials" : [ { "type" : "password", "value" : "password" } @@ -54,7 +54,7 @@ { "username" : "APP_COMPOSITE_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user5@localhost", "credentials" : [ { "type" : "password", "value" : "password" } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java index 0ceb0b8600..8857e44988 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java @@ -8,6 +8,7 @@ import org.keycloak.models.ApplicationModel; import org.keycloak.models.ClientModel; import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; import org.keycloak.models.UserCredentialModel; @@ -84,14 +85,20 @@ public class UsersResource { @Path("{username}") @PUT @Consumes("application/json") - public void updateUser(final @PathParam("username") String username, final UserRepresentation rep) { + public Response updateUser(final @PathParam("username") String username, final UserRepresentation rep) { auth.requireManage(); - UserModel user = realm.getUser(username); - if (user == null) { - throw new NotFoundException("User not found"); + try { + UserModel user = realm.getUser(username); + if (user == null) { + throw new NotFoundException("User not found"); + } + updateUserFromRep(user, rep); + + return Response.noContent().build(); + } catch (ModelDuplicateException e) { + return Flows.errors().exists("User exists with same username or email"); } - updateUserFromRep(user, rep); } @POST @@ -99,17 +106,14 @@ public class UsersResource { public Response createUser(final @Context UriInfo uriInfo, final UserRepresentation rep) { auth.requireManage(); - if (realm.getUser(rep.getUsername()) != null) { - return Flows.errors().exists("User with username " + rep.getUsername() + " already exists"); - } - UserModel user = realm.addUser(rep.getUsername()); - if (user == null) { - throw new NotFoundException("User not found"); - } + try { + UserModel user = realm.addUser(rep.getUsername()); + updateUserFromRep(user, rep); - updateUserFromRep(user, rep); - - return Response.created(uriInfo.getAbsolutePathBuilder().path(user.getLoginName()).build()).build(); + return Response.created(uriInfo.getAbsolutePathBuilder().path(user.getLoginName()).build()).build(); + } catch (ModelDuplicateException e) { + return Flows.errors().exists("User exists with same username or email"); + } } private void updateUserFromRep(UserModel user, UserRepresentation rep) { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java index aeea46283b..429e5f90a9 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java @@ -75,12 +75,12 @@ public class RegisterTest { loginPage.clickRegister(); registerPage.assertCurrent(); - registerPage.register("firstName", "lastName", "email", "test-user@localhost", "password", "password"); + registerPage.register("firstName", "lastName", "registerExistingUseremail", "test-user@localhost", "password", "password"); registerPage.assertCurrent(); Assert.assertEquals("Username already exists", registerPage.getError()); - events.expectRegister("test-user@localhost", "email").user((String) null).error("username_in_use").assertEvent(); + events.expectRegister("test-user@localhost", "registerExistingUseremail").user((String) null).error("username_in_use").assertEvent(); } @Test @@ -89,12 +89,12 @@ public class RegisterTest { loginPage.clickRegister(); registerPage.assertCurrent(); - registerPage.register("firstName", "lastName", "email", "registerUserInvalidPasswordConfirm", "password", "invalid"); + registerPage.register("firstName", "lastName", "registerUserInvalidPasswordConfirmemail", "registerUserInvalidPasswordConfirm", "password", "invalid"); registerPage.assertCurrent(); Assert.assertEquals("Password confirmation doesn't match", registerPage.getError()); - events.expectRegister("registerUserInvalidPasswordConfirm", "email").user((String) null).error("invalid_registration").assertEvent(); + events.expectRegister("registerUserInvalidPasswordConfirm", "registerUserInvalidPasswordConfirmemail").user((String) null).error("invalid_registration").assertEvent(); } @Test @@ -103,12 +103,12 @@ public class RegisterTest { loginPage.clickRegister(); registerPage.assertCurrent(); - registerPage.register("firstName", "lastName", "email", "registerUserMissingPassword", null, null); + registerPage.register("firstName", "lastName", "registerUserMissingPasswordemail", "registerUserMissingPassword", null, null); registerPage.assertCurrent(); Assert.assertEquals("Please specify password.", registerPage.getError()); - events.expectRegister("registerUserMissingPassword", "email").user((String) null).error("invalid_registration").assertEvent(); + events.expectRegister("registerUserMissingPassword", "registerUserMissingPasswordemail").user((String) null).error("invalid_registration").assertEvent(); } @Test @@ -125,17 +125,17 @@ public class RegisterTest { loginPage.clickRegister(); registerPage.assertCurrent(); - registerPage.register("firstName", "lastName", "email", "registerPasswordPolicy", "pass", "pass"); + registerPage.register("firstName", "lastName", "registerPasswordPolicyemail", "registerPasswordPolicy", "pass", "pass"); registerPage.assertCurrent(); Assert.assertEquals("Invalid password: minimum length 8", registerPage.getError()); - events.expectRegister("registerPasswordPolicy", "email").user((String) null).error("invalid_registration").assertEvent(); + events.expectRegister("registerPasswordPolicy", "registerPasswordPolicyemail").user((String) null).error("invalid_registration").assertEvent(); - registerPage.register("firstName", "lastName", "email", "registerPasswordPolicy", "password", "password"); + registerPage.register("firstName", "lastName", "registerPasswordPolicyemail", "registerPasswordPolicy", "password", "password"); Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); - String userId = events.expectRegister("registerPasswordPolicy", "email").assertEvent().getUserId(); + String userId = events.expectRegister("registerPasswordPolicy", "registerPasswordPolicyemail").assertEvent().getUserId(); events.expectLogin().user(userId).detail(Details.USERNAME, "registerPasswordPolicy").assertEvent(); } finally { @@ -154,12 +154,12 @@ public class RegisterTest { loginPage.clickRegister(); registerPage.assertCurrent(); - registerPage.register("firstName", "lastName", "email", null, "password", "password"); + registerPage.register("firstName", "lastName", "registerUserMissingUsernameemail", null, "password", "password"); registerPage.assertCurrent(); Assert.assertEquals("Please specify username", registerPage.getError()); - events.expectRegister(null, "email").removeDetail("username").error("invalid_registration").assertEvent(); + events.expectRegister(null, "registerUserMissingUsernameemail").removeDetail("username").error("invalid_registration").assertEvent(); } @Test @@ -168,11 +168,11 @@ public class RegisterTest { loginPage.clickRegister(); registerPage.assertCurrent(); - registerPage.register("firstName", "lastName", "email", "registerUserSuccess", "password", "password"); + registerPage.register("firstName", "lastName", "registerUserSuccessemail", "registerUserSuccess", "password", "password"); Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); - String userId = events.expectRegister("registerUserSuccess", "email").assertEvent().getUserId(); + String userId = events.expectRegister("registerUserSuccess", "registerUserSuccessemail").assertEvent().getUserId(); events.expectLogin().detail("username", "registerUserSuccess").user(userId).assertEvent(); } diff --git a/testsuite/integration/src/test/resources/testcomposite.json b/testsuite/integration/src/test/resources/testcomposite.json index 61038ea65b..9b0878447e 100755 --- a/testsuite/integration/src/test/resources/testcomposite.json +++ b/testsuite/integration/src/test/resources/testcomposite.json @@ -18,7 +18,7 @@ { "username" : "REALM_COMPOSITE_1_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user1@localhost", "credentials" : [ { "type" : "password", "value" : "password" } @@ -27,7 +27,7 @@ { "username" : "REALM_ROLE_1_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user2@localhost", "credentials" : [ { "type" : "password", "value" : "password" } @@ -36,7 +36,7 @@ { "username" : "REALM_APP_COMPOSITE_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user3@localhost", "credentials" : [ { "type" : "password", "value" : "password" } @@ -45,7 +45,7 @@ { "username" : "REALM_APP_ROLE_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user4@localhost", "credentials" : [ { "type" : "password", "value" : "password" } @@ -54,7 +54,7 @@ { "username" : "APP_COMPOSITE_USER", "enabled": true, - "email" : "test-user@localhost", + "email" : "test-user5@localhost", "credentials" : [ { "type" : "password", "value" : "password" }