Skip to content

Commit b9a7152

Browse files
pedroigorpedroigor
committed
Avoid commiting the transaction prematurely when creating users through the User API
Closes keycloak#28217 Signed-off-by: Pedro Igor <[email protected]>
1 parent fe538cb commit b9a7152

File tree

7 files changed

+72
-25
lines changed

7 files changed

+72
-25
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222

2323
import org.jboss.logging.Logger;
2424
import org.keycloak.models.AbstractKeycloakTransaction;
25-
import org.keycloak.models.ModelException;
25+
import org.keycloak.models.ModelValidationException;
2626
import org.keycloak.storage.ldap.LDAPStorageProvider;
2727
import org.keycloak.storage.ldap.idm.model.LDAPObject;
2828

@@ -51,7 +51,7 @@ protected void commitImpl() {
5151
logger.trace("Transaction commit! Updating LDAP attributes for object " + ldapUser.getDn() + ", attributes: " + ldapUser.getAttributes());
5252
}
5353
if (ldapUser.isWaitingForExecutionOnMandatoryAttributesComplete()) {
54-
throw new ModelException("LDAPObject cannot be commited because some mandatory attributes are not set: "
54+
throw new ModelValidationException("LDAPObject cannot be commited because some mandatory attributes are not set: "
5555
+ ldapUser.getMandatoryAttributeNamesRemaining());
5656
}
5757

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/*
2+
* Copyright 2024 Red Hat, Inc. and/or its affiliates
3+
* and other contributors as indicated by the @author tags.
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
package org.keycloak.models;
19+
20+
/**
21+
* <p>Thrown to indicate that an error is expected as a result of the validations run against a model. Such
22+
* exceptions are not considered internal server errors but an expected error when validating a model where the client
23+
* has the opportunity to fix and retry the request.
24+
*
25+
* <p>Some validations can only happen during the commit phase and should not be handled as an unknown error by the default exception
26+
* handling mechanism.
27+
*/
28+
public class ModelValidationException extends ModelException {
29+
30+
public ModelValidationException(String message) {
31+
super(message);
32+
}
33+
}

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import jakarta.ws.rs.ext.Provider;
3636
import java.util.Objects;
3737
import org.keycloak.models.KeycloakSession;
38+
import org.keycloak.models.ModelException;
3839
import org.keycloak.models.OrganizationModel;
3940
import org.keycloak.models.RealmModel;
4041
import org.keycloak.models.UserModel;
@@ -88,17 +89,19 @@ public Response addMember(UserRepresentation rep) {
8889
Response response = usersResource.createUser(rep);
8990

9091
if (Status.CREATED.getStatusCode() == response.getStatus()) {
91-
return KeycloakModelUtils.runJobInTransactionWithResult(session.getKeycloakSessionFactory(), session.getContext(), session -> {
92-
RealmModel realm = session.getContext().getRealm();
93-
UserModel member = session.users().getUserByUsername(realm, rep.getEmail());
94-
OrganizationProvider provider = session.getProvider(OrganizationProvider.class);
92+
RealmModel realm = session.getContext().getRealm();
93+
UserModel member = session.users().getUserByUsername(realm, rep.getEmail());
94+
OrganizationProvider provider = session.getProvider(OrganizationProvider.class);
9595

96+
try {
9697
if (provider.addOrganizationMember(realm, organization, member)) {
9798
return Response.created(session.getContext().getUri().getAbsolutePathBuilder().path(member.getId()).build()).build();
9899
}
100+
} catch (ModelException me) {
101+
throw new BadRequestException(me.getMessage());
102+
}
99103

100-
throw new BadRequestException();
101-
});
104+
throw new BadRequestException();
102105
}
103106

104107
return response;

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.keycloak.models.KeycloakSessionTaskWithResult;
1414
import org.keycloak.models.KeycloakTransaction;
1515
import org.keycloak.models.ModelDuplicateException;
16+
import org.keycloak.models.ModelValidationException;
1617
import org.keycloak.models.RealmModel;
1718
import org.keycloak.models.utils.KeycloakModelUtils;
1819
import org.keycloak.representations.idm.OAuth2ErrorRepresentation;
@@ -121,7 +122,8 @@ private static int getStatusCode(Throwable throwable) {
121122
WebApplicationException ex = (WebApplicationException) throwable;
122123
status = ex.getResponse().getStatus();
123124
}
124-
if (throwable instanceof JsonProcessingException) {
125+
if (throwable instanceof JsonProcessingException
126+
|| throwable instanceof ModelValidationException) {
125127
status = Response.Status.BAD_REQUEST.getStatusCode();
126128
}
127129

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

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -152,25 +152,12 @@ public Response createUser(final UserRepresentation rep) {
152152
RepresentationToModel.createCredentials(rep, session, realm, user, true);
153153
adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri(), user.getId()).representation(rep).success();
154154

155-
if (session.getTransactionManager().isActive()) {
156-
session.getTransactionManager().commit();
157-
}
158-
159155
return Response.created(session.getContext().getUri().getAbsolutePathBuilder().path(user.getId()).build()).build();
160156
} catch (ModelDuplicateException e) {
161-
if (session.getTransactionManager().isActive()) {
162-
session.getTransactionManager().setRollbackOnly();
163-
}
164157
throw ErrorResponse.exists("User exists with same username or email");
165158
} catch (PasswordPolicyNotMetException e) {
166-
if (session.getTransactionManager().isActive()) {
167-
session.getTransactionManager().setRollbackOnly();
168-
}
169159
throw ErrorResponse.error("Password policy not met", Response.Status.BAD_REQUEST);
170160
} catch (ModelException me){
171-
if (session.getTransactionManager().isActive()) {
172-
session.getTransactionManager().setRollbackOnly();
173-
}
174161
logger.warn("Could not create user", me);
175162
throw ErrorResponse.error("Could not create user", Response.Status.BAD_REQUEST);
176163
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,8 @@
4141
import org.keycloak.representations.AccessToken;
4242
import org.keycloak.representations.idm.ComponentRepresentation;
4343
import org.keycloak.representations.idm.CredentialRepresentation;
44-
import org.keycloak.representations.idm.ErrorRepresentation;
4544
import org.keycloak.representations.idm.EventRepresentation;
45+
import org.keycloak.representations.idm.OAuth2ErrorRepresentation;
4646
import org.keycloak.representations.idm.RealmRepresentation;
4747
import org.keycloak.representations.idm.UserRepresentation;
4848
import org.keycloak.services.managers.RealmManager;
@@ -215,8 +215,8 @@ public void testSyncRegistrationEmailRDNNoDefault() {
215215
UserRepresentation newUser1 = AbstractAuthTest.createUserRepresentation("newuser1", null, "newuser1", "newuser1", true);
216216
try (Response resp = testRealm().users().create(newUser1)) {
217217
Assert.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), resp.getStatus());
218-
ErrorRepresentation error = resp.readEntity(ErrorRepresentation.class);
219-
Assert.assertEquals("Could not create user", error.getErrorMessage());
218+
OAuth2ErrorRepresentation error = resp.readEntity(OAuth2ErrorRepresentation.class);
219+
Assert.assertEquals("unknown_error", error.getError());
220220
}
221221
Assert.assertTrue(testRealm().users().search("newuser1").isEmpty());
222222

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import static org.junit.Assert.assertNotNull;
2323
import static org.junit.Assert.assertTrue;
2424
import static org.junit.Assert.fail;
25+
import static org.keycloak.models.OrganizationModel.USER_ORGANIZATION_ATTRIBUTE;
2526

2627
import java.util.ArrayList;
2728
import java.util.List;
@@ -35,6 +36,9 @@
3536
import org.keycloak.common.Profile.Feature;
3637
import org.keycloak.representations.idm.OrganizationRepresentation;
3738
import org.keycloak.representations.idm.UserRepresentation;
39+
import org.keycloak.representations.userprofile.config.UPConfig;
40+
import org.keycloak.representations.userprofile.config.UPConfig.UnmanagedAttributePolicy;
41+
import org.keycloak.testsuite.admin.ApiUtil;
3842
import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
3943

4044
@EnableFeature(Feature.ORGANIZATION)
@@ -62,6 +66,24 @@ public void testUpdate() {
6266
assertEquals(expected.getLastName(), existing.getLastName());
6367
}
6468

69+
@Test
70+
public void testFailCreateUser() {
71+
UPConfig upConfig = testRealm().users().userProfile().getConfiguration();
72+
upConfig.setUnmanagedAttributePolicy(UnmanagedAttributePolicy.ENABLED);
73+
testRealm().users().userProfile().update(upConfig);
74+
OrganizationResource organization = testRealm().organizations().get(createOrganization().getId());
75+
UserRepresentation expected = new UserRepresentation();
76+
77+
expected.setEmail("[email protected]");
78+
expected.setUsername(expected.getEmail());
79+
expected.singleAttribute(USER_ORGANIZATION_ATTRIBUTE, "invalid");
80+
81+
try (Response response = organization.members().addMember(expected)) {
82+
assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());
83+
assertTrue(testRealm().users().search("[email protected]").isEmpty());
84+
}
85+
}
86+
6587
@Test
6688
public void testGet() {
6789
OrganizationResource organization = testRealm().organizations().get(createOrganization().getId());

0 commit comments

Comments
 (0)