SONAR-16159 Fix SSF-235 (#5644)

(cherry picked from commit b4ca52a480e944211b00159039bd5eb4a9eb3c20)

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 93fd356408fbea95f7583be525e0fe2a2f083116..6c65c03e4306a61b058421cab6d1b5521b9dccd0 100644 (file)
--- 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<String> 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<ComponentDto> 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<ComponentDto> 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<ComponentDto> 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<ComponentDto> 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 (file)
index 0000000..ea718fc
--- /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<Object[]> 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 263d14f3fae29f6a955154c30f0fff79ffe6830d..a8ca9c14bc87b26bbfcc16d23e26462a93a725f7 100644 (file)
--- 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<String, String> 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<String, String> 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();