From: Benjamin Campomenosi <109955405+benjamin-campomenosi-sonarsource@users.noreply.github.com>
Date: Mon, 13 Mar 2023 15:46:03 +0000 (+0100)
Subject: SONAR-18573 - Improve login validation for User creation
X-Git-Tag: 10.0.0.68432~153
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=74ba0dce648e1e3c696bfe5fe53d6fb822e2d970;p=sonarqube.git

SONAR-18573 - Improve login validation for User creation
---

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<String> 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<String, String> 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