Skip to content

Commit f970803

Browse files
rmartincpedroigor
rmartinc
authored and
pedroigor
committed
Check email and username for duplicated if isLoginWithEmailAllowed
Closes #27297 Signed-off-by: rmartinc <rmartinc@redhat.com>
1 parent 137907f commit f970803

File tree

9 files changed

+123
-31
lines changed

9 files changed

+123
-31
lines changed

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

+7
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@
7878
import org.keycloak.services.managers.BruteForceProtector;
7979
import org.keycloak.services.managers.UserConsentManager;
8080
import org.keycloak.services.managers.UserSessionManager;
81+
import org.keycloak.services.messages.Messages;
8182
import org.keycloak.services.resources.KeycloakOpenAPI;
8283
import org.keycloak.services.resources.LoginActionsService;
8384
import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator;
@@ -241,6 +242,12 @@ public static Response validateUserProfile(UserProfile profile, KeycloakSession
241242
} catch (ValidationException pve) {
242243
List<ErrorRepresentation> errors = new ArrayList<>();
243244
for (ValidationException.Error error : pve.getErrors()) {
245+
// some messages are managed directly as before
246+
switch (error.getMessage()) {
247+
case Messages.MISSING_USERNAME -> throw ErrorResponse.error("User name is missing", Response.Status.BAD_REQUEST);
248+
case Messages.USERNAME_EXISTS -> throw ErrorResponse.exists("User exists with same username");
249+
case Messages.EMAIL_EXISTS -> throw ErrorResponse.exists("User exists with same email");
250+
}
244251
errors.add(new ErrorRepresentation(error.getAttribute(), error.getMessage(), error.getMessageParameters()));
245252
}
246253

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

-18
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import org.jboss.resteasy.reactive.NoCache;
2525
import org.keycloak.common.ClientConnection;
2626
import org.keycloak.common.Profile;
27-
import org.keycloak.common.util.ObjectUtil;
2827
import org.keycloak.events.admin.OperationType;
2928
import org.keycloak.events.admin.ResourceType;
3029
import org.keycloak.models.Constants;
@@ -133,23 +132,6 @@ public Response createUser(final UserRepresentation rep) {
133132
if(realm.isRegistrationEmailAsUsername()) {
134133
username = rep.getEmail();
135134
}
136-
if (ObjectUtil.isBlank(username)) {
137-
throw ErrorResponse.error("User name is missing", Response.Status.BAD_REQUEST);
138-
}
139-
140-
// Double-check duplicated username and email here due to federation
141-
if (session.users().getUserByUsername(realm, username) != null) {
142-
throw ErrorResponse.exists("User exists with same username");
143-
}
144-
if (rep.getEmail() != null && !realm.isDuplicateEmailsAllowed()) {
145-
try {
146-
if(session.users().getUserByEmail(realm, rep.getEmail()) != null) {
147-
throw ErrorResponse.exists("User exists with same email");
148-
}
149-
} catch (ModelDuplicateException e) {
150-
throw ErrorResponse.exists("User exists with same email");
151-
}
152-
}
153135

154136
UserProfileProvider profileProvider = session.getProvider(UserProfileProvider.class);
155137

‎services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProviderFactory.java

+7-2
Original file line numberDiff line numberDiff line change
@@ -440,9 +440,14 @@ private UserProfileMetadata createUserResourceValidation(Config.Scope config) {
440440
UserProfileMetadata metadata = new UserProfileMetadata(USER_API);
441441

442442

443-
metadata.addAttribute(UserModel.USERNAME, -2, new AttributeValidatorMetadata(UsernameHasValueValidator.ID))
443+
metadata.addAttribute(UserModel.USERNAME, -2,
444+
new AttributeValidatorMetadata(UsernameHasValueValidator.ID),
445+
new AttributeValidatorMetadata(DuplicateUsernameValidator.ID))
444446
.addWriteCondition(DeclarativeUserProfileProviderFactory::editUsernameCondition);
445-
metadata.addAttribute(UserModel.EMAIL, -1, new AttributeValidatorMetadata(EmailValidator.ID, ValidatorConfig.builder().config(EmailValidator.IGNORE_EMPTY_VALUE, true).build()))
447+
metadata.addAttribute(UserModel.EMAIL, -1,
448+
new AttributeValidatorMetadata(DuplicateEmailValidator.ID),
449+
new AttributeValidatorMetadata(EmailExistsAsUsernameValidator.ID),
450+
new AttributeValidatorMetadata(EmailValidator.ID, ValidatorConfig.builder().config(EmailValidator.IGNORE_EMPTY_VALUE, true).build()))
446451
.addWriteCondition(DeclarativeUserProfileProviderFactory::editEmailCondition);
447452

448453
List<AttributeValidatorMetadata> readonlyValidators = new ArrayList<>();

‎services/src/main/java/org/keycloak/userprofile/validator/DuplicateEmailValidator.java

+7
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,13 @@ public ValidationContext validate(Object input, String inputHint, ValidationCont
7070
if (userByEmail != null && (user == null || !userByEmail.getId().equals(user.getId()))) {
7171
context.addError(new ValidationError(ID, inputHint, Messages.EMAIL_EXISTS)
7272
.setStatusCode(Response.Status.CONFLICT));
73+
} else if (realm.isLoginWithEmailAllowed()) {
74+
// check for duplicated username
75+
userByEmail = session.users().getUserByUsername(realm, value);
76+
if (userByEmail != null && (user == null || !userByEmail.getId().equals(user.getId()))) {
77+
context.addError(new ValidationError(ID, inputHint, Messages.EMAIL_EXISTS)
78+
.setStatusCode(Response.Status.CONFLICT));
79+
}
7380
}
7481
}
7582

‎services/src/main/java/org/keycloak/userprofile/validator/DuplicateUsernameValidator.java

+11-2
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,21 @@ public ValidationContext validate(Object input, String inputHint, ValidationCont
6464
KeycloakSession session = context.getSession();
6565
UserModel existing = session.users().getUserByUsername(session.getContext().getRealm(), value);
6666
UserModel user = UserProfileAttributeValidationContext.from(context).getAttributeContext().getUser();
67+
String valueLowercased = value.toLowerCase();
6768

68-
if (! KeycloakModelUtils.isUsernameCaseSensitive(session.getContext().getRealm())) value = value.toLowerCase();
69+
if (! KeycloakModelUtils.isUsernameCaseSensitive(session.getContext().getRealm())) value = valueLowercased;
6970

70-
if (user != null && !value.equals(user.getFirstAttribute(UserModel.USERNAME)) && (existing != null && !existing.getId().equals(user.getId()))) {
71+
RealmModel realm = session.getContext().getRealm();
72+
if (existing != null && (user == null || !existing.getId().equals(user.getId()))) {
7173
context.addError(new ValidationError(ID, inputHint, Messages.USERNAME_EXISTS)
7274
.setStatusCode(Response.Status.CONFLICT));
75+
} else if (realm.isLoginWithEmailAllowed() && value.indexOf('@') > 0) {
76+
// check the username does not collide with an email
77+
existing = session.users().getUserByEmail(realm, valueLowercased);
78+
if (existing != null && (user == null || !existing.getId().equals(user.getId()))) {
79+
context.addError(new ValidationError(ID, inputHint, Messages.USERNAME_EXISTS)
80+
.setStatusCode(Response.Status.CONFLICT));
81+
}
7382
}
7483

7584
return context;

‎testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateProfileTest.java

+50
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,56 @@ public void updateProfileDuplicatedEmail() {
332332
events.assertEmpty();
333333
}
334334

335+
@Test
336+
public void updateProfileDuplicateUsernameWithEmail() {
337+
getCleanup().addUserId(createUser(TEST_REALM_NAME, "user1@local.com", "password", "user1", "user1", "user1@local.org"));
338+
339+
loginPage.open();
340+
341+
loginPage.login("john-doh@localhost", "password");
342+
343+
updateProfilePage.assertCurrent();
344+
345+
updateProfilePage.prepareUpdate().username("user1@local.org").firstName("New first").lastName("New last").email("new@email.com").submit();
346+
347+
updateProfilePage.assertCurrent();
348+
349+
// assert that form holds submitted values during validation error
350+
Assert.assertEquals("New first", updateProfilePage.getFirstName());
351+
Assert.assertEquals("New last", updateProfilePage.getLastName());
352+
Assert.assertEquals("new@email.com", updateProfilePage.getEmail());
353+
Assert.assertEquals("user1@local.org", updateProfilePage.getUsername());
354+
355+
Assert.assertEquals("Username already exists.", updateProfilePage.getInputErrors().getUsernameError());
356+
357+
events.assertEmpty();
358+
}
359+
360+
@Test
361+
public void updateProfileDuplicatedEmailWithUsername() {
362+
getCleanup().addUserId(createUser(TEST_REALM_NAME, "user1@local.com", "password", "user1", "user1", "user1@local.org"));
363+
364+
loginPage.open();
365+
366+
loginPage.login("test-user@localhost", "password");
367+
368+
updateProfilePage.assertCurrent();
369+
370+
updateProfilePage.prepareUpdate().username("test-user@localhost").firstName("New first").lastName("New last")
371+
.email("user1@local.com").submit();
372+
373+
updateProfilePage.assertCurrent();
374+
375+
// assert that form holds submitted values during validation error
376+
Assert.assertEquals("New first", updateProfilePage.getFirstName());
377+
Assert.assertEquals("New last", updateProfilePage.getLastName());
378+
Assert.assertEquals("user1@local.com", updateProfilePage.getEmail());
379+
380+
Assert.assertEquals("Email already exists.", updateProfilePage.getInputErrors().getEmailError());
381+
382+
events.assertEmpty();
383+
}
384+
335385
@Test
336386
public void updateProfileExpiredCookies() {
337387
loginPage.open();

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

+35-2
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,39 @@ public void createDuplicatedUser2() {
325325
}
326326
}
327327

328+
@Test
329+
public void createDuplicatedUsernameWithEmail() {
330+
createUser("user1@local.com", "user1@local.org");
331+
332+
UserRepresentation user = new UserRepresentation();
333+
user.setUsername("user1@local.org");
334+
user.setEmail("user2@localhost");
335+
try (Response response = realm.users().create(user)) {
336+
assertEquals(409, response.getStatus());
337+
assertAdminEvents.assertEmpty();
338+
339+
ErrorRepresentation error = response.readEntity(ErrorRepresentation.class);
340+
Assert.assertEquals("User exists with same username", error.getErrorMessage());
341+
}
342+
}
343+
344+
@Test
345+
public void createDuplicatedEmailWithUsername() {
346+
createUser("user1@local.com", "user1@local.org");
347+
348+
UserRepresentation user = new UserRepresentation();
349+
user.setUsername("user2");
350+
user.setEmail("user1@local.com");
351+
352+
try (Response response = realm.users().create(user)) {
353+
assertEquals(409, response.getStatus());
354+
assertAdminEvents.assertEmpty();
355+
356+
ErrorRepresentation error = response.readEntity(ErrorRepresentation.class);
357+
Assert.assertEquals("User exists with same email", error.getErrorMessage());
358+
}
359+
}
360+
328361
//KEYCLOAK-14611
329362
@Test
330363
public void createDuplicateEmailWithExistingDuplicates() {
@@ -352,7 +385,7 @@ public void createDuplicateEmailWithExistingDuplicates() {
352385
try (Response response = realm.users().create(user)) {
353386
assertEquals(409, response.getStatus());
354387
ErrorRepresentation error = response.readEntity(ErrorRepresentation.class);
355-
Assert.assertEquals("User exists with same email", error.getErrorMessage());
388+
Assert.assertEquals("User exists with same username or email", error.getErrorMessage());
356389
assertAdminEvents.assertEmpty();
357390
}
358391
}
@@ -2619,7 +2652,7 @@ public void updateUserWithExistingEmail() {
26192652
assertThat(e.getResponse().getStatus(), is(409));
26202653

26212654
ErrorRepresentation error = e.getResponse().readEntity(ErrorRepresentation.class);
2622-
Assert.assertEquals("User exists with same username or email", error.getErrorMessage());
2655+
Assert.assertEquals("User exists with same email", error.getErrorMessage());
26232656
assertAdminEvents.assertEmpty();
26242657
}
26252658
}

‎testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/kerberos/KerberosStandaloneTest.java

+4-5
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,20 @@
2626
import org.junit.ClassRule;
2727
import org.junit.Test;
2828
import org.keycloak.admin.client.resource.UserResource;
29-
import org.keycloak.common.Profile;
3029
import org.keycloak.common.constants.KerberosConstants;
3130
import org.keycloak.federation.kerberos.CommonKerberosConfig;
3231
import org.keycloak.federation.kerberos.KerberosConfig;
3332
import org.keycloak.federation.kerberos.KerberosFederationProviderFactory;
3433
import org.keycloak.models.UserModel;
3534
import org.keycloak.representations.idm.ComponentRepresentation;
36-
import org.keycloak.representations.idm.RealmRepresentation;
35+
import org.keycloak.representations.idm.ErrorRepresentation;
3736
import org.keycloak.representations.idm.UserProfileAttributeMetadata;
3837
import org.keycloak.representations.idm.UserRepresentation;
3938
import org.keycloak.storage.UserStorageProvider;
4039
import org.keycloak.testsuite.ActionURIUtils;
4140
import org.keycloak.testsuite.KerberosEmbeddedServer;
4241
import org.keycloak.testsuite.admin.ApiUtil;
43-
import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
4442
import org.keycloak.testsuite.arquillian.annotation.UncaughtServerErrorExpected;
45-
import org.keycloak.testsuite.forms.VerifyProfileTest;
4643
import org.keycloak.testsuite.util.KerberosRule;
4744

4845
import static org.keycloak.userprofile.UserProfileUtil.USER_METADATA_GROUP;
@@ -181,7 +178,9 @@ public void handleUnknownKerberosRealm() throws Exception {
181178
UserRepresentation john = new UserRepresentation();
182179
john.setUsername("john");
183180
Response response = testRealmResource().users().create(john);
184-
Assert.assertEquals(500, response.getStatus());
181+
Assert.assertEquals(400, response.getStatus());
182+
ErrorRepresentation error = response.readEntity(ErrorRepresentation.class);
183+
Assert.assertEquals("Could not create user", error.getErrorMessage());
185184
response.close();
186185
}
187186

‎testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ public void testCustomAttributeInAnyContext() {
202202
private static void testCustomAttributeInAnyContext(KeycloakSession session) {
203203
Map<String, Object> attributes = new HashMap<>();
204204

205-
attributes.put(UserModel.USERNAME, "profiled-user");
205+
attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId());
206206
attributes.put(UserModel.FIRST_NAME, "John");
207207
attributes.put(UserModel.LAST_NAME, "Doe");
208208
attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org");
@@ -243,7 +243,7 @@ private static void testResolveProfile(KeycloakSession session) {
243243

244244
Map<String, Object> attributes = new HashMap<>();
245245

246-
attributes.put(UserModel.USERNAME, "profiled-user");
246+
attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org");
247247
attributes.put(UserModel.FIRST_NAME, "John");
248248
attributes.put(UserModel.LAST_NAME, "Doe");
249249
attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org");

0 commit comments

Comments
 (0)