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 8aaf278
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 108 deletions.
35 changes: 30 additions & 5 deletions 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,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]]"));
}
}

Expand Down Expand Up @@ -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]"));
}
}

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
146 changes: 73 additions & 73 deletions src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ private AlreadyResolvedKey(final IndicesOptions indicesOptions, final boolean en
}

private AlreadyResolvedKey(
final IndicesOptions indicesOptions,
final boolean enableCrossClusterResolution,
final String[] original
final IndicesOptions indicesOptions,
final boolean enableCrossClusterResolution,
final String[] original
) {
this.indicesOptions = indicesOptions;
this.enableCrossClusterResolution = enableCrossClusterResolution;
Expand All @@ -191,8 +191,8 @@ public boolean equals(Object o) {
if (o == null || getClass() != o.getClass()) return false;
AlreadyResolvedKey that = (AlreadyResolvedKey) o;
return enableCrossClusterResolution == that.enableCrossClusterResolution
&& Objects.equals(indicesOptions, that.indicesOptions)
&& Arrays.equals(original, that.original);
&& Objects.equals(indicesOptions, that.indicesOptions)
&& Arrays.equals(original, that.original);
}

@Override
Expand All @@ -213,10 +213,10 @@ public int hashCode() {
}

private void resolveIndexPatterns(
final String name,
final IndicesOptions indicesOptions,
final boolean enableCrossClusterResolution,
final String[] original
final String name,
final IndicesOptions indicesOptions,
final boolean enableCrossClusterResolution,
final String[] original
) {
final boolean isTraceEnabled = log.isTraceEnabled();
if (isTraceEnabled) {
Expand All @@ -239,11 +239,11 @@ private void resolveIndexPatterns(
if (remoteClusterService.isCrossClusterSearchEnabled() && enableCrossClusterResolution) {
remoteIndices = new HashSet<>();
final Map<String, OriginalIndices> remoteClusterIndices = OpenSearchSecurityPlugin.GuiceHolder.getRemoteClusterService()
.groupIndices(indicesOptions, original, idx -> resolver.hasIndexAbstraction(idx, clusterService.state()));
.groupIndices(indicesOptions, original, idx -> resolver.hasIndexAbstraction(idx, clusterService.state()));
final Set<String> remoteClusters = remoteClusterIndices.keySet()
.stream()
.filter(k -> !RemoteClusterService.LOCAL_CLUSTER_GROUP_KEY.equals(k))
.collect(Collectors.toSet());
.stream()
.filter(k -> !RemoteClusterService.LOCAL_CLUSTER_GROUP_KEY.equals(k))
.collect(Collectors.toSet());
for (String remoteCluster : remoteClusters) {
for (String remoteIndex : remoteClusterIndices.get(remoteCluster).indices()) {
remoteIndices.add(RemoteClusterService.buildRemoteIndexName(remoteCluster, remoteIndex));
Expand All @@ -261,10 +261,10 @@ private void resolveIndexPatterns(

if (isTraceEnabled) {
log.trace(
"CCS is enabled, we found this local patterns "
+ localRequestedPatterns
+ " and this remote patterns: "
+ remoteIndices
"CCS is enabled, we found this local patterns "
+ localRequestedPatterns
+ " and this remote patterns: "
+ remoteIndices
);
}

Expand Down Expand Up @@ -294,31 +294,31 @@ private void resolveIndexPatterns(
else {
final ClusterState state = clusterService.state();
final Set<String> dateResolvedLocalRequestedPatterns = localRequestedPatterns.stream()
.map(resolver::resolveDateMathExpression)
.collect(Collectors.toSet());
.map(resolver::resolveDateMathExpression)
.collect(Collectors.toSet());
final WildcardMatcher dateResolvedMatcher = WildcardMatcher.from(dateResolvedLocalRequestedPatterns);
// fill matchingAliases
final Map<String, IndexAbstraction> lookup = state.metadata().getIndicesLookup();
matchingAliases = lookup.entrySet()
.stream()
.filter(e -> e.getValue().getType() == ALIAS)
.map(Map.Entry::getKey)
.filter(dateResolvedMatcher)
.collect(Collectors.toSet());
.stream()
.filter(e -> e.getValue().getType() == ALIAS)
.map(Map.Entry::getKey)
.filter(dateResolvedMatcher)
.collect(Collectors.toSet());

final boolean isDebugEnabled = log.isDebugEnabled();
try {
matchingAllIndices = Arrays.asList(
resolver.concreteIndexNames(state, indicesOptions, localRequestedPatterns.toArray(new String[0]))
resolver.concreteIndexNames(state, indicesOptions, localRequestedPatterns.toArray(new String[0]))
);
matchingDataStreams = resolver.dataStreamNames(state, indicesOptions, localRequestedPatterns.toArray(new String[0]));

if (isDebugEnabled) {
log.debug(
"Resolved pattern {} to indices: {} and data-streams: {}",
localRequestedPatterns,
matchingAllIndices,
matchingDataStreams
"Resolved pattern {} to indices: {} and data-streams: {}",
localRequestedPatterns,
matchingAllIndices,
matchingDataStreams
);
}
} catch (IndexNotFoundException e1) {
Expand All @@ -336,15 +336,15 @@ private void resolveIndexPatterns(

if (isTraceEnabled) {
log.trace(
"Resolved patterns {} for {} ({}) to [aliases {}, allIndices {}, dataStreams {}, originalRequested{}, remote indices {}]",
original,
name,
this.name,
matchingAliases,
matchingAllIndices,
matchingDataStreams,
Arrays.toString(original),
remoteIndices
"Resolved patterns {} for {} ({}) to [aliases {}, allIndices {}, dataStreams {}, originalRequested{}, remote indices {}]",
original,
name,
this.name,
matchingAliases,
matchingAllIndices,
matchingDataStreams,
Arrays.toString(original),
remoteIndices
);
}

Expand All @@ -358,11 +358,11 @@ private void resolveToLocalAll() {
}

private void resolveTo(
Iterable<String> matchingAliases,
Iterable<String> matchingAllIndices,
Iterable<String> matchingDataStreams,
String[] original,
Iterable<String> remoteIndices
Iterable<String> matchingAliases,
Iterable<String> matchingAllIndices,
Iterable<String> matchingDataStreams,
String[] original,
Iterable<String> remoteIndices
) {
aliases.addAll(matchingAliases);
allIndices.addAll(matchingAllIndices);
Expand All @@ -375,8 +375,8 @@ private void resolveTo(
public String[] provide(String[] original, Object localRequest, boolean supportsReplace) {
final IndicesOptions indicesOptions = indicesOptionsFrom(localRequest);
final boolean enableCrossClusterResolution = localRequest instanceof FieldCapabilitiesRequest
|| localRequest instanceof SearchRequest
|| localRequest instanceof ResolveIndexAction.Request;
|| localRequest instanceof SearchRequest
|| localRequest instanceof ResolveIndexAction.Request;
// skip the whole thing if we have seen this exact resolveIndexPatterns request
final AlreadyResolvedKey alreadyResolvedKey;
if (original != null) {
Expand All @@ -392,8 +392,8 @@ public String[] provide(String[] original, Object localRequest, boolean supports

Resolved resolved(IndicesOptions indicesOptions) {
final Resolved resolved = alreadyResolved.isEmpty()
? Resolved._LOCAL_ALL
: new Resolved(aliases.build(), allIndices.build(), originalRequested.build(), remoteIndices.build(), indicesOptions);
? Resolved._LOCAL_ALL
: new Resolved(aliases.build(), allIndices.build(), originalRequested.build(), remoteIndices.build(), indicesOptions);

if (log.isTraceEnabled()) {
log.trace("Finally resolved for {}: {}", name, resolved);
Expand All @@ -413,7 +413,7 @@ public String[] provide(String[] original, Object request, boolean supportsRepla
if (retainMode && !isAllWithNoRemote(original)) {
final Resolved resolved = resolveRequest(request);
final List<String> retained = WildcardMatcher.from(resolved.getAllIndices())
.getMatchAny(replacements, Collectors.toList());
.getMatchAny(replacements, Collectors.toList());
retained.addAll(resolved.getRemoteIndices());
return retained.toArray(new String[0]);
}
Expand Down Expand Up @@ -442,11 +442,11 @@ public final static class Resolved {
private static final ImmutableSet<String> All_SET = ImmutableSet.of(ANY);
private static final Set<String> types = All_SET;
public static final Resolved _LOCAL_ALL = new Resolved(
All_SET,
All_SET,
All_SET,
ImmutableSet.of(),
SearchRequest.DEFAULT_INDICES_OPTIONS
All_SET,
All_SET,
All_SET,
ImmutableSet.of(),
SearchRequest.DEFAULT_INDICES_OPTIONS
);

private final Set<String> aliases;
Expand All @@ -457,18 +457,18 @@ public final static class Resolved {
private final IndicesOptions indicesOptions;

public Resolved(
final ImmutableSet<String> aliases,
final ImmutableSet<String> allIndices,
final ImmutableSet<String> originalRequested,
final ImmutableSet<String> remoteIndices,
IndicesOptions indicesOptions
final ImmutableSet<String> aliases,
final ImmutableSet<String> allIndices,
final ImmutableSet<String> originalRequested,
final ImmutableSet<String> remoteIndices,
IndicesOptions indicesOptions
) {
this.aliases = aliases;
this.allIndices = allIndices;
this.originalRequested = originalRequested;
this.remoteIndices = remoteIndices;
this.isLocalAll = IndexResolverReplacer.isLocalAll(originalRequested.toArray(new String[0]))
|| (aliases.contains("*") && allIndices.contains("*"));
|| (aliases.contains("*") && allIndices.contains("*"));
this.indicesOptions = indicesOptions;
}

Expand Down Expand Up @@ -507,16 +507,16 @@ public Set<String> getRemoteIndices() {
@Override
public String toString() {
return "Resolved [aliases="
+ aliases
+ ", allIndices="
+ allIndices
+ ", types="
+ types
+ ", originalRequested="
+ originalRequested
+ ", remoteIndices="
+ remoteIndices
+ "]";
+ aliases
+ ", allIndices="
+ allIndices
+ ", types="
+ types
+ ", originalRequested="
+ originalRequested
+ ", remoteIndices="
+ remoteIndices
+ "]";
}

@Override
Expand Down Expand Up @@ -690,14 +690,14 @@ private boolean getOrReplaceAllIndices(final Object request, final IndicesProvid

if (snapshotInfo == null) {
log.warn(
"snapshot repository '" + restoreRequest.repository() + "', snapshot '" + restoreRequest.snapshot() + "' not found"
"snapshot repository '" + restoreRequest.repository() + "', snapshot '" + restoreRequest.snapshot() + "' not found"
);
provider.provide(new String[] { "*" }, request, false);
} else {
final List<String> requestedResolvedIndices = IndexUtils.filterIndices(
snapshotInfo.indices(),
restoreRequest.indices(),
restoreRequest.indicesOptions()
snapshotInfo.indices(),
restoreRequest.indices(),
restoreRequest.indicesOptions()
);
final List<String> renamedTargetIndices = renamedIndices(restoreRequest, requestedResolvedIndices);
// final Set<String> indices = new HashSet<>(requestedResolvedIndices);
Expand Down
8 changes: 6 additions & 2 deletions src/test/java/org/opensearch/security/IntegrationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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\" : ["
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 8aaf278

Please sign in to comment.