SONAR-22363 Fix SSF-572

3 files changed, 44 insertions, 2 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 d71cc0406af..5c01f93e509 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
@@ -295,6 +295,36 @@ public class GitlabConfigurationServiceIT {
     verify(managedInstanceService, times(0)).queueSynchronisationTask();
   }
 
+  @Test
+  public void updateConfiguration_whenURLChangesWithoutSecret_shouldFail() {
+    gitlabConfigurationService.createConfiguration(buildGitlabConfiguration(JIT));
+
+    UpdateGitlabConfigurationRequest updateUrlRequest = builder()
+      .gitlabConfigurationId(UNIQUE_GITLAB_CONFIGURATION_ID)
+      .url(withValueOrThrow("http://malicious.url"))
+      .build();
+
+    assertThatThrownBy(() -> gitlabConfigurationService.updateConfiguration(updateUrlRequest))
+      .isInstanceOf(IllegalArgumentException.class)
+      .hasMessage("For security reasons, the URL can't be updated without providing the secret.");
+  }
+
+  @Test
+  public void updateConfiguration_whenURLChangesWithAllSecrets_shouldUpdate() {
+    gitlabConfigurationService.createConfiguration(buildGitlabConfiguration(JIT));
+
+    UpdateGitlabConfigurationRequest updateUrlRequest = builder()
+      .gitlabConfigurationId(UNIQUE_GITLAB_CONFIGURATION_ID)
+      .url(withValueOrThrow("http://new.url"))
+      .secret(withValueOrThrow("new_secret"))
+      .build();
+
+    gitlabConfigurationService.updateConfiguration(updateUrlRequest);
+
+    verifySettingWasSet(GITLAB_AUTH_URL, "http://new.url");
+    verifySettingWasSet(GITLAB_AUTH_SECRET, "new_secret");
+  }
+
   private static void assertConfigurationFields(GitlabConfiguration configuration) {
     assertThat(configuration).isNotNull();
     assertThat(configuration.id()).isEqualTo("gitlab-configuration");
diff --git a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/UpdatedValue.java b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/UpdatedValue.java
index 0b4c6e420bb..c2aa53142e7 100644
--- a/server/sonar-webserver-common/src/main/java/org/sonar/server/common/UpdatedValue.java
+++ b/server/sonar-webserver-common/src/main/java/org/sonar/server/common/UpdatedValue.java
@@ -19,11 +19,11 @@
  */
 package org.sonar.server.common;
 
-import java.util.StringJoiner;
-import javax.annotation.Nullable;
 import java.util.Objects;
+import java.util.StringJoiner;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import javax.annotation.Nullable;
 
 public class UpdatedValue<T> {
   final T value;
@@ -62,6 +62,10 @@ public class UpdatedValue<T> {
     return false;
   }
 
+  public boolean isDefined() {
+    return isDefined;
+  }
+
   @Override
   public boolean equals(Object o) {
     if (this == o) {
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 52521d0d765..40126853aa0 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
@@ -42,6 +42,7 @@ import static java.lang.String.format;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 import static org.sonar.alm.client.gitlab.GitlabGlobalSettingsValidator.ValidationMode.AUTH_ONLY;
 import static org.sonar.alm.client.gitlab.GitlabGlobalSettingsValidator.ValidationMode.COMPLETE;
+import static org.sonar.api.utils.Preconditions.checkArgument;
 import static org.sonar.api.utils.Preconditions.checkState;
 import static org.sonar.auth.gitlab.GitLabSettings.GITLAB_AUTH_ALLOWED_GROUPS;
 import static org.sonar.auth.gitlab.GitLabSettings.GITLAB_AUTH_ALLOW_USERS_TO_SIGNUP;
@@ -86,6 +87,7 @@ public class GitlabConfigurationService {
 
   public GitlabConfiguration updateConfiguration(UpdateGitlabConfigurationRequest updateRequest) {
     UpdatedValue<Boolean> provisioningEnabled = updateRequest.provisioningType().map(GitlabConfigurationService::shouldEnableAutoProvisioning);
+    throwIfUrlIsUpdatedWithoutSecrets(updateRequest);
     try (DbSession dbSession = dbClient.openSession(true)) {
       throwIfConfigurationDoesntExist(dbSession);
       GitlabConfiguration currentConfiguration = getConfiguration(updateRequest.gitlabConfigurationId(), dbSession);
@@ -114,6 +116,12 @@ public class GitlabConfigurationService {
     }
   }
 
+  private static void throwIfUrlIsUpdatedWithoutSecrets(UpdateGitlabConfigurationRequest request) {
+    if (request.url().isDefined()) {
+      checkArgument(request.secret().isDefined(), "For security reasons, the URL can't be updated without providing the secret.");
+    }
+  }
+
   private void setIfDefined(DbSession dbSession, String propertyName, UpdatedValue<String> value) {
     value
       .map(definedValue -> new PropertyDto().setKey(propertyName).setValue(definedValue))