SONAR-22363 Fix SSF-572

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 d71cc0406af717825313142ae74eb60de6aca0a7..5c01f93e5090a98d668514668d7650bcf2ed1b08 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
@@ -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 0b4c6e420bb7bcbb3650bac5588e422c12243d2d..c2aa53142e7ab069af84315a2cee484c1bf44014 100644 (file)
--- 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 52521d0d765977553c2ea53013548fc4e59a4ecd..40126853aa0cdaf9179dd326cfc27ae3a842d25d 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
@@ -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))

diff --git a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/setting/ws/SetActionIT.java b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/setting/ws/SetActionIT.java
index 314851cca4f444d87094c5135b678cd392d47b5b..e61fd9d52eb339378f41cc8d80eacdec7aa69462 100644 (file)
--- a/server/sonar-webserver-webapi/src/it/java/org/sonar/server/setting/ws/SetActionIT.java
+++ b/server/sonar-webserver-webapi/src/it/java/org/sonar/server/setting/ws/SetActionIT.java
@@ -1128,6 +1128,16 @@ public class SetActionIT {
       .hasMessage(format("Setting '%s' can only be used in sonar.properties", settingKey));
   }
 
+  @Test
+  public void fail_when_setting_key_is_forbidden() {
+    TestRequest testRequest = ws.newRequest()
+      .setParam("key", "sonar.auth.gitlab.url")
+      .setParam("value", "http://malicious.url");
+    assertThatThrownBy(testRequest::execute)
+      .isInstanceOf(IllegalArgumentException.class)
+      .hasMessage("For security reasons, the key 'sonar.auth.gitlab.url' cannot be updated using this webservice. Please use the API v2");
+  }
+
   @Test
   public void definition() {
     WebService.Action definition = ws.getDef();

diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SetAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SetAction.java
index 5a87916e21df34ff3af4e64d961251f9a36740a7..034a2478dcd1acdf0131a8defd3769bb921ce9a7 100644 (file)
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SetAction.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SetAction.java
@@ -69,8 +69,9 @@ public class SetAction implements SettingsWsAction {
   private static final Collector<CharSequence, ?, String> COMMA_JOINER = Collectors.joining(",");
   private static final String MSG_NO_EMPTY_VALUE = "A non empty value must be provided";
   private static final int VALUE_MAXIMUM_LENGTH = 4000;
-  private static final TypeToken<Map<String, String>> MAP_TYPE_TOKEN = new TypeToken<>() {
-  };
+  private static final TypeToken<Map<String, String>> MAP_TYPE_TOKEN = new TypeToken<>() {};
+  private static final Set<String> FORBIDDEN_KEYS = Set.of("sonar.auth.gitlab.url");
+
 
   private final PropertyDefinitions propertyDefinitions;
   private final DbClient dbClient;
@@ -138,12 +139,19 @@ public class SetAction implements SettingsWsAction {
   public void handle(Request request, Response response) throws Exception {
     try (DbSession dbSession = dbClient.openSession(false)) {
       SetRequest wsRequest = toWsRequest(request);
+      throwIfForbiddenKey(wsRequest.getKey());
       SettingsWsSupport.validateKey(wsRequest.getKey());
       doHandle(dbSession, wsRequest);
     }
     response.noContent();
   }
 
+  private static void throwIfForbiddenKey(String key) {
+    if (FORBIDDEN_KEYS.contains(key)) {
+      throw new IllegalArgumentException(format("For security reasons, the key '%s' cannot be updated using this webservice. Please use the API v2", key));
+    }
+  }
+
   private void doHandle(DbSession dbSession, SetRequest request) {
     Optional<EntityDto> component = searchEntity(dbSession, request);
     String projectKey = component.map(EntityDto::getKey).orElse(null);