From 424216bbde1a4c1f6741c4f8ea0fbb885645b056 Mon Sep 17 00:00:00 2001 From: Zipeng WU Date: Tue, 7 Sep 2021 11:09:08 +0200 Subject: [PATCH] SONAR-15338 Filter out SQ secured properties on settings endpoint --- build.gradle | 2 +- .../sonar/server/setting/ws/ValuesAction.java | 24 ++++++------------- .../server/setting/ws/ValuesActionTest.java | 23 ++++++++++++------ .../settings/AbstractSettingsLoader.java | 4 +++- .../settings/AbstractSettingsLoaderTest.java | 16 +++++++++++++ 5 files changed, 43 insertions(+), 26 deletions(-) diff --git a/build.gradle b/build.gradle index c0bd025d181..2be903ef795 100644 --- a/build.gradle +++ b/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 -> diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/ValuesAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/ValuesAction.java index 2ea6e062bee..bf0cb210972 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/ValuesAction.java +++ b/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.
" + "If no value has been set for a setting, then the default value is returned.
" + "The settings from conf/sonar.properties are excluded from results.
" + - "Requires 'Browse' or 'Execute Analysis' permission when a component is specified.
" + - "To access secured settings, one of the following permissions is required: " + - "") + "Requires 'Browse' or 'Execute Analysis' permission when a component is specified.
") .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 loadGlobalSettings(DbSession dbSession, Set keys) { - Set 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 properties = dbClient.propertiesDao().selectGlobalPropertiesByKeys(dbSession, allowedKeys); + List properties = dbClient.propertiesDao().selectGlobalPropertiesByKeys(dbSession, keys); List 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) { diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/ValuesActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/ValuesActionTest.java index 7170642a5db..8ac58cda4e2 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/ValuesActionTest.java +++ b/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 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) { diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/AbstractSettingsLoader.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/AbstractSettingsLoader.java index 47760715ef8..49fe0f1c1cc 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/repository/settings/AbstractSettingsLoader.java +++ b/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()); + } } } } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/AbstractSettingsLoaderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/AbstractSettingsLoaderTest.java index 5b539e312e7..b7b94871a57 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/repository/settings/AbstractSettingsLoaderTest.java +++ b/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(); -- 2.39.5