From fde4bc99aa5481c5d99e0dc8bd01b1c8d1d5fdf7 Mon Sep 17 00:00:00 2001 From: dcampagna Date: Thu, 17 Sep 2015 12:55:32 +0200 Subject: [PATCH 1/6] Allow username change at first login (KEYCLOAK-1849) --- .../theme/base/login/login-update-profile.ftl | 9 +++ .../login/freemarker/model/RealmBean.java | 4 ++ .../requiredactions/UpdateProfile.java | 6 +- .../org/keycloak/testsuite/AssertEvents.java | 25 +++++++ .../RequiredActionMultipleActionsTest.java | 2 +- .../RequiredActionUpdateProfileTest.java | 68 ++++++++++++++++--- .../broker/AbstractIdentityProviderTest.java | 7 +- .../pages/LoginUpdateProfilePage.java | 11 ++- .../src/test/resources/testrealm.json | 19 +++++- 9 files changed, 132 insertions(+), 19 deletions(-) diff --git a/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl b/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl index 22575087ffc..93ec4fe1d64 100755 --- a/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl +++ b/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl @@ -6,6 +6,15 @@ ${msg("loginProfileTitle")} <#elseif section = "form">
+
style="display:none"> +
+ +
+
+ +
+
+
diff --git a/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/model/RealmBean.java b/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/model/RealmBean.java index b161ad21fdf..e6ae21de988 100755 --- a/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/model/RealmBean.java +++ b/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/model/RealmBean.java @@ -66,6 +66,10 @@ public class RealmBean { return realm.isInternationalizationEnabled(); } + public boolean isEditUsernameAllowed() { + return realm.isEditUsernameAllowed(); + } + public boolean isPassword() { for (RequiredCredentialModel r : realm.getRequiredCredentials()) { if (r.getType().equals(CredentialRepresentation.PASSWORD)) { diff --git a/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateProfile.java b/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateProfile.java index 42c2e023481..ac7862b7ad7 100755 --- a/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateProfile.java +++ b/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateProfile.java @@ -52,7 +52,7 @@ public class UpdateProfile implements RequiredActionProvider, RequiredActionFact RealmModel realm = context.getRealm(); - List errors = Validation.validateUpdateProfileForm(formData); + List errors = Validation.validateUpdateProfileForm(realm, formData); if (errors != null && !errors.isEmpty()) { Response challenge = context.form() .setErrors(errors) @@ -62,6 +62,10 @@ public class UpdateProfile implements RequiredActionProvider, RequiredActionFact return; } + if (realm.isEditUsernameAllowed()) { + user.setUsername(formData.getFirst("username")); + } + user.setFirstName(formData.getFirst("firstName")); user.setLastName(formData.getFirst("lastName")); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/AssertEvents.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/AssertEvents.java index 77b6d19f6ff..f16dcee7dbc 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/AssertEvents.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/AssertEvents.java @@ -123,6 +123,10 @@ public class AssertEvents implements TestRule, EventListenerProviderFactory { return expectLogin().event(event).removeDetail(Details.CONSENT).session(isUUID()); } + public ExpectedEvent expectRequiredActionEnabledUsername(EventType event, String newUsername) { + return expectLogin(newUsername).event(event).removeDetail(Details.CONSENT).session(isUUID()); + } + public ExpectedEvent expectLogin() { return expect(EventType.LOGIN) .detail(Details.CODE_ID, isCodeId()) @@ -134,6 +138,17 @@ public class AssertEvents implements TestRule, EventListenerProviderFactory { .session(isUUID()); } + public ExpectedEvent expectLogin(String username) { + return expect(EventType.LOGIN, username) + .detail(Details.CODE_ID, isCodeId()) + //.detail(Details.USERNAME, DEFAULT_USERNAME) + //.detail(Details.AUTH_METHOD, OIDCLoginProtocol.LOGIN_PROTOCOL) + //.detail(Details.AUTH_TYPE, AuthorizationEndpoint.CODE_AUTH_TYPE) + .detail(Details.REDIRECT_URI, DEFAULT_REDIRECT_URI) + .detail(Details.CONSENT, Details.CONSENT_VALUE_NO_CONSENT_REQUIRED) + .session(isUUID()); + } + public ExpectedEvent expectClientLogin() { return expect(EventType.CLIENT_LOGIN) .detail(Details.CODE_ID, isCodeId()) @@ -202,6 +217,16 @@ public class AssertEvents implements TestRule, EventListenerProviderFactory { .event(event); } + public ExpectedEvent expect(EventType event, String username) { + return new ExpectedEvent() + .realm(DEFAULT_REALM) + .client(DEFAULT_CLIENT_ID) + .user(keycloak.getUser(DEFAULT_REALM, username).getId()) + .ipAddress(DEFAULT_IP_ADDRESS) + .session((String) null) + .event(event); + } + @Override public EventListenerProvider create(KeycloakSession session) { return new EventListenerProvider() { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionMultipleActionsTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionMultipleActionsTest.java index 868a76cc502..87a5640aeb5 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionMultipleActionsTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionMultipleActionsTest.java @@ -121,7 +121,7 @@ public class RequiredActionMultipleActionsTest { } public String updateProfile(String sessionId) { - updateProfilePage.update("New first", "New last", "new@email.com"); + updateProfilePage.update("New first", "New last", "new@email.com", "test-user@localhost"); AssertEvents.ExpectedEvent expectedEvent = events.expectRequiredAction(EventType.UPDATE_EMAIL).detail(Details.PREVIOUS_EMAIL, "test-user@localhost").detail(Details.UPDATED_EMAIL, "new@email.com"); if (sessionId != null) { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java index e3d6ac921d1..57ce53c1cd6 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java @@ -21,11 +21,7 @@ */ package org.keycloak.testsuite.actions; -import org.junit.Assert; -import org.junit.Before; -import org.junit.ClassRule; -import org.junit.Rule; -import org.junit.Test; +import org.junit.*; import org.keycloak.events.Details; import org.keycloak.events.EventType; import org.keycloak.models.RealmModel; @@ -75,6 +71,8 @@ public class RequiredActionUpdateProfileTest { public void config(RealmManager manager, RealmModel defaultRealm, RealmModel appRealm) { UserModel user = manager.getSession().users().getUserByUsername("test-user@localhost", appRealm); user.addRequiredAction(UserModel.RequiredAction.UPDATE_PROFILE); + UserModel anotherUser = manager.getSession().users().getUserByEmail("john-doh@localhost", appRealm); + anotherUser.addRequiredAction(UserModel.RequiredAction.UPDATE_PROFILE); } }); } @@ -87,7 +85,7 @@ public class RequiredActionUpdateProfileTest { updateProfilePage.assertCurrent(); - updateProfilePage.update("New first", "New last", "new@email.com"); + updateProfilePage.update("New first", "New last", "new@email.com", "test-user@localhost"); String sessionId = events.expectRequiredAction(EventType.UPDATE_EMAIL).detail(Details.PREVIOUS_EMAIL, "test-user@localhost").detail(Details.UPDATED_EMAIL, "new@email.com").assertEvent().getSessionId(); events.expectRequiredAction(EventType.UPDATE_PROFILE).session(sessionId).assertEvent(); @@ -101,6 +99,31 @@ public class RequiredActionUpdateProfileTest { Assert.assertEquals("New first", user.getFirstName()); Assert.assertEquals("New last", user.getLastName()); Assert.assertEquals("new@email.com", user.getEmail()); + Assert.assertEquals("test-user@localhost", user.getUsername()); + } + + @Test + public void updateUsername() { + loginPage.open(); + + loginPage.login("john-doh@localhost", "password"); + + updateProfilePage.assertCurrent(); + + updateProfilePage.update("New first", "New last", "john-doh@localhost", "new"); + + String sessionId = events.expectRequiredActionEnabledUsername(EventType.UPDATE_PROFILE, "new").assertEvent().getSessionId(); + + Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); + + events.expectLogin("new").session(sessionId).assertEvent(); + + // assert user is really updated in persistent store + UserRepresentation user = keycloakRule.getUser("test", "new"); + Assert.assertEquals("New first", user.getFirstName()); + Assert.assertEquals("New last", user.getLastName()); + Assert.assertEquals("john-doh@localhost", user.getEmail()); + Assert.assertEquals("new", user.getUsername()); } @Test @@ -111,7 +134,7 @@ public class RequiredActionUpdateProfileTest { updateProfilePage.assertCurrent(); - updateProfilePage.update("", "New last", "new@email.com"); + updateProfilePage.update("", "New last", "new@email.com", "new"); updateProfilePage.assertCurrent(); @@ -133,7 +156,7 @@ public class RequiredActionUpdateProfileTest { updateProfilePage.assertCurrent(); - updateProfilePage.update("New first", "", "new@email.com"); + updateProfilePage.update("New first", "", "new@email.com", "new"); updateProfilePage.assertCurrent(); @@ -155,7 +178,7 @@ public class RequiredActionUpdateProfileTest { updateProfilePage.assertCurrent(); - updateProfilePage.update("New first", "New last", ""); + updateProfilePage.update("New first", "New last", "", "new"); updateProfilePage.assertCurrent(); @@ -177,7 +200,7 @@ public class RequiredActionUpdateProfileTest { updateProfilePage.assertCurrent(); - updateProfilePage.update("New first", "New last", "invalidemail"); + updateProfilePage.update("New first", "New last", "invalidemail", "invalid"); updateProfilePage.assertCurrent(); @@ -191,6 +214,29 @@ public class RequiredActionUpdateProfileTest { events.assertEmpty(); } + @Test + public void updateProfileMissingUsername() { + loginPage.open(); + + loginPage.login("john-doh@localhost", "password"); + + updateProfilePage.assertCurrent(); + + updateProfilePage.update("New first", "New last", "new@email.com", ""); + + 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("", updateProfilePage.getUsername()); + + Assert.assertEquals("Please specify username.", updateProfilePage.getError()); + + events.assertEmpty(); + } + @Test public void updateProfileDuplicatedEmail() { loginPage.open(); @@ -199,7 +245,7 @@ public class RequiredActionUpdateProfileTest { updateProfilePage.assertCurrent(); - updateProfilePage.update("New first", "New last", "keycloak-user@localhost"); + updateProfilePage.update("New first", "New last", "keycloak-user@localhost", "test-user@localhost"); updateProfilePage.assertCurrent(); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java index fb2b5dfd4bf..9037e2b4a00 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java @@ -17,7 +17,6 @@ */ package org.keycloak.testsuite.broker; -import org.codehaus.jackson.map.ObjectMapper; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -452,7 +451,7 @@ public abstract class AbstractIdentityProviderTest { doAfterProviderAuthentication(); this.updateProfilePage.assertCurrent(); - this.updateProfilePage.update("Test", "User", "psilva@redhat.com"); + this.updateProfilePage.update("Test", "User", "psilva@redhat.com", "psilva"); WebElement element = this.driver.findElement(By.className("kc-feedback-text")); @@ -461,7 +460,7 @@ public abstract class AbstractIdentityProviderTest { assertEquals("Email already exists.", element.getText()); this.updateProfilePage.assertCurrent(); - this.updateProfilePage.update("Test", "User", "test-user@redhat.com"); + this.updateProfilePage.update("Test", "User", "test-user@redhat.com", "test-user"); assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8081/test-app")); @@ -725,7 +724,7 @@ public abstract class AbstractIdentityProviderTest { // update profile this.updateProfilePage.assertCurrent(); - this.updateProfilePage.update(userFirstName, userLastName, userEmail); + this.updateProfilePage.update(userFirstName, userLastName, userEmail, username); } } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java index d67862c7928..f1bbe5f477b 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java @@ -38,19 +38,24 @@ public class LoginUpdateProfilePage extends AbstractPage { @FindBy(id = "email") private WebElement emailInput; + @FindBy(id = "username") + private WebElement usernameInput; + @FindBy(css = "input[type=\"submit\"]") private WebElement submitButton; @FindBy(className = "feedback-error") private WebElement loginErrorMessage; - public void update(String firstName, String lastName, String email) { + public void update(String firstName, String lastName, String email, String username) { firstNameInput.clear(); firstNameInput.sendKeys(firstName); lastNameInput.clear(); lastNameInput.sendKeys(lastName); emailInput.clear(); emailInput.sendKeys(email); + usernameInput.clear(); + usernameInput.sendKeys(username); submitButton.click(); } @@ -70,6 +75,10 @@ public class LoginUpdateProfilePage extends AbstractPage { return emailInput.getAttribute("value"); } + public String getUsername() { + return usernameInput.getAttribute("value"); + } + public boolean isCurrent() { return driver.getTitle().equals("Update Account Information"); } diff --git a/testsuite/integration/src/test/resources/testrealm.json b/testsuite/integration/src/test/resources/testrealm.json index 8ad29ccfcf8..5344ad0e741 100755 --- a/testsuite/integration/src/test/resources/testrealm.json +++ b/testsuite/integration/src/test/resources/testrealm.json @@ -5,6 +5,7 @@ "sslRequired": "external", "registrationAllowed": true, "resetPasswordAllowed": true, + "editUsernameAllowed" : true, "privateKey": "MIICXAIBAAKBgQCrVrCuTtArbgaZzL1hvh0xtL5mc7o0NqPVnYXkLvgcwiC3BjLGw1tGEGoJaXDuSaRllobm53JBhjx33UNv+5z/UMG4kytBWxheNVKnL6GgqlNabMaFfPLPCF8kAgKnsi79NMo+n6KnSY8YeUmec/p2vjO2NjsSAVcWEQMVhJ31LwIDAQABAoGAfmO8gVhyBxdqlxmIuglbz8bcjQbhXJLR2EoS8ngTXmN1bo2L90M0mUKSdc7qF10LgETBzqL8jYlQIbt+e6TH8fcEpKCjUlyq0Mf/vVbfZSNaVycY13nTzo27iPyWQHK5NLuJzn1xvxxrUeXI6A2WFpGEBLbHjwpx5WQG9A+2scECQQDvdn9NE75HPTVPxBqsEd2z10TKkl9CZxu10Qby3iQQmWLEJ9LNmy3acvKrE3gMiYNWb6xHPKiIqOR1as7L24aTAkEAtyvQOlCvr5kAjVqrEKXalj0Tzewjweuxc0pskvArTI2Oo070h65GpoIKLc9jf+UA69cRtquwP93aZKtW06U8dQJAF2Y44ks/mK5+eyDqik3koCI08qaC8HYq2wVl7G2QkJ6sbAaILtcvD92ToOvyGyeE0flvmDZxMYlvaZnaQ0lcSQJBAKZU6umJi3/xeEbkJqMfeLclD27XGEFoPeNrmdx0q10Azp4NfJAY+Z8KRyQCR2BEG+oNitBOZ+YXF9KCpH3cdmECQHEigJhYg+ykOvr1aiZUMFT72HU0jnmQe2FVekuG+LJUt2Tm7GtMjTFoGpf0JwrVuZN39fOYAlo+nTixgeW7X8Y=", "publicKey": "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCrVrCuTtArbgaZzL1hvh0xtL5mc7o0NqPVnYXkLvgcwiC3BjLGw1tGEGoJaXDuSaRllobm53JBhjx33UNv+5z/UMG4kytBWxheNVKnL6GgqlNabMaFfPLPCF8kAgKnsi79NMo+n6KnSY8YeUmec/p2vjO2NjsSAVcWEQMVhJ31LwIDAQAB", "requiredCredentials": [ "password" ], @@ -32,7 +33,23 @@ } }, { - "username" : "keycloak-user@localhost", + "username" : "john-doh@localhost", + "enabled": true, + "email" : "john-doh@localhost", + "firstName": "John", + "lastName": "Doh", + "credentials" : [ + { "type" : "password", + "value" : "password" } + ], + "realmRoles": ["user"], + "clientRoles": { + "test-app": [ "customer-user" ], + "account": [ "view-profile", "manage-account" ] + } + }, + { + "username" : "keycloak-user@localhost", "enabled": true, "email" : "keycloak-user@localhost", "credentials" : [ From f7a8bbbcc8c305a19db694cb97e7033f6e261db0 Mon Sep 17 00:00:00 2001 From: dcampagna Date: Thu, 17 Sep 2015 15:16:23 +0200 Subject: [PATCH 2/6] Allow username change at first login (KEYCLOAK-1857) --- .../theme/base/login/login-update-profile.ftl | 17 +++++++------ .../org/keycloak/testsuite/AssertEvents.java | 25 ------------------- .../RequiredActionUpdateProfileTest.java | 14 +++++++++-- 3 files changed, 21 insertions(+), 35 deletions(-) diff --git a/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl b/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl index 93ec4fe1d64..803d4b2af8e 100755 --- a/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl +++ b/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl @@ -6,15 +6,16 @@ ${msg("loginProfileTitle")} <#elseif section = "form"> -
style="display:none"> -
- + <#if !realm.editUsernameAllowed> +
+
+ +
+
+ +
-
- -
-
- +
diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/AssertEvents.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/AssertEvents.java index f16dcee7dbc..77b6d19f6ff 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/AssertEvents.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/AssertEvents.java @@ -123,10 +123,6 @@ public class AssertEvents implements TestRule, EventListenerProviderFactory { return expectLogin().event(event).removeDetail(Details.CONSENT).session(isUUID()); } - public ExpectedEvent expectRequiredActionEnabledUsername(EventType event, String newUsername) { - return expectLogin(newUsername).event(event).removeDetail(Details.CONSENT).session(isUUID()); - } - public ExpectedEvent expectLogin() { return expect(EventType.LOGIN) .detail(Details.CODE_ID, isCodeId()) @@ -138,17 +134,6 @@ public class AssertEvents implements TestRule, EventListenerProviderFactory { .session(isUUID()); } - public ExpectedEvent expectLogin(String username) { - return expect(EventType.LOGIN, username) - .detail(Details.CODE_ID, isCodeId()) - //.detail(Details.USERNAME, DEFAULT_USERNAME) - //.detail(Details.AUTH_METHOD, OIDCLoginProtocol.LOGIN_PROTOCOL) - //.detail(Details.AUTH_TYPE, AuthorizationEndpoint.CODE_AUTH_TYPE) - .detail(Details.REDIRECT_URI, DEFAULT_REDIRECT_URI) - .detail(Details.CONSENT, Details.CONSENT_VALUE_NO_CONSENT_REQUIRED) - .session(isUUID()); - } - public ExpectedEvent expectClientLogin() { return expect(EventType.CLIENT_LOGIN) .detail(Details.CODE_ID, isCodeId()) @@ -217,16 +202,6 @@ public class AssertEvents implements TestRule, EventListenerProviderFactory { .event(event); } - public ExpectedEvent expect(EventType event, String username) { - return new ExpectedEvent() - .realm(DEFAULT_REALM) - .client(DEFAULT_CLIENT_ID) - .user(keycloak.getUser(DEFAULT_REALM, username).getId()) - .ipAddress(DEFAULT_IP_ADDRESS) - .session((String) null) - .event(event); - } - @Override public EventListenerProvider create(KeycloakSession session) { return new EventListenerProvider() { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java index 57ce53c1cd6..d5c341d2a5f 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java @@ -108,15 +108,25 @@ public class RequiredActionUpdateProfileTest { loginPage.login("john-doh@localhost", "password"); + String userId = keycloakRule.getUser("test", "john-doh@localhost").getId(); + updateProfilePage.assertCurrent(); updateProfilePage.update("New first", "New last", "john-doh@localhost", "new"); - String sessionId = events.expectRequiredActionEnabledUsername(EventType.UPDATE_PROFILE, "new").assertEvent().getSessionId(); + String sessionId = events + .expectLogin() + .event(EventType.UPDATE_PROFILE) + .detail(Details.USERNAME, "john-doh@localhost") + .user(userId) + .session(AssertEvents.isUUID()) + .removeDetail(Details.CONSENT) + .assertEvent() + .getSessionId(); Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); - events.expectLogin("new").session(sessionId).assertEvent(); + events.expectLogin().detail(Details.USERNAME, "john-doh@localhost").user(userId).session(sessionId).assertEvent(); // assert user is really updated in persistent store UserRepresentation user = keycloakRule.getUser("test", "new"); From fe2278b320f5b5e57b3df872dd05122fc02c8ff2 Mon Sep 17 00:00:00 2001 From: dcampagna Date: Thu, 17 Sep 2015 17:19:13 +0200 Subject: [PATCH 3/6] Fix broken tests (KEYCLOAK-1857) --- .../theme/base/login/login-update-profile.ftl | 2 +- .../RequiredActionMultipleActionsTest.java | 7 +-- .../RequiredActionUpdateProfileTest.java | 4 +- .../broker/AbstractIdentityProviderTest.java | 6 +-- ...nUpdateProfileEditUsernameAllowedPage.java | 54 +++++++++++++++++++ .../pages/LoginUpdateProfilePage.java | 11 +--- 6 files changed, 63 insertions(+), 21 deletions(-) create mode 100644 testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfileEditUsernameAllowedPage.java diff --git a/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl b/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl index 803d4b2af8e..584bea322b9 100755 --- a/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl +++ b/forms/common-themes/src/main/resources/theme/base/login/login-update-profile.ftl @@ -6,7 +6,7 @@ ${msg("loginProfileTitle")} <#elseif section = "form"> - <#if !realm.editUsernameAllowed> + <#if realm.editUsernameAllowed>
diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionMultipleActionsTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionMultipleActionsTest.java index 87a5640aeb5..47b4e1d866b 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionMultipleActionsTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionMultipleActionsTest.java @@ -33,11 +33,8 @@ import org.keycloak.models.UserModel.RequiredAction; import org.keycloak.services.managers.RealmManager; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.OAuthClient; -import org.keycloak.testsuite.pages.AppPage; +import org.keycloak.testsuite.pages.*; import org.keycloak.testsuite.pages.AppPage.RequestType; -import org.keycloak.testsuite.pages.LoginPage; -import org.keycloak.testsuite.pages.LoginPasswordUpdatePage; -import org.keycloak.testsuite.pages.LoginUpdateProfilePage; import org.keycloak.testsuite.rule.KeycloakRule; import org.keycloak.testsuite.rule.KeycloakRule.KeycloakSetup; import org.keycloak.testsuite.rule.WebResource; @@ -83,7 +80,7 @@ public class RequiredActionMultipleActionsTest { protected LoginPasswordUpdatePage changePasswordPage; @WebResource - protected LoginUpdateProfilePage updateProfilePage; + protected LoginUpdateProfileEditUsernameAllowedPage updateProfilePage; @Test public void updateProfileAndPassword() throws Exception { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java index d5c341d2a5f..531ee8a3a69 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java @@ -32,7 +32,7 @@ import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.pages.AppPage; import org.keycloak.testsuite.pages.AppPage.RequestType; import org.keycloak.testsuite.pages.LoginPage; -import org.keycloak.testsuite.pages.LoginUpdateProfilePage; +import org.keycloak.testsuite.pages.LoginUpdateProfileEditUsernameAllowedPage; import org.keycloak.testsuite.rule.KeycloakRule; import org.keycloak.testsuite.rule.WebResource; import org.keycloak.testsuite.rule.WebRule; @@ -62,7 +62,7 @@ public class RequiredActionUpdateProfileTest { protected LoginPage loginPage; @WebResource - protected LoginUpdateProfilePage updateProfilePage; + protected LoginUpdateProfileEditUsernameAllowedPage updateProfilePage; @Before public void before() { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java index 9037e2b4a00..d7084c15b0c 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractIdentityProviderTest.java @@ -451,7 +451,7 @@ public abstract class AbstractIdentityProviderTest { doAfterProviderAuthentication(); this.updateProfilePage.assertCurrent(); - this.updateProfilePage.update("Test", "User", "psilva@redhat.com", "psilva"); + this.updateProfilePage.update("Test", "User", "psilva@redhat.com"); WebElement element = this.driver.findElement(By.className("kc-feedback-text")); @@ -460,7 +460,7 @@ public abstract class AbstractIdentityProviderTest { assertEquals("Email already exists.", element.getText()); this.updateProfilePage.assertCurrent(); - this.updateProfilePage.update("Test", "User", "test-user@redhat.com", "test-user"); + this.updateProfilePage.update("Test", "User", "test-user@redhat.com"); assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8081/test-app")); @@ -724,7 +724,7 @@ public abstract class AbstractIdentityProviderTest { // update profile this.updateProfilePage.assertCurrent(); - this.updateProfilePage.update(userFirstName, userLastName, userEmail, username); + this.updateProfilePage.update(userFirstName, userLastName, userEmail); } } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfileEditUsernameAllowedPage.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfileEditUsernameAllowedPage.java new file mode 100644 index 00000000000..a4f9da7b19d --- /dev/null +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfileEditUsernameAllowedPage.java @@ -0,0 +1,54 @@ +/* + * JBoss, Home of Professional Open Source. + * Copyright 2012, Red Hat, Inc., and individual contributors + * as indicated by the @author tags. See the copyright.txt file in the + * distribution for a full listing of individual contributors. + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as + * published by the Free Software Foundation; either version 2.1 of + * the License, or (at your option) any later version. + * + * This software is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this software; if not, write to the Free + * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA, or see the FSF site: http://www.fsf.org. + */ +package org.keycloak.testsuite.pages; + +import org.openqa.selenium.WebElement; +import org.openqa.selenium.support.FindBy; + +/** + * @author Stian Thorgersen + */ +public class LoginUpdateProfileEditUsernameAllowedPage extends LoginUpdateProfilePage { + + @FindBy(id = "username") + private WebElement usernameInput; + + public void update(String firstName, String lastName, String email, String username) { + usernameInput.clear(); + usernameInput.sendKeys(username); + update(firstName, lastName, email); + } + + public String getUsername() { + return usernameInput.getAttribute("value"); + } + + public boolean isCurrent() { + return driver.getTitle().equals("Update Account Information"); + } + + @Override + public void open() { + throw new UnsupportedOperationException(); + } + +} diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java index f1bbe5f477b..d67862c7928 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfilePage.java @@ -38,24 +38,19 @@ public class LoginUpdateProfilePage extends AbstractPage { @FindBy(id = "email") private WebElement emailInput; - @FindBy(id = "username") - private WebElement usernameInput; - @FindBy(css = "input[type=\"submit\"]") private WebElement submitButton; @FindBy(className = "feedback-error") private WebElement loginErrorMessage; - public void update(String firstName, String lastName, String email, String username) { + public void update(String firstName, String lastName, String email) { firstNameInput.clear(); firstNameInput.sendKeys(firstName); lastNameInput.clear(); lastNameInput.sendKeys(lastName); emailInput.clear(); emailInput.sendKeys(email); - usernameInput.clear(); - usernameInput.sendKeys(username); submitButton.click(); } @@ -75,10 +70,6 @@ public class LoginUpdateProfilePage extends AbstractPage { return emailInput.getAttribute("value"); } - public String getUsername() { - return usernameInput.getAttribute("value"); - } - public boolean isCurrent() { return driver.getTitle().equals("Update Account Information"); } From 880e831e71fa58a68b5591ba8340b5b6c93b223e Mon Sep 17 00:00:00 2001 From: dcampagna Date: Fri, 18 Sep 2015 09:01:26 +0200 Subject: [PATCH 4/6] Removed comment (KEYCLOAK-1857) --- .../pages/LoginUpdateProfileEditUsernameAllowedPage.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfileEditUsernameAllowedPage.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfileEditUsernameAllowedPage.java index a4f9da7b19d..bba3f93787a 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfileEditUsernameAllowedPage.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/pages/LoginUpdateProfileEditUsernameAllowedPage.java @@ -24,9 +24,6 @@ package org.keycloak.testsuite.pages; import org.openqa.selenium.WebElement; import org.openqa.selenium.support.FindBy; -/** - * @author Stian Thorgersen - */ public class LoginUpdateProfileEditUsernameAllowedPage extends LoginUpdateProfilePage { @FindBy(id = "username") From 43541d3028d975fcdbf980bd75b1d001fbeac0f4 Mon Sep 17 00:00:00 2001 From: dcampagna Date: Fri, 18 Sep 2015 10:15:14 +0200 Subject: [PATCH 5/6] Show error when username already exists (KEYCLOAK-1857) --- .../requiredactions/UpdateProfile.java | 20 +++++++++++++++- .../RequiredActionUpdateProfileTest.java | 23 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateProfile.java b/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateProfile.java index ac7862b7ad7..9630f3b10a0 100755 --- a/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateProfile.java +++ b/services/src/main/java/org/keycloak/authentication/requiredactions/UpdateProfile.java @@ -63,7 +63,25 @@ public class UpdateProfile implements RequiredActionProvider, RequiredActionFact } if (realm.isEditUsernameAllowed()) { - user.setUsername(formData.getFirst("username")); + String username = formData.getFirst("username"); + String oldUsername = user.getUsername(); + + boolean usernameChanged = oldUsername != null ? !oldUsername.equals(username) : username != null; + + if (usernameChanged) { + + if (session.users().getUserByUsername(username, realm) != null) { + Response challenge = context.form() + .setError(Messages.USERNAME_EXISTS) + .setFormData(formData) + .createResponse(UserModel.RequiredAction.UPDATE_PROFILE); + context.challenge(challenge); + return; + } + + user.setUsername(username); + } + } user.setFirstName(formData.getFirst("firstName")); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java index 531ee8a3a69..545ce0cf94b 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java @@ -247,6 +247,29 @@ public class RequiredActionUpdateProfileTest { events.assertEmpty(); } + @Test + public void updateProfileDuplicateUsername() { + loginPage.open(); + + loginPage.login("john-doh@localhost", "password"); + + updateProfilePage.assertCurrent(); + + updateProfilePage.update("New first", "New last", "new@email.com", "test-user@localhost"); + + 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("", updateProfilePage.getUsername()); + + Assert.assertEquals("Username already exists.", updateProfilePage.getError()); + + events.assertEmpty(); + } + @Test public void updateProfileDuplicatedEmail() { loginPage.open(); From 9d8d69010088bb849942d4291f59d5b080c3bca0 Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Tue, 22 Sep 2015 08:46:02 +0200 Subject: [PATCH 6/6] KEYCLOAK-1857 Prefill update profile form with username from user or formdata --- .../java/org/keycloak/login/freemarker/model/ProfileBean.java | 2 ++ .../testsuite/actions/RequiredActionUpdateProfileTest.java | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/model/ProfileBean.java b/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/model/ProfileBean.java index 6f73cc5878a..e730c1470c8 100755 --- a/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/model/ProfileBean.java +++ b/forms/login-freemarker/src/main/java/org/keycloak/login/freemarker/model/ProfileBean.java @@ -70,6 +70,8 @@ public class ProfileBean { } + public String getUsername() { return formData != null ? formData.getFirst("username") : user.getUsername(); } + public String getFirstName() { return formData != null ? formData.getFirst("firstName") : user.getFirstName(); } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java index 545ce0cf94b..492cd4bb131 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java @@ -263,7 +263,7 @@ public class RequiredActionUpdateProfileTest { Assert.assertEquals("New first", updateProfilePage.getFirstName()); Assert.assertEquals("New last", updateProfilePage.getLastName()); Assert.assertEquals("new@email.com", updateProfilePage.getEmail()); - Assert.assertEquals("", updateProfilePage.getUsername()); + Assert.assertEquals("test-user@localhost", updateProfilePage.getUsername()); Assert.assertEquals("Username already exists.", updateProfilePage.getError());