diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index 15f4288424..bebc39db4a 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -78,6 +78,7 @@ import org.keycloak.services.managers.AuthenticationManager; import org.keycloak.services.managers.BruteForceProtector; import org.keycloak.services.managers.UserConsentManager; import org.keycloak.services.managers.UserSessionManager; +import org.keycloak.services.messages.Messages; import org.keycloak.services.resources.KeycloakOpenAPI; import org.keycloak.services.resources.LoginActionsService; import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator; @@ -241,6 +242,12 @@ public class UserResource { } catch (ValidationException pve) { List errors = new ArrayList<>(); for (ValidationException.Error error : pve.getErrors()) { + // some messages are managed directly as before + switch (error.getMessage()) { + case Messages.MISSING_USERNAME -> throw ErrorResponse.error("User name is missing", Response.Status.BAD_REQUEST); + case Messages.USERNAME_EXISTS -> throw ErrorResponse.exists("User exists with same username"); + case Messages.EMAIL_EXISTS -> throw ErrorResponse.exists("User exists with same email"); + } errors.add(new ErrorRepresentation(error.getAttribute(), error.getMessage(), error.getMessageParameters())); } 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 8ae54e7f0b..3abae22668 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 @@ -24,7 +24,6 @@ import org.jboss.logging.Logger; import org.jboss.resteasy.reactive.NoCache; import org.keycloak.common.ClientConnection; import org.keycloak.common.Profile; -import org.keycloak.common.util.ObjectUtil; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; import org.keycloak.models.Constants; @@ -133,23 +132,6 @@ public class UsersResource { if(realm.isRegistrationEmailAsUsername()) { username = rep.getEmail(); } - if (ObjectUtil.isBlank(username)) { - throw ErrorResponse.error("User name is missing", Response.Status.BAD_REQUEST); - } - - // Double-check duplicated username and email here due to federation - if (session.users().getUserByUsername(realm, username) != null) { - throw ErrorResponse.exists("User exists with same username"); - } - if (rep.getEmail() != null && !realm.isDuplicateEmailsAllowed()) { - try { - if(session.users().getUserByEmail(realm, rep.getEmail()) != null) { - throw ErrorResponse.exists("User exists with same email"); - } - } catch (ModelDuplicateException e) { - throw ErrorResponse.exists("User exists with same email"); - } - } UserProfileProvider profileProvider = session.getProvider(UserProfileProvider.class); diff --git a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProviderFactory.java b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProviderFactory.java index 4cc112f383..366706193b 100644 --- a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProviderFactory.java +++ b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProviderFactory.java @@ -440,9 +440,14 @@ public class DeclarativeUserProfileProviderFactory implements UserProfileProvide UserProfileMetadata metadata = new UserProfileMetadata(USER_API); - metadata.addAttribute(UserModel.USERNAME, -2, new AttributeValidatorMetadata(UsernameHasValueValidator.ID)) + metadata.addAttribute(UserModel.USERNAME, -2, + new AttributeValidatorMetadata(UsernameHasValueValidator.ID), + new AttributeValidatorMetadata(DuplicateUsernameValidator.ID)) .addWriteCondition(DeclarativeUserProfileProviderFactory::editUsernameCondition); - metadata.addAttribute(UserModel.EMAIL, -1, new AttributeValidatorMetadata(EmailValidator.ID, ValidatorConfig.builder().config(EmailValidator.IGNORE_EMPTY_VALUE, true).build())) + metadata.addAttribute(UserModel.EMAIL, -1, + new AttributeValidatorMetadata(DuplicateEmailValidator.ID), + new AttributeValidatorMetadata(EmailExistsAsUsernameValidator.ID), + new AttributeValidatorMetadata(EmailValidator.ID, ValidatorConfig.builder().config(EmailValidator.IGNORE_EMPTY_VALUE, true).build())) .addWriteCondition(DeclarativeUserProfileProviderFactory::editEmailCondition); List readonlyValidators = new ArrayList<>(); diff --git a/services/src/main/java/org/keycloak/userprofile/validator/DuplicateEmailValidator.java b/services/src/main/java/org/keycloak/userprofile/validator/DuplicateEmailValidator.java index 0fd29478a8..5b8e6cab9b 100644 --- a/services/src/main/java/org/keycloak/userprofile/validator/DuplicateEmailValidator.java +++ b/services/src/main/java/org/keycloak/userprofile/validator/DuplicateEmailValidator.java @@ -70,6 +70,13 @@ public class DuplicateEmailValidator implements SimpleValidator { if (userByEmail != null && (user == null || !userByEmail.getId().equals(user.getId()))) { context.addError(new ValidationError(ID, inputHint, Messages.EMAIL_EXISTS) .setStatusCode(Response.Status.CONFLICT)); + } else if (realm.isLoginWithEmailAllowed()) { + // check for duplicated username + userByEmail = session.users().getUserByUsername(realm, value); + if (userByEmail != null && (user == null || !userByEmail.getId().equals(user.getId()))) { + context.addError(new ValidationError(ID, inputHint, Messages.EMAIL_EXISTS) + .setStatusCode(Response.Status.CONFLICT)); + } } } diff --git a/services/src/main/java/org/keycloak/userprofile/validator/DuplicateUsernameValidator.java b/services/src/main/java/org/keycloak/userprofile/validator/DuplicateUsernameValidator.java index 206ec31d16..c6f1a12524 100644 --- a/services/src/main/java/org/keycloak/userprofile/validator/DuplicateUsernameValidator.java +++ b/services/src/main/java/org/keycloak/userprofile/validator/DuplicateUsernameValidator.java @@ -64,12 +64,21 @@ public class DuplicateUsernameValidator implements SimpleValidator { KeycloakSession session = context.getSession(); UserModel existing = session.users().getUserByUsername(session.getContext().getRealm(), value); UserModel user = UserProfileAttributeValidationContext.from(context).getAttributeContext().getUser(); + String valueLowercased = value.toLowerCase(); - if (! KeycloakModelUtils.isUsernameCaseSensitive(session.getContext().getRealm())) value = value.toLowerCase(); + if (! KeycloakModelUtils.isUsernameCaseSensitive(session.getContext().getRealm())) value = valueLowercased; - if (user != null && !value.equals(user.getFirstAttribute(UserModel.USERNAME)) && (existing != null && !existing.getId().equals(user.getId()))) { + RealmModel realm = session.getContext().getRealm(); + if (existing != null && (user == null || !existing.getId().equals(user.getId()))) { context.addError(new ValidationError(ID, inputHint, Messages.USERNAME_EXISTS) .setStatusCode(Response.Status.CONFLICT)); + } else if (realm.isLoginWithEmailAllowed() && value.indexOf('@') > 0) { + // check the username does not collide with an email + existing = session.users().getUserByEmail(realm, valueLowercased); + if (existing != null && (user == null || !existing.getId().equals(user.getId()))) { + context.addError(new ValidationError(ID, inputHint, Messages.USERNAME_EXISTS) + .setStatusCode(Response.Status.CONFLICT)); + } } return context; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java index 5481983996..1841d9f5fb 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java @@ -332,6 +332,56 @@ public class RequiredActionUpdateProfileTest extends AbstractTestRealmKeycloakTe events.assertEmpty(); } + @Test + public void updateProfileDuplicateUsernameWithEmail() { + getCleanup().addUserId(createUser(TEST_REALM_NAME, "user1@local.com", "password", "user1", "user1", "user1@local.org")); + + loginPage.open(); + + loginPage.login("john-doh@localhost", "password"); + + updateProfilePage.assertCurrent(); + + updateProfilePage.prepareUpdate().username("user1@local.org").firstName("New first").lastName("New last").email("new@email.com").submit(); + + updateProfilePage.assertCurrent(); + + // assert that form holds submitted values during validation error + Assert.assertEquals("New first", updateProfilePage.getFirstName()); + Assert.assertEquals("New last", updateProfilePage.getLastName()); + Assert.assertEquals("new@email.com", updateProfilePage.getEmail()); + Assert.assertEquals("user1@local.org", updateProfilePage.getUsername()); + + Assert.assertEquals("Username already exists.", updateProfilePage.getInputErrors().getUsernameError()); + + events.assertEmpty(); + } + + @Test + public void updateProfileDuplicatedEmailWithUsername() { + getCleanup().addUserId(createUser(TEST_REALM_NAME, "user1@local.com", "password", "user1", "user1", "user1@local.org")); + + loginPage.open(); + + loginPage.login("test-user@localhost", "password"); + + updateProfilePage.assertCurrent(); + + updateProfilePage.prepareUpdate().username("test-user@localhost").firstName("New first").lastName("New last") + .email("user1@local.com").submit(); + + updateProfilePage.assertCurrent(); + + // assert that form holds submitted values during validation error + Assert.assertEquals("New first", updateProfilePage.getFirstName()); + Assert.assertEquals("New last", updateProfilePage.getLastName()); + Assert.assertEquals("user1@local.com", updateProfilePage.getEmail()); + + Assert.assertEquals("Email already exists.", updateProfilePage.getInputErrors().getEmailError()); + + events.assertEmpty(); + } + @Test public void updateProfileExpiredCookies() { loginPage.open(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java index 6aebb7cbea..7b7f56dcf9 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java @@ -325,6 +325,39 @@ public class UserTest extends AbstractAdminTest { } } + @Test + public void createDuplicatedUsernameWithEmail() { + createUser("user1@local.com", "user1@local.org"); + + UserRepresentation user = new UserRepresentation(); + user.setUsername("user1@local.org"); + user.setEmail("user2@localhost"); + try (Response response = realm.users().create(user)) { + assertEquals(409, response.getStatus()); + assertAdminEvents.assertEmpty(); + + ErrorRepresentation error = response.readEntity(ErrorRepresentation.class); + Assert.assertEquals("User exists with same username", error.getErrorMessage()); + } + } + + @Test + public void createDuplicatedEmailWithUsername() { + createUser("user1@local.com", "user1@local.org"); + + UserRepresentation user = new UserRepresentation(); + user.setUsername("user2"); + user.setEmail("user1@local.com"); + + try (Response response = realm.users().create(user)) { + assertEquals(409, response.getStatus()); + assertAdminEvents.assertEmpty(); + + ErrorRepresentation error = response.readEntity(ErrorRepresentation.class); + Assert.assertEquals("User exists with same email", error.getErrorMessage()); + } + } + //KEYCLOAK-14611 @Test public void createDuplicateEmailWithExistingDuplicates() { @@ -352,7 +385,7 @@ public class UserTest extends AbstractAdminTest { try (Response response = realm.users().create(user)) { assertEquals(409, response.getStatus()); ErrorRepresentation error = response.readEntity(ErrorRepresentation.class); - Assert.assertEquals("User exists with same email", error.getErrorMessage()); + Assert.assertEquals("User exists with same username or email", error.getErrorMessage()); assertAdminEvents.assertEmpty(); } } @@ -2619,7 +2652,7 @@ public class UserTest extends AbstractAdminTest { assertThat(e.getResponse().getStatus(), is(409)); ErrorRepresentation error = e.getResponse().readEntity(ErrorRepresentation.class); - Assert.assertEquals("User exists with same username or email", error.getErrorMessage()); + Assert.assertEquals("User exists with same email", error.getErrorMessage()); assertAdminEvents.assertEmpty(); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/kerberos/KerberosStandaloneTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/kerberos/KerberosStandaloneTest.java index a561ba073a..f9a8c848ad 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/kerberos/KerberosStandaloneTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/kerberos/KerberosStandaloneTest.java @@ -26,23 +26,20 @@ import org.keycloak.testsuite.Assert; import org.junit.ClassRule; import org.junit.Test; import org.keycloak.admin.client.resource.UserResource; -import org.keycloak.common.Profile; import org.keycloak.common.constants.KerberosConstants; import org.keycloak.federation.kerberos.CommonKerberosConfig; import org.keycloak.federation.kerberos.KerberosConfig; import org.keycloak.federation.kerberos.KerberosFederationProviderFactory; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.ComponentRepresentation; -import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.representations.idm.ErrorRepresentation; import org.keycloak.representations.idm.UserProfileAttributeMetadata; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.storage.UserStorageProvider; import org.keycloak.testsuite.ActionURIUtils; import org.keycloak.testsuite.KerberosEmbeddedServer; import org.keycloak.testsuite.admin.ApiUtil; -import org.keycloak.testsuite.arquillian.annotation.EnableFeature; import org.keycloak.testsuite.arquillian.annotation.UncaughtServerErrorExpected; -import org.keycloak.testsuite.forms.VerifyProfileTest; import org.keycloak.testsuite.util.KerberosRule; import static org.keycloak.userprofile.UserProfileUtil.USER_METADATA_GROUP; @@ -181,7 +178,9 @@ public class KerberosStandaloneTest extends AbstractKerberosSingleRealmTest { UserRepresentation john = new UserRepresentation(); john.setUsername("john"); Response response = testRealmResource().users().create(john); - Assert.assertEquals(500, response.getStatus()); + Assert.assertEquals(400, response.getStatus()); + ErrorRepresentation error = response.readEntity(ErrorRepresentation.class); + Assert.assertEquals("Could not create user", error.getErrorMessage()); response.close(); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java index 507ad80c3d..b090a611a4 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java @@ -202,7 +202,7 @@ public class UserProfileTest extends AbstractUserProfileTest { private static void testCustomAttributeInAnyContext(KeycloakSession session) { Map attributes = new HashMap<>(); - attributes.put(UserModel.USERNAME, "profiled-user"); + attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId()); attributes.put(UserModel.FIRST_NAME, "John"); attributes.put(UserModel.LAST_NAME, "Doe"); attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); @@ -243,7 +243,7 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); - attributes.put(UserModel.USERNAME, "profiled-user"); + attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); attributes.put(UserModel.FIRST_NAME, "John"); attributes.put(UserModel.LAST_NAME, "Doe"); attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org");