Skip to content

Commit

Permalink
Remove special handling for do_not_fail_on_forbidden on cluster actions
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@nibix
nibix committed Jun 24, 2024
1 parent 37afcaa commit bf5d09f
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 68 deletions.
22 changes: 21 additions & 1 deletion src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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)) {
Expand Down
28 changes: 0 additions & 28 deletions src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
39 changes: 0 additions & 39 deletions src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<Item> 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<SearchRequest> 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<TermVectorsRequest>) () -> ((MultiTermVectorsRequest) request).iterator()) {
result = getOrReplaceAllIndices(ar, provider, false) && result;
}

} else if (request instanceof PutMappingRequest) {
PutMappingRequest pmr = (PutMappingRequest) request;
Index concreteIndex = pmr.getConcreteIndex();
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit bf5d09f

Please sign in to comment.