From 221aad98770647ee8059b000ec4b0c32da899ba2 Mon Sep 17 00:00:00 2001 From: k-tamura Date: Tue, 14 Jan 2020 18:23:13 +0900 Subject: [PATCH] KEYCLOAK-11511 Improve exception handling of REST user creation --- .../resources/admin/UsersResource.java | 18 ++++++-- .../keycloak/testsuite/admin/UserTest.java | 45 +++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) 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 70b9b88471..551b70cbfe 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 @@ -21,6 +21,7 @@ import org.jboss.resteasy.annotations.cache.NoCache; import javax.ws.rs.NotFoundException; import org.jboss.resteasy.spi.ResteasyProviderFactory; import org.keycloak.common.ClientConnection; +import org.keycloak.common.util.ObjectUtil; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; import org.keycloak.models.Constants; @@ -31,6 +32,7 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.models.utils.RepresentationToModel; +import org.keycloak.policy.PasswordPolicyNotMetException; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.services.ErrorResponse; import org.keycloak.services.ForbiddenException; @@ -105,8 +107,13 @@ public class UsersResource { public Response createUser(final UserRepresentation rep) { auth.users().requireManage(); + String username = rep.getUsername(); + if (ObjectUtil.isBlank(username)) { + return ErrorResponse.error("User name is missing", Response.Status.BAD_REQUEST); + } + // Double-check duplicated username and email here due to federation - if (session.users().getUserByUsername(rep.getUsername(), realm) != null) { + if (session.users().getUserByUsername(username, realm) != null) { return ErrorResponse.exists("User exists with same username"); } if (rep.getEmail() != null && !realm.isDuplicateEmailsAllowed() && session.users().getUserByEmail(rep.getEmail(), realm) != null) { @@ -114,7 +121,7 @@ public class UsersResource { } try { - UserModel user = session.users().addUser(realm, rep.getUsername()); + UserModel user = session.users().addUser(realm, username); Set emptySet = Collections.emptySet(); UserResource.updateUserFromRep(user, rep, emptySet, realm, session, false); @@ -131,12 +138,17 @@ public class UsersResource { session.getTransactionManager().setRollbackOnly(); } return ErrorResponse.exists("User exists with same username or email"); + } catch (PasswordPolicyNotMetException e) { + if (session.getTransactionManager().isActive()) { + session.getTransactionManager().setRollbackOnly(); + } + return 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); - return ErrorResponse.exists("Could not create user"); + return ErrorResponse.error("Could not create user", Response.Status.BAD_REQUEST); } } /** diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java index bfd346bc9b..8fa8f0c8ad 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java @@ -438,6 +438,51 @@ public class UserTest extends AbstractAdminTest { assertEquals(user.getFederationLink(), createdUser.getFederationLink()); } + @Test + public void createUserWithoutUsername() { + UserRepresentation user = new UserRepresentation(); + user.setEmail("user1@localhost"); + Response response = realm.users().create(user); + assertEquals(400, response.getStatus()); + ErrorRepresentation error = response.readEntity(ErrorRepresentation.class); + Assert.assertEquals("User name is missing", error.getErrorMessage()); + response.close(); + } + + @Test + public void createUserWithEmptyUsername() { + UserRepresentation user = new UserRepresentation(); + user.setUsername(""); + user.setEmail("user2@localhost"); + Response response = realm.users().create(user); + assertEquals(400, response.getStatus()); + ErrorRepresentation error = response.readEntity(ErrorRepresentation.class); + Assert.assertEquals("User name is missing", error.getErrorMessage()); + response.close(); + } + + @Test + public void createUserWithInvalidPolicyPassword() { + RealmRepresentation rep = realm.toRepresentation(); + String passwordPolicy = rep.getPasswordPolicy(); + rep.setPasswordPolicy("length(8)"); + realm.update(rep); + UserRepresentation user = new UserRepresentation(); + user.setUsername("user4"); + user.setEmail("user4@localhost"); + CredentialRepresentation rawPassword = new CredentialRepresentation(); + rawPassword.setValue("ABCD"); + rawPassword.setType(CredentialRepresentation.PASSWORD); + user.setCredentials(Arrays.asList(rawPassword)); + Response response = realm.users().create(user); + assertEquals(400, response.getStatus()); + ErrorRepresentation error = response.readEntity(ErrorRepresentation.class); + Assert.assertEquals("Password policy not met", error.getErrorMessage()); + rep.setPasswordPolicy(passwordPolicy); + realm.update(rep); + response.close(); + } + private List createUsers() { List ids = new ArrayList<>();