From 7ced41738e2f4ece2669491c0d30ed8bbf2a968d Mon Sep 17 00:00:00 2001 From: Chang Liu Date: Tue, 29 Mar 2022 15:04:32 -0700 Subject: [PATCH] Fix min_doc_count handling when using Document Level Security (#1714) Signed-off-by: cliu123 --- .../configuration/DlsFlsValveImpl.java | 11 + .../security/dlic/dlsfls/DlsTest.java | 239 ++++++++++++++++++ src/test/resources/dlsfls/roles.yml | 9 + src/test/resources/dlsfls/roles_mapping.yml | 4 + 4 files changed, 263 insertions(+) diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index f0ba03c9e1..e2c63d1c2d 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -28,6 +28,7 @@ import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.ConstantScoreQuery; import org.apache.lucene.util.BytesRef; +import org.opensearch.OpenSearchException; import org.opensearch.OpenSearchSecurityException; import org.opensearch.SpecialPermission; import org.opensearch.action.ActionListener; @@ -52,6 +53,7 @@ import org.opensearch.rest.RestStatus; import org.opensearch.search.DocValueFormat; import org.opensearch.search.aggregations.AggregationBuilder; +import org.opensearch.search.aggregations.AggregatorFactories; import org.opensearch.search.aggregations.BucketOrder; import org.opensearch.search.aggregations.InternalAggregation; import org.opensearch.search.aggregations.InternalAggregations; @@ -266,6 +268,15 @@ public boolean invoke(String action, ActionRequest request, final ActionListener final SearchSourceBuilder source = ((SearchRequest) request).source(); if (source != null) { + AggregatorFactories.Builder aggregations = source.aggregations(); + if (aggregations != null) { + for (AggregationBuilder factory : aggregations.getAggregatorFactories()) { + if (factory instanceof TermsAggregationBuilder && ((TermsAggregationBuilder) factory).minDocCount() == 0) { + listener.onFailure(new OpenSearchException("min_doc_count 0 is not supported when DLS is activated")); + return false; + } + } + } if (source.profile()) { listener.onFailure(new OpenSearchSecurityException("Profiling is not supported when DLS is activated")); diff --git a/src/test/java/org/opensearch/security/dlic/dlsfls/DlsTest.java b/src/test/java/org/opensearch/security/dlic/dlsfls/DlsTest.java index 2a1a5b5ad2..aa201dcbf8 100644 --- a/src/test/java/org/opensearch/security/dlic/dlsfls/DlsTest.java +++ b/src/test/java/org/opensearch/security/dlic/dlsfls/DlsTest.java @@ -15,7 +15,9 @@ package org.opensearch.security.dlic.dlsfls; +import com.google.common.collect.ImmutableMap; import org.apache.http.HttpStatus; +import org.opensearch.action.admin.indices.create.CreateIndexRequest; import org.opensearch.action.index.IndexRequest; import org.opensearch.action.search.SearchRequest; import org.opensearch.action.support.WriteRequest.RefreshPolicy; @@ -267,4 +269,241 @@ public void testDlsCache() throws Exception { res = rh.executeGetRequest("/deals/deals/0?pretty", encodeBasicHeader("dept_manager", "password")); Assert.assertTrue(res.getBody().contains("\"found\" : false")); } + + @Test + public void testDlsWithMinDocCountZeroAggregations() throws Exception { + setup(); + + try (Client client = getClient()) { + client.admin().indices().create(new CreateIndexRequest("logs").mapping("_doc", ImmutableMap.of("properties", ImmutableMap.of("termX", ImmutableMap.of("type", "keyword"))))).actionGet(); + + for (int i = 0; i < 3; i++) { + client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", i, "termX", "A", "timestamp", "2022-01-06T09:05:00Z")).actionGet(); + client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", i, "termX", "B", "timestamp", "2022-01-06T09:08:00Z")).actionGet(); + client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", i, "termX", "C", "timestamp", "2022-01-06T09:09:00Z")).actionGet(); + client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", i, "termX", "D", "timestamp", "2022-01-06T09:10:00Z")).actionGet(); + } + client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", 0, "termX", "E", "timestamp", "2022-01-06T09:11:00Z")).actionGet(); + } + // Terms Aggregation + // Non-admin user with setting "min_doc_count":0. Expected to get error message "min_doc_count 0 is not supported when DLS is activated". + String query1 = "{\n" + + " \"size\":0,\n" + + " \"query\":{\n" + + " \"bool\":{\n" + + " \"must\":[\n" + + " {\n" + + " \"range\":{\n" + + " \"amount\":{\"gte\":1,\"lte\":100}\n" + + " }\n" + + " }\n" + + " ]\n" + + " }\n" + + " },\n" + + " \"aggs\":{\n" + + " \"a\": {\n" + + " \"terms\": {\n" + + " \"field\": \"termX\",\n" + + " \"min_doc_count\":0,\n" + + "\"size\": 10,\n" + + "\"order\": { \"_count\": \"desc\" }\n" + + " }\n" + + " }\n" + + " }\n" + + "}"; + + HttpResponse response1 = rh.executePostRequest("logs*/_search", query1, encodeBasicHeader("dept_manager", "password")); + + Assert.assertEquals(HttpStatus.SC_INTERNAL_SERVER_ERROR, response1.getStatusCode()); + Assert.assertTrue(response1.getBody(), response1.getBody().contains("min_doc_count 0 is not supported when DLS is activated")); + + // Non-admin user without setting "min_doc_count". Expected to only have access to buckets for dept_manager excluding E with 0 doc_count". + String query2 = "{\n" + + " \"size\":0,\n" + + " \"query\":{\n" + + " \"bool\":{\n" + + " \"must\":[\n" + + " {\n" + + " \"range\":{\n" + + " \"amount\":{\"gte\":1,\"lte\":100}\n" + + " }\n" + + " }\n" + + " ]\n" + + " }\n" + + " },\n" + + " \"aggs\":{\n" + + " \"a\": {\n" + + " \"terms\": {\n" + + " \"field\": \"termX\",\n" + + "\"size\": 10,\n" + + "\"order\": { \"_count\": \"desc\" }\n" + + " }\n" + + " }\n" + + " }\n" + + "}"; + + HttpResponse response2 = rh.executePostRequest("logs*/_search", query2, encodeBasicHeader("dept_manager", "password")); + + Assert.assertEquals(HttpStatus.SC_OK, response2.getStatusCode()); + Assert.assertTrue(response2.getBody(), response2.getBody().contains("\"key\":\"A\"")); + Assert.assertFalse(response2.getBody(), response2.getBody().contains("\"key\":\"B\"")); + Assert.assertFalse(response2.getBody(), response2.getBody().contains("\"key\":\"C\"")); + Assert.assertFalse(response2.getBody(), response2.getBody().contains("\"key\":\"D\"")); + Assert.assertFalse(response2.getBody(), response2.getBody().contains("\"key\":\"E\"")); + + // Admin with setting "min_doc_count":0. Expected to have access to all buckets". + HttpResponse response3 = rh.executePostRequest("logs*/_search", query1, encodeBasicHeader("admin", "admin")); + + Assert.assertEquals(HttpStatus.SC_OK, response3.getStatusCode()); + Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"A\"")); + Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"B\"")); + Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"C\"")); + Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"D\"")); + Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"E\",\"doc_count\":0")); + + // Admin without setting "min_doc_count". Expected to have access to all buckets excluding E with 0 doc_count". + HttpResponse response4 = rh.executePostRequest("logs*/_search", query2, encodeBasicHeader("admin", "admin")); + + Assert.assertEquals(HttpStatus.SC_OK, response4.getStatusCode()); + Assert.assertTrue(response4.getBody(), response4.getBody().contains("\"key\":\"A\"")); + Assert.assertTrue(response4.getBody(), response4.getBody().contains("\"key\":\"B\"")); + Assert.assertTrue(response4.getBody(), response4.getBody().contains("\"key\":\"C\"")); + Assert.assertTrue(response4.getBody(), response4.getBody().contains("\"key\":\"D\"")); + Assert.assertFalse(response4.getBody(), response4.getBody().contains("\"key\":\"E\"")); + + // Significant Text Aggregation is not impacted. + // Non-admin user with setting "min_doc_count=0". Expected to only have access to buckets for dept_manager". + String query3 = "{\"aggregations\":{\"significant_termX\":{\"significant_terms\":{\"field\":\"termX.keyword\",\"min_doc_count\":0}}}}"; + HttpResponse response5 = rh.executePostRequest("logs*/_search", query3, encodeBasicHeader("dept_manager", "password")); + + Assert.assertEquals(HttpStatus.SC_OK, response5.getStatusCode()); + Assert.assertTrue(response5.getBody(), response5.getBody().contains("\"termX\":\"A\"")); + Assert.assertFalse(response5.getBody(), response5.getBody().contains("\"termX\":\"B\"")); + Assert.assertFalse(response5.getBody(), response5.getBody().contains("\"termX\":\"C\"")); + Assert.assertFalse(response5.getBody(), response5.getBody().contains("\"termX\":\"D\"")); + Assert.assertFalse(response5.getBody(), response5.getBody().contains("\"termX\":\"E\"")); + + // Non-admin user without setting "min_doc_count". Expected to only have access to buckets for dept_manager". + String query4 = "{\"aggregations\":{\"significant_termX\":{\"significant_terms\":{\"field\":\"termX.keyword\"}}}}"; + + HttpResponse response6 = rh.executePostRequest("logs*/_search", query4, encodeBasicHeader("dept_manager", "password")); + + Assert.assertEquals(HttpStatus.SC_OK, response6.getStatusCode()); + Assert.assertTrue(response6.getBody(), response6.getBody().contains("\"termX\":\"A\"")); + Assert.assertFalse(response6.getBody(), response6.getBody().contains("\"termX\":\"B\"")); + Assert.assertFalse(response6.getBody(), response6.getBody().contains("\"termX\":\"C\"")); + Assert.assertFalse(response6.getBody(), response6.getBody().contains("\"termX\":\"D\"")); + Assert.assertFalse(response6.getBody(), response6.getBody().contains("\"termX\":\"E\"")); + + // Admin with setting "min_doc_count":0. Expected to have access to all buckets". + HttpResponse response7 = rh.executePostRequest("logs*/_search", query3, encodeBasicHeader("admin", "admin")); + + Assert.assertEquals(HttpStatus.SC_OK, response7.getStatusCode()); + Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"A\"")); + Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"B\"")); + Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"C\"")); + Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"D\"")); + Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"E\"")); + + // Admin without setting "min_doc_count". Expected to have access to all buckets". + HttpResponse response8 = rh.executePostRequest("logs*/_search", query4, encodeBasicHeader("admin", "admin")); + + Assert.assertEquals(HttpStatus.SC_OK, response8.getStatusCode()); + Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"A\"")); + Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"B\"")); + Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"C\"")); + Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"D\"")); + Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"E\"")); + + // Histogram Aggregation is not impacted. + // Non-admin user with setting "min_doc_count=0". Expected to only have access to buckets for dept_manager". + String query5 = "{\"aggs\":{\"amount\":{\"histogram\":{\"field\":\"amount\",\"interval\":1,\"min_doc_count\":0}}}}"; + + HttpResponse response9 = rh.executePostRequest("logs*/_search", query5, encodeBasicHeader("dept_manager", "password")); + + Assert.assertEquals(HttpStatus.SC_OK, response9.getStatusCode()); + Assert.assertTrue(response9.getBody(), response9.getBody().contains("\"termX\":\"A\"")); + Assert.assertFalse(response9.getBody(), response9.getBody().contains("\"termX\":\"B\"")); + Assert.assertFalse(response9.getBody(), response9.getBody().contains("\"termX\":\"C\"")); + Assert.assertFalse(response9.getBody(), response9.getBody().contains("\"termX\":\"D\"")); + Assert.assertFalse(response9.getBody(), response9.getBody().contains("\"termX\":\"E\"")); + + // Non-admin user without setting "min_doc_count". Expected to only have access to buckets for dept_manager". + String query6 = "{\"aggs\":{\"amount\":{\"histogram\":{\"field\":\"amount\",\"interval\":1}}}}"; + + HttpResponse response10 = rh.executePostRequest("logs*/_search", query6, encodeBasicHeader("dept_manager", "password")); + + Assert.assertEquals(HttpStatus.SC_OK, response10.getStatusCode()); + Assert.assertTrue(response10.getBody(), response10.getBody().contains("\"termX\":\"A\"")); + Assert.assertFalse(response10.getBody(), response10.getBody().contains("\"termX\":\"B\"")); + Assert.assertFalse(response10.getBody(), response10.getBody().contains("\"termX\":\"C\"")); + Assert.assertFalse(response10.getBody(), response10.getBody().contains("\"termX\":\"D\"")); + Assert.assertFalse(response10.getBody(), response10.getBody().contains("\"termX\":\"E\"")); + + // Admin with setting "min_doc_count":0. Expected to have access to all buckets". + HttpResponse response11 = rh.executePostRequest("logs*/_search", query5, encodeBasicHeader("admin", "admin")); + + Assert.assertEquals(HttpStatus.SC_OK, response11.getStatusCode()); + Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"A\"")); + Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"B\"")); + Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"C\"")); + Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"D\"")); + Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"E\"")); + + + // Admin without setting "min_doc_count". Expected to have access to all buckets". + HttpResponse response12 = rh.executePostRequest("logs*/_search", query6, encodeBasicHeader("admin", "admin")); + + Assert.assertEquals(HttpStatus.SC_OK, response12.getStatusCode()); + Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"A\"")); + Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"B\"")); + Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"C\"")); + Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"D\"")); + Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"E\"")); + + // Date Histogram Aggregation is not impacted. + // Non-admin user with setting "min_doc_count=0". Expected to only have access to buckets for dept_manager". + String query7 = "{\"aggs\":{\"timestamp\":{\"date_histogram\":{\"field\":\"timestamp\",\"calendar_interval\":\"month\",\"min_doc_count\":0}}}}"; + + HttpResponse response13 = rh.executePostRequest("logs*/_search", query7, encodeBasicHeader("dept_manager", "password")); + + Assert.assertEquals(HttpStatus.SC_OK, response13.getStatusCode()); + Assert.assertTrue(response13.getBody(), response13.getBody().contains("\"termX\":\"A\"")); + Assert.assertFalse(response13.getBody(), response13.getBody().contains("\"termX\":\"B\"")); + Assert.assertFalse(response13.getBody(), response13.getBody().contains("\"termX\":\"C\"")); + Assert.assertFalse(response13.getBody(), response13.getBody().contains("\"termX\":\"D\"")); + Assert.assertFalse(response13.getBody(), response13.getBody().contains("\"termX\":\"E\"")); + + // Non-admin user without setting "min_doc_count". Expected to only have access to buckets for dept_manager". + String query8 = "{\"aggs\":{\"timestamp\":{\"date_histogram\":{\"field\":\"timestamp\",\"calendar_interval\":\"month\"}}}}"; + + HttpResponse response14 = rh.executePostRequest("logs*/_search", query8, encodeBasicHeader("dept_manager", "password")); + + Assert.assertEquals(HttpStatus.SC_OK, response14.getStatusCode()); + Assert.assertTrue(response14.getBody(), response14.getBody().contains("\"termX\":\"A\"")); + Assert.assertFalse(response14.getBody(), response14.getBody().contains("\"termX\":\"B\"")); + Assert.assertFalse(response14.getBody(), response14.getBody().contains("\"termX\":\"C\"")); + Assert.assertFalse(response14.getBody(), response14.getBody().contains("\"termX\":\"D\"")); + Assert.assertFalse(response14.getBody(), response14.getBody().contains("\"termX\":\"E\"")); + + // Admin with setting "min_doc_count":0. Expected to have access to all buckets". + HttpResponse response15 = rh.executePostRequest("logs*/_search", query7, encodeBasicHeader("admin", "admin")); + + Assert.assertEquals(HttpStatus.SC_OK, response15.getStatusCode()); + Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"A\"")); + Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"B\"")); + Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"C\"")); + Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"D\"")); + Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"E\"")); + + // Admin without setting "min_doc_count". Expected to have access to all buckets". + HttpResponse response16 = rh.executePostRequest("logs*/_search", query8, encodeBasicHeader("admin", "admin")); + + Assert.assertEquals(HttpStatus.SC_OK, response16.getStatusCode()); + Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"A\"")); + Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"B\"")); + Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"C\"")); + Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"D\"")); + Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"E\"")); + } } diff --git a/src/test/resources/dlsfls/roles.yml b/src/test/resources/dlsfls/roles.yml index df93cb2318..c692f73ceb 100644 --- a/src/test/resources/dlsfls/roles.yml +++ b/src/test/resources/dlsfls/roles.yml @@ -2473,3 +2473,12 @@ opendistro_security_combined: allowed_actions: - "OPENDISTRO_SECURITY_READ" tenant_permissions: [] + +logs_index_with_dls: + index_permissions: + - index_patterns: + - "logs" + dls: "{\"bool\": {\"filter\": {\"term\" : {\"termX\" : \"A\" }}}}" + masked_fields: null + allowed_actions: + - "OPENDISTRO_SECURITY_READ" diff --git a/src/test/resources/dlsfls/roles_mapping.yml b/src/test/resources/dlsfls/roles_mapping.yml index 092770014c..27cf71c3bb 100644 --- a/src/test/resources/dlsfls/roles_mapping.yml +++ b/src/test/resources/dlsfls/roles_mapping.yml @@ -243,3 +243,7 @@ opendistro_security_mapped: users: [] and_backend_roles: [] description: "Security role mapping to backend role prolemapped" + +logs_index_with_dls: + users: + - dept_manager