Merge pull request #691 from mposolda/master

Preserve form values in account mgmt after error. Allow valid email during registration or account mgmt

forms/account-api/src/main/java/org/keycloak/account/AccountProvider.java

@@ -6,6 +6,7 @@ import org.keycloak.models.UserModel;
 import org.keycloak.models.UserSessionModel;
 import org.keycloak.provider.Provider;
 
+import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;
 import java.util.List;
@@ -27,6 +28,8 @@ public interface AccountProvider extends Provider {
 
     AccountProvider setUser(UserModel user);
 
+    AccountProvider setProfileFormData(MultivaluedMap<String, String> formData);
+
     AccountProvider setStatus(Response.Status status);
 
     AccountProvider setRealm(RealmModel realm);

forms/account-freemarker/src/main/java/org/keycloak/account/freemarker/FreeMarkerAccountProvider.java

@@ -25,6 +25,7 @@ import org.keycloak.models.UserModel;
 import org.keycloak.models.UserSessionModel;
 
 import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriBuilder;
 import javax.ws.rs.core.UriInfo;
@@ -43,6 +44,7 @@ public class FreeMarkerAccountProvider implements AccountProvider {
     private static final Logger logger = Logger.getLogger(FreeMarkerAccountProvider.class);
 
     private UserModel user;
+    private MultivaluedMap<String, String> profileFormData;
     private Response.Status status = Response.Status.OK;
     private RealmModel realm;
     private String[] referrer;
@@ -126,7 +128,7 @@ public class FreeMarkerAccountProvider implements AccountProvider {
 
         switch (page) {
             case ACCOUNT:
-                attributes.put("account", new AccountBean(user));
+                attributes.put("account", new AccountBean(user, profileFormData));
                 break;
             case TOTP:
                 attributes.put("totp", new TotpBean(user, baseUri));
@@ -187,6 +189,12 @@ public class FreeMarkerAccountProvider implements AccountProvider {
         return this;
     }
 
+    @Override
+    public AccountProvider setProfileFormData(MultivaluedMap<String, String> formData) {
+        this.profileFormData = formData;
+        return this;
+    }
+
     @Override
     public AccountProvider setRealm(RealmModel realm) {
         this.realm = realm;

forms/account-freemarker/src/main/java/org/keycloak/account/freemarker/model/AccountBean.java

@@ -1,5 +1,7 @@
 package org.keycloak.account.freemarker.model;
 
+import javax.ws.rs.core.MultivaluedMap;
+
 import org.keycloak.models.UserModel;
 
 /**
@@ -7,18 +9,20 @@ import org.keycloak.models.UserModel;
  */
 public class AccountBean {
 
-    private UserModel user;
+    private final UserModel user;
+    private final MultivaluedMap<String, String> profileFormData;
 
-    public AccountBean(UserModel user) {
+    public AccountBean(UserModel user, MultivaluedMap<String, String> profileFormData) {
         this.user = user;
+        this.profileFormData = profileFormData;
     }
 
     public String getFirstName() {
-        return user.getFirstName();
+        return profileFormData != null ?  profileFormData.getFirst("firstName") : user.getFirstName();
     }
 
     public String getLastName() {
-        return user.getLastName();
+        return profileFormData != null ?  profileFormData.getFirst("lastName") :user.getLastName();
     }
 
     public String getUsername() {
@@ -26,7 +30,7 @@ public class AccountBean {
     }
 
     public String getEmail() {
-        return user.getEmail();
+        return profileFormData != null ?  profileFormData.getFirst("email") :user.getEmail();
     }
 
 }

forms/common-themes/src/main/resources/theme/account/base/messages/messages.properties

@@ -10,6 +10,7 @@ successHeader=Success!
 username=Username
 
 missingFirstName=Please specify first name
+invalidEmail=Invalid email address
 missingLastName=Please specify last name
 missingEmail=Please specify email
 missingPassword=Please specify password.

forms/common-themes/src/main/resources/theme/login/base/messages/messages.properties

@@ -31,6 +31,7 @@ clientCertificate=Client Certificate
 
 invalidUser=Invalid username or password.
 invalidPassword=Invalid username or password.
+invalidEmail=Invalid email address
 accountDisabled=Account is disabled, contact admin
 accountTemporarilyDisabled=Account is temporarily disabled, contact admin or try again later
 

services/src/main/java/org/keycloak/services/messages/Messages.java

@@ -35,6 +35,8 @@ public class Messages {
 
     public static final String INVALID_PASSWORD_CONFIRM = "invalidPasswordConfirm";
 
+    public static final String INVALID_EMAIL = "invalidEmail";
+
     public static final String INVALID_USER = "invalidUser";
 
     public static final String READ_ONLY_USER = "readOnlyUser";

services/src/main/java/org/keycloak/services/resources/AccountService.java

@@ -407,7 +407,7 @@ public class AccountService {
         String error = Validation.validateUpdateProfileForm(formData);
         if (error != null) {
             setReferrerOnPage();
-            return account.setError(error).createResponse(AccountPages.ACCOUNT);
+            return account.setError(error).setProfileFormData(formData).createResponse(AccountPages.ACCOUNT);
         }
 
         try {
@@ -430,7 +430,7 @@ public class AccountService {
             return account.setSuccess("accountUpdated").createResponse(AccountPages.ACCOUNT);
         } catch (ModelReadOnlyException roe) {
             setReferrerOnPage();
-            return account.setError(Messages.READ_ONLY_USER).createResponse(AccountPages.ACCOUNT);
+            return account.setError(Messages.READ_ONLY_USER).setProfileFormData(formData).createResponse(AccountPages.ACCOUNT);
         }
     }
 

services/src/main/java/org/keycloak/services/validation/Validation.java

@@ -6,9 +6,13 @@ import org.keycloak.services.messages.Messages;
 
 import javax.ws.rs.core.MultivaluedMap;
 import java.util.List;
+import java.util.regex.Pattern;
 
 public class Validation {
 
+    // Actually allow same emails like angular. See ValidationTest.testEmailValidation()
+    private static final Pattern EMAIL_PATTERN = Pattern.compile("[a-zA-Z0-9!#$%&'*+/=?^_`{|}~.-]+@[a-zA-Z0-9-]+(\\.[a-zA-Z0-9-]+)*");
+
     public static String validateRegistrationForm(MultivaluedMap<String, String> formData, List<String> requiredCredentialTypes) {
         if (isEmpty(formData.getFirst("firstName"))) {
             return Messages.MISSING_FIRST_NAME;
@@ -22,6 +26,10 @@ public class Validation {
             return Messages.MISSING_EMAIL;
         }
 
+        if (!isEmailValid(formData.getFirst("email"))) {
+            return Messages.INVALID_EMAIL;
+        }
+
         if (isEmpty(formData.getFirst("username"))) {
             return Messages.MISSING_USERNAME;
         }
@@ -56,6 +64,10 @@ public class Validation {
             return Messages.MISSING_EMAIL;
         }
 
+        if (!isEmailValid(formData.getFirst("email"))) {
+            return Messages.INVALID_EMAIL;
+        }
+
         return null;
     }
 
@@ -63,4 +75,9 @@ public class Validation {
         return s == null || s.length() == 0;
     }
 
+    public static boolean isEmailValid(String email) {
+        return EMAIL_PATTERN.matcher(email).matches();
+    }
+
+
 }

services/src/test/java/org/keycloak/test/ValidationTest.java

@@ -0,0 +1,27 @@
+package org.keycloak.test;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.keycloak.services.validation.Validation;
+
+/**
+ * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
+ */
+public class ValidationTest {
+
+    @Test
+    public void testEmailValidation() {
+        Assert.assertTrue(Validation.isEmailValid("abc@abc.cz"));
+        Assert.assertTrue(Validation.isEmailValid("435@coco.foo.cz"));
+        Assert.assertTrue(Validation.isEmailValid("A@something"));
+        Assert.assertTrue(Validation.isEmailValid("A@some-thing.foo"));
+        Assert.assertTrue(Validation.isEmailValid("1@A"));
+        Assert.assertFalse(Validation.isEmailValid("A@some_thing.foo"));
+        Assert.assertFalse(Validation.isEmailValid("@some_thing.foo"));
+        Assert.assertFalse(Validation.isEmailValid("abc@"));
+        Assert.assertFalse(Validation.isEmailValid("abc@."));
+        Assert.assertFalse(Validation.isEmailValid("abc@.foo"));
+        Assert.assertFalse(Validation.isEmailValid("abc@foo."));
+        Assert.assertFalse(Validation.isEmailValid("abc@foo..bar"));
+    }
+}

testsuite/integration/src/test/java/org/keycloak/testsuite/account/AccountTest.java

@@ -282,23 +282,31 @@ public class AccountTest {
 
         Assert.assertEquals("Please specify first name", profilePage.getError());
         Assert.assertEquals("", profilePage.getFirstName());
-        Assert.assertEquals("", profilePage.getLastName());
+        Assert.assertEquals("New last", profilePage.getLastName());
-        Assert.assertEquals("test-user@localhost", profilePage.getEmail());
+        Assert.assertEquals("new@email.com", profilePage.getEmail());
 
         events.assertEmpty();
 
         profilePage.updateProfile("New first", "", "new@email.com");
 
         Assert.assertEquals("Please specify last name", profilePage.getError());
-        Assert.assertEquals("", profilePage.getFirstName());
+        Assert.assertEquals("New first", profilePage.getFirstName());
         Assert.assertEquals("", profilePage.getLastName());
-        Assert.assertEquals("test-user@localhost", profilePage.getEmail());
+        Assert.assertEquals("new@email.com", profilePage.getEmail());
 
         events.assertEmpty();
 
         profilePage.updateProfile("New first", "New last", "");
 
         Assert.assertEquals("Please specify email", profilePage.getError());
+        Assert.assertEquals("New first", profilePage.getFirstName());
+        Assert.assertEquals("New last", profilePage.getLastName());
+        Assert.assertEquals("", profilePage.getEmail());
+
+        events.assertEmpty();
+
+        profilePage.clickCancel();
+
         Assert.assertEquals("", profilePage.getFirstName());
         Assert.assertEquals("", profilePage.getLastName());
         Assert.assertEquals("test-user@localhost", profilePage.getEmail());

testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java

@@ -135,9 +135,9 @@ public class RequiredActionEmailVerificationTest {
     public void verifyEmailRegister() throws IOException, MessagingException {
         loginPage.open();
         loginPage.clickRegister();
-        registerPage.register("firstName", "lastName", "email", "verifyEmail", "password", "password");
+        registerPage.register("firstName", "lastName", "email@mail.com", "verifyEmail", "password", "password");
 
-        String userId = events.expectRegister("verifyEmail", "email").assertEvent().getUserId();
+        String userId = events.expectRegister("verifyEmail", "email@mail.com").assertEvent().getUserId();
 
         Assert.assertTrue(verifyEmailPage.isCurrent());
 
@@ -147,7 +147,7 @@ public class RequiredActionEmailVerificationTest {
 
         String body = (String) message.getContent();
 
-        Event sendEvent = events.expectRequiredAction(EventType.SEND_VERIFY_EMAIL).user(userId).detail("username", "verifyEmail").detail("email", "email").assertEvent();
+        Event sendEvent = events.expectRequiredAction(EventType.SEND_VERIFY_EMAIL).user(userId).detail("username", "verifyEmail").detail("email", "email@mail.com").assertEvent();
         String sessionId = sendEvent.getSessionId();
 
         String mailCodeId = sendEvent.getDetails().get(Details.CODE_ID);
@@ -158,7 +158,7 @@ public class RequiredActionEmailVerificationTest {
 
         Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType());
 
-        events.expectRequiredAction(EventType.VERIFY_EMAIL).user(userId).session(sessionId).detail("username", "verifyEmail").detail("email", "email").detail(Details.CODE_ID, mailCodeId).assertEvent();
+        events.expectRequiredAction(EventType.VERIFY_EMAIL).user(userId).session(sessionId).detail("username", "verifyEmail").detail("email", "email@mail.com").detail(Details.CODE_ID, mailCodeId).assertEvent();
 
         events.expectLogin().user(userId).session(sessionId).detail("username", "verifyEmail").detail(Details.CODE_ID, mailCodeId).assertEvent();
     }

testsuite/integration/src/test/java/org/keycloak/testsuite/actions/RequiredActionTotpSetupTest.java

@@ -99,9 +99,9 @@ public class RequiredActionTotpSetupTest {
     public void setupTotpRegister() {
         loginPage.open();
         loginPage.clickRegister();
-        registerPage.register("firstName", "lastName", "email", "setupTotp", "password", "password");
+        registerPage.register("firstName", "lastName", "email@mail.com", "setupTotp", "password", "password");
 
-        String userId = events.expectRegister("setupTotp", "email").assertEvent().getUserId();
+        String userId = events.expectRegister("setupTotp", "email@mail.com").assertEvent().getUserId();
 
         totpPage.assertCurrent();
 
@@ -149,9 +149,9 @@ public class RequiredActionTotpSetupTest {
         // Register new user
         loginPage.open();
         loginPage.clickRegister();
-        registerPage.register("firstName2", "lastName2", "email2", "setupTotp2", "password2", "password2");
+        registerPage.register("firstName2", "lastName2", "email2@mail.com", "setupTotp2", "password2", "password2");
 
-        String userId = events.expectRegister("setupTotp2", "email2").assertEvent().getUserId();
+        String userId = events.expectRegister("setupTotp2", "email2@mail.com").assertEvent().getUserId();
 
         // Configure totp
         totpPage.assertCurrent();

testsuite/integration/src/test/java/org/keycloak/testsuite/forms/FederationProvidersIntegrationTest.java

@@ -210,7 +210,7 @@ public class FederationProvidersIntegrationTest {
         registerPage.assertCurrent();
 
         // check existing username
-        registerPage.register("firstName", "lastName", "email", "existing", "Password1", "Password1");
+        registerPage.register("firstName", "lastName", "email@mail.cz", "existing", "Password1", "Password1");
         registerPage.assertCurrent();
         Assert.assertEquals("Username already exists", registerPage.getError());
 
@@ -226,7 +226,7 @@ public class FederationProvidersIntegrationTest {
         loginPage.clickRegister();
         registerPage.assertCurrent();
 
-        registerPage.register("firstName", "lastName", "email2", "registerUserSuccess2", "Password1", "Password1");
+        registerPage.register("firstName", "lastName", "email2@check.cz", "registerUserSuccess2", "Password1", "Password1");
         Assert.assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType());
 
         KeycloakSession session = keycloakRule.startSession();

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", "registerExistingUseremail", "test-user@localhost", "password", "password");
+        registerPage.register("firstName", "lastName", "registerExistingUser@email", "test-user@localhost", "password", "password");
 
         registerPage.assertCurrent();
         Assert.assertEquals("Username already exists", registerPage.getError());
 
-        events.expectRegister("test-user@localhost", "registerExistingUseremail").user((String) null).error("username_in_use").assertEvent();
+        events.expectRegister("test-user@localhost", "registerExistingUser@email").user((String) null).error("username_in_use").assertEvent();
     }
 
     @Test
@@ -89,12 +89,12 @@ public class RegisterTest {
         loginPage.clickRegister();
         registerPage.assertCurrent();
 
-        registerPage.register("firstName", "lastName", "registerUserInvalidPasswordConfirmemail", "registerUserInvalidPasswordConfirm", "password", "invalid");
+        registerPage.register("firstName", "lastName", "registerUserInvalidPasswordConfirm@email", "registerUserInvalidPasswordConfirm", "password", "invalid");
 
         registerPage.assertCurrent();
         Assert.assertEquals("Password confirmation doesn't match", registerPage.getError());
 
-        events.expectRegister("registerUserInvalidPasswordConfirm", "registerUserInvalidPasswordConfirmemail").user((String) null).error("invalid_registration").assertEvent();
+        events.expectRegister("registerUserInvalidPasswordConfirm", "registerUserInvalidPasswordConfirm@email").user((String) null).error("invalid_registration").assertEvent();
     }
 
     @Test
@@ -103,12 +103,12 @@ public class RegisterTest {
         loginPage.clickRegister();
         registerPage.assertCurrent();
 
-        registerPage.register("firstName", "lastName", "registerUserMissingPasswordemail", "registerUserMissingPassword", null, null);
+        registerPage.register("firstName", "lastName", "registerUserMissingPassword@email", "registerUserMissingPassword", null, null);
 
         registerPage.assertCurrent();
         Assert.assertEquals("Please specify password.", registerPage.getError());
 
-        events.expectRegister("registerUserMissingPassword", "registerUserMissingPasswordemail").user((String) null).error("invalid_registration").assertEvent();
+        events.expectRegister("registerUserMissingPassword", "registerUserMissingPassword@email").user((String) null).error("invalid_registration").assertEvent();
     }
 
     @Test
@@ -125,17 +125,17 @@ public class RegisterTest {
             loginPage.clickRegister();
             registerPage.assertCurrent();
 
-            registerPage.register("firstName", "lastName", "registerPasswordPolicyemail", "registerPasswordPolicy", "pass", "pass");
+            registerPage.register("firstName", "lastName", "registerPasswordPolicy@email", "registerPasswordPolicy", "pass", "pass");
 
             registerPage.assertCurrent();
             Assert.assertEquals("Invalid password: minimum length 8", registerPage.getError());
 
-            events.expectRegister("registerPasswordPolicy", "registerPasswordPolicyemail").user((String) null).error("invalid_registration").assertEvent();
+            events.expectRegister("registerPasswordPolicy", "registerPasswordPolicy@email").user((String) null).error("invalid_registration").assertEvent();
 
-            registerPage.register("firstName", "lastName", "registerPasswordPolicyemail", "registerPasswordPolicy", "password", "password");
+            registerPage.register("firstName", "lastName", "registerPasswordPolicy@email", "registerPasswordPolicy", "password", "password");
             Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType());
 
-            String userId = events.expectRegister("registerPasswordPolicy", "registerPasswordPolicyemail").assertEvent().getUserId();
+            String userId = events.expectRegister("registerPasswordPolicy", "registerPasswordPolicy@email").assertEvent().getUserId();
 
             events.expectLogin().user(userId).detail(Details.USERNAME, "registerPasswordPolicy").assertEvent();
         } finally {
@@ -154,12 +154,29 @@ public class RegisterTest {
         loginPage.clickRegister();
         registerPage.assertCurrent();
 
-        registerPage.register("firstName", "lastName", "registerUserMissingUsernameemail", null, "password", "password");
+        registerPage.register("firstName", "lastName", "registerUserMissingUsername@email", null, "password", "password");
 
         registerPage.assertCurrent();
         Assert.assertEquals("Please specify username", registerPage.getError());
 
-        events.expectRegister(null, "registerUserMissingUsernameemail").removeDetail("username").error("invalid_registration").assertEvent();
+        events.expectRegister(null, "registerUserMissingUsername@email").removeDetail("username").error("invalid_registration").assertEvent();
+    }
+
+    @Test
+    public void registerUserMissingOrInvalidEmail() {
+        loginPage.open();
+        loginPage.clickRegister();
+        registerPage.assertCurrent();
+
+        registerPage.register("firstName", "lastName", null, "registerUserMissingEmail", "password", "password");
+        registerPage.assertCurrent();
+        Assert.assertEquals("Please specify email", registerPage.getError());
+        events.expectRegister("registerUserMissingEmail", null).removeDetail("email").error("invalid_registration").assertEvent();
+
+        registerPage.register("firstName", "lastName", "registerUserInvalidEmailemail", "registerUserInvalidEmail", "password", "password");
+        registerPage.assertCurrent();
+        Assert.assertEquals("Invalid email address", registerPage.getError());
+        events.expectRegister("registerUserInvalidEmail", "registerUserInvalidEmailemail").error("invalid_registration").assertEvent();
     }
 
     @Test
@@ -168,11 +185,11 @@ public class RegisterTest {
         loginPage.clickRegister();
         registerPage.assertCurrent();
 
-        registerPage.register("firstName", "lastName", "registerUserSuccessemail", "registerUserSuccess", "password", "password");
+        registerPage.register("firstName", "lastName", "registerUserSuccess@email", "registerUserSuccess", "password", "password");
 
         Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType());
 
-        String userId = events.expectRegister("registerUserSuccess", "registerUserSuccessemail").assertEvent().getUserId();
+        String userId = events.expectRegister("registerUserSuccess", "registerUserSuccess@email").assertEvent().getUserId();
         events.expectLogin().detail("username", "registerUserSuccess").user(userId).assertEvent();
     }
 

testsuite/integration/src/test/java/org/keycloak/testsuite/pages/AccountUpdateProfilePage.java

@@ -48,9 +48,12 @@ public class AccountUpdateProfilePage extends AbstractAccountPage {
     @FindBy(id = "referrer")
     private WebElement backToApplicationLink;
 
-    @FindBy(css = "button[type=\"submit\"]")
+    @FindBy(css = "button[type=\"submit\"][value=\"Save\"]")
     private WebElement submitButton;
 
+    @FindBy(css = "button[type=\"submit\"][value=\"Cancel\"]")
+    private WebElement cancelButton;
+
     @FindBy(className = "alert-success")
     private WebElement successMessage;
 
@@ -68,6 +71,11 @@ public class AccountUpdateProfilePage extends AbstractAccountPage {
         submitButton.click();
     }
 
+    public void clickCancel() {
+        cancelButton.click();
+    }
+
+
     public String getFirstName() {
         return firstNameInput.getAttribute("value");
     }