Skip to content

Commit c18a50a

Browse files
cliu123cliu123
authored
Fix min_doc_count handling when using Document Level Security (#1712)
Signed-off-by: cliu123 <lc12251109@gmail.com>
1 parent 09b3b54 commit c18a50a

File tree

4 files changed

+268
-3
lines changed

4 files changed

+268
-3
lines changed

‎src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,14 @@
1515

1616
package org.opensearch.security.configuration;
1717

18+
import org.opensearch.OpenSearchException;
1819
import org.opensearch.rest.RestStatus;
20+
import org.opensearch.search.aggregations.AggregationBuilder;
21+
import org.opensearch.search.aggregations.AggregatorFactories;
22+
import org.opensearch.search.aggregations.BucketOrder;
23+
import org.opensearch.search.aggregations.InternalAggregation;
24+
import org.opensearch.search.aggregations.InternalAggregations;
25+
import org.opensearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
1926
import org.opensearch.security.support.SecurityUtils;
2027
import org.slf4j.LoggerFactory;
2128
import org.slf4j.Logger;
@@ -47,9 +54,6 @@
4754
import org.opensearch.common.xcontent.NamedXContentRegistry;
4855
import org.opensearch.index.query.ParsedQuery;
4956
import org.opensearch.search.DocValueFormat;
50-
import org.opensearch.search.aggregations.BucketOrder;
51-
import org.opensearch.search.aggregations.InternalAggregation;
52-
import org.opensearch.search.aggregations.InternalAggregations;
5357
import org.opensearch.search.aggregations.bucket.MultiBucketsAggregation.Bucket;
5458
import org.opensearch.search.aggregations.bucket.terms.InternalTerms;
5559
import org.opensearch.search.aggregations.bucket.terms.StringTerms;
@@ -129,6 +133,15 @@ public boolean invoke(final String action, final ActionRequest request, final Ac
129133
if(request instanceof SearchRequest) {
130134
final SearchSourceBuilder source = ((SearchRequest)request).source();
131135
if(source != null) {
136+
AggregatorFactories.Builder aggregations = source.aggregations();
137+
if (aggregations != null) {
138+
for (AggregationBuilder factory : aggregations.getAggregatorFactories()) {
139+
if (factory instanceof TermsAggregationBuilder && ((TermsAggregationBuilder) factory).minDocCount() == 0) {
140+
listener.onFailure(new OpenSearchException("min_doc_count 0 is not supported when DLS is activated"));
141+
return false;
142+
}
143+
}
144+
}
132145

133146
if(source.profile()) {
134147
listener.onFailure(new OpenSearchSecurityException("Profiling is not supported when DLS is activated"));

‎src/test/java/org/opensearch/security/dlic/dlsfls/DlsTest.java‎

Lines changed: 241 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,16 @@
1515

1616
package org.opensearch.security.dlic.dlsfls;
1717

18+
import com.google.common.collect.ImmutableMap;
1819
import org.apache.http.HttpStatus;
20+
import org.opensearch.action.admin.indices.create.CreateIndexRequest;
1921
import org.opensearch.action.index.IndexRequest;
2022
import org.opensearch.action.search.SearchRequest;
2123
import org.opensearch.action.support.WriteRequest.RefreshPolicy;
24+
import org.opensearch.client.Client;
2225
import org.opensearch.client.transport.TransportClient;
2326
import org.opensearch.common.Strings;
27+
import org.opensearch.common.settings.Settings;
2428
import org.opensearch.common.xcontent.XContentType;
2529
import org.junit.Assert;
2630
import org.junit.Test;
@@ -267,4 +271,241 @@ public void testDlsCache() throws Exception {
267271
res = rh.executeGetRequest("/deals/deals/0?pretty", encodeBasicHeader("dept_manager", "password"));
268272
Assert.assertTrue(res.getBody().contains("\"found\" : false"));
269273
}
274+
275+
@Test
276+
public void testDlsWithMinDocCountZeroAggregations() throws Exception {
277+
setup();
278+
279+
try (Client client = getInternalTransportClient(this.clusterInfo, Settings.EMPTY)) {
280+
client.admin().indices().create(new CreateIndexRequest("logs").mapping("_doc", ImmutableMap.of("properties", ImmutableMap.of("termX", ImmutableMap.of("type", "keyword"))))).actionGet();
281+
282+
for (int i = 0; i < 3; i++) {
283+
client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", i, "termX", "A", "timestamp", "2022-01-06T09:05:00Z")).actionGet();
284+
client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", i, "termX", "B", "timestamp", "2022-01-06T09:08:00Z")).actionGet();
285+
client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", i, "termX", "C", "timestamp", "2022-01-06T09:09:00Z")).actionGet();
286+
client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", i, "termX", "D", "timestamp", "2022-01-06T09:10:00Z")).actionGet();
287+
}
288+
client.index(new IndexRequest("logs").setRefreshPolicy(RefreshPolicy.IMMEDIATE).source("amount", 0, "termX", "E", "timestamp", "2022-01-06T09:11:00Z")).actionGet();
289+
}
290+
// Terms Aggregation
291+
// 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".
292+
String query1 = "{\n"
293+
+ " \"size\":0,\n"
294+
+ " \"query\":{\n"
295+
+ " \"bool\":{\n"
296+
+ " \"must\":[\n"
297+
+ " {\n"
298+
+ " \"range\":{\n"
299+
+ " \"amount\":{\"gte\":1,\"lte\":100}\n"
300+
+ " }\n"
301+
+ " }\n"
302+
+ " ]\n"
303+
+ " }\n"
304+
+ " },\n"
305+
+ " \"aggs\":{\n"
306+
+ " \"a\": {\n"
307+
+ " \"terms\": {\n"
308+
+ " \"field\": \"termX\",\n"
309+
+ " \"min_doc_count\":0,\n"
310+
+ "\"size\": 10,\n"
311+
+ "\"order\": { \"_count\": \"desc\" }\n"
312+
+ " }\n"
313+
+ " }\n"
314+
+ " }\n"
315+
+ "}";
316+
317+
HttpResponse response1 = rh.executePostRequest("logs*/_search", query1, encodeBasicHeader("dept_manager", "password"));
318+
319+
Assert.assertEquals(HttpStatus.SC_INTERNAL_SERVER_ERROR, response1.getStatusCode());
320+
Assert.assertTrue(response1.getBody(), response1.getBody().contains("min_doc_count 0 is not supported when DLS is activated"));
321+
322+
// Non-admin user without setting "min_doc_count". Expected to only have access to buckets for dept_manager excluding E with 0 doc_count".
323+
String query2 = "{\n"
324+
+ " \"size\":0,\n"
325+
+ " \"query\":{\n"
326+
+ " \"bool\":{\n"
327+
+ " \"must\":[\n"
328+
+ " {\n"
329+
+ " \"range\":{\n"
330+
+ " \"amount\":{\"gte\":1,\"lte\":100}\n"
331+
+ " }\n"
332+
+ " }\n"
333+
+ " ]\n"
334+
+ " }\n"
335+
+ " },\n"
336+
+ " \"aggs\":{\n"
337+
+ " \"a\": {\n"
338+
+ " \"terms\": {\n"
339+
+ " \"field\": \"termX\",\n"
340+
+ "\"size\": 10,\n"
341+
+ "\"order\": { \"_count\": \"desc\" }\n"
342+
+ " }\n"
343+
+ " }\n"
344+
+ " }\n"
345+
+ "}";
346+
347+
HttpResponse response2 = rh.executePostRequest("logs*/_search", query2, encodeBasicHeader("dept_manager", "password"));
348+
349+
Assert.assertEquals(HttpStatus.SC_OK, response2.getStatusCode());
350+
Assert.assertTrue(response2.getBody(), response2.getBody().contains("\"key\":\"A\""));
351+
Assert.assertFalse(response2.getBody(), response2.getBody().contains("\"key\":\"B\""));
352+
Assert.assertFalse(response2.getBody(), response2.getBody().contains("\"key\":\"C\""));
353+
Assert.assertFalse(response2.getBody(), response2.getBody().contains("\"key\":\"D\""));
354+
Assert.assertFalse(response2.getBody(), response2.getBody().contains("\"key\":\"E\""));
355+
356+
// Admin with setting "min_doc_count":0. Expected to have access to all buckets".
357+
HttpResponse response3 = rh.executePostRequest("logs*/_search", query1, encodeBasicHeader("admin", "admin"));
358+
359+
Assert.assertEquals(HttpStatus.SC_OK, response3.getStatusCode());
360+
Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"A\""));
361+
Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"B\""));
362+
Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"C\""));
363+
Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"D\""));
364+
Assert.assertTrue(response3.getBody(), response3.getBody().contains("\"key\":\"E\",\"doc_count\":0"));
365+
366+
// Admin without setting "min_doc_count". Expected to have access to all buckets excluding E with 0 doc_count".
367+
HttpResponse response4 = rh.executePostRequest("logs*/_search", query2, encodeBasicHeader("admin", "admin"));
368+
369+
Assert.assertEquals(HttpStatus.SC_OK, response4.getStatusCode());
370+
Assert.assertTrue(response4.getBody(), response4.getBody().contains("\"key\":\"A\""));
371+
Assert.assertTrue(response4.getBody(), response4.getBody().contains("\"key\":\"B\""));
372+
Assert.assertTrue(response4.getBody(), response4.getBody().contains("\"key\":\"C\""));
373+
Assert.assertTrue(response4.getBody(), response4.getBody().contains("\"key\":\"D\""));
374+
Assert.assertFalse(response4.getBody(), response4.getBody().contains("\"key\":\"E\""));
375+
376+
// Significant Text Aggregation is not impacted.
377+
// Non-admin user with setting "min_doc_count=0". Expected to only have access to buckets for dept_manager".
378+
String query3 = "{\"aggregations\":{\"significant_termX\":{\"significant_terms\":{\"field\":\"termX.keyword\",\"min_doc_count\":0}}}}";
379+
HttpResponse response5 = rh.executePostRequest("logs*/_search", query3, encodeBasicHeader("dept_manager", "password"));
380+
381+
Assert.assertEquals(HttpStatus.SC_OK, response5.getStatusCode());
382+
Assert.assertTrue(response5.getBody(), response5.getBody().contains("\"termX\":\"A\""));
383+
Assert.assertFalse(response5.getBody(), response5.getBody().contains("\"termX\":\"B\""));
384+
Assert.assertFalse(response5.getBody(), response5.getBody().contains("\"termX\":\"C\""));
385+
Assert.assertFalse(response5.getBody(), response5.getBody().contains("\"termX\":\"D\""));
386+
Assert.assertFalse(response5.getBody(), response5.getBody().contains("\"termX\":\"E\""));
387+
388+
// Non-admin user without setting "min_doc_count". Expected to only have access to buckets for dept_manager".
389+
String query4 = "{\"aggregations\":{\"significant_termX\":{\"significant_terms\":{\"field\":\"termX.keyword\"}}}}";
390+
391+
HttpResponse response6 = rh.executePostRequest("logs*/_search", query4, encodeBasicHeader("dept_manager", "password"));
392+
393+
Assert.assertEquals(HttpStatus.SC_OK, response6.getStatusCode());
394+
Assert.assertTrue(response6.getBody(), response6.getBody().contains("\"termX\":\"A\""));
395+
Assert.assertFalse(response6.getBody(), response6.getBody().contains("\"termX\":\"B\""));
396+
Assert.assertFalse(response6.getBody(), response6.getBody().contains("\"termX\":\"C\""));
397+
Assert.assertFalse(response6.getBody(), response6.getBody().contains("\"termX\":\"D\""));
398+
Assert.assertFalse(response6.getBody(), response6.getBody().contains("\"termX\":\"E\""));
399+
400+
// Admin with setting "min_doc_count":0. Expected to have access to all buckets".
401+
HttpResponse response7 = rh.executePostRequest("logs*/_search", query3, encodeBasicHeader("admin", "admin"));
402+
403+
Assert.assertEquals(HttpStatus.SC_OK, response7.getStatusCode());
404+
Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"A\""));
405+
Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"B\""));
406+
Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"C\""));
407+
Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"D\""));
408+
Assert.assertTrue(response7.getBody(), response7.getBody().contains("\"termX\":\"E\""));
409+
410+
// Admin without setting "min_doc_count". Expected to have access to all buckets".
411+
HttpResponse response8 = rh.executePostRequest("logs*/_search", query4, encodeBasicHeader("admin", "admin"));
412+
413+
Assert.assertEquals(HttpStatus.SC_OK, response8.getStatusCode());
414+
Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"A\""));
415+
Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"B\""));
416+
Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"C\""));
417+
Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"D\""));
418+
Assert.assertTrue(response8.getBody(), response8.getBody().contains("\"termX\":\"E\""));
419+
420+
// Histogram Aggregation is not impacted.
421+
// Non-admin user with setting "min_doc_count=0". Expected to only have access to buckets for dept_manager".
422+
String query5 = "{\"aggs\":{\"amount\":{\"histogram\":{\"field\":\"amount\",\"interval\":1,\"min_doc_count\":0}}}}";
423+
424+
HttpResponse response9 = rh.executePostRequest("logs*/_search", query5, encodeBasicHeader("dept_manager", "password"));
425+
426+
Assert.assertEquals(HttpStatus.SC_OK, response9.getStatusCode());
427+
Assert.assertTrue(response9.getBody(), response9.getBody().contains("\"termX\":\"A\""));
428+
Assert.assertFalse(response9.getBody(), response9.getBody().contains("\"termX\":\"B\""));
429+
Assert.assertFalse(response9.getBody(), response9.getBody().contains("\"termX\":\"C\""));
430+
Assert.assertFalse(response9.getBody(), response9.getBody().contains("\"termX\":\"D\""));
431+
Assert.assertFalse(response9.getBody(), response9.getBody().contains("\"termX\":\"E\""));
432+
433+
// Non-admin user without setting "min_doc_count". Expected to only have access to buckets for dept_manager".
434+
String query6 = "{\"aggs\":{\"amount\":{\"histogram\":{\"field\":\"amount\",\"interval\":1}}}}";
435+
436+
HttpResponse response10 = rh.executePostRequest("logs*/_search", query6, encodeBasicHeader("dept_manager", "password"));
437+
438+
Assert.assertEquals(HttpStatus.SC_OK, response10.getStatusCode());
439+
Assert.assertTrue(response10.getBody(), response10.getBody().contains("\"termX\":\"A\""));
440+
Assert.assertFalse(response10.getBody(), response10.getBody().contains("\"termX\":\"B\""));
441+
Assert.assertFalse(response10.getBody(), response10.getBody().contains("\"termX\":\"C\""));
442+
Assert.assertFalse(response10.getBody(), response10.getBody().contains("\"termX\":\"D\""));
443+
Assert.assertFalse(response10.getBody(), response10.getBody().contains("\"termX\":\"E\""));
444+
445+
// Admin with setting "min_doc_count":0. Expected to have access to all buckets".
446+
HttpResponse response11 = rh.executePostRequest("logs*/_search", query5, encodeBasicHeader("admin", "admin"));
447+
448+
Assert.assertEquals(HttpStatus.SC_OK, response11.getStatusCode());
449+
Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"A\""));
450+
Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"B\""));
451+
Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"C\""));
452+
Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"D\""));
453+
Assert.assertTrue(response11.getBody(), response11.getBody().contains("\"termX\":\"E\""));
454+
455+
456+
// Admin without setting "min_doc_count". Expected to have access to all buckets".
457+
HttpResponse response12 = rh.executePostRequest("logs*/_search", query6, encodeBasicHeader("admin", "admin"));
458+
459+
Assert.assertEquals(HttpStatus.SC_OK, response12.getStatusCode());
460+
Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"A\""));
461+
Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"B\""));
462+
Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"C\""));
463+
Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"D\""));
464+
Assert.assertTrue(response12.getBody(), response12.getBody().contains("\"termX\":\"E\""));
465+
466+
// Date Histogram Aggregation is not impacted.
467+
// Non-admin user with setting "min_doc_count=0". Expected to only have access to buckets for dept_manager".
468+
String query7 = "{\"aggs\":{\"timestamp\":{\"date_histogram\":{\"field\":\"timestamp\",\"calendar_interval\":\"month\",\"min_doc_count\":0}}}}";
469+
470+
HttpResponse response13 = rh.executePostRequest("logs*/_search", query7, encodeBasicHeader("dept_manager", "password"));
471+
472+
Assert.assertEquals(HttpStatus.SC_OK, response13.getStatusCode());
473+
Assert.assertTrue(response13.getBody(), response13.getBody().contains("\"termX\":\"A\""));
474+
Assert.assertFalse(response13.getBody(), response13.getBody().contains("\"termX\":\"B\""));
475+
Assert.assertFalse(response13.getBody(), response13.getBody().contains("\"termX\":\"C\""));
476+
Assert.assertFalse(response13.getBody(), response13.getBody().contains("\"termX\":\"D\""));
477+
Assert.assertFalse(response13.getBody(), response13.getBody().contains("\"termX\":\"E\""));
478+
479+
// Non-admin user without setting "min_doc_count". Expected to only have access to buckets for dept_manager".
480+
String query8 = "{\"aggs\":{\"timestamp\":{\"date_histogram\":{\"field\":\"timestamp\",\"calendar_interval\":\"month\"}}}}";
481+
482+
HttpResponse response14 = rh.executePostRequest("logs*/_search", query8, encodeBasicHeader("dept_manager", "password"));
483+
484+
Assert.assertEquals(HttpStatus.SC_OK, response14.getStatusCode());
485+
Assert.assertTrue(response14.getBody(), response14.getBody().contains("\"termX\":\"A\""));
486+
Assert.assertFalse(response14.getBody(), response14.getBody().contains("\"termX\":\"B\""));
487+
Assert.assertFalse(response14.getBody(), response14.getBody().contains("\"termX\":\"C\""));
488+
Assert.assertFalse(response14.getBody(), response14.getBody().contains("\"termX\":\"D\""));
489+
Assert.assertFalse(response14.getBody(), response14.getBody().contains("\"termX\":\"E\""));
490+
491+
// Admin with setting "min_doc_count":0. Expected to have access to all buckets".
492+
HttpResponse response15 = rh.executePostRequest("logs*/_search", query7, encodeBasicHeader("admin", "admin"));
493+
494+
Assert.assertEquals(HttpStatus.SC_OK, response15.getStatusCode());
495+
Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"A\""));
496+
Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"B\""));
497+
Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"C\""));
498+
Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"D\""));
499+
Assert.assertTrue(response15.getBody(), response15.getBody().contains("\"termX\":\"E\""));
500+
501+
// Admin without setting "min_doc_count". Expected to have access to all buckets".
502+
HttpResponse response16 = rh.executePostRequest("logs*/_search", query8, encodeBasicHeader("admin", "admin"));
503+
504+
Assert.assertEquals(HttpStatus.SC_OK, response16.getStatusCode());
505+
Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"A\""));
506+
Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"B\""));
507+
Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"C\""));
508+
Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"D\""));
509+
Assert.assertTrue(response16.getBody(), response16.getBody().contains("\"termX\":\"E\""));
510+
}
270511
}

‎src/test/resources/dlsfls/roles.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2466,3 +2466,11 @@ opendistro_security_combined:
24662466
allowed_actions:
24672467
- "OPENDISTRO_SECURITY_READ"
24682468
tenant_permissions: []
2469+
logs_index_with_dls:
2470+
index_permissions:
2471+
- index_patterns:
2472+
- "logs"
2473+
dls: "{\"bool\": {\"filter\": {\"term\" : {\"termX\" : \"A\" }}}}"
2474+
masked_fields: null
2475+
allowed_actions:
2476+
- "OPENDISTRO_SECURITY_READ"

0 commit comments

Comments
 (0)