Skip to content

Commit fba55ac

Browse files
committed
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 <nils.bandener@eliatra.com>
1 parent faa8bf9 commit fba55ac

File tree

3 files changed

+36
-35
lines changed

3 files changed

+36
-35
lines changed

src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java

+30-5
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ public void shouldSearchForDocumentsViaAll_negative() throws IOException {
278278
@Test
279279
public void shouldMGetDocument_positive() throws IOException {
280280
try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
281-
MultiGetRequest request = new MultiGetRequest().add(BOTH_INDEX_PATTERN, ID_1).add(BOTH_INDEX_PATTERN, ID_4);
281+
MultiGetRequest request = new MultiGetRequest().add(MARVELOUS_SONGS, ID_1).add(MARVELOUS_SONGS, ID_4);
282282

283283
MultiGetResponse response = restHighLevelClient.mget(request, DEFAULT);
284284

@@ -296,12 +296,35 @@ public void shouldMGetDocument_positive() throws IOException {
296296
}
297297
}
298298

299+
@Test
300+
public void shouldMGetDocument_partial() throws Exception {
301+
try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
302+
MultiGetRequest request = new MultiGetRequest().add(MARVELOUS_SONGS, ID_1).add(HORRIBLE_SONGS, ID_4);
303+
304+
MultiGetResponse response = restHighLevelClient.mget(request, DEFAULT);
305+
306+
MultiGetItemResponse[] responses = response.getResponses();
307+
assertThat(responses, arrayWithSize(2));
308+
MultiGetItemResponse firstResult = responses[0];
309+
MultiGetItemResponse secondResult = responses[1];
310+
assertThat(firstResult.getFailure(), nullValue());
311+
assertThat(
312+
firstResult.getResponse(),
313+
allOf(containDocument(MARVELOUS_SONGS, ID_1), documentContainField(FIELD_TITLE, TITLE_MAGNUM_OPUS))
314+
);
315+
assertThat(secondResult.getFailure().getMessage(), containsString("no permissions for [indices:data/read/mget[shard]]"));
316+
}
317+
}
318+
299319
@Test
300320
public void shouldMGetDocument_negative() throws IOException {
301321
try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
302322
MultiGetRequest request = new MultiGetRequest().add(HORRIBLE_SONGS, ID_4);
303-
304-
assertThatThrownBy(() -> restHighLevelClient.mget(request, DEFAULT), statusException(FORBIDDEN));
323+
MultiGetResponse response = restHighLevelClient.mget(request, DEFAULT);
324+
MultiGetItemResponse[] responses = response.getResponses();
325+
assertThat(responses, arrayWithSize(1));
326+
MultiGetItemResponse firstResult = responses[0];
327+
assertThat(firstResult.getFailure().getMessage(), containsString("no permissions for [indices:data/read/mget[shard]]"));
305328
}
306329
}
307330

@@ -331,8 +354,10 @@ public void shouldMSearchDocument_negative() throws IOException {
331354
try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
332355
MultiSearchRequest request = new MultiSearchRequest();
333356
request.add(queryStringQueryRequest(FORBIDDEN_INDEX_ALIAS, QUERY_TITLE_POISON));
334-
335-
assertThatThrownBy(() -> restHighLevelClient.msearch(request, DEFAULT), statusException(FORBIDDEN));
357+
MultiSearchResponse response = restHighLevelClient.msearch(request, DEFAULT);
358+
MultiSearchResponse.Item[] responses = response.getResponses();
359+
assertThat(responses, Matchers.arrayWithSize(1));
360+
assertThat(responses[0].getFailure().getMessage(), containsString("no permissions for [indices:data/read/search]"));
336361
}
337362
}
338363

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

-28
Original file line numberDiff line numberDiff line change
@@ -411,34 +411,6 @@ public PrivilegesEvaluatorResponse evaluate(
411411
}
412412
}
413413

414-
if (dnfofEnabled && (action0.startsWith("indices:data/read/")) && !requestedResolved.getAllIndices().isEmpty()) {
415-
416-
if (requestedResolved.getAllIndices().isEmpty()) {
417-
presponse.missingPrivileges.clear();
418-
presponse.allowed = true;
419-
return presponse;
420-
}
421-
422-
Set<String> reduced = securityRoles.reduce(
423-
requestedResolved,
424-
user,
425-
new String[] { action0 },
426-
resolver,
427-
clusterService
428-
);
429-
430-
if (reduced.isEmpty()) {
431-
presponse.allowed = false;
432-
return presponse;
433-
}
434-
435-
if (irr.replace(request, true, reduced.toArray(new String[0]))) {
436-
presponse.missingPrivileges.clear();
437-
presponse.allowed = true;
438-
return presponse;
439-
}
440-
}
441-
442414
if (isDebugEnabled) {
443415
log.debug("Allowed because we have cluster permissions for {}", action0);
444416
}

src/test/java/org/opensearch/security/IntegrationTests.java

+6-2
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,9 @@ public void testDnfof() throws Exception {
659659
+ System.lineSeparator();
660660

661661
resc = rh.executePostRequest("_msearch?pretty", msearchBody, encodeBasicHeader("user_b", "user_b"));
662-
Assert.assertEquals(403, resc.getStatusCode());
662+
Assert.assertEquals(resc.getBody(), 200, resc.getStatusCode());
663+
Assert.assertEquals(resc.getBody(), "security_exception", resc.findValueInJson("responses[0].error.type"));
664+
Assert.assertEquals(resc.getBody(), "security_exception", resc.findValueInJson("responses[1].error.type"));
663665

664666
String mgetBody = "{"
665667
+ "\"docs\" : ["
@@ -696,7 +698,9 @@ public void testDnfof() throws Exception {
696698
+ "}";
697699

698700
resc = rh.executePostRequest("_mget?pretty", mgetBody, encodeBasicHeader("user_b", "user_b"));
699-
Assert.assertEquals(403, resc.getStatusCode());
701+
Assert.assertEquals(resc.getBody(), 200, resc.getStatusCode());
702+
Assert.assertEquals(resc.getBody(), "index_not_found_exception", resc.findValueInJson("docs[0].error.type"));
703+
Assert.assertEquals(resc.getBody(), "index_not_found_exception", resc.findValueInJson("docs[1].error.type"));
700704

701705
Assert.assertEquals(
702706
HttpStatus.SC_OK,

0 commit comments

Comments
 (0)