Skip to content

Commit

Permalink
Fix Document GET with DLS terms query (opensearch-project#3136)
Browse files Browse the repository at this point in the history
### Description

Fixes an error on document retrieval for users that are mapped to roles
with DLS rules including a terms query. The bug was introduced by a
change in Lucene and reported on Slack
[here](https://opensearch.slack.com/archives/C0539F41Z5X/p1690714779140659).
This fix adds a test to catch an error like this at build time and
assert the intended behavior.

* Category (Enhancement, New feature, Bug fix, Test fix, Refactoring,
Maintenance, Documentation)

Bug fix

### Issues Resolved

opensearch-project#3088

### Check List
- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <[email protected]>
  • Loading branch information
cwperks authored Aug 14, 2023
1 parent 4593be7 commit 88b6d23
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ public DlsGetEvaluator(final Query dlsQuery, final LeafReader in, boolean applyD
// https://github.com/apache/lucene-solr/blob/branch_6_3/lucene/misc/src/java/org/apache/lucene/index/PKIndexSplitter.java
final IndexSearcher searcher = new IndexSearcher(DlsFlsFilterLeafReader.this);
searcher.setQueryCache(null);
final Weight preserveWeight = searcher.createWeight(dlsQuery, ScoreMode.COMPLETE_NO_SCORES, 1f);
final Weight preserveWeight = searcher.rewrite(dlsQuery).createWeight(searcher, ScoreMode.COMPLETE_NO_SCORES, 1f);

final int maxDoc = in.maxDoc();
final FixedBitSet bits = new FixedBitSet(maxDoc);
Expand Down
34 changes: 34 additions & 0 deletions src/test/java/org/opensearch/security/dlic/dlsfls/DlsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ protected void populateData(Client tc) {
new IndexRequest("deals").id("1").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("{\"amount\": 1500}", XContentType.JSON)
).actionGet();

tc.index(
new IndexRequest("terms").id("0").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("{\"foo\": \"bar\"}", XContentType.JSON)
).actionGet();
tc.index(
new IndexRequest("terms").id("1").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("{\"foo\": \"baz\"}", XContentType.JSON)
).actionGet();

try {
Thread.sleep(3000);
} catch (InterruptedException e) {
Expand All @@ -44,6 +51,7 @@ protected void populateData(Client tc) {
System.out.println("q");
System.out.println(Strings.toString(XContentType.JSON, tc.search(new SearchRequest().indices(".opendistro_security")).actionGet()));
tc.search(new SearchRequest().indices("deals")).actionGet();
tc.search(new SearchRequest().indices("terms")).actionGet();
}

@Test
Expand Down Expand Up @@ -251,6 +259,32 @@ public void testDls() throws Exception {

}

@Test
public void testDlsWithTermsQuery() throws Exception {

setup();

HttpResponse res;

Assert.assertEquals(
HttpStatus.SC_OK,
(res = rh.executeGetRequest("/terms/_search?pretty", encodeBasicHeader("dept_manager", "password"))).getStatusCode()
);
Assert.assertEquals(res.getTextFromJsonBody("/hits/total/value"), "1");
Assert.assertEquals(res.getTextFromJsonBody("/_shards/failed"), "0");

Assert.assertEquals(
HttpStatus.SC_OK,
(res = rh.executeGetRequest("/terms/_doc/0", encodeBasicHeader("dept_manager", "password"))).getStatusCode()
);
Assert.assertEquals(res.getTextFromJsonBody("/_source/foo"), "bar");

Assert.assertEquals(
HttpStatus.SC_NOT_FOUND,
rh.executeGetRequest("/terms/_doc/1", encodeBasicHeader("dept_manager", "password")).getStatusCode()
);
}

@Test
public void testNonDls() throws Exception {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLEngine;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import org.apache.commons.lang3.StringUtils;
import org.apache.hc.client5.http.async.methods.SimpleHttpRequest;
Expand Down Expand Up @@ -433,6 +434,22 @@ public boolean isJsonContentType() {
return ct.contains("application/json");
}

public String getTextFromJsonBody(String jsonPointer) {
return getJsonNodeAt(jsonPointer).asText();
}

private JsonNode getJsonNodeAt(String jsonPointer) {
try {
return toJsonNode().at(jsonPointer);
} catch (IOException e) {
throw new IllegalArgumentException("Cound not convert response body to JSON node ", e);
}
}

private JsonNode toJsonNode() throws JsonProcessingException, IOException {
return DefaultObjectMapper.objectMapper.readTree(getBody());
}

public SimpleHttpResponse getInner() {
return inner;
}
Expand Down
9 changes: 9 additions & 0 deletions src/test/resources/dlsfls/roles.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2482,3 +2482,12 @@ logs_index_with_dls:
masked_fields: null
allowed_actions:
- "OPENDISTRO_SECURITY_READ"

terms_index_with_dls:
index_permissions:
- index_patterns:
- "terms"
dls: "{ \"terms\": { \"foo\" : [\"bar\"] } }"
masked_fields: null
allowed_actions:
- "OPENDISTRO_SECURITY_READ"
4 changes: 4 additions & 0 deletions src/test/resources/dlsfls/roles_mapping.yml
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,7 @@ opendistro_security_mapped:
logs_index_with_dls:
users:
- dept_manager

terms_index_with_dls:
users:
- dept_manager

0 comments on commit 88b6d23

Please sign in to comment.