Fix min_doc_count handling when using Document Level Security (#1714)

Signed-off-by: cliu123 <[email protected]>
opensearch-project · Mar 29, 2022 · 7ced417 · 7ced417
1 parent 51e492c
commit 7ced417
Show file tree

Hide file tree

Showing 4 changed files with 263 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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