From 7ee9c020980cfcc88e652f6987a4211068c366cb Mon Sep 17 00:00:00 2001 From: Aurelien <100427063+aurelien-poscia-sonarsource@users.noreply.github.com> Date: Thu, 24 Mar 2022 14:50:02 +0100 Subject: [PATCH] SONAR-16159 Fix SSF-235 (#5644) (cherry picked from commit b4ca52a480e944211b00159039bd5eb4a9eb3c20) --- .../server/setting/ws/SettingsWsSupport.java | 27 ++- .../setting/ws/SettingsWsSupportTest.java | 182 ++++++++++++++++++ .../server/setting/ws/ValuesActionTest.java | 38 ++++ 3 files changed, 240 insertions(+), 7 deletions(-) create mode 100644 server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/SettingsWsSupportTest.java diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SettingsWsSupport.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SettingsWsSupport.java index 93fd356408f..6c65c03e430 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SettingsWsSupport.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/setting/ws/SettingsWsSupport.java @@ -19,7 +19,10 @@ */ package org.sonar.server.setting.ws; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableSet; import java.util.Optional; +import java.util.Set; import org.sonar.api.server.ServerSide; import org.sonar.api.web.UserRole; import org.sonar.db.component.ComponentDto; @@ -34,6 +37,8 @@ import static org.sonar.api.web.UserRole.ADMIN; @ServerSide public class SettingsWsSupport { public static final String DOT_SECURED = ".secured"; + @VisibleForTesting + static final Set ADMIN_ONLY_SETTINGS = ImmutableSet.of("sonar.auth.bitbucket.workspaces", "sonar.auth.github.organizations"); private final UserSession userSession; @@ -51,26 +56,34 @@ public class SettingsWsSupport { } boolean isVisible(String key, Optional component) { - return hasPermission(GlobalPermission.SCAN, UserRole.SCAN, component) || verifySecuredSetting(key, component); + if (isAdmin(component)) { + return true; + } + return hasPermission(GlobalPermission.SCAN, UserRole.SCAN, component) || !isProtected(key); + } + + private boolean isAdmin(Optional component) { + return userSession.isSystemAdministrator() || hasPermission(GlobalPermission.ADMINISTER, ADMIN, component); + } + + private static boolean isProtected(String key) { + return isSecured(key) || isAdminOnly(key); } static boolean isSecured(String key) { return key.endsWith(DOT_SECURED); } - private boolean verifySecuredSetting(String key, Optional component) { - return (!isSecured(key) || hasPermission(GlobalPermission.ADMINISTER, ADMIN, component)); + private static boolean isAdminOnly(String key) { + return ADMIN_ONLY_SETTINGS.contains(key); } private boolean hasPermission(GlobalPermission orgPermission, String projectPermission, Optional component) { - if (userSession.isSystemAdministrator()) { - return true; - } if (userSession.hasPermission(orgPermission)) { return true; } return component - .map(c -> userSession.hasPermission(orgPermission) || userSession.hasComponentPermission(projectPermission, c)) + .map(c -> userSession.hasComponentPermission(projectPermission, c)) .orElse(false); } diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/SettingsWsSupportTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/SettingsWsSupportTest.java new file mode 100644 index 00000000000..ea718fcc2c7 --- /dev/null +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/setting/ws/SettingsWsSupportTest.java @@ -0,0 +1,182 @@ +/* + * SonarQube + * Copyright (C) 2009-2021 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.server.setting.ws; + +import java.util.Arrays; +import java.util.Collection; +import java.util.Optional; +import java.util.StringJoiner; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.sonar.api.web.UserRole; +import org.sonar.db.component.ComponentDto; +import org.sonar.db.permission.GlobalPermission; +import org.sonar.server.user.UserSession; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; +import static org.mockito.MockitoAnnotations.initMocks; + +@RunWith(value = Parameterized.class) +public class SettingsWsSupportTest { + + private static final String KEY_REQUIRING_ADMIN_PERMISSION = "sonar.auth.bitbucket.workspaces"; + private static final String STANDARD_KEY = "sonar.auth.bitbucket.enabled"; + private static final String SECURED_KEY = "sonar.auth.bitbucket" + SettingsWsSupport.DOT_SECURED; + + @Parameterized.Parameters(name = "{index}: {0}") + public static Collection data() { + return Arrays.asList(new Object[][] { + // admin cases + {testCaseBuilderForAdminOnlyKey().isAdmin(true).expectedIsVisible(true).build()}, + {testCaseBuilderForStandardKey().isAdmin(true).expectedIsVisible(true).build()}, + {testCaseBuilderForSecuredKey().isAdmin(true).expectedIsVisible(true).build()}, + + // non-admin cases + {testCaseBuilderForAdminOnlyKey().isAdmin(false).hasGlobalPermission(true).hasComponentPermission(true).expectedIsVisible(true).build()}, + {testCaseBuilderForAdminOnlyKey().isAdmin(false).hasGlobalPermission(true).hasComponentPermission(false).expectedIsVisible(true).build()}, + {testCaseBuilderForAdminOnlyKey().isAdmin(false).hasGlobalPermission(false).hasComponentPermission(true).expectedIsVisible(true).build()}, + {testCaseBuilderForAdminOnlyKey().isAdmin(false).hasGlobalPermission(false).hasComponentPermission(false).expectedIsVisible(false).build()}, + {testCaseBuilderForSecuredKey().isAdmin(false).hasGlobalPermission(true).hasComponentPermission(true).expectedIsVisible(true).build()}, + {testCaseBuilderForSecuredKey().isAdmin(false).hasGlobalPermission(true).hasComponentPermission(false).expectedIsVisible(true).build()}, + {testCaseBuilderForSecuredKey().isAdmin(false).hasGlobalPermission(false).hasComponentPermission(true).expectedIsVisible(true).build()}, + {testCaseBuilderForSecuredKey().isAdmin(false).hasGlobalPermission(false).hasComponentPermission(false).expectedIsVisible(false).build()}, + {testCaseBuilderForStandardKey().isAdmin(false).hasGlobalPermission(false).hasComponentPermission(false).expectedIsVisible(true).build()}, + {testCaseBuilderForStandardKey().isAdmin(false).hasGlobalPermission(false).hasComponentPermission(true).expectedIsVisible(true).build()}, + {testCaseBuilderForStandardKey().isAdmin(false).hasGlobalPermission(true).hasComponentPermission(true).expectedIsVisible(true).build()}, + {testCaseBuilderForStandardKey().isAdmin(false).hasGlobalPermission(true).hasComponentPermission(false).expectedIsVisible(true).build()}, + }); + } + + private final boolean isAdmin; + private final String property; + private final boolean hasGlobalPermission; + private final boolean hasComponentPermission; + private final boolean expectedIsVisible; + + @Mock + private ComponentDto componentDto; + @Mock + private UserSession userSession; + @InjectMocks + private SettingsWsSupport settingsWsSupport; + + public SettingsWsSupportTest(TestCase testCase) { + this.isAdmin = testCase.isAdmin; + this.property = testCase.property; + this.hasGlobalPermission = testCase.hasGlobalPermission; + this.hasComponentPermission = testCase.hasComponentPermission; + this.expectedIsVisible = testCase.expectedIsVisible; + } + + @Test + public void isVisible() { + initMocks(this); + when(userSession.isSystemAdministrator()).thenReturn(isAdmin); + when(userSession.hasPermission(GlobalPermission.SCAN)).thenReturn(hasGlobalPermission); + when(userSession.hasComponentPermission(UserRole.SCAN, componentDto)).thenReturn(hasComponentPermission); + + boolean isVisible = settingsWsSupport.isVisible(property, Optional.of(componentDto)); + assertThat(isVisible).isEqualTo(expectedIsVisible); + } + + private static TestCase.TestCaseBuilder testCaseBuilderForAdminOnlyKey() { + return testCaseBuilder().property(KEY_REQUIRING_ADMIN_PERMISSION); + } + + private static TestCase.TestCaseBuilder testCaseBuilderForSecuredKey() { + return testCaseBuilder().property(SECURED_KEY); + } + + private static TestCase.TestCaseBuilder testCaseBuilderForStandardKey() { + return testCaseBuilder().property(STANDARD_KEY); + } + + private static TestCase.TestCaseBuilder testCaseBuilder() { + return new TestCase.TestCaseBuilder(); + } + + static final class TestCase { + private final boolean isAdmin; + private final String property; + private final boolean hasGlobalPermission; + private final boolean hasComponentPermission; + private final boolean expectedIsVisible; + + TestCase(boolean isAdmin, String property, boolean hasGlobalPermission, boolean hasComponentPermission, boolean expectedIsVisible) { + this.isAdmin = isAdmin; + this.property = property; + this.hasGlobalPermission = hasGlobalPermission; + this.hasComponentPermission = hasComponentPermission; + this.expectedIsVisible = expectedIsVisible; + } + + @Override public String toString() { + return new StringJoiner(", ", TestCase.class.getSimpleName() + "[", "]") + .add("isAdmin=" + isAdmin) + .add("property='" + property + "'") + .add("hasComponentPermission=" + hasComponentPermission) + .add("hasGlobalPermission=" + hasGlobalPermission) + .add("expectedIsVisible=" + expectedIsVisible) + .toString(); + } + + static final class TestCaseBuilder { + private boolean isAdmin = false; + private String property; + private boolean hasGlobalPermission = false; + private boolean hasComponentPermission = false; + private boolean expectedIsVisible; + + TestCaseBuilder isAdmin(boolean isAdmin) { + this.isAdmin = isAdmin; + return this; + } + + TestCaseBuilder property(String property) { + this.property = property; + return this; + } + + TestCaseBuilder hasGlobalPermission(boolean hasGlobalPermission) { + this.hasGlobalPermission = hasGlobalPermission; + return this; + } + + TestCaseBuilder hasComponentPermission(boolean hasComponentPermission) { + this.hasComponentPermission = hasComponentPermission; + return this; + } + + TestCaseBuilder expectedIsVisible(boolean expectedIsVisible) { + this.expectedIsVisible = expectedIsVisible; + return this; + } + + TestCase build() { + return new TestCase(isAdmin, property, hasGlobalPermission, hasComponentPermission, expectedIsVisible); + } + } + } + +} 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 263d14f3fae..a8ca9c14bc8 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 @@ -621,6 +621,44 @@ public class ValuesActionTest { assertFieldValues(result.getSettings(0), ImmutableMap.of("key", "key1", "secret.secured", "123456")); } + @Test + public void return_admin_only_settings_in_property_set_when_system_admin() { + String anyAdminOnlySettingKey = SettingsWsSupport.ADMIN_ONLY_SETTINGS.iterator().next(); + logInAsAdmin(); + definitions.addComponent(PropertyDefinition + .builder(anyAdminOnlySettingKey) + .type(PropertyType.PROPERTY_SET) + .fields(asList( + PropertyFieldDefinition.build(anyAdminOnlySettingKey).name("Key admnin only").build(), + PropertyFieldDefinition.build(anyAdminOnlySettingKey).name("Value admin only").build())) + .build()); + ImmutableMap keyValuePairs = ImmutableMap.of(anyAdminOnlySettingKey, "test_val"); + propertyDb.insertPropertySet(anyAdminOnlySettingKey, null, keyValuePairs); + + ValuesWsResponse result = executeRequestForGlobalProperties(); + + assertFieldValues(result.getSettings(0), keyValuePairs); + } + + @Test + public void return_admin_only_settings_in_property_not_set_when_simple_user() { + String anyAdminOnlySettingKey = SettingsWsSupport.ADMIN_ONLY_SETTINGS.iterator().next(); + logIn(); + definitions.addComponent(PropertyDefinition + .builder(anyAdminOnlySettingKey) + .type(PropertyType.PROPERTY_SET) + .fields(asList( + PropertyFieldDefinition.build(anyAdminOnlySettingKey).name("Key admnin only").build(), + PropertyFieldDefinition.build(anyAdminOnlySettingKey).name("Value admin only").build())) + .build()); + ImmutableMap keyValuePairs = ImmutableMap.of(anyAdminOnlySettingKey, "test_val"); + propertyDb.insertPropertySet(anyAdminOnlySettingKey, null, keyValuePairs); + + ValuesWsResponse result = executeRequestForGlobalProperties(); + + assertThat(result.getSettingsList()).isEmpty(); + } + @Test public void return_global_settings_from_definitions_when_no_component_and_no_keys() { logInAsAdmin(); -- 2.39.5