Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Document GET with DLS terms query due to Lucene upgrade #3136

Merged
merged 3 commits into from
Aug 14, 2023
Merged

Fix Document GET with DLS terms query due to Lucene upgrade #3136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c7ad9c
Fix Document GET with DLS terms query
cwperks Aug 9, 2023
c233b07
Use getTextFromJsonBody
cwperks Aug 9, 2023
a6be927
Merge branch 'main' into fix-dls-term-query
cwperks Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/configuration/DlsFlsFilterLeafReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@
// 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);

Check warning on line 235 in src/main/java/org/opensearch/security/configuration/DlsFlsFilterLeafReader.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/configuration/DlsFlsFilterLeafReader.java#L235 

Added line #L235 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! What a terrible side effect. It doesn't seem clear how we can identify issues like this if we have them elsewhere other than having had a functional test case fail. How confident are you in this fix, and our coverage on these scenarios?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not caught with CI because there is no instance of a document GET request from a user mapped to a role with a DLS terms query associated with it. The test added in this PR would ensure that this would be caught at build time.

We could probably enhance this test suite by running different kinds of requests in addition to search requests and aggregations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you create a follow up task to analyze what test we should have / are missing?

Copy link
Collaborator

@willyborankin willyborankin Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it work for indices that were created prior to 2.9 or 2.10 with the shard version, e.g., 7.10.x?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peternied I filed an issue to asses the current state of FLS, DLS and Field Masking tests and determine what other scenarios should be tested: #3139

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @willyborankin , I plan to test this out today. Thank you for pointing out potential issues with backward compatibility depending on the version of Lucene the shards are created with.

Copy link
Member Author

@cwperks cwperks Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willyborankin I tested this by creating an index with ES 7, doing a full cluster upgrade to 2.8 w/ this change included and verified that it works.


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
View file
Open in desktop
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)
peternied marked this conversation as resolved.
Show resolved Hide resolved
).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()));
Comment on lines 51 to 52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope of this PR; but we can get rid of these sysouts to boost test performance.

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
17 changes: 17 additions & 0 deletions src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java
View file
Open in desktop
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: If the method getJsonNodeAt(jsonPointer) does not find the specified node, what is returned? Is it null? If it's a possible scenario, we might want to handle it or at least document the expected behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into this when testing to figure out the correct JSON path. It returns an empty string.

This method is used in assertion statements like this:

Assert.assertEquals(res.getTextFromJsonBody("/hits/total/value"), "1");

so the assertion will fail.

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
View file
Open in desktop
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
View file
Open in desktop
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
Loading