From b9a7152a2911c9ca0f9f9d44de2695372b4b72b9 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Tue, 26 Mar 2024 18:15:41 -0300 Subject: [PATCH] Avoid commiting the transaction prematurely when creating users through the User API Closes #28217 Signed-off-by: Pedro Igor --- .../storage/ldap/mappers/LDAPTransaction.java | 4 +-- .../models/ModelValidationException.java | 33 +++++++++++++++++++ .../resource/OrganizationMemberResource.java | 15 +++++---- .../services/error/KeycloakErrorHandler.java | 4 ++- .../resources/admin/UsersResource.java | 13 -------- .../ldap/LDAPProvidersIntegrationTest.java | 6 ++-- .../admin/OrganizationMemberTest.java | 22 +++++++++++++ 7 files changed, 72 insertions(+), 25 deletions(-) create mode 100644 server-spi/src/main/java/org/keycloak/models/ModelValidationException.java diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/LDAPTransaction.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/LDAPTransaction.java index 78e75624e7..1bbb48f763 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/LDAPTransaction.java +++ b/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()); } diff --git a/server-spi/src/main/java/org/keycloak/models/ModelValidationException.java b/server-spi/src/main/java/org/keycloak/models/ModelValidationException.java new file mode 100644 index 0000000000..2e1f5c90df --- /dev/null +++ b/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; + +/** + *

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. + * + *

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); + } +} diff --git a/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationMemberResource.java b/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationMemberResource.java index e5d8e08f50..6c34d9d1a0 100644 --- a/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationMemberResource.java +++ b/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(); - UserModel member = session.users().getUserByUsername(realm, rep.getEmail()); - OrganizationProvider provider = session.getProvider(OrganizationProvider.class); + RealmModel realm = session.getContext().getRealm(); + UserModel member = session.users().getUserByUsername(realm, rep.getEmail()); + 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; diff --git a/services/src/main/java/org/keycloak/services/error/KeycloakErrorHandler.java b/services/src/main/java/org/keycloak/services/error/KeycloakErrorHandler.java index e547f7eda1..9ddea7acf6 100644 --- a/services/src/main/java/org/keycloak/services/error/KeycloakErrorHandler.java +++ b/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 { 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(); } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java index 48bb987301..ea6e2da1e7 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java +++ b/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); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java index 2a7459ab1b..e847039cf9 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java +++ b/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); - Assert.assertEquals("Could not create user", error.getErrorMessage()); + OAuth2ErrorRepresentation error = resp.readEntity(OAuth2ErrorRepresentation.class); + Assert.assertEquals("unknown_error", error.getError()); } Assert.assertTrue(testRealm().users().search("newuser1").isEmpty()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationMemberTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationMemberTest.java index 9065d0bb34..796a07b9cb 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationMemberTest.java +++ b/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());