From fba55ac4107306fba7b1a8d6c4ce6ad279c22e43 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Mon, 24 Jun 2024 17:13:49 +0200 Subject: [PATCH] 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 #4485 Signed-off-by: Nils Bandener --- .../security/DoNotFailOnForbiddenTests.java | 35 ++++++++++++++++--- .../privileges/PrivilegesEvaluator.java | 28 --------------- .../opensearch/security/IntegrationTests.java | 8 +++-- 3 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java b/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java index 88c57c3321..456d1ebada 100644 --- 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 index 23a8022945..b2ae499879 100644 --- 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 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 index 0777594834..ebe1f799e9 100644 --- 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,