Giving admin priority over backendrole filtering (#850)

* giving admin priority over backend role filtering Signed-off-by: Amit Galitzky <[email protected]> * fix security tests Signed-off-by: Amit Galitzky <[email protected]> * remove redundent line in test case Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]>
opensearch-project · Apr 11, 2023 · b8df172 · b8df172
1 parent fd2fd5d
commit b8df172
Show file tree

Hide file tree

Showing 4 changed files with 115 additions and 3 deletions.
diff --git a/src/main/java/org/opensearch/ad/transport/handler/ADSearchHandler.java b/src/main/java/org/opensearch/ad/transport/handler/ADSearchHandler.java
@@ -15,6 +15,7 @@
 import static org.opensearch.ad.settings.AnomalyDetectorSettings.FILTER_BY_BACKEND_ROLES;
 import static org.opensearch.ad.util.ParseUtils.addUserBackendRolesFilter;
 import static org.opensearch.ad.util.ParseUtils.getUserContext;
+import static org.opensearch.ad.util.ParseUtils.isAdmin;
 import static org.opensearch.ad.util.RestHandlerUtils.wrapRestActionListener;
 
 import org.apache.logging.log4j.LogManager;
@@ -62,13 +63,14 @@ public void search(SearchRequest request, ActionListener<SearchResponse> actionL
     }
 
     private void validateRole(SearchRequest request, User user, ActionListener<SearchResponse> listener) {
-        if (user == null || !filterEnabled) {
+        if (user == null || !filterEnabled || isAdmin(user)) {
             // Case 1: user == null when 1. Security is disabled. 2. When user is super-admin
             // Case 2: If Security is enabled and filter is disabled, proceed with search as
             // user is already authenticated to hit this API.
+            // case 3: user is admin which means we don't have to check backend role filtering
             client.search(request, listener);
         } else {
-            // Security is enabled and filter is enabled
+            // Security is enabled, filter is enabled and user isn't admin
             try {
                 addUserBackendRolesFilter(user, request.source());
                 logger.debug("Filtering result by " + user.getBackendRoles());

diff --git a/src/main/java/org/opensearch/ad/util/ParseUtils.java b/src/main/java/org/opensearch/ad/util/ParseUtils.java
@@ -559,7 +559,7 @@ public static void onGetAdResponse(
                 AnomalyDetector detector = AnomalyDetector.parse(parser);
                 User resourceUser = detector.getUser();
 
-                if (!filterByBackendRole || checkUserPermissions(requestUser, resourceUser, detectorId)) {
+                if (!filterByBackendRole || checkUserPermissions(requestUser, resourceUser, detectorId) || isAdmin(requestUser)) {
                     function.accept(detector);
                 } else {
                     logger.debug("User: " + requestUser.getName() + " does not have permissions to access detector: " + detectorId);
@@ -573,6 +573,16 @@ public static void onGetAdResponse(
         }
     }
 
+    /**
+     *  'all_access' role users are treated as admins.
+     */
+    public static boolean isAdmin(User user) {
+        if (user == null) {
+            return false;
+        }
+        return user.getRoles().contains("all_access");
+    }
+
     private static boolean checkUserPermissions(User requestedUser, User resourceUser, String detectorId) throws Exception {
         if (resourceUser.getBackendRoles() == null || requestedUser.getBackendRoles() == null) {
             return false;

diff --git a/src/test/java/org/opensearch/ad/rest/SecureADRestIT.java b/src/test/java/org/opensearch/ad/rest/SecureADRestIT.java
@@ -16,8 +16,11 @@
 import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Map;
 
+import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.message.BasicHeader;
 import org.hamcrest.CoreMatchers;
 import org.hamcrest.MatcherAssert;
 import org.junit.After;
@@ -186,6 +189,77 @@ public void testGetApiFilterByEnabled() throws IOException {
             );
     }
 
+    private void confirmingClientIsAdmin() throws IOException {
+        Response resp = TestHelpers
+            .makeRequest(
+                client(),
+                "GET",
+                "_plugins/_security/api/account",
+                null,
+                "",
+                ImmutableList.of(new BasicHeader(HttpHeaders.USER_AGENT, "admin"))
+            );
+        Map<String, Object> responseMap = entityAsMap(resp);
+        ArrayList<String> roles = (ArrayList<String>) responseMap.get("roles");
+        assertTrue(roles.contains("all_access"));
+    }
+
+    public void testGetApiFilterByEnabledForAdmin() throws IOException {
+        // User Alice has AD full access, should be able to create a detector and has backend role "odfe"
+        AnomalyDetector aliceDetector = createRandomAnomalyDetector(false, false, aliceClient);
+        enableFilterBy();
+        confirmingClientIsAdmin();
+        AnomalyDetector detector = getAnomalyDetector(aliceDetector.getDetectorId(), client());
+        Assert
+            .assertArrayEquals(
+                "User backend role of detector doesn't change",
+                new String[] { "odfe" },
+                detector.getUser().getBackendRoles().toArray(new String[0])
+            );
+    }
+
+    public void testUpdateApiFilterByEnabledForAdmin() throws IOException {
+        // User Alice has AD full access, should be able to create a detector and has backend role "odfe"
+        AnomalyDetector aliceDetector = createRandomAnomalyDetector(false, false, aliceClient);
+        enableFilterBy();
+
+        AnomalyDetector newDetector = new AnomalyDetector(
+            aliceDetector.getDetectorId(),
+            aliceDetector.getVersion(),
+            aliceDetector.getName(),
+            randomAlphaOfLength(10),
+            aliceDetector.getTimeField(),
+            aliceDetector.getIndices(),
+            aliceDetector.getFeatureAttributes(),
+            aliceDetector.getFilterQuery(),
+            aliceDetector.getDetectionInterval(),
+            aliceDetector.getWindowDelay(),
+            aliceDetector.getShingleSize(),
+            aliceDetector.getUiMetadata(),
+            aliceDetector.getSchemaVersion(),
+            Instant.now(),
+            aliceDetector.getCategoryField(),
+            new User(
+                randomAlphaOfLength(5),
+                ImmutableList.of("odfe", randomAlphaOfLength(5)),
+                ImmutableList.of(randomAlphaOfLength(5)),
+                ImmutableList.of(randomAlphaOfLength(5))
+            ),
+            null
+        );
+        // User client has admin all access, and has "opensearch" backend role so client should be able to update detector
+        // But the detector's backend role should not be replaced as client's backend roles (all_access).
+        Response response = updateAnomalyDetector(aliceDetector.getDetectorId(), newDetector, client());
+        Assert.assertEquals(response.getStatusLine().getStatusCode(), 200);
+        AnomalyDetector anomalyDetector = getAnomalyDetector(aliceDetector.getDetectorId(), aliceClient);
+        Assert
+            .assertArrayEquals(
+                "odfe is still the backendrole, not opensearch",
+                new String[] { "odfe" },
+                anomalyDetector.getUser().getBackendRoles().toArray(new String[0])
+            );
+    }
+
     public void testUpdateApiFilterByEnabled() throws IOException {
         // User Alice has AD full access, should be able to create a detector
         AnomalyDetector aliceDetector = createRandomAnomalyDetector(false, false, aliceClient);

diff --git a/src/test/java/org/opensearch/ad/util/ParseUtilsTests.java b/src/test/java/org/opensearch/ad/util/ParseUtilsTests.java
@@ -12,6 +12,7 @@
 package org.opensearch.ad.util;
 
 import static org.opensearch.ad.util.ParseUtils.addUserBackendRolesFilter;
+import static org.opensearch.ad.util.ParseUtils.isAdmin;
 
 import java.io.IOException;
 import java.time.Instant;
@@ -287,4 +288,29 @@ public void testGetFeatureFieldNames() throws IOException {
         assertTrue(fieldNames.contains("field-name1"));
         assertTrue(fieldNames.contains("field-name2"));
     }
+
+    public void testIsAdmin() {
+        User user1 = new User(
+            randomAlphaOfLength(5),
+            ImmutableList.of(),
+            ImmutableList.of("all_access"),
+            ImmutableList.of(randomAlphaOfLength(5))
+        );
+        assertTrue(isAdmin(user1));
+    }
+
+    public void testIsAdminBackendRoleIsAllAccess() {
+        String backendRole1 = "all_access";
+        User user1 = new User(
+            randomAlphaOfLength(5),
+            ImmutableList.of(backendRole1),
+            ImmutableList.of(randomAlphaOfLength(5)),
+            ImmutableList.of(randomAlphaOfLength(5))
+        );
+        assertFalse(isAdmin(user1));
+    }
+
+    public void testIsAdminNull() {
+        assertFalse(isAdmin(null));
+    }
 }