Remove special handling for do_not_fail_on_forbidden on cluster actions

This special handling does not provide any benefits - rather it just causes some inconsistent behavior. Fixes opensearch-project#4485 Signed-off-by: Nils Bandener <[email protected]>
nibix · Jun 24, 2024 · 09bb8c5 · 09bb8c5
1 parent 37afcaa
commit 09bb8c5
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 35 deletions.
diff --git a/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java b/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java
@@ -278,7 +278,7 @@ public void shouldSearchForDocumentsViaAll_negative() throws IOException {
     @Test
     public void shouldMGetDocument_positive() throws IOException {
         try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
-            MultiGetRequest request = new MultiGetRequest().add(BOTH_INDEX_PATTERN, ID_1).add(BOTH_INDEX_PATTERN, ID_4);
+            MultiGetRequest request = new MultiGetRequest().add(MARVELOUS_SONGS, ID_1).add(MARVELOUS_SONGS, ID_4);
 
             MultiGetResponse response = restHighLevelClient.mget(request, DEFAULT);
 
@@ -296,12 +296,35 @@ public void shouldMGetDocument_positive() throws IOException {
         }
     }
 
+    @Test
+    public void shouldMGetDocument_partial() throws Exception {
+        try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
+            MultiGetRequest request = new MultiGetRequest().add(MARVELOUS_SONGS, ID_1).add(HORRIBLE_SONGS, ID_4);
+
+            MultiGetResponse response = restHighLevelClient.mget(request, DEFAULT);
+
+            MultiGetItemResponse[] responses = response.getResponses();
+            assertThat(responses, arrayWithSize(2));
+            MultiGetItemResponse firstResult = responses[0];
+            MultiGetItemResponse secondResult = responses[1];
+            assertThat(firstResult.getFailure(), nullValue());
+            assertThat(
+                firstResult.getResponse(),
+                allOf(containDocument(MARVELOUS_SONGS, ID_1), documentContainField(FIELD_TITLE, TITLE_MAGNUM_OPUS))
+            );
+            assertThat(secondResult.getFailure().getMessage(), containsString("no permissions for [indices:data/read/mget[shard]]"));
+        }
+    }
+
     @Test
     public void shouldMGetDocument_negative() throws IOException {
         try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
             MultiGetRequest request = new MultiGetRequest().add(HORRIBLE_SONGS, ID_4);
-
-            assertThatThrownBy(() -> restHighLevelClient.mget(request, DEFAULT), statusException(FORBIDDEN));
+            MultiGetResponse response = restHighLevelClient.mget(request, DEFAULT);
+            MultiGetItemResponse[] responses = response.getResponses();
+            assertThat(responses, arrayWithSize(1));
+            MultiGetItemResponse firstResult = responses[0];
+            assertThat(firstResult.getFailure().getMessage(), containsString("no permissions for [indices:data/read/mget[shard]]"));
         }
     }
 
@@ -331,8 +354,10 @@ public void shouldMSearchDocument_negative() throws IOException {
         try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
             MultiSearchRequest request = new MultiSearchRequest();
             request.add(queryStringQueryRequest(FORBIDDEN_INDEX_ALIAS, QUERY_TITLE_POISON));
-
-            assertThatThrownBy(() -> restHighLevelClient.msearch(request, DEFAULT), statusException(FORBIDDEN));
+            MultiSearchResponse response = restHighLevelClient.msearch(request, DEFAULT);
+            MultiSearchResponse.Item[] responses = response.getResponses();
+            assertThat(responses, Matchers.arrayWithSize(1));
+            assertThat(responses[0].getFailure().getMessage(), containsString("no permissions for [indices:data/read/search]"));
         }
     }
 

diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
@@ -411,34 +411,6 @@ public PrivilegesEvaluatorResponse evaluate(
                         }
                     }
 
-                    if (dnfofEnabled && (action0.startsWith("indices:data/read/")) && !requestedResolved.getAllIndices().isEmpty()) {
-
-                        if (requestedResolved.getAllIndices().isEmpty()) {
-                            presponse.missingPrivileges.clear();
-                            presponse.allowed = true;
-                            return presponse;
-                        }
-
-                        Set<String> reduced = securityRoles.reduce(
-                            requestedResolved,
-                            user,
-                            new String[] { action0 },
-                            resolver,
-                            clusterService
-                        );
-
-                        if (reduced.isEmpty()) {
-                            presponse.allowed = false;
-                            return presponse;
-                        }
-
-                        if (irr.replace(request, true, reduced.toArray(new String[0]))) {
-                            presponse.missingPrivileges.clear();
-                            presponse.allowed = true;
-                            return presponse;
-                        }
-                    }
-
                     if (isDebugEnabled) {
                         log.debug("Allowed because we have cluster permissions for {}", action0);
                     }

diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java
@@ -659,7 +659,9 @@ public void testDnfof() throws Exception {
             + System.lineSeparator();
 
         resc = rh.executePostRequest("_msearch?pretty", msearchBody, encodeBasicHeader("user_b", "user_b"));
-        Assert.assertEquals(403, resc.getStatusCode());
+        Assert.assertEquals(resc.getBody(), 200, resc.getStatusCode());
+        Assert.assertEquals(resc.getBody(), "security_exception", resc.findValueInJson("responses[0].error.type"));
+        Assert.assertEquals(resc.getBody(), "security_exception", resc.findValueInJson("responses[1].error.type"));
 
         String mgetBody = "{"
             + "\"docs\" : ["
@@ -696,7 +698,9 @@ public void testDnfof() throws Exception {
             + "}";
 
         resc = rh.executePostRequest("_mget?pretty", mgetBody, encodeBasicHeader("user_b", "user_b"));
-        Assert.assertEquals(403, resc.getStatusCode());
+        Assert.assertEquals(resc.getBody(), 200, resc.getStatusCode());
+        Assert.assertEquals(resc.getBody(), "index_not_found_exception", resc.findValueInJson("docs[0].error.type"));
+        Assert.assertEquals(resc.getBody(), "index_not_found_exception", resc.findValueInJson("docs[1].error.type"));
 
         Assert.assertEquals(
             HttpStatus.SC_OK,