From 94c0a436b52ed600c3f290a2b0638a02389e6ec2 Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 8 Jul 2015 15:27:05 +0200 Subject: [PATCH] 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();