SONAR-17469 improve SCM account validation on user creation/update. (#6905)

We don't allow leading and/or trailing whitespace on SCM account names.
We also don't allow duplicate SCM accounts.

4 files changed, 98 insertions, 60 deletions

diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/CreateAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/CreateAction.java
index 5f358f5f680..0be83ef5fb9 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/CreateAction.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/CreateAction.java
@@ -19,8 +19,9 @@
  */
 package org.sonar.server.user.ws;
 
+import java.util.HashSet;
 import java.util.List;
-import java.util.stream.Collectors;
+import java.util.Set;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nullable;
 import org.sonar.api.server.ws.Change;
@@ -161,8 +162,8 @@ public class CreateAction implements UsersWsAction {
   private static CreateRequest toWsRequest(Request request) {
     return CreateRequest.builder()
       .setLogin(request.mandatoryParam(PARAM_LOGIN))
+      .setName(request.mandatoryParam(PARAM_NAME))
       .setPassword(request.param(PARAM_PASSWORD))
-      .setName(request.param(PARAM_NAME))
       .setEmail(request.param(PARAM_EMAIL))
       .setScmAccounts(parseScmAccounts(request))
       .setLocal(request.mandatoryParamAsBoolean(PARAM_LOCAL))
@@ -171,17 +172,27 @@ public class CreateAction implements UsersWsAction {
 
   public static List<String> parseScmAccounts(Request request) {
     if (request.hasParam(PARAM_SCM_ACCOUNT)) {
-      return formatScmAccounts(request.multiParam(PARAM_SCM_ACCOUNT));
+      List<String> scmAccounts = request.multiParam(PARAM_SCM_ACCOUNT);
+      validateScmAccounts(scmAccounts);
+      return scmAccounts;
     }
-
     return emptyList();
   }
 
-  private static List<String> formatScmAccounts(List<String> scmAccounts) {
-    return scmAccounts
-      .stream()
-      .map(String::trim)
-      .collect(Collectors.toList());
+  private static void validateScmAccounts(List<String> scmAccounts) {
+    scmAccounts.forEach(CreateAction::validateScmAccountFormat);
+    validateNoDuplicates(scmAccounts);
+  }
+
+  private static void validateScmAccountFormat(String scmAccount) {
+    checkArgument(scmAccount.equals(scmAccount.strip()), "SCM account cannot start or end with whitespace: '%s'", scmAccount);
+  }
+
+  private static void validateNoDuplicates(List<String> scmAccounts) {
+    Set<String> duplicateCheck = new HashSet<>();
+    for (String account : scmAccounts) {
+      checkArgument(duplicateCheck.add(account), "Duplicate SCM account: '%s'", account);
+    }
   }
 
   static class CreateRequest {
diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateAction.java
index e72edefa93b..d004aeef4fb 100644
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateAction.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateAction.java
@@ -76,7 +76,8 @@ public class UpdateAction implements UsersWsAction {
       .setDescription("Update a user.<br/>" +
         "Requires Administer System permission")
       .setSince("3.7")
-      .setChangelog(new Change("5.2", "User's password can only be changed using the 'change_password' action."))
+      .setChangelog(
+        new Change("5.2", "User's password can only be changed using the 'change_password' action."))
       .setPost(true)
       .setHandler(this)
       .setResponseExample(getClass().getResource("update-example.json"));
diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/CreateActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/CreateActionTest.java
index d18c7d4f472..7d97ad456cb 100644
--- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/CreateActionTest.java
+++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/CreateActionTest.java
@@ -104,12 +104,12 @@ public class CreateActionTest {
 
     // exists in index
     assertThat(es.client().search(EsClient.prepareSearch(UserIndexDefinition.TYPE_USER)
-        .source(new SearchSourceBuilder()
-          .query(boolQuery()
-            .must(termQuery(FIELD_LOGIN, "john"))
-            .must(termQuery(FIELD_NAME, "John"))
-            .must(termQuery(FIELD_EMAIL, "john@email.com"))
-            .must(termQuery(FIELD_SCM_ACCOUNTS, "jn")))))
+      .source(new SearchSourceBuilder()
+        .query(boolQuery()
+          .must(termQuery(FIELD_LOGIN, "john"))
+          .must(termQuery(FIELD_NAME, "John"))
+          .must(termQuery(FIELD_EMAIL, "john@email.com"))
+          .must(termQuery(FIELD_SCM_ACCOUNTS, "jn")))))
       .getHits().getHits()).hasSize(1);
 
     // exists in db
@@ -166,20 +166,38 @@ public class CreateActionTest {
     assertThat(response.getUser().getScmAccountsList()).containsOnly("j,n");
   }
 
-
   @Test
-  public void create_user_ignores_duplicates_containing_whitespace_characters_in_scm_account_values() {
+  public void fail_when_whitespace_characters_in_scm_account_values() {
     logInAsSystemAdministrator();
 
-    CreateWsResponse response = call(CreateRequest.builder()
-      .setLogin("john")
-      .setName("John")
-      .setEmail("john@email.com")
-      .setScmAccounts(List.of("admin", "  admin  "))
-      .setPassword("1234")
-      .build());
+    assertThatThrownBy(() -> {
+      call(CreateRequest.builder()
+        .setLogin("john")
+        .setName("John")
+        .setEmail("john@email.com")
+        .setScmAccounts(List.of("admin", "  admin  "))
+        .setPassword("1234")
+        .build());
+    })
+      .isInstanceOf(IllegalArgumentException.class)
+      .hasMessage("SCM account cannot start or end with whitespace: '  admin  '");
+  }
 
-    assertThat(response.getUser().getScmAccountsList()).containsOnly("admin");
+  @Test
+  public void fail_when_duplicates_characters_in_scm_account_values() {
+    logInAsSystemAdministrator();
+
+    assertThatThrownBy(() -> {
+      call(CreateRequest.builder()
+        .setLogin("john")
+        .setName("John")
+        .setEmail("john@email.com")
+        .setScmAccounts(List.of("admin", "admin"))
+        .setPassword("1234")
+        .build());
+    })
+      .isInstanceOf(IllegalArgumentException.class)
+      .hasMessage("Duplicate SCM account: 'admin'");
   }
 
   @Test
@@ -296,13 +314,14 @@ public class CreateActionTest {
   public void fail_when_password_is_set_on_none_local_user() {
     logInAsSystemAdministrator();
 
+    TestRequest request =tester.newRequest()
+      .setParam("login","john")
+      .setParam("name","John")
+      .setParam("password", "1234")
+      .setParam("local", "false");
+
     assertThatThrownBy(() -> {
-      call(CreateRequest.builder()
-        .setLogin("john")
-        .setName("John")
-        .setPassword("1234")
-        .setLocal(false)
-        .build());
+      request.execute();
     })
       .isInstanceOf(IllegalArgumentException.class)
       .hasMessage("Password should only be set on local user");
@@ -312,13 +331,15 @@ public class CreateActionTest {
   public void fail_when_email_is_invalid() {
     logInAsSystemAdministrator();
 
+    CreateRequest request = CreateRequest.builder()
+      .setLogin("pipo")
+      .setName("John")
+      .setPassword("1234")
+      .setEmail("invalid-email")
+      .build();
+
     assertThatThrownBy(() -> {
-      call(CreateRequest.builder()
-        .setLogin("pipo")
-        .setName("John")
-        .setPassword("1234")
-        .setEmail("invalid-email")
-        .build());
+      call(request);
     })
       .isInstanceOf(IllegalArgumentException.class)
       .hasMessage("Email 'invalid-email' is not valid");
@@ -362,7 +383,7 @@ public class CreateActionTest {
     ofNullable(createRequest.getEmail()).ifPresent(e2 -> request.setParam("email", e2));
     ofNullable(createRequest.getPassword()).ifPresent(e1 -> request.setParam("password", e1));
     ofNullable(createRequest.getScmAccounts()).ifPresent(e -> request.setMultiParam("scmAccount", e));
-    request.setParam("local", createRequest.isLocal() ? "true" : "false");
+    request.setParam("local", Boolean.toString(createRequest.isLocal()));
     return request.executeProtobuf(CreateWsResponse.class);
   }
 
diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java
index dd05ba411e6..5dc4faaf297 100644
--- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java
+++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java
@@ -39,6 +39,7 @@ import org.sonar.server.user.NewUserNotifier;
 import org.sonar.server.user.UserUpdater;
 import org.sonar.server.user.index.UserIndexer;
 import org.sonar.server.usergroups.DefaultGroupFinder;
+import org.sonar.server.ws.TestRequest;
 import org.sonar.server.ws.WsActionTester;
 
 import static com.google.common.collect.Lists.newArrayList;
@@ -190,29 +191,33 @@ public class UpdateActionTest {
   }
 
   @Test
-  public void update_scm_account_ignores_duplicates() {
+  public void fail_when_duplicates_characters_in_scm_account_values() {
     createUser();
 
-    ws.newRequest()
-      .setParam("login", "john")
-      .setMultiParam("scmAccount", Arrays.asList("jon.snow", "jon.snow", "jon.jon", "jon.snow"))
-      .execute();
+    assertThatThrownBy(() -> {
+      ws.newRequest()
+        .setParam("login", "john")
+        .setMultiParam("scmAccount", Arrays.asList("jon.snow", "jon.snow", "jon.jon", "jon.snow"))
+        .execute();
+    }).isInstanceOf(IllegalArgumentException.class)
+      .hasMessage("Duplicate SCM account: 'jon.snow'");
+    ;
 
-    UserDto user = dbClient.userDao().selectByLogin(dbSession, "john");
-    assertThat(user.getScmAccountsAsList()).containsExactlyInAnyOrder("jon.jon", "jon.snow");
   }
 
   @Test
-  public void update_scm_account_ignores_duplicates_containing_whitespace_characters_in_scm_account_values() {
+  public void fail_when_whitespace_characters_in_scm_account_values() {
     createUser();
 
-    ws.newRequest()
-      .setParam("login", "john")
-      .setMultiParam("scmAccount", Arrays.asList("jon.snow", "jon.snow", "jon.jon", "   jon.snow  "))
-      .execute();
+    assertThatThrownBy(() -> {
+      ws.newRequest()
+        .setParam("login", "john")
+        .setMultiParam("scmAccount", Arrays.asList("jon.snow", "jon.snow", "jon.jon", "   jon.snow  "))
+        .execute();
+    }).isInstanceOf(IllegalArgumentException.class)
+      .hasMessage("SCM account cannot start or end with whitespace: '   jon.snow  '");
+    ;
 
-    UserDto user = dbClient.userDao().selectByLogin(dbSession, "john");
-    assertThat(user.getScmAccountsAsList()).containsExactlyInAnyOrder("jon.jon", "jon.snow");
   }
 
   @Test
@@ -256,10 +261,10 @@ public class UpdateActionTest {
   public void fail_on_disabled_user() {
     db.users().insertUser(u -> u.setLogin("john").setActive(false));
 
+    TestRequest request = ws.newRequest()
+      .setParam("login", "john");
     assertThatThrownBy(() -> {
-      ws.newRequest()
-        .setParam("login", "john")
-        .execute();
+      request.execute();
     })
       .isInstanceOf(NotFoundException.class)
       .hasMessage("User 'john' doesn't exist");
@@ -269,11 +274,11 @@ public class UpdateActionTest {
   public void fail_on_invalid_email() {
     createUser();
 
+    TestRequest request = ws.newRequest()
+      .setParam("login", "john")
+      .setParam("email", "invalid-email");
     assertThatThrownBy(() -> {
-      ws.newRequest()
-        .setParam("login", "john")
-        .setParam("email", "invalid-email")
-        .execute();
+      request.execute();
     })
       .isInstanceOf(IllegalArgumentException.class)
       .hasMessage("Email 'invalid-email' is not valid");