keycloak-aplcache

KEYCLOAK-1823 Annoying behaviour of validations in user

10/13/2015 3:14:39 AM

Details

diff --git a/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java b/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java
index f29ad39..c94e8cb 100755
--- a/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java
+++ b/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java
@@ -18,10 +18,7 @@ import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;
 import java.net.URI;
-import java.util.HashMap;
-import java.util.LinkedList;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 /**
 * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
@@ -116,6 +113,7 @@ public class FormAuthenticationFlow implements AuthenticationFlow {
 
     private class ValidationContextImpl extends FormContextImpl implements ValidationContext {
         FormAction action;
+        String error;
 
         private ValidationContextImpl(AuthenticationExecutionModel executionModel, FormAction action) {
             super(executionModel);
@@ -131,6 +129,10 @@ public class FormAuthenticationFlow implements AuthenticationFlow {
             this.formData = formData;
         }
 
+        public void error(String error) {
+            this.error = error;
+        }
+
         @Override
         public void success() {
            success = true;
@@ -145,6 +147,7 @@ public class FormAuthenticationFlow implements AuthenticationFlow {
         Map<String, ClientSessionModel.ExecutionStatus> executionStatus = new HashMap<>();
         List<FormAction> requiredActions = new LinkedList<>();
         List<ValidationContextImpl> successes = new LinkedList<>();
+        List<ValidationContextImpl> errors = new LinkedList<>();
         for (AuthenticationExecutionModel formActionExecution : formActionExecutions) {
             if (!formActionExecution.isEnabled()) {
                 executionStatus.put(formActionExecution.getId(), ClientSessionModel.ExecutionStatus.SKIPPED);
@@ -183,10 +186,26 @@ public class FormAuthenticationFlow implements AuthenticationFlow {
                 executionStatus.put(formActionExecution.getId(), ClientSessionModel.ExecutionStatus.SUCCESS);
                 successes.add(result);
             } else {
-                processor.logFailure();
                 executionStatus.put(formActionExecution.getId(), ClientSessionModel.ExecutionStatus.CHALLENGED);
-                return renderForm(result.formData, result.errors);
+                errors.add(result);
+            }
+        }
+
+        if (!errors.isEmpty()) {
+            processor.logFailure();
+            List<FormMessage> messages = new LinkedList<>();
+            Set<String> fields = new HashSet<>();
+            for (ValidationContextImpl v : errors) {
+                for (FormMessage m : v.errors) {
+                    if (!fields.contains(m.getField())) {
+                        fields.add(m.getField());
+                        messages.add(m);
+                    }
+                }
             }
+            ValidationContextImpl first = errors.get(0);
+            first.getEvent().error(first.error);
+            return renderForm(first.formData, messages);
         }
 
         for (ValidationContextImpl context : successes) {
diff --git a/services/src/main/java/org/keycloak/authentication/forms/RegistrationPassword.java b/services/src/main/java/org/keycloak/authentication/forms/RegistrationPassword.java
index ade4e00..ff2442f 100755
--- a/services/src/main/java/org/keycloak/authentication/forms/RegistrationPassword.java
+++ b/services/src/main/java/org/keycloak/authentication/forms/RegistrationPassword.java
@@ -59,7 +59,7 @@ public class RegistrationPassword implements FormAction, FormActionFactory {
         }
 
         if (errors.size() > 0) {
-            context.getEvent().error(Errors.INVALID_REGISTRATION);
+            context.error(Errors.INVALID_REGISTRATION);
             formData.remove(RegistrationPage.FIELD_PASSWORD);
             formData.remove(RegistrationPage.FIELD_PASSWORD_CONFIRM);
             context.validationError(formData, errors);
diff --git a/services/src/main/java/org/keycloak/authentication/forms/RegistrationProfile.java b/services/src/main/java/org/keycloak/authentication/forms/RegistrationProfile.java
index efec19a..3baae6f 100755
--- a/services/src/main/java/org/keycloak/authentication/forms/RegistrationProfile.java
+++ b/services/src/main/java/org/keycloak/authentication/forms/RegistrationProfile.java
@@ -56,14 +56,17 @@ public class RegistrationProfile implements FormAction, FormActionFactory {
         }
 
         String email = formData.getFirst(Validation.FIELD_EMAIL);
+        boolean emailValid = true;
         if (Validation.isBlank(email)) {
             errors.add(new FormMessage(RegistrationPage.FIELD_EMAIL, Messages.MISSING_EMAIL));
+            emailValid = false;
         } else if (!Validation.isEmailValid(email)) {
             context.getEvent().detail(Details.EMAIL, email);
             errors.add(new FormMessage(RegistrationPage.FIELD_EMAIL, Messages.INVALID_EMAIL));
+            emailValid = false;
         }
 
-        if (context.getSession().users().getUserByEmail(email, context.getRealm()) != null) {
+        if (emailValid && context.getSession().users().getUserByEmail(email, context.getRealm()) != null) {
             eventError = Errors.EMAIL_IN_USE;
             formData.remove(Validation.FIELD_EMAIL);
             context.getEvent().detail(Details.EMAIL, email);
@@ -71,7 +74,7 @@ public class RegistrationProfile implements FormAction, FormActionFactory {
         }
 
         if (errors.size() > 0) {
-            context.getEvent().error(eventError);
+            context.error(eventError);
             context.validationError(formData, errors);
             return;
 
diff --git a/services/src/main/java/org/keycloak/authentication/forms/RegistrationRecaptcha.java b/services/src/main/java/org/keycloak/authentication/forms/RegistrationRecaptcha.java
index 3c1817c..eb10250 100755
--- a/services/src/main/java/org/keycloak/authentication/forms/RegistrationRecaptcha.java
+++ b/services/src/main/java/org/keycloak/authentication/forms/RegistrationRecaptcha.java
@@ -108,7 +108,7 @@ public class RegistrationRecaptcha implements FormAction, FormActionFactory, Con
         } else {
             errors.add(new FormMessage(null, Messages.RECAPTCHA_FAILED));
             formData.remove(G_RECAPTCHA_RESPONSE);
-            context.getEvent().error(Errors.INVALID_REGISTRATION);
+            context.error(Errors.INVALID_REGISTRATION);
             context.validationError(formData, errors);
             return;
 
diff --git a/services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java b/services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java
index 40d2fb0..dfc2a89 100755
--- a/services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java
+++ b/services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java
@@ -56,9 +56,8 @@ public class RegistrationUserCreation implements FormAction, FormActionFactory {
 
         String usernameField = RegistrationPage.FIELD_USERNAME;
         if (context.getRealm().isRegistrationEmailAsUsername()) {
-            username = email;
-            context.getEvent().detail(Details.USERNAME, username);
-            usernameField = RegistrationPage.FIELD_EMAIL;
+            context.getEvent().detail(Details.USERNAME, email);
+
             if (Validation.isBlank(email)) {
                 errors.add(new FormMessage(RegistrationPage.FIELD_EMAIL, Messages.MISSING_EMAIL));
             } else if (!Validation.isEmailValid(email)) {
@@ -66,33 +65,32 @@ public class RegistrationUserCreation implements FormAction, FormActionFactory {
                 formData.remove(Validation.FIELD_EMAIL);
             }
             if (errors.size() > 0) {
-                context.getEvent().error(Errors.INVALID_REGISTRATION);
+                context.error(Errors.INVALID_REGISTRATION);
                 context.validationError(formData, errors);
                 return;
             }
             if (email != null && context.getSession().users().getUserByEmail(email, context.getRealm()) != null) {
-                context.getEvent().error(Errors.USERNAME_IN_USE);
+                context.error(Errors.EMAIL_IN_USE);
                 formData.remove(Validation.FIELD_EMAIL);
-                errors.add(new FormMessage(RegistrationPage.FIELD_EMAIL, Messages.USERNAME_EXISTS));
+                errors.add(new FormMessage(RegistrationPage.FIELD_EMAIL, Messages.EMAIL_EXISTS));
                 context.validationError(formData, errors);
                 return;
             }
         } else {
             if (Validation.isBlank(username)) {
-                context.getEvent().error(Errors.INVALID_REGISTRATION);
+                context.error(Errors.INVALID_REGISTRATION);
                 errors.add(new FormMessage(RegistrationPage.FIELD_USERNAME, Messages.MISSING_USERNAME));
                 context.validationError(formData, errors);
                 return;
             }
 
-        }
-        if (context.getSession().users().getUserByUsername(username, context.getRealm()) != null) {
-            context.getEvent().error(Errors.USERNAME_IN_USE);
-            errors.add(new FormMessage(usernameField, Messages.USERNAME_EXISTS));
-            formData.remove(Validation.FIELD_USERNAME);
-            formData.remove(Validation.FIELD_EMAIL);
-            context.validationError(formData, errors);
-            return;
+            if (context.getSession().users().getUserByUsername(username, context.getRealm()) != null) {
+                context.error(Errors.USERNAME_IN_USE);
+                errors.add(new FormMessage(usernameField, Messages.USERNAME_EXISTS));
+                formData.remove(Validation.FIELD_USERNAME);
+                context.validationError(formData, errors);
+                return;
+            }
 
         }
         context.success();
diff --git a/services/src/main/java/org/keycloak/authentication/ValidationContext.java b/services/src/main/java/org/keycloak/authentication/ValidationContext.java
index b0c456e..ce96b68 100755
--- a/services/src/main/java/org/keycloak/authentication/ValidationContext.java
+++ b/services/src/main/java/org/keycloak/authentication/ValidationContext.java
@@ -21,6 +21,8 @@ public interface ValidationContext extends FormContext {
      */
     void validationError(MultivaluedMap<String, String> formData, List<FormMessage> errors);
 
+    void error(String error);
+
     /**
      * Mark this validation as sucessful
      *
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 932630b..55c7e61 100755
--- a/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java
+++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java
@@ -84,7 +84,7 @@ public class RegisterTest {
         // assert form keeps form fields on error
         assertEquals("firstName", registerPage.getFirstName());
         assertEquals("lastName", registerPage.getLastName());
-        assertEquals("", registerPage.getEmail());
+        assertEquals("registerExistingUser@email", registerPage.getEmail());
         assertEquals("", registerPage.getUsername());
         assertEquals("", registerPage.getPassword());
         assertEquals("", registerPage.getPasswordConfirm());
@@ -199,15 +199,15 @@ public class RegisterTest {
         loginPage.clickRegister();
         registerPage.assertCurrent();
 
-        registerPage.register(null, null, null, "registerUserManyErrors", null, "password");
+        registerPage.register(null, null, null, null, null, null);
 
         registerPage.assertCurrent();
 
-        System.out.println(registerPage.getError());
-
-        assertEquals("Please specify first name.\n" +
+        assertEquals("Please specify username.\n" +
+                "Please specify first name.\n" +
                 "Please specify last name.\n" +
-                "Please specify email.", registerPage.getError());
+                "Please specify email.\n" +
+                "Please specify password.", registerPage.getError());
 
         events.expectRegister(null, "registerUserMissingUsername@email")
                 .removeDetail(Details.USERNAME)
@@ -290,9 +290,9 @@ public class RegisterTest {
             registerPage.registerWithEmailAsUsername("firstName", "lastName", "test-user@localhost", "password", "password");
 
             registerPage.assertCurrent();
-            assertEquals("Username already exists.", registerPage.getError());
+            assertEquals("Email already exists.", registerPage.getError());
 
-            events.expectRegister("test-user@localhost", "test-user@localhost").user((String) null).error("username_in_use").assertEvent();
+            events.expectRegister("test-user@localhost", "test-user@localhost").user((String) null).error("email_in_use").assertEvent();
         } finally {
             configureRelamRegistrationEmailAsUsername(false);
         }