Avoid commiting the transaction prematurely when creating users through the User API

Closes #28217

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/LDAPTransaction.java

@@ -22,7 +22,7 @@ import java.util.Set;
 
 import org.jboss.logging.Logger;
 import org.keycloak.models.AbstractKeycloakTransaction;
-import org.keycloak.models.ModelException;
+import org.keycloak.models.ModelValidationException;
 import org.keycloak.storage.ldap.LDAPStorageProvider;
 import org.keycloak.storage.ldap.idm.model.LDAPObject;
 
@@ -51,7 +51,7 @@ public class LDAPTransaction extends AbstractKeycloakTransaction {
             logger.trace("Transaction commit! Updating LDAP attributes for object " + ldapUser.getDn() + ", attributes: " + ldapUser.getAttributes());
         }
         if (ldapUser.isWaitingForExecutionOnMandatoryAttributesComplete()) {
-            throw new ModelException("LDAPObject cannot be commited because some mandatory attributes are not set: "
+            throw new ModelValidationException("LDAPObject cannot be commited because some mandatory attributes are not set: "
                     + ldapUser.getMandatoryAttributeNamesRemaining());
         }
 

server-spi/src/main/java/org/keycloak/models/ModelValidationException.java

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2024 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.keycloak.models;
+
+/**
+ * <p>Thrown to indicate that an error is expected as a result of the validations run against a model. Such
+ * exceptions are not considered internal server errors but an expected error when validating a model where the client
+ * has the opportunity to fix and retry the request.
+ *
+ * <p>Some validations can only happen during the commit phase and should not be handled as an unknown error by the default exception
+ * handling mechanism.
+ */
+public class ModelValidationException extends ModelException {
+
+    public ModelValidationException(String message) {
+        super(message);
+    }
+}

services/src/main/java/org/keycloak/organization/admin/resource/OrganizationMemberResource.java

@@ -35,6 +35,7 @@ import jakarta.ws.rs.core.Response.Status;
 import jakarta.ws.rs.ext.Provider;
 import java.util.Objects;
 import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.ModelException;
 import org.keycloak.models.OrganizationModel;
 import org.keycloak.models.RealmModel;
 import org.keycloak.models.UserModel;
@@ -88,17 +89,19 @@ public class OrganizationMemberResource {
         Response response = usersResource.createUser(rep);
 
         if (Status.CREATED.getStatusCode() == response.getStatus()) {
-            return KeycloakModelUtils.runJobInTransactionWithResult(session.getKeycloakSessionFactory(), session.getContext(), session -> {
+            RealmModel realm = session.getContext().getRealm();
-                RealmModel realm = session.getContext().getRealm();
+            UserModel member = session.users().getUserByUsername(realm, rep.getEmail());
-                UserModel member = session.users().getUserByUsername(realm, rep.getEmail());
+            OrganizationProvider provider = session.getProvider(OrganizationProvider.class);
-                OrganizationProvider provider = session.getProvider(OrganizationProvider.class);
 
+            try {
                 if (provider.addOrganizationMember(realm, organization, member)) {
                     return Response.created(session.getContext().getUri().getAbsolutePathBuilder().path(member.getId()).build()).build();
                 }
+            } catch (ModelException me) {
+                throw new BadRequestException(me.getMessage());
+            }
 
-                throw new BadRequestException();
+            throw new BadRequestException();
-            });
         }
 
         return response;

services/src/main/java/org/keycloak/services/error/KeycloakErrorHandler.java

@@ -13,6 +13,7 @@ import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.KeycloakSessionTaskWithResult;
 import org.keycloak.models.KeycloakTransaction;
 import org.keycloak.models.ModelDuplicateException;
+import org.keycloak.models.ModelValidationException;
 import org.keycloak.models.RealmModel;
 import org.keycloak.models.utils.KeycloakModelUtils;
 import org.keycloak.representations.idm.OAuth2ErrorRepresentation;
@@ -121,7 +122,8 @@ public class KeycloakErrorHandler implements ExceptionMapper<Throwable> {
             WebApplicationException ex = (WebApplicationException) throwable;
             status = ex.getResponse().getStatus();
         }
-        if (throwable instanceof JsonProcessingException) {
+        if (throwable instanceof JsonProcessingException
+                || throwable instanceof ModelValidationException) {
             status = Response.Status.BAD_REQUEST.getStatusCode();
         }
 

services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java

@@ -152,25 +152,12 @@ public class UsersResource {
             RepresentationToModel.createCredentials(rep, session, realm, user, true);
             adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri(), user.getId()).representation(rep).success();
 
-            if (session.getTransactionManager().isActive()) {
-                session.getTransactionManager().commit();
-            }
-
             return Response.created(session.getContext().getUri().getAbsolutePathBuilder().path(user.getId()).build()).build();
         } catch (ModelDuplicateException e) {
-            if (session.getTransactionManager().isActive()) {
-                session.getTransactionManager().setRollbackOnly();
-            }
             throw ErrorResponse.exists("User exists with same username or email");
         } catch (PasswordPolicyNotMetException e) {
-            if (session.getTransactionManager().isActive()) {
-                session.getTransactionManager().setRollbackOnly();
-            }
             throw ErrorResponse.error("Password policy not met", Response.Status.BAD_REQUEST);
         } catch (ModelException me){
-            if (session.getTransactionManager().isActive()) {
-                session.getTransactionManager().setRollbackOnly();
-            }
             logger.warn("Could not create user", me);
             throw ErrorResponse.error("Could not create user", Response.Status.BAD_REQUEST);
         }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java

@@ -41,8 +41,8 @@ import org.keycloak.models.utils.KeycloakModelUtils;
 import org.keycloak.representations.AccessToken;
 import org.keycloak.representations.idm.ComponentRepresentation;
 import org.keycloak.representations.idm.CredentialRepresentation;
-import org.keycloak.representations.idm.ErrorRepresentation;
 import org.keycloak.representations.idm.EventRepresentation;
+import org.keycloak.representations.idm.OAuth2ErrorRepresentation;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.representations.idm.UserRepresentation;
 import org.keycloak.services.managers.RealmManager;
@@ -215,8 +215,8 @@ public class LDAPProvidersIntegrationTest extends AbstractLDAPTest {
             UserRepresentation newUser1 = AbstractAuthTest.createUserRepresentation("newuser1", null, "newuser1", "newuser1", true);
             try (Response resp = testRealm().users().create(newUser1)) {
                 Assert.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), resp.getStatus());
-                ErrorRepresentation error = resp.readEntity(ErrorRepresentation.class);
+                OAuth2ErrorRepresentation error = resp.readEntity(OAuth2ErrorRepresentation.class);
-                Assert.assertEquals("Could not create user", error.getErrorMessage());
+                Assert.assertEquals("unknown_error", error.getError());
             }
             Assert.assertTrue(testRealm().users().search("newuser1").isEmpty());
 

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationMemberTest.java

@@ -22,6 +22,7 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.keycloak.models.OrganizationModel.USER_ORGANIZATION_ATTRIBUTE;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -35,6 +36,9 @@ import org.keycloak.admin.client.resource.OrganizationResource;
 import org.keycloak.common.Profile.Feature;
 import org.keycloak.representations.idm.OrganizationRepresentation;
 import org.keycloak.representations.idm.UserRepresentation;
+import org.keycloak.representations.userprofile.config.UPConfig;
+import org.keycloak.representations.userprofile.config.UPConfig.UnmanagedAttributePolicy;
+import org.keycloak.testsuite.admin.ApiUtil;
 import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
 
 @EnableFeature(Feature.ORGANIZATION)
@@ -62,6 +66,24 @@ public class OrganizationMemberTest extends AbstractOrganizationTest {
         assertEquals(expected.getLastName(), existing.getLastName());
     }
 
+    @Test
+    public void testFailCreateUser() {
+        UPConfig upConfig = testRealm().users().userProfile().getConfiguration();
+        upConfig.setUnmanagedAttributePolicy(UnmanagedAttributePolicy.ENABLED);
+        testRealm().users().userProfile().update(upConfig);
+        OrganizationResource organization = testRealm().organizations().get(createOrganization().getId());
+        UserRepresentation expected = new UserRepresentation();
+
+        expected.setEmail("u@o.org");
+        expected.setUsername(expected.getEmail());
+        expected.singleAttribute(USER_ORGANIZATION_ATTRIBUTE, "invalid");
+
+        try (Response response = organization.members().addMember(expected)) {
+            assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());
+            assertTrue(testRealm().users().search("u@o.org").isEmpty());
+        }
+    }
+
     @Test
     public void testGet() {
         OrganizationResource organization = testRealm().organizations().get(createOrganization().getId());