Fix SSF-530.

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 cb7e8cd888953e4c141a4ee4bad64f6959220845..d71cc0406af717825313142ae74eb60de6aca0a7 100644 (file)
--- 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<String> message =  gitlabConfigurationService.validate(gitlabConfiguration);
+    Optional<String> 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 214c196133d6715d145be007e00880c56db29bda..25d1a4977aa0ce7cf9d15adc93a760bbfe15ee20 100644 (file)
--- 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<Boolean> provisioningEnabled =
-      updateRequest.provisioningType().map(GitlabConfigurationService::shouldEnableAutoProvisioning);
+    UpdatedValue<Boolean> 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<String> 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<ProvisioningType> 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<String> 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<String> 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 f85a38e4bbacab8c5e66289b9b446c7b5593ebb5..d9c30bb8c5c4329fd7af9a37ef85d42e851eab9a 100644 (file)
--- 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<String> getGroups(List<String> groups) {
+  private static Set<String> getGroups(@Nullable List<String> 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 7d548a846425878939d26159a87b02d473ab2e02..5ba14e7db54fc0a4d3a54c6b2ed3775b91c9ec72 100644 (file)
--- 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<String> 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 217a0a63b0dc768e8ce8d6fc28142a3ec44e5327..26e1ae364f67e1a67c0d58a1647235e107478626 100644 (file)
--- 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();