From 71c9f2c2590be229dbac46d6b48f325b37fe8ad5 Mon Sep 17 00:00:00 2001 From: Wojtek Wajerowicz <115081248+wojciech-wajerowicz-sonarsource@users.noreply.github.com> Date: Tue, 30 Jan 2024 15:50:57 +0100 Subject: [PATCH] Fix SSF-530. --- .../config/GitlabConfigurationServiceIT.java | 40 ++++++++++++++-- .../config/GitlabConfigurationService.java | 33 ++++++++----- .../DefaultGitlabConfigurationController.java | 12 ++--- .../GitlabConfigurationCreateRestRequest.java | 9 +++- ...aultGitlabConfigurationControllerTest.java | 48 +++++++++++++++++++ 5 files changed, 116 insertions(+), 26 deletions(-) diff --git a/server/sonar-webserver-common/src/it/java/org/sonar/server/common/gitlab/config/GitlabConfigurationServiceIT.java b/server/sonar-webserver-common/src/it/java/org/sonar/server/common/gitlab/config/GitlabConfigurationServiceIT.java index cb7e8cd8889..d71cc0406af 100644 --- a/server/sonar-webserver-common/src/it/java/org/sonar/server/common/gitlab/config/GitlabConfigurationServiceIT.java +++ b/server/sonar-webserver-common/src/it/java/org/sonar/server/common/gitlab/config/GitlabConfigurationServiceIT.java @@ -158,6 +158,19 @@ public class GitlabConfigurationServiceIT { .withMessage("GitLab configuration doesn't exist."); } + @Test + public void updateConfiguration_whenAllowedGroupsIsEmptyAndAutoProvisioningIsEnabled_throwsException() { + gitlabConfigurationService.createConfiguration(buildGitlabConfiguration(AUTO_PROVISIONING)); + UpdateGitlabConfigurationRequest updateGitlabConfigurationRequest = builder() + .gitlabConfigurationId(UNIQUE_GITLAB_CONFIGURATION_ID) + .allowedGroups(withValueOrThrow(new LinkedHashSet<>())) + .build(); + + assertThatThrownBy(() -> gitlabConfigurationService.updateConfiguration(updateGitlabConfigurationRequest)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("allowedGroups cannot be empty when Auto-provisioning is enabled."); + } + @Test public void updateConfiguration_whenAllUpdateFieldDefined_updatesEverything() { gitlabConfigurationService.createConfiguration(buildGitlabConfiguration(JIT)); @@ -306,6 +319,25 @@ public class GitlabConfigurationServiceIT { .hasMessage("GitLab configuration already exists. Only one Gitlab configuration is supported."); } + @Test + public void createConfiguration_whenAllowedGroupsIsEmptyAndAutoProvisioningIsEnabled_shouldReturnBadRequest() { + GitlabConfiguration configuration = new GitlabConfiguration( + UNIQUE_GITLAB_CONFIGURATION_ID, + true, + "applicationId", + "url", + "secret", + true, + Set.of(), + true, + AUTO_PROVISIONING, + "provisioningToken"); + + assertThatThrownBy(() -> gitlabConfigurationService.createConfiguration(configuration)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("allowedGroups cannot be empty when Auto-provisioning is enabled."); + } + @Test public void createConfiguration_whenAutoProvisioning_shouldCreateCorrectConfigurationAndScheduleSync() { GitlabConfiguration configuration = buildGitlabConfiguration(AUTO_PROVISIONING); @@ -332,8 +364,7 @@ public class GitlabConfigurationServiceIT { Set.of("group1", "group2", "group3"), true, AUTO_PROVISIONING, - null - ); + null); assertThatThrownBy(() -> gitlabConfigurationService.createConfiguration(configuration)) .isInstanceOf(IllegalStateException.class) @@ -379,8 +410,7 @@ public class GitlabConfigurationServiceIT { Set.of("group1", "group2", "group3"), true, JIT, - null - ); + null); GitlabConfiguration createdConfiguration = gitlabConfigurationService.createConfiguration(configuration); @@ -526,7 +556,7 @@ public class GitlabConfigurationServiceIT { Exception exception = new IllegalStateException("Invalid configuration"); when(gitlabConfigurationService.validate(gitlabConfiguration)).thenThrow(exception); - Optional message = gitlabConfigurationService.validate(gitlabConfiguration); + Optional message = gitlabConfigurationService.validate(gitlabConfiguration); assertThat(message).contains("Invalid configuration"); } diff --git a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/gitlab/config/GitlabConfigurationService.java b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/gitlab/config/GitlabConfigurationService.java index 214c196133d..25d1a4977aa 100644 --- a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/gitlab/config/GitlabConfigurationService.java +++ b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/gitlab/config/GitlabConfigurationService.java @@ -85,11 +85,15 @@ public class GitlabConfigurationService { } public GitlabConfiguration updateConfiguration(UpdateGitlabConfigurationRequest updateRequest) { - UpdatedValue provisioningEnabled = - updateRequest.provisioningType().map(GitlabConfigurationService::shouldEnableAutoProvisioning); + UpdatedValue provisioningEnabled = updateRequest.provisioningType().map(GitlabConfigurationService::shouldEnableAutoProvisioning); try (DbSession dbSession = dbClient.openSession(true)) { throwIfConfigurationDoesntExist(dbSession); GitlabConfiguration currentConfiguration = getConfiguration(updateRequest.gitlabConfigurationId(), dbSession); + + ProvisioningType provisioningType = updateRequest.provisioningType().orElse(currentConfiguration.provisioningType()); + Set allowedGroups = updateRequest.allowedGroups().orElse(currentConfiguration.allowedGroups()); + throwIfAllowedGroupsEmptyAndAutoProvisioning(provisioningType, allowedGroups); + setIfDefined(dbSession, GITLAB_AUTH_ENABLED, updateRequest.enabled().map(String::valueOf)); setIfDefined(dbSession, GITLAB_AUTH_APPLICATION_ID, updateRequest.applicationId()); setIfDefined(dbSession, GITLAB_AUTH_URL, updateRequest.url()); @@ -99,8 +103,7 @@ public class GitlabConfigurationService { setIfDefined(dbSession, GITLAB_AUTH_PROVISIONING_ENABLED, provisioningEnabled.map(String::valueOf)); setIfDefined(dbSession, GITLAB_AUTH_ALLOW_USERS_TO_SIGNUP, updateRequest.allowUsersToSignUp().map(String::valueOf)); setIfDefined(dbSession, GITLAB_AUTH_PROVISIONING_TOKEN, updateRequest.provisioningToken()); - boolean shouldTriggerProvisioning = - provisioningEnabled.orElse(false) && !currentConfiguration.provisioningType().equals(AUTO_PROVISIONING); + boolean shouldTriggerProvisioning = provisioningEnabled.orElse(false) && !currentConfiguration.provisioningType().equals(AUTO_PROVISIONING); deleteExternalGroupsWhenDisablingAutoProvisioning(dbSession, currentConfiguration, updateRequest.provisioningType()); GitlabConfiguration updatedConfiguration = getConfiguration(UNIQUE_GITLAB_CONFIGURATION_ID, dbSession); if (shouldTriggerProvisioning) { @@ -122,9 +125,8 @@ public class GitlabConfigurationService { DbSession dbSession, GitlabConfiguration currentConfiguration, UpdatedValue provisioningTypeFromUpdate) { - boolean disableAutoProvisioning = - provisioningTypeFromUpdate.map(provisioningType -> provisioningType.equals(JIT)).orElse(false) - && currentConfiguration.provisioningType().equals(AUTO_PROVISIONING); + boolean disableAutoProvisioning = provisioningTypeFromUpdate.map(provisioningType -> provisioningType.equals(JIT)).orElse(false) + && currentConfiguration.provisioningType().equals(AUTO_PROVISIONING); if (disableAutoProvisioning) { dbClient.externalGroupDao().deleteByExternalIdentityProvider(dbSession, GitLabIdentityProvider.KEY); } @@ -188,6 +190,7 @@ public class GitlabConfigurationService { public GitlabConfiguration createConfiguration(GitlabConfiguration configuration) { throwIfConfigurationAlreadyExists(); + throwIfAllowedGroupsEmptyAndAutoProvisioning(configuration.provisioningType(), configuration.allowedGroups()); boolean enableAutoProvisioning = shouldEnableAutoProvisioning(configuration.provisioningType()); try (DbSession dbSession = dbClient.openSession(false)) { @@ -216,6 +219,12 @@ public class GitlabConfigurationService { }); } + private static void throwIfAllowedGroupsEmptyAndAutoProvisioning(ProvisioningType provisioningType, Set allowedGroups) { + if (provisioningType == AUTO_PROVISIONING && allowedGroups.isEmpty()) { + throw new IllegalArgumentException("allowedGroups cannot be empty when Auto-provisioning is enabled."); + } + } + private static boolean shouldEnableAutoProvisioning(ProvisioningType provisioningType) { return AUTO_PROVISIONING.equals(provisioningType); } @@ -237,16 +246,15 @@ public class GitlabConfigurationService { getAllowedGroups(dbSession), getBooleanOrFalse(dbSession, GITLAB_AUTH_ALLOW_USERS_TO_SIGNUP), toProvisioningType(getBooleanOrFalse(dbSession, GITLAB_AUTH_PROVISIONING_ENABLED)), - getStringPropertyOrNull(dbSession, GITLAB_AUTH_PROVISIONING_TOKEN) - ); + getStringPropertyOrNull(dbSession, GITLAB_AUTH_PROVISIONING_TOKEN)); } private Set getAllowedGroups(DbSession dbSession) { return Optional.ofNullable(dbClient.propertiesDao().selectGlobalProperty(dbSession, GITLAB_AUTH_ALLOWED_GROUPS)) .map(dto -> Arrays.stream(dto.getValue().split(",")) .filter(s -> !s.isEmpty()) - .collect(Collectors.toSet()) - ).orElse(Set.of()); + .collect(Collectors.toSet())) + .orElse(Set.of()); } public void triggerRun() { @@ -289,8 +297,7 @@ public class GitlabConfigurationService { gitlabGlobalSettingsValidator.validate( toValidationMode(gitlabConfiguration.provisioningType()), url, - gitlabConfiguration.provisioningToken() - ); + gitlabConfiguration.provisioningToken()); } catch (Exception e) { return Optional.of(e.getMessage()); } diff --git a/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/gitlab/config/controller/DefaultGitlabConfigurationController.java b/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/gitlab/config/controller/DefaultGitlabConfigurationController.java index f85a38e4bba..d9c30bb8c5c 100644 --- a/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/gitlab/config/controller/DefaultGitlabConfigurationController.java +++ b/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/gitlab/config/controller/DefaultGitlabConfigurationController.java @@ -23,6 +23,7 @@ import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; +import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; import org.sonar.server.common.gitlab.config.GitlabConfiguration; import org.sonar.server.common.gitlab.config.GitlabConfigurationService; @@ -35,6 +36,7 @@ import org.sonar.server.v2.api.gitlab.config.resource.GitlabConfigurationResourc import org.sonar.server.v2.api.gitlab.config.response.GitlabConfigurationSearchRestResponse; import org.sonar.server.v2.api.response.PageRestResponse; +import static org.sonar.api.utils.Preconditions.checkArgument; import static org.sonar.server.common.gitlab.config.GitlabConfigurationService.UNIQUE_GITLAB_CONFIGURATION_ID; public class DefaultGitlabConfigurationController implements GitlabConfigurationController { @@ -73,7 +75,6 @@ public class DefaultGitlabConfigurationController implements GitlabConfiguration return toGitLabConfigurationResource(createdConfiguration); } - private static GitlabConfiguration toGitlabConfiguration(GitlabConfigurationCreateRestRequest createRestRequest) { return new GitlabConfiguration( UNIQUE_GITLAB_CONFIGURATION_ID, @@ -85,8 +86,7 @@ public class DefaultGitlabConfigurationController implements GitlabConfiguration Set.copyOf(createRestRequest.allowedGroups()), createRestRequest.allowUsersToSignUp() != null && createRestRequest.allowUsersToSignUp(), toProvisioningType(createRestRequest.provisioningType()), - createRestRequest.provisioningToken() - ); + createRestRequest.provisioningToken()); } private GitlabConfigurationResource getGitlabConfigurationResource(String id) { @@ -116,7 +116,8 @@ public class DefaultGitlabConfigurationController implements GitlabConfiguration .build(); } - private static Set getGroups(List groups) { + private static Set getGroups(@Nullable List groups) { + checkArgument(groups != null, "allowedGroups must not be null"); return new HashSet<>(groups); } @@ -132,8 +133,7 @@ public class DefaultGitlabConfigurationController implements GitlabConfiguration configuration.allowUsersToSignUp(), toRestProvisioningType(configuration), StringUtils.isNotEmpty(configuration.provisioningToken()), - configurationError.orElse(null) - ); + configurationError.orElse(null)); } private static org.sonar.server.v2.api.gitlab.config.resource.ProvisioningType toRestProvisioningType(GitlabConfiguration configuration) { diff --git a/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/gitlab/config/request/GitlabConfigurationCreateRestRequest.java b/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/gitlab/config/request/GitlabConfigurationCreateRestRequest.java index 7d548a84642..5ba14e7db54 100644 --- a/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/gitlab/config/request/GitlabConfigurationCreateRestRequest.java +++ b/server/sonar-webserver-webapi-v2/src/main/java/org/sonar/server/v2/api/gitlab/config/request/GitlabConfigurationCreateRestRequest.java @@ -49,8 +49,13 @@ public record GitlabConfigurationCreateRestRequest( @Schema(description = "Set whether to synchronize groups") Boolean synchronizeGroups, - @NotEmpty - @ArraySchema(arraySchema = @Schema(description = "GitLab groups allowed to authenticate and provisioned (for Auto-Provisioning only). Subgroups will automatically be included")) + @NotNull + @ArraySchema(arraySchema = @Schema(description = """ + GitLab groups allowed to authenticate. + Subgroups will automatically be included. + When Auto-provisioning is enabled, members of these groups will be automatically provisioned in SonarQube. + This field is required to be non-empty for Auto-provisioning. + """)) List allowedGroups, @NotNull diff --git a/server/sonar-webserver-webapi-v2/src/test/java/org/sonar/server/v2/api/gitlab/config/DefaultGitlabConfigurationControllerTest.java b/server/sonar-webserver-webapi-v2/src/test/java/org/sonar/server/v2/api/gitlab/config/DefaultGitlabConfigurationControllerTest.java index 217a0a63b0d..26e1ae364f6 100644 --- a/server/sonar-webserver-webapi-v2/src/test/java/org/sonar/server/v2/api/gitlab/config/DefaultGitlabConfigurationControllerTest.java +++ b/server/sonar-webserver-webapi-v2/src/test/java/org/sonar/server/v2/api/gitlab/config/DefaultGitlabConfigurationControllerTest.java @@ -193,6 +193,30 @@ public class DefaultGitlabConfigurationControllerTest { content().json("{\"message\":\"Insufficient privileges\"}")); } + @Test + public void updateConfiguration_whenAllowedGroupsIsNull_shouldReturnBadRequest() throws Exception { + userSession.logIn().setSystemAdministrator(); + + mockMvc.perform(patch(GITLAB_CONFIGURATION_ENDPOINT + "/existing-id") + .contentType(JSON_MERGE_PATCH_CONTENT_TYPE) + .content(""" + { + "enabled": true, + "applicationId": "application-id", + "secret": "123", + "url": "www.url.com", + "synchronizeGroups": true, + "allowedGroups": null, + "provisioningType": "AUTO_PROVISIONING", + "allowUsersToSignUp": true + } + """)) + .andExpectAll( + status().isBadRequest(), + content().json("{\"message\":\"allowedGroups must not be null\"}")); + + } + @Test public void updateConfiguration_whenAllFieldsUpdated_performUpdates() throws Exception { userSession.logIn().setSystemAdministrator(); @@ -398,6 +422,30 @@ public class DefaultGitlabConfigurationControllerTest { } + @Test + public void create_whenAllowedGroupsIsNull_shouldReturnBadRequest() throws Exception { + userSession.logIn().setSystemAdministrator(); + mockMvc.perform( + post(GITLAB_CONFIGURATION_ENDPOINT) + .contentType(MediaType.APPLICATION_JSON_VALUE) + .content(""" + { + "enabled": true, + "applicationId": "application-id", + "secret": "123", + "url": "www.url.com", + "synchronizeGroups": true, + "allowedGroups": null, + "provisioningType": "JIT" + } + + """)) + .andExpectAll( + status().isBadRequest(), + content().json("{\"message\":\"Value {} for field allowedGroups was rejected. Error: must not be null.\"}")); + + } + @Test public void create_whenRequiredParameterIsMissing_shouldReturnBadRequest() throws Exception { userSession.logIn().setSystemAdministrator(); -- 2.39.5