diff --git a/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java b/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java index f29ad398d1..c94e8cbc08 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 Bill Burke @@ -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 executionStatus = new HashMap<>(); List requiredActions = new LinkedList<>(); List successes = new LinkedList<>(); + List errors = new LinkedList<>(); for (AuthenticationExecutionModel formActionExecution : formActionExecutions) { if (!formActionExecution.isEnabled()) { executionStatus.put(formActionExecution.getId(), ClientSessionModel.ExecutionStatus.SKIPPED); @@ -183,12 +186,28 @@ 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 messages = new LinkedList<>(); + Set 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) { context.action.success(context); } diff --git a/services/src/main/java/org/keycloak/authentication/ValidationContext.java b/services/src/main/java/org/keycloak/authentication/ValidationContext.java index b0c456eb22..ce96b682c1 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 formData, List errors); + void error(String error); + /** * Mark this validation as sucessful * 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 ade4e00a48..ff2442f3d5 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 2fd3c85f01..3baae6fe3a 100755 --- a/services/src/main/java/org/keycloak/authentication/forms/RegistrationProfile.java +++ b/services/src/main/java/org/keycloak/authentication/forms/RegistrationProfile.java @@ -56,15 +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)) { - formData.remove(Validation.FIELD_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); @@ -72,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 3c1817ce5e..eb10250946 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 40d2fb075f..dfc2a89731 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/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java index 9c2852e59d..55c7e618d1 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 @@ -21,10 +21,7 @@ */ package org.keycloak.testsuite.forms; -import org.junit.Assert; -import org.junit.ClassRule; -import org.junit.Rule; -import org.junit.Test; +import org.junit.*; import org.keycloak.events.Details; import org.keycloak.models.KeycloakSession; import org.keycloak.models.PasswordPolicy; @@ -42,6 +39,8 @@ import org.keycloak.testsuite.rule.WebResource; import org.keycloak.testsuite.rule.WebRule; import org.openqa.selenium.WebDriver; +import static org.junit.Assert.assertEquals; + /** * @author Stian Thorgersen */ @@ -80,15 +79,15 @@ public class RegisterTest { registerPage.register("firstName", "lastName", "registerExistingUser@email", "test-user@localhost", "password", "password"); registerPage.assertCurrent(); - Assert.assertEquals("Username already exists.", registerPage.getError()); + assertEquals("Username already exists.", registerPage.getError()); // assert form keeps form fields on error - Assert.assertEquals("firstName", registerPage.getFirstName()); - Assert.assertEquals("lastName", registerPage.getLastName()); - Assert.assertEquals("", registerPage.getEmail()); - Assert.assertEquals("", registerPage.getUsername()); - Assert.assertEquals("", registerPage.getPassword()); - Assert.assertEquals("", registerPage.getPasswordConfirm()); + assertEquals("firstName", registerPage.getFirstName()); + assertEquals("lastName", registerPage.getLastName()); + assertEquals("registerExistingUser@email", registerPage.getEmail()); + assertEquals("", registerPage.getUsername()); + assertEquals("", registerPage.getPassword()); + assertEquals("", registerPage.getPasswordConfirm()); events.expectRegister("test-user@localhost", "registerExistingUser@email") .removeDetail(Details.EMAIL) @@ -104,15 +103,15 @@ public class RegisterTest { registerPage.register("firstName", "lastName", "registerUserInvalidPasswordConfirm@email", "registerUserInvalidPasswordConfirm", "password", "invalid"); registerPage.assertCurrent(); - Assert.assertEquals("Password confirmation doesn't match.", registerPage.getError()); + assertEquals("Password confirmation doesn't match.", registerPage.getError()); // assert form keeps form fields on error - Assert.assertEquals("firstName", registerPage.getFirstName()); - Assert.assertEquals("lastName", registerPage.getLastName()); - Assert.assertEquals("registerUserInvalidPasswordConfirm@email", registerPage.getEmail()); - Assert.assertEquals("registerUserInvalidPasswordConfirm", registerPage.getUsername()); - Assert.assertEquals("", registerPage.getPassword()); - Assert.assertEquals("", registerPage.getPasswordConfirm()); + assertEquals("firstName", registerPage.getFirstName()); + assertEquals("lastName", registerPage.getLastName()); + assertEquals("registerUserInvalidPasswordConfirm@email", registerPage.getEmail()); + assertEquals("registerUserInvalidPasswordConfirm", registerPage.getUsername()); + assertEquals("", registerPage.getPassword()); + assertEquals("", registerPage.getPasswordConfirm()); events.expectRegister("registerUserInvalidPasswordConfirm", "registerUserInvalidPasswordConfirm@email") .removeDetail(Details.USERNAME) @@ -129,7 +128,7 @@ public class RegisterTest { registerPage.register("firstName", "lastName", "registerUserMissingPassword@email", "registerUserMissingPassword", null, null); registerPage.assertCurrent(); - Assert.assertEquals("Please specify password.", registerPage.getError()); + assertEquals("Please specify password.", registerPage.getError()); events.expectRegister("registerUserMissingPassword", "registerUserMissingPassword@email") .removeDetail(Details.USERNAME) @@ -154,7 +153,7 @@ public class RegisterTest { registerPage.register("firstName", "lastName", "registerPasswordPolicy@email", "registerPasswordPolicy", "pass", "pass"); registerPage.assertCurrent(); - Assert.assertEquals("Invalid password: minimum length 8.", registerPage.getError()); + assertEquals("Invalid password: minimum length 8.", registerPage.getError()); events.expectRegister("registerPasswordPolicy", "registerPasswordPolicy@email") .removeDetail(Details.USERNAME) @@ -162,7 +161,7 @@ public class RegisterTest { .user((String) null).error("invalid_registration").assertEvent(); registerPage.register("firstName", "lastName", "registerPasswordPolicy@email", "registerPasswordPolicy", "password", "password"); - Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); + assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); String userId = events.expectRegister("registerPasswordPolicy", "registerPasswordPolicy@email").assertEvent().getUserId(); @@ -186,7 +185,7 @@ public class RegisterTest { registerPage.register("firstName", "lastName", "registerUserMissingUsername@email", null, "password", "password"); registerPage.assertCurrent(); - Assert.assertEquals("Please specify username.", registerPage.getError()); + assertEquals("Please specify username.", registerPage.getError()); events.expectRegister(null, "registerUserMissingUsername@email") .removeDetail(Details.USERNAME) @@ -195,21 +194,51 @@ public class RegisterTest { } @Test - public void registerUserMissingOrInvalidEmail() { + public void registerUserManyErrors() { + loginPage.open(); + loginPage.clickRegister(); + registerPage.assertCurrent(); + + registerPage.register(null, null, null, null, null, null); + + registerPage.assertCurrent(); + + assertEquals("Please specify username.\n" + + "Please specify first name.\n" + + "Please specify last name.\n" + + "Please specify email.\n" + + "Please specify password.", registerPage.getError()); + + events.expectRegister(null, "registerUserMissingUsername@email") + .removeDetail(Details.USERNAME) + .removeDetail(Details.EMAIL) + .error("invalid_registration").assertEvent(); + } + + @Test + public void registerUserMissingEmail() { loginPage.open(); loginPage.clickRegister(); registerPage.assertCurrent(); registerPage.register("firstName", "lastName", null, "registerUserMissingEmail", "password", "password"); registerPage.assertCurrent(); - Assert.assertEquals("Please specify email.", registerPage.getError()); + assertEquals("Please specify email.", registerPage.getError()); events.expectRegister("registerUserMissingEmail", null) .removeDetail("email") .error("invalid_registration").assertEvent(); + } + + @Test + public void registerUserInvalidEmail() { + loginPage.open(); + loginPage.clickRegister(); + registerPage.assertCurrent(); registerPage.register("firstName", "lastName", "registerUserInvalidEmailemail", "registerUserInvalidEmail", "password", "password"); registerPage.assertCurrent(); - Assert.assertEquals("Invalid email address.", registerPage.getError()); + assertEquals("registerUserInvalidEmailemail", registerPage.getEmail()); + assertEquals("Invalid email address.", registerPage.getError()); events.expectRegister("registerUserInvalidEmail", "registerUserInvalidEmailemail") .error("invalid_registration").assertEvent(); } @@ -222,7 +251,7 @@ public class RegisterTest { registerPage.register("firstName", "lastName", "registerUserSuccess@email", "registerUserSuccess", "password", "password"); - Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); + assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); String userId = events.expectRegister("registerUserSuccess", "registerUserSuccess@email").assertEvent().getUserId(); events.expectLogin().detail("username", "registerusersuccess").user(userId).assertEvent(); @@ -233,10 +262,10 @@ public class RegisterTest { // test that timestamp is current with 10s tollerance Assert.assertTrue((System.currentTimeMillis() - user.getCreatedTimestamp()) < 10000); // test user info is set from form - Assert.assertEquals("registerusersuccess", user.getUsername()); - Assert.assertEquals("registerusersuccess@email", user.getEmail()); - Assert.assertEquals("firstName", user.getFirstName()); - Assert.assertEquals("lastName", user.getLastName()); + assertEquals("registerusersuccess", user.getUsername()); + assertEquals("registerusersuccess@email", user.getEmail()); + assertEquals("firstName", user.getFirstName()); + assertEquals("lastName", user.getLastName()); } protected UserModel getUser(String userId) { @@ -261,9 +290,9 @@ public class RegisterTest { registerPage.registerWithEmailAsUsername("firstName", "lastName", "test-user@localhost", "password", "password"); registerPage.assertCurrent(); - Assert.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); } @@ -280,12 +309,12 @@ public class RegisterTest { registerPage.registerWithEmailAsUsername("firstName", "lastName", null, "password", "password"); registerPage.assertCurrent(); - Assert.assertEquals("Please specify email.", registerPage.getError()); + assertEquals("Please specify email.", registerPage.getError()); events.expectRegister(null, null).removeDetail("username").removeDetail("email").error("invalid_registration").assertEvent(); registerPage.registerWithEmailAsUsername("firstName", "lastName", "registerUserInvalidEmailemail", "password", "password"); registerPage.assertCurrent(); - Assert.assertEquals("Invalid email address.", registerPage.getError()); + assertEquals("Invalid email address.", registerPage.getError()); events.expectRegister("registerUserInvalidEmailemail", "registerUserInvalidEmailemail").error("invalid_registration").assertEvent(); } finally { configureRelamRegistrationEmailAsUsername(false); @@ -303,7 +332,7 @@ public class RegisterTest { registerPage.registerWithEmailAsUsername("firstName", "lastName", "registerUserSuccessE@email", "password", "password"); - Assert.assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); + assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType()); String userId = events.expectRegister("registerUserSuccessE@email", "registerUserSuccessE@email").assertEvent().getUserId(); events.expectLogin().detail("username", "registerusersuccesse@email").user(userId).assertEvent();