SONAR-15338 Filter out SQ secured properties on settings endpoint

build.gradle

@@ -173,7 +173,7 @@ subprojects {
   }
 
   ext {
-    protobufVersion = '3.11.4'
+    protobufVersion = '3.17.3'
 
     // define a method which can be called by project to change Java version to compile to
     configureCompileJavaToVersion = { javaVersion ->

server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/ValuesAction.java

@@ -43,7 +43,6 @@ import org.sonar.api.server.ws.Request;
 import org.sonar.api.server.ws.Response;
 import org.sonar.api.server.ws.WebService;
 import org.sonar.api.web.UserRole;
-import org.sonar.core.util.stream.MoreCollectors;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.component.ComponentDto;
@@ -100,16 +99,11 @@ public class ValuesAction implements SettingsWsAction {
       .setDescription("List settings values.<br>" +
         "If no value has been set for a setting, then the default value is returned.<br>" +
         "The settings from conf/sonar.properties are excluded from results.<br>" +
-        "Requires 'Browse' or 'Execute Analysis' permission when a component is specified.<br/>" +
-        "To access secured settings, one of the following permissions is required: " +
-        "<ul>" +
-        "<li>'Execute Analysis'</li>" +
-        "<li>'Administer System'</li>" +
-        "<li>'Administer' rights on the specified component</li>" +
-        "</ul>")
+        "Requires 'Browse' or 'Execute Analysis' permission when a component is specified.<br/>")
       .setResponseExample(getClass().getResource("values-example.json"))
       .setSince("6.3")
       .setChangelog(
+        new Change("9.1", "The value of secured settings are no longer returned"),
         new Change("7.6", String.format("The use of module keys in parameter '%s' is deprecated", PARAM_COMPONENT)),
         new Change("7.1", "The settings from conf/sonar.properties are excluded from results."))
       .setHandler(this);
@@ -196,18 +190,11 @@ public class ValuesAction implements SettingsWsAction {
   }
 
   private List<Setting> loadGlobalSettings(DbSession dbSession, Set<String> keys) {
-    Set<String> allowedKeys;
-    if (isSonarCloud && !userSession.isSystemAdministrator()) {
-      // remove the global settings that require admin permission
-      allowedKeys = keys.stream().filter(k -> !isSecured(k)).collect(Collectors.toSet());
-    } else {
-      allowedKeys = keys;
-    }
-    List<PropertyDto> properties = dbClient.propertiesDao().selectGlobalPropertiesByKeys(dbSession, allowedKeys);
+    List<PropertyDto> properties = dbClient.propertiesDao().selectGlobalPropertiesByKeys(dbSession, keys);
     List<PropertyDto> propertySets = dbClient.propertiesDao().selectGlobalPropertiesByKeys(dbSession, getPropertySetKeys(properties));
     return properties.stream()
       .map(property -> Setting.createFromDto(property, getPropertySets(property.getKey(), propertySets, null), propertyDefinitions.get(property.getKey())))
-      .collect(MoreCollectors.toList(properties.size()));
+      .collect(Collectors.toList());
   }
 
   /**
@@ -286,6 +273,9 @@ public class ValuesAction implements SettingsWsAction {
     }
 
     private void setValue(Setting setting, Settings.Setting.Builder valueBuilder) {
+      if (isSecured(setting.getKey())) {
+        return;
+      }
       PropertyDefinition definition = setting.getDefinition();
       String value = setting.getValue();
       if (definition == null) {

server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/ValuesActionTest.java

@@ -21,7 +21,10 @@ package org.sonar.server.setting.ws;
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableMap;
+import java.util.Comparator;
+import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import org.junit.Before;
 import org.junit.Rule;
@@ -56,6 +59,7 @@ import org.sonarqube.ws.Settings.ValuesWsResponse;
 
 import static java.lang.String.format;
 import static java.util.Arrays.asList;
+import static java.util.Comparator.comparing;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.groups.Tuple.tuple;
 import static org.sonar.api.resources.Qualifiers.MODULE;
@@ -608,7 +612,9 @@ public class ValuesActionTest {
 
     ValuesWsResponse result = executeRequestForProjectProperties();
 
-    assertThat(result.getSettingsList()).extracting(Settings.Setting::getKey).containsOnly("foo", "global.secret.secured", "secret.secured");
+    List<Settings.Setting> settingsList = result.getSettingsList().stream().sorted(comparing(Settings.Setting::getKey)).collect(Collectors.toList());
+    assertThat(settingsList).extracting(Settings.Setting::getKey).containsExactly("foo", "global.secret.secured", "secret.secured");
+    assertThat(settingsList).extracting(Settings.Setting::hasValue).containsExactly(true, false, false);
   }
 
   @Test
@@ -804,7 +810,7 @@ public class ValuesActionTest {
   }
 
   @Test
-  public void sonarcloud_global_secured_properties_require_system_admin_permission() {
+  public void global_secured_properties_require_system_admin_permission() {
     PropertyDefinition securedDef = PropertyDefinition.builder("my.password.secured").build();
     PropertyDefinition standardDef = PropertyDefinition.builder("my.property").build();
     definitions.addComponents(asList(securedDef, standardDef));
@@ -813,26 +819,29 @@ public class ValuesActionTest {
       newGlobalPropertyDto().setKey(standardDef.key()).setValue("standardValue"));
 
     // anonymous
-    WsActionTester tester = newSonarCloudTester();
+    WsActionTester tester = newTester();
     ValuesWsResponse response = executeRequest(tester, null, securedDef.key(), standardDef.key());
-    assertThat(response.getSettingsList()).extracting(Settings.Setting::getValue).containsExactly("standardValue");
+    assertThat(response.getSettingsList()).extracting(Settings.Setting::getKey).containsExactly("my.property");
 
     // only scan global permission
     userSession.logIn()
       .addPermission(GlobalPermission.SCAN);
     response = executeRequest(tester, null, securedDef.key(), standardDef.key());
-    assertThat(response.getSettingsList()).extracting(Settings.Setting::getValue).containsExactly("standardValue");
+    assertThat(response.getSettingsList()).extracting(Settings.Setting::getKey).containsExactly("my.password.secured", "my.property");
+    assertThat(response.getSettingsList()).extracting(Settings.Setting::hasValue).containsExactly(false, true);
 
     // global administrator
     userSession.logIn()
       .addPermission(GlobalPermission.ADMINISTER);
     response = executeRequest(tester, null, securedDef.key(), standardDef.key());
-    assertThat(response.getSettingsList()).extracting(Settings.Setting::getValue).containsExactly("standardValue");
+    assertThat(response.getSettingsList()).extracting(Settings.Setting::getKey).containsExactly("my.password.secured", "my.property");
+    assertThat(response.getSettingsList()).extracting(Settings.Setting::hasValue).containsExactly(false, true);
 
     // system administrator
     userSession.logIn().setSystemAdministrator();
     response = executeRequest(tester, null, securedDef.key(), standardDef.key());
-    assertThat(response.getSettingsList()).extracting(Settings.Setting::getValue).containsExactlyInAnyOrder("securedValue", "standardValue");
+    assertThat(response.getSettingsList()).extracting(Settings.Setting::getKey).containsExactly("my.password.secured", "my.property");
+    assertThat(response.getSettingsList()).extracting(Settings.Setting::hasValue).containsExactly(false, true);
   }
 
   private ValuesWsResponse executeRequestForComponentProperties(ComponentDto componentDto, String... keys) {

sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/AbstractSettingsLoader.java

@@ -86,7 +86,9 @@ public abstract class AbstractSettingsLoader {
             convertPropertySetToProps(result, s);
             break;
           default:
-            throw new IllegalStateException("Unknown property value for " + s.getKey());
+            if (!s.getKey().endsWith(".secured")) {
+              throw new IllegalStateException("Unknown property value for " + s.getKey());
+            }
         }
       }
     }

sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/AbstractSettingsLoaderTest.java

@@ -28,6 +28,7 @@ import org.sonarqube.ws.Settings.Values;
 
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.entry;
 
 public class AbstractSettingsLoaderTest {
@@ -48,6 +49,21 @@ public class AbstractSettingsLoaderTest {
         .containsExactly(entry("sonar.preview.supportedPlugins", "\"ja,va\",\"p\"\"hp\""));
   }
 
+  @Test
+  public void should_throw_exception_when_no_value_of_non_secured_settings() {
+    assertThatThrownBy(() -> AbstractSettingsLoader.toMap(singletonList(Setting.newBuilder()
+      .setKey("sonar.open.setting").build())))
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessage("Unknown property value for sonar.open.setting");
+  }
+
+  @Test
+  public void should_filter_secured_settings_without_value() {
+    assertThat(AbstractSettingsLoader.toMap(singletonList(Setting.newBuilder()
+      .setKey("sonar.setting.secured").build())))
+        .isEmpty();
+  }
+
   @Test
   public void should_load_global_propertyset_settings() {
     Builder valuesBuilder = Value.newBuilder();