From bf5d09f918c70947fe9681e82cbfab1735911d45 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Mon, 24 Jun 2024 13:24:31 +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 | 22 ++++++++++- .../privileges/PrivilegesEvaluator.java | 28 ------------- .../resolver/IndexResolverReplacer.java | 39 ------------------- 3 files changed, 21 insertions(+), 68 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java b/src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java index 88c57c3321..7b0deab7f7 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,6 +296,26 @@ 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)) { 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/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java index 4a4e714348..69f5b9ab49 100644 --- a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java +++ b/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java @@ -33,7 +33,6 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; -import java.util.ListIterator; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -44,7 +43,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.opensearch.action.ActionRequest; import org.opensearch.action.DocWriteRequest; import org.opensearch.action.IndicesRequest; import org.opensearch.action.IndicesRequest.Replaceable; @@ -63,20 +61,15 @@ import org.opensearch.action.delete.DeleteRequest; import org.opensearch.action.fieldcaps.FieldCapabilitiesIndexRequest; import org.opensearch.action.fieldcaps.FieldCapabilitiesRequest; -import org.opensearch.action.get.MultiGetRequest; -import org.opensearch.action.get.MultiGetRequest.Item; import org.opensearch.action.index.IndexRequest; import org.opensearch.action.main.MainRequest; import org.opensearch.action.search.ClearScrollRequest; -import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchScrollRequest; import org.opensearch.action.support.IndicesOptions; import org.opensearch.action.support.nodes.BaseNodesRequest; import org.opensearch.action.support.replication.ReplicationRequest; import org.opensearch.action.support.single.shard.SingleShardRequest; -import org.opensearch.action.termvectors.MultiTermVectorsRequest; -import org.opensearch.action.termvectors.TermVectorsRequest; import org.opensearch.action.update.UpdateRequest; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.metadata.IndexAbstraction; @@ -635,32 +628,6 @@ private boolean getOrReplaceAllIndices(final Object request, final IndicesProvid result = getOrReplaceAllIndices(ar, provider, false) && result; } - } else if (request instanceof MultiGetRequest) { - - for (ListIterator it = ((MultiGetRequest) request).getItems().listIterator(); it.hasNext();) { - Item item = it.next(); - result = getOrReplaceAllIndices(item, provider, false) && result; - /*if(item.index() == null || item.indices() == null || item.indices().length == 0) { - it.remove(); - }*/ - } - - } else if (request instanceof MultiSearchRequest) { - - for (ListIterator it = ((MultiSearchRequest) request).requests().listIterator(); it.hasNext();) { - SearchRequest ar = it.next(); - result = getOrReplaceAllIndices(ar, provider, false) && result; - /*if(ar.indices() == null || ar.indices().length == 0) { - it.remove(); - }*/ - } - - } else if (request instanceof MultiTermVectorsRequest) { - - for (ActionRequest ar : (Iterable) () -> ((MultiTermVectorsRequest) request).iterator()) { - result = getOrReplaceAllIndices(ar, provider, false) && result; - } - } else if (request instanceof PutMappingRequest) { PutMappingRequest pmr = (PutMappingRequest) request; Index concreteIndex = pmr.getConcreteIndex(); @@ -766,12 +733,6 @@ private boolean getOrReplaceAllIndices(final Object request, final IndicesProvid return false; } ((ReplicationRequest) request).index(newIndices.length != 1 ? null : newIndices[0]); - } else if (request instanceof MultiGetRequest.Item) { - String[] newIndices = provider.provide(((MultiGetRequest.Item) request).indices(), request, true); - if (checkIndices(request, newIndices, true, allowEmptyIndices) == false) { - return false; - } - ((MultiGetRequest.Item) request).index(newIndices.length != 1 ? null : newIndices[0]); } else if (request instanceof CreateIndexRequest) { String[] newIndices = provider.provide(((CreateIndexRequest) request).indices(), request, true); if (checkIndices(request, newIndices, true, allowEmptyIndices) == false) {