From b0e3013bed9a7c215b7c1d227617767b895fe0e8 Mon Sep 17 00:00:00 2001 From: Vilmos Nagy Date: Wed, 5 Apr 2023 17:00:09 +0200 Subject: [PATCH 1/5] BUG: Inconsistent users created in the database when there's an error in registration Adding a failing test from #17644. The #19488 PR seems not to solve it --- .../federation/storage/UserStorageTest.java | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java index 77df0a9dd138..fe468f7ff619 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java @@ -44,6 +44,8 @@ import org.keycloak.testsuite.federation.UserPropertyFileStorageFactory; import org.keycloak.testsuite.pages.AppPage; import org.keycloak.testsuite.pages.LoginPage; +import org.keycloak.testsuite.pages.LoginPasswordResetPage; +import org.keycloak.testsuite.pages.ErrorPage; import org.keycloak.testsuite.pages.RegisterPage; import org.keycloak.testsuite.pages.VerifyEmailPage; import org.keycloak.testsuite.updaters.RealmAttributeUpdater; @@ -116,6 +118,12 @@ public class UserStorageTest extends AbstractAuthTest { @Page protected RegisterPage registerPage; + @Page + protected LoginPasswordResetPage resetPage; + + @Page + protected ErrorPage errorPage; + @Page protected VerifyEmailPage verifyEmailPage; @@ -1065,6 +1073,69 @@ public void testCRUDCredentialsOfDifferentUser() { Assert.assertTrue(ObjectUtil.isEqualOrBothNull(otpCredential.getPriority(), otpCredentialLoaded.getPriority())); } + @Test + public void testRegisterShouldFailBeforeUserCreationWhenUserIsInContext() throws Exception { + try (AutoCloseable c = new RealmAttributeUpdater(testRealmResource()) + .updateWith(r -> { + Map config = new HashMap<>(); + config.put("from", "auto@keycloak.org"); + config.put("host", "localhost"); + config.put("port", "3025"); + r.setSmtpServer(config); + r.setRegistrationAllowed(true); + r.setVerifyEmail(true); + r.setResetPasswordAllowed(true); + r.setRegistrationEmailAsUsername(true); + }) + .update()) { + + UserRepresentation userWhoPreExistsInRealm = new UserRepresentation(); + userWhoPreExistsInRealm.setEmail("keycloak-dev@realcity.io"); + ApiUtil.createUserAndResetPasswordWithAdminClient(testRealmResource(), userWhoPreExistsInRealm, "password"); + + loginPage.open(); + loginPage.clickRegister(); + registerPage.clickBackToLogin(); + loginPage.assertCurrent(testRealmResource().toRepresentation().getRealm()); + + loginPage.resetPassword(); + resetPage.assertCurrent(); + resetPage.changePassword("keycloak-dev@realcity.io"); + + driver.navigate().back(); + driver.navigate().back(); + driver.navigate().back(); + registerPage.assertCurrent(); + + registerPage.registerWithEmailAsUsername( + "Vilmos", + "Szabó-Nagy", + "vilmos.nagy@realcity.io", + "TestPassword123", + "TestPassword123" + ); + + if (errorPage.isCurrent()) { + // in this case the error page is shown + errorPage.assertCurrent(); + + // yet, the user is created in the database + final UserRepresentation userByUsername = ApiUtil.findUserByUsername(testRealmResource(), "vilmos.nagy@realcity.io"); + if (userByUsername != null) { + // if the user was created then the user should have a password + assertNotNull("The user is created even when an error page was shown, yet the user has no password", userByUsername.getCredentials()); + assertFalse("The user is created even when an error page was shown, yet the user has no password", userByUsername.getCredentials().isEmpty()); + } + } else { + throw new UnsupportedOperationException("If someone ever refactors the reset password flow, and the previous steps no more cause an error, " + + "then we should check the following: \n" + + " - the newly created user exists\n" + + " - the registration flow is executed until the last step (eg.: the user has a password)\n" + + " - no error page was shown"); + } + } + } + private void assertOrder(List creds, String... expectedIds) { org.keycloak.testsuite.Assert.assertEquals(expectedIds.length, creds.size()); From 2e39ead8c7cda25736e181827765fef0c25ef4cf Mon Sep 17 00:00:00 2001 From: Vilmos Nagy Date: Fri, 14 Apr 2023 17:28:01 +0200 Subject: [PATCH 2/5] Possible fix for #17644 --- .../org/keycloak/authentication/FormContext.java | 4 +++- .../authentication/FormAuthenticationFlow.java | 1 + .../federation/storage/UserStorageTest.java | 15 ++++++++++----- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/authentication/FormContext.java b/server-spi-private/src/main/java/org/keycloak/authentication/FormContext.java index fb340ca640dd..f4a5bbb938aa 100755 --- a/server-spi-private/src/main/java/org/keycloak/authentication/FormContext.java +++ b/server-spi-private/src/main/java/org/keycloak/authentication/FormContext.java @@ -58,7 +58,7 @@ public interface FormContext { AuthenticationExecutionModel getExecution(); /** - * Current user attached to this flow. It can return null if no uesr has been identified yet + * Current user attached to this flow. It can return null if no user has been identified yet * * @return */ @@ -66,6 +66,8 @@ public interface FormContext { /** * Attach a specific user to this flow. + * + * If there was another user attached to this flow calling this method overrides the previous setting. * * @param user */ diff --git a/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java b/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java index 2a0569f35714..5822cd288fc3 100755 --- a/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java +++ b/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java @@ -101,6 +101,7 @@ public UserModel getUser() { @Override public void setUser(UserModel user) { + processor.clearAuthenticatedUser(); processor.setAutheticatedUser(user); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java index fe468f7ff619..0d19282ff477 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java @@ -1105,6 +1105,8 @@ public void testRegisterShouldFailBeforeUserCreationWhenUserIsInContext() throws driver.navigate().back(); driver.navigate().back(); driver.navigate().back(); + + int greenMailMessageCountBeforeRegistrer = greenMail.getReceivedMessages().length; registerPage.assertCurrent(); registerPage.registerWithEmailAsUsername( @@ -1127,11 +1129,14 @@ public void testRegisterShouldFailBeforeUserCreationWhenUserIsInContext() throws assertFalse("The user is created even when an error page was shown, yet the user has no password", userByUsername.getCredentials().isEmpty()); } } else { - throw new UnsupportedOperationException("If someone ever refactors the reset password flow, and the previous steps no more cause an error, " + - "then we should check the following: \n" + - " - the newly created user exists\n" + - " - the registration flow is executed until the last step (eg.: the user has a password)\n" + - " - no error page was shown"); + verifyEmailPage.assertCurrent(); + + Assert.assertEquals(1+greenMailMessageCountBeforeRegistrer, greenMail.getReceivedMessages().length); + MimeMessage message = greenMail.getReceivedMessages()[greenMailMessageCountBeforeRegistrer]; + String verificationUrl = getPasswordResetEmailLink(message); + + driver.navigate().to(verificationUrl.trim()); + appPage.assertCurrent(); } } } From ba65d0a4e4edc7d106d27e7a515180e2f16b5a1a Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Fri, 1 Sep 2023 13:26:10 +0200 Subject: [PATCH 3/5] Reworked fix, as it shouldn't remove the safety condition --- .../keycloak/authentication/FormAuthenticationFlow.java | 1 - .../authentication/forms/RegistrationUserCreation.java | 7 +++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java b/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java index 5822cd288fc3..2a0569f35714 100755 --- a/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java +++ b/services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java @@ -101,7 +101,6 @@ public UserModel getUser() { @Override public void setUser(UserModel user) { - processor.clearAuthenticatedUser(); processor.setAutheticatedUser(user); } 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 84b33afd4226..5320926a57bb 100755 --- a/services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java +++ b/services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java @@ -18,6 +18,8 @@ package org.keycloak.authentication.forms; import org.keycloak.Config; +import org.keycloak.authentication.AuthenticationFlowError; +import org.keycloak.authentication.AuthenticationFlowException; import org.keycloak.authentication.FormAction; import org.keycloak.authentication.FormActionFactory; import org.keycloak.authentication.FormContext; @@ -111,6 +113,11 @@ public void buildPage(FormContext context, LoginFormsProvider form) { @Override public void success(FormContext context) { + if (context.getUser() != null) { + // the user probably did some back navigation in the browser, hitting this page in a strange state + throw new AuthenticationFlowException(AuthenticationFlowError.USER_CONFLICT); + } + MultivaluedMap formData = context.getHttpRequest().getDecodedFormParameters(); String email = formData.getFirst(UserModel.EMAIL); From b5af7ba598c07a844568bd3321d637975f6def9e Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Fri, 1 Sep 2023 14:37:46 +0200 Subject: [PATCH 4/5] Updated error message to be "Login timeout. Please sign in again." --- .../keycloak/authentication/forms/RegistrationUserCreation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5320926a57bb..42152ec93c20 100755 --- a/services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java +++ b/services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java @@ -115,7 +115,7 @@ public void buildPage(FormContext context, LoginFormsProvider form) { public void success(FormContext context) { if (context.getUser() != null) { // the user probably did some back navigation in the browser, hitting this page in a strange state - throw new AuthenticationFlowException(AuthenticationFlowError.USER_CONFLICT); + throw new AuthenticationFlowException(AuthenticationFlowError.EXPIRED_CODE); } MultivaluedMap formData = context.getHttpRequest().getDecodedFormParameters(); From 1a6c9cfbb375488cbd14dfeb545189f44958dfdd Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Tue, 5 Sep 2023 10:35:54 +0200 Subject: [PATCH 5/5] Fixing Javadoc --- .../src/main/java/org/keycloak/authentication/FormContext.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/authentication/FormContext.java b/server-spi-private/src/main/java/org/keycloak/authentication/FormContext.java index f4a5bbb938aa..f901af097951 100755 --- a/server-spi-private/src/main/java/org/keycloak/authentication/FormContext.java +++ b/server-spi-private/src/main/java/org/keycloak/authentication/FormContext.java @@ -66,8 +66,6 @@ public interface FormContext { /** * Attach a specific user to this flow. - * - * If there was another user attached to this flow calling this method overrides the previous setting. * * @param user */