]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-17469 improve SCM account validation on user creation/update. (#6905)
authorWojtek Wajerowicz <115081248+wojciech-wajerowicz-sonarsource@users.noreply.github.com>
Thu, 20 Oct 2022 08:01:09 +0000 (10:01 +0200)
committersonartech <sonartech@sonarsource.com>
Thu, 20 Oct 2022 20:03:02 +0000 (20:03 +0000)
We don't allow leading and/or trailing whitespace on SCM account names.
We also don't allow duplicate SCM accounts.

server/sonar-web/src/main/js/apps/users/components/UserForm.tsx
server/sonar-web/src/main/js/apps/users/components/__tests__/UserForm-test.tsx
server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/CreateAction.java
server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/UpdateAction.java
server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/CreateActionTest.java
server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/UpdateActionTest.java

index 3bd0a8cc0e9209dcc8dcb607e0a43043b4049301..74be9ade5b30872067b398aa12e418aced0b8f6b 100644 (file)
@@ -17,7 +17,6 @@
  * along with this program; if not, write to the Free Software Foundation,
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
-import { uniq } from 'lodash';
 import * as React from 'react';
 import { createUser, updateUser } from '../../../api/users';
 import { Button, ResetButtonLink, SubmitButton } from '../../../components/controls/buttons';
@@ -108,7 +107,7 @@ export default class UserForm extends React.PureComponent<Props, State> {
       login: this.state.login,
       name: this.state.name,
       password: this.state.password,
-      scmAccount: uniq(this.state.scmAccounts)
+      scmAccount: this.state.scmAccounts
     }).then(() => {
       this.props.onUpdateUsers();
       this.props.onClose();
@@ -121,7 +120,7 @@ export default class UserForm extends React.PureComponent<Props, State> {
       email: user!.local ? this.state.email : undefined,
       login: this.state.login,
       name: user!.local ? this.state.name : undefined,
-      scmAccount: uniq(this.state.scmAccounts)
+      scmAccount: this.state.scmAccounts
     }).then(() => {
       this.props.onUpdateUsers();
       this.props.onClose();
index 3880743d8ed9958cf918762558976a72314991c5..66ed4cf79279d6b11486dd967eaf676bb3f8dfa4 100644 (file)
@@ -88,7 +88,7 @@ it('should correctly create a new user', () => {
     login,
     name,
     password,
-    scmAccount: ['gh', 'bitbucket']
+    scmAccount: ['gh', 'gh', 'bitbucket']
   });
 });
 
@@ -105,7 +105,7 @@ it('should correctly update a local user', () => {
     email,
     login,
     name,
-    scmAccount: ['gh', 'bitbucket']
+    scmAccount: ['gh', 'gh', 'bitbucket']
   });
 });
 
index 5f358f5f6802c7a1e7956940a6c6d5042ba0c787..0be83ef5fb9a24db309830d1ae44778835a83816 100644 (file)
@@ -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 {
index e72edefa93bb6aaf286bfa769d4603cd6c4ca996..d004aeef4fbea700859c820da6101ddfc4258dd5 100644 (file)
@@ -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"));
index d18c7d4f472f01af8c3c7ebf75c6eec5c3930c27..7d97ad456cbc30c2a8e9d023a0c5a712d91f7380 100644 (file)
@@ -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);
   }
 
index dd05ba411e6ffe4c424d3d3eed8f16441681da31..5dc4faaf297293f95cbafb6e517c7f26a977947c 100644 (file)
@@ -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");