Skip to content

Fix #17644, the registration either should finish properly or not create the user at all #17646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vilmosnagy wants to merge 1 commit into from
Closed

Fix #17644, the registration either should finish properly or not create the user at all #17646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ public String getFlowPath() {
return flowPath;
}

public void setAutheticatedUser(UserModel user) {
public void setAuthenticatedUser(UserModel user) {
UserModel previousUser = getAuthenticationSession().getAuthenticatedUser();
if (previousUser != null && !user.getId().equals(previousUser.getId()))
throw new AuthenticationFlowException(AuthenticationFlowError.USER_CONFLICT);
Expand Down Expand Up @@ -431,7 +431,7 @@ public UserModel getUser() {

@Override
public void setUser(UserModel user) {
setAutheticatedUser(user);
setAuthenticatedUser(user);
}

@Override
Expand Down
15 changes: 12 additions & 3 deletions services/src/main/java/org/keycloak/authentication/FormAuthenticationFlow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.keycloak.models.ClientModel;
import org.keycloak.models.Constants;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.KeycloakTransactionManager;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserModel;
import org.keycloak.models.utils.FormMessage;
Expand Down Expand Up @@ -101,7 +102,7 @@ public UserModel getUser() {

@Override
public void setUser(UserModel user) {
processor.setAutheticatedUser(user);
processor.setAuthenticatedUser(user);
}

@Override
Expand Down Expand Up @@ -248,8 +249,16 @@ public Response processAction(String actionExecution) {
return renderForm(first.formData, messages);
}

for (ValidationContextImpl context : successes) {
context.action.success(context);
KeycloakTransactionManager transaction = processor.getSession().getTransactionManager();
transaction.begin();
try {
for (ValidationContextImpl context : successes) {
context.action.success(context);
}
transaction.commit();
} catch (Exception e) {
transaction.rollback();
throw e;
}
// set status and required actions only if form is fully successful
for (Map.Entry<String, AuthenticationSessionModel.ExecutionStatus> entry : executionStatus.entrySet()) {
Expand Down
7 changes: 7 additions & 0 deletions services/src/main/java/org/keycloak/authentication/forms/RegistrationUserCreation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -111,6 +113,11 @@ public void buildPage(FormContext context, LoginFormsProvider form) {

@Override
public void success(FormContext context) {
// fail early, fail fast
if (context.getUser() != null) {
throw new AuthenticationFlowException(AuthenticationFlowError.USER_CONFLICT);
}

MultivaluedMap<String, String> formData = context.getHttpRequest().getDecodedFormParameters();

String email = formData.getFirst(UserModel.EMAIL);
Expand Down
62 changes: 61 additions & 1 deletion ...n/tests/base/src/test/java/org/keycloak/testsuite/federation/storage/UserStorageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@
import org.keycloak.testsuite.federation.UserMapStorage;
import org.keycloak.testsuite.federation.UserMapStorageFactory;
import org.keycloak.testsuite.federation.UserPropertyFileStorageFactory;
import org.keycloak.testsuite.pages.ErrorPage;
import org.keycloak.testsuite.pages.LoginPage;
import org.keycloak.testsuite.pages.LoginPasswordResetPage;
import org.keycloak.testsuite.pages.RegisterPage;
import org.keycloak.testsuite.pages.VerifyEmailPage;
import org.keycloak.testsuite.updaters.RealmAttributeUpdater;
Expand Down Expand Up @@ -73,12 +75,12 @@
import static java.util.Calendar.DAY_OF_WEEK;
import static java.util.Calendar.HOUR_OF_DAY;
import static java.util.Calendar.MINUTE;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
Expand Down Expand Up @@ -113,6 +115,12 @@ public class UserStorageTest extends AbstractAuthTest {
@Page
protected LoginPage loginPage;

@Page
protected LoginPasswordResetPage resetPage;

@Page
protected ErrorPage errorPage;

@Page
protected RegisterPage registerPage;

Expand Down Expand Up @@ -365,6 +373,58 @@ public void testUpdate() {
}
assertFalse(foundRole);
}

@Test
public void testRegisterShouldFailBeforeUserCreationWhenUserIsInContext() throws Exception {
try (AutoCloseable c = new RealmAttributeUpdater(testRealmResource())
.updateWith(r -> {
Map<String, String> 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");
String uid = ApiUtil.createUserAndResetPasswordWithAdminClient(testRealmResource(), userWhoPreExistsInRealm, "password");

testRealmAccountPage.navigateTo();
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"
);

errorPage.assertCurrent();
assertNotNull("Some error happens", errorPage.getError());

// the user entity should not be created if the registration flow is not executed till the end
final UserRepresentation userByUsername = ApiUtil.findUserByUsername(testRealmResource(), "vilmos.nagy@realcity.io");
assertNull(userByUsername);
}

}

@Test
public void testRegisterWithRequiredEmail() throws Exception {
Expand Down