From 74ba0dce648e1e3c696bfe5fe53d6fb822e2d970 Mon Sep 17 00:00:00 2001 From: Benjamin Campomenosi <109955405+benjamin-campomenosi-sonarsource@users.noreply.github.com> Date: Mon, 13 Mar 2023 16:46:03 +0100 Subject: [PATCH] SONAR-18573 - Improve login validation for User creation --- .../org/sonar/server/user/UserUpdater.java | 16 ++- .../server/user/UserUpdaterCreateTest.java | 112 ++++++++++++++---- .../server/user/UserUpdaterUpdateTest.java | 6 +- 3 files changed, 102 insertions(+), 32 deletions(-) diff --git a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java index 468613e53f8..576f919d1e3 100644 --- a/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java +++ b/server/sonar-webserver-auth/src/main/java/org/sonar/server/user/UserUpdater.java @@ -67,7 +67,7 @@ public class UserUpdater { private static final String PASSWORD_PARAM = "Password"; private static final String NAME_PARAM = "Name"; private static final String EMAIL_PARAM = "Email"; - private static final Pattern START_WITH_SPECIFIC_AUTHORIZED_CHARACTERS = Pattern.compile("^[\\.\\-_@].*$"); + private static final Pattern START_WITH_SPECIFIC_AUTHORIZED_CHARACTERS = Pattern.compile("\\w+"); private static final Pattern CONTAINS_ONLY_AUTHORIZED_CHARACTERS = Pattern.compile("\\A\\w[\\w\\.\\-@]+\\z"); public static final int LOGIN_MIN_LENGTH = 2; @@ -79,7 +79,6 @@ public class UserUpdater { private final DbClient dbClient; private final UserIndexer userIndexer; private final DefaultGroupFinder defaultGroupFinder; - private final Configuration config; private final AuditPersister auditPersister; private final CredentialsLocalAuthentication localAuthentication; @@ -90,7 +89,6 @@ public class UserUpdater { this.dbClient = dbClient; this.userIndexer = userIndexer; this.defaultGroupFinder = defaultGroupFinder; - this.config = config; this.auditPersister = auditPersister; this.localAuthentication = localAuthentication; } @@ -323,8 +321,8 @@ public class UserUpdater { } else if (login.length() > LOGIN_MAX_LENGTH) { messages.add(format(Validation.IS_TOO_LONG_MESSAGE, LOGIN_PARAM, LOGIN_MAX_LENGTH)); return false; - } else if (START_WITH_SPECIFIC_AUTHORIZED_CHARACTERS.matcher(login).matches()){ - messages.add("Login should not start with .-_@"); + } else if (!startWithUnderscoreOrAlphanumeric(login)) { + messages.add("Login should start with _ or alphanumeric."); return false; } else if (!CONTAINS_ONLY_AUTHORIZED_CHARACTERS.matcher(login).matches()) { messages.add("Login should contain only letters, numbers, and .-_@"); @@ -334,6 +332,14 @@ public class UserUpdater { return isValid; } + private static boolean startWithUnderscoreOrAlphanumeric(String login) { + String firstCharacter = login.substring(0, 1); + if ("_".equals(firstCharacter)) { + return true; + } + return START_WITH_SPECIFIC_AUTHORIZED_CHARACTERS.matcher(firstCharacter).matches(); + } + private static boolean validateNameFormat(@Nullable String name, List messages) { boolean isValid = checkNotEmptyParam(name, NAME_PARAM, messages); if (name != null && name.length() > NAME_MAX_LENGTH) { diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java index 46e1720f758..17d886d00dd 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterCreateTest.java @@ -21,10 +21,14 @@ package org.sonar.server.user; import com.google.common.collect.ImmutableList; import com.google.common.collect.Multimap; +import com.tngtech.java.junit.dataprovider.DataProvider; +import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import com.tngtech.java.junit.dataprovider.UseDataProvider; import java.util.List; import org.elasticsearch.search.SearchHit; import org.junit.Rule; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.sonar.api.config.internal.MapSettings; import org.sonar.api.impl.utils.AlwaysIncreasingSystem2; @@ -49,12 +53,14 @@ import static org.apache.commons.lang.RandomStringUtils.randomAlphabetic; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.data.MapEntry.entry; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.sonar.db.user.UserTesting.newLocalUser; import static org.sonar.server.user.ExternalIdentity.SQ_AUTHORITY; +@RunWith(DataProviderRunner.class) public class UserUpdaterCreateTest { private static final String DEFAULT_LOGIN = "marius"; @@ -282,9 +288,34 @@ public class UserUpdaterCreateTest { } @Test - public void fail_to_create_user_with_invalid_login() { - NewUser newUser = newUserBuilder().setLogin("/marius/").build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + @UseDataProvider("loginWithAuthorizedSuffix") + public void createAndCommit_should_createUserWithoutException_when_loginHasAuthorizedSuffix(String login) { + createDefaultGroup(); + + NewUser user = NewUser.builder().setLogin(login).setName("aName").build(); + underTest.createAndCommit(session, user, u -> { + }); + + UserDto dto = dbClient.userDao().selectByLogin(session, login); + assertNotNull(dto); + } + + @DataProvider + public static Object[][] loginWithAuthorizedSuffix() { + return new Object[][]{ + {"1Login"}, + {"AnotherLogin"}, + {"alogin"}, + {"_technicalUser"}, + {"_.toto"} + }; + } + + @Test + public void fail_to_create_user_with_invalid_characters_in_login() { + NewUser newUser = newUserBuilder().setLogin("_amarius/").build(); + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Login should contain only letters, numbers, and .-_@"); } @@ -292,7 +323,8 @@ public class UserUpdaterCreateTest { @Test public void fail_to_create_user_with_space_in_login() { NewUser newUser = newUserBuilder().setLogin("mari us").build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Login should contain only letters, numbers, and .-_@"); } @@ -300,17 +332,35 @@ public class UserUpdaterCreateTest { @Test public void fail_to_create_user_with_too_short_login() { NewUser newUser = newUserBuilder().setLogin("m").build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Login is too short (minimum is 2 characters)"); } + @Test - public void fail_to_create_user_login_start_with_underscore() { - NewUser newUser = newUserBuilder().setLogin("_marbalous").build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + @UseDataProvider("loginWithUnauthorizedSuffix") + public void createAndCommit_should_ThrowBadRequestExceptionWithSpecificMessage_when_loginHasUnauthorizedSuffix(String login) { + + NewUser newUser = NewUser.builder().setLogin(login).build(); + + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) - .hasMessage("Login should not start with .-_@"); + .hasMessage("Login should start with _ or alphanumeric."); + + } + + @DataProvider + public static Object[][] loginWithUnauthorizedSuffix() { + return new Object[][]{ + {".Toto"}, + {"@Toto"}, + {"-Tutu"}, + {" Tutut"}, + {"#nesp"}, + }; } @Test @@ -319,7 +369,8 @@ public class UserUpdaterCreateTest { String login = "name_with_underscores"; NewUser newUser = newUserBuilder().setLogin(login).build(); - underTest.createAndCommit(session, newUser, u -> {}); + underTest.createAndCommit(session, newUser, u -> { + }); assertThat(dbClient.userDao().selectByLogin(session, login)).isNotNull(); } @@ -327,7 +378,8 @@ public class UserUpdaterCreateTest { @Test public void fail_to_create_user_with_too_long_login() { NewUser newUser = newUserBuilder().setLogin(randomAlphabetic(256)).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Login is too long (maximum is 255 characters)"); } @@ -339,7 +391,8 @@ public class UserUpdaterCreateTest { .setEmail("marius@mail.com") .setPassword("password") .build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Name can't be empty"); } @@ -347,7 +400,8 @@ public class UserUpdaterCreateTest { @Test public void fail_to_create_user_with_too_long_name() { NewUser newUser = newUserBuilder().setName(randomAlphabetic(201)).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Name is too long (maximum is 200 characters)"); } @@ -355,7 +409,8 @@ public class UserUpdaterCreateTest { @Test public void fail_to_create_user_with_too_long_email() { NewUser newUser = newUserBuilder().setEmail(randomAlphabetic(101)).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Email is too long (maximum is 100 characters)"); } @@ -370,7 +425,8 @@ public class UserUpdaterCreateTest { .build(); try { - underTest.createAndCommit(session, newUser, u -> {}); + underTest.createAndCommit(session, newUser, u -> { + }); fail(); } catch (BadRequestException e) { assertThat(e.errors()).containsExactlyInAnyOrder("Name can't be empty", "Password can't be empty"); @@ -382,7 +438,8 @@ public class UserUpdaterCreateTest { db.users().insertUser(newLocalUser("john", "John", null).setScmAccounts(singletonList("jo"))); NewUser newUser = newUserBuilder().setScmAccounts(singletonList("jo")).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("The scm account 'jo' is already used by user(s) : 'John (john)'"); } @@ -393,7 +450,8 @@ public class UserUpdaterCreateTest { db.users().insertUser(newLocalUser("technical-account", "Technical account", null).setScmAccounts(singletonList("john@email.com"))); NewUser newUser = newUserBuilder().setScmAccounts(List.of("john@email.com")).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("The scm account 'john@email.com' is already used by user(s) : 'John (john), Technical account (technical-account)'"); } @@ -401,15 +459,17 @@ public class UserUpdaterCreateTest { @Test public void fail_to_create_user_when_scm_account_is_user_login() { NewUser newUser = newUserBuilder().setLogin(DEFAULT_LOGIN).setScmAccounts(singletonList(DEFAULT_LOGIN)).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Login and email are automatically considered as SCM accounts"); } @Test public void fail_to_create_user_when_scm_account_is_user_email() { - NewUser newUser = newUserBuilder().setEmail("marius2@mail.com").setScmAccounts(singletonList("marius2@mail.com")).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + NewUser newUser = newUserBuilder().setEmail("marius2@mail.com").setScmAccounts(singletonList("marius2@mail.com")).build(); + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(BadRequestException.class) .hasMessage("Login and email are automatically considered as SCM accounts"); } @@ -420,7 +480,8 @@ public class UserUpdaterCreateTest { UserDto existingUser = db.users().insertUser(u -> u.setLogin("existing_login")); NewUser newUser = newUserBuilder().setLogin(existingUser.getLogin()).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(IllegalArgumentException.class) .hasMessage("A user with login 'existing_login' already exists"); } @@ -435,7 +496,8 @@ public class UserUpdaterCreateTest { .setName("User") .setExternalIdentity(new ExternalIdentity(existingUser.getExternalIdentityProvider(), existingUser.getExternalLogin(), existingUser.getExternalId())) .build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(IllegalArgumentException.class) .hasMessage("A user with provider id 'existing_external_id' and identity provider 'existing_external_provider' already exists"); } @@ -464,7 +526,8 @@ public class UserUpdaterCreateTest { GroupDto defaultGroup = createDefaultGroup(); NewUser newUser = newUserBuilder().build(); - underTest.createAndCommit(session, newUser, u -> {}); + underTest.createAndCommit(session, newUser, u -> { + }); Multimap groups = dbClient.groupMembershipDao().selectGroupsByLogins(session, singletonList(newUser.login())); assertThat(groups.get(newUser.login())).containsOnly(defaultGroup.getName()); @@ -473,7 +536,8 @@ public class UserUpdaterCreateTest { @Test public void fail_to_associate_default_group_when_default_group_does_not_exist() { NewUser newUser = newUserBuilder().setScmAccounts(asList("u1", "u_1")).build(); - assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> {})) + assertThatThrownBy(() -> underTest.createAndCommit(session, newUser, u -> { + })) .isInstanceOf(IllegalStateException.class) .hasMessage("Default group cannot be found"); } diff --git a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java index 4b65cb39689..d3d3cccf60f 100644 --- a/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java +++ b/server/sonar-webserver-auth/src/test/java/org/sonar/server/user/UserUpdaterUpdateTest.java @@ -246,7 +246,7 @@ public class UserUpdaterUpdateTest { new PropertyDto().setKey(DEFAULT_ISSUE_ASSIGNEE).setValue(oldUser.getLogin()).setComponentUuid(project1.uuid())); db.properties().insertProperties(oldUser.getLogin(), project2.getKey(), project2.name(), project2.qualifier(), new PropertyDto().setKey(DEFAULT_ISSUE_ASSIGNEE).setValue(oldUser.getLogin()).setComponentUuid(project2.uuid())); - db.properties().insertProperties(oldUser.getLogin(), anotherProject.getKey(),anotherProject.name(), anotherProject.qualifier(), + db.properties().insertProperties(oldUser.getLogin(), anotherProject.getKey(), anotherProject.name(), anotherProject.qualifier(), new PropertyDto().setKey(DEFAULT_ISSUE_ASSIGNEE).setValue("another login").setComponentUuid(anotherProject.uuid())); userIndexer.indexAll(); @@ -636,11 +636,11 @@ public class UserUpdaterUpdateTest { UserDto user = db.users().insertUser(); createDefaultGroup(); - UpdateUser updateUser = new UpdateUser().setLogin("_StartWithUnderscore"); + UpdateUser updateUser = new UpdateUser().setLogin("#StartWithUnderscore"); assertThatThrownBy(() -> underTest.updateAndCommit(session, user, updateUser, EMPTY_USER_CONSUMER)) .isInstanceOf(BadRequestException.class) - .hasMessage("Login should not start with .-_@"); + .hasMessage("Login should start with _ or alphanumeric."); } @Test -- 2.39.5