From 344dcf9cea680693d62ca73a19eb7c652d5ca271 Mon Sep 17 00:00:00 2001 From: Wojtek Wajerowicz <115081248+wojciech-wajerowicz-sonarsource@users.noreply.github.com> Date: Thu, 20 Oct 2022 10:01:09 +0200 Subject: [PATCH] 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. --- .../js/apps/users/components/UserForm.tsx | 5 +- .../components/__tests__/UserForm-test.tsx | 4 +- .../sonar/server/user/ws/CreateAction.java | 29 ++++--- .../sonar/server/user/ws/UpdateAction.java | 3 +- .../server/user/ws/CreateActionTest.java | 79 ++++++++++++------- .../server/user/ws/UpdateActionTest.java | 47 ++++++----- 6 files changed, 102 insertions(+), 65 deletions(-) diff --git a/server/sonar-web/src/main/js/apps/users/components/UserForm.tsx b/server/sonar-web/src/main/js/apps/users/components/UserForm.tsx index 3bd0a8cc0e9..74be9ade5b3 100644 --- a/server/sonar-web/src/main/js/apps/users/components/UserForm.tsx +++ b/server/sonar-web/src/main/js/apps/users/components/UserForm.tsx @@ -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 { 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 { 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(); diff --git a/server/sonar-web/src/main/js/apps/users/components/__tests__/UserForm-test.tsx b/server/sonar-web/src/main/js/apps/users/components/__tests__/UserForm-test.tsx index 3880743d8ed..66ed4cf7927 100644 --- a/server/sonar-web/src/main/js/apps/users/components/__tests__/UserForm-test.tsx +++ b/server/sonar-web/src/main/js/apps/users/components/__tests__/UserForm-test.tsx @@ -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'] }); }); 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 parseScmAccounts(Request request) { if (request.hasParam(PARAM_SCM_ACCOUNT)) { - return formatScmAccounts(request.multiParam(PARAM_SCM_ACCOUNT)); + List scmAccounts = request.multiParam(PARAM_SCM_ACCOUNT); + validateScmAccounts(scmAccounts); + return scmAccounts; } - return emptyList(); } - private static List formatScmAccounts(List scmAccounts) { - return scmAccounts - .stream() - .map(String::trim) - .collect(Collectors.toList()); + private static void validateScmAccounts(List 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 scmAccounts) { + Set 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.
" + "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"); -- 2.39.5