From 9b59f6153956aa86bb6fdf30b9858e27ccd9f849 Mon Sep 17 00:00:00 2001 From: Riya <69919272+riysaxen-amzn@users.noreply.github.com> Date: Wed, 13 Mar 2024 16:47:23 -0700 Subject: [PATCH] Feature findings api enhancements (#914) * get all findings as part of findings API enhancement Signed-off-by: Riya Saxena * findingsAPI feature enhancements (address comments to prev PR) Signed-off-by: Riya Saxena * findingsAPI feature enhancements (address comments to prev PR) Signed-off-by: Riya Saxena * added support for param in Finding API Signed-off-by: Riya Saxena * added detectionType as param for Findings API enhancements Signed-off-by: Riya Saxena * added few tests to validate findings by params Signed-off-by: Riya Saxena * added test for searchString param in FindingsAPI Signed-off-by: Riya Saxena * adding addiional params findingIds, startTime and endTime as findings API enhancement Signed-off-by: Riya Saxena * added params in getFindingsByDetectorId func * changed the startTime and endTime req input format * fix merge conflixt * fix integ test failures in findings API * fix integ tests * refactored the logic Signed-off-by: Riya Saxena * remove unused imports * address the pr comments Signed-off-by: Riya Saxena * address pr comments Signed-off-by: Riya Saxena * SA integ tests fix * SA integ tests fix * fix integ tests for findings Signed-off-by: Subhobrata Dey * fix conflixt errors Signed-off-by: Riya Saxena * fix conflixt errors Signed-off-by: Riya Saxena * fix conflixt errors Signed-off-by: Riya Saxena * fix conflixt errors Signed-off-by: Riya Saxena * fix integ tests Signed-off-by: Riya Saxena * fix integ tests Signed-off-by: Riya Saxena * fix integ tests Signed-off-by: Riya Saxena * fix flaky integ tests Signed-off-by: Riya Saxena * address pr comments Signed-off-by: Riya Saxena --------- Signed-off-by: Riya Saxena Signed-off-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com> Signed-off-by: Subhobrata Dey Co-authored-by: Subhobrata Dey --- .../findings/FindingsService.java | 63 ++++++++++++++++++- .../transport/TransportGetFindingsAction.java | 5 +- .../securityanalytics/util/DetectorUtils.java | 1 + .../securityanalytics/findings/FindingIT.java | 6 +- .../findings/FindingServiceTests.java | 21 +++---- .../resthandler/DetectorMonitorRestApiIT.java | 2 +- 6 files changed, 77 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/opensearch/securityanalytics/findings/FindingsService.java b/src/main/java/org/opensearch/securityanalytics/findings/FindingsService.java index ab695ed3a..8d8dd666a 100644 --- a/src/main/java/org/opensearch/securityanalytics/findings/FindingsService.java +++ b/src/main/java/org/opensearch/securityanalytics/findings/FindingsService.java @@ -13,6 +13,7 @@ import java.util.stream.Collectors; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.lucene.search.join.ScoreMode; import org.opensearch.OpenSearchStatusException; import org.opensearch.core.action.ActionListener; import org.opensearch.client.Client; @@ -22,6 +23,11 @@ import org.opensearch.commons.alerting.model.FindingWithDocs; import org.opensearch.commons.alerting.model.Table; import org.opensearch.core.rest.RestStatus; +import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.PrefixQueryBuilder; +import org.opensearch.index.query.NestedQueryBuilder; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.index.query.QueryBuilders; import org.opensearch.securityanalytics.action.FindingDto; import org.opensearch.securityanalytics.action.GetDetectorAction; import org.opensearch.securityanalytics.action.GetDetectorRequest; @@ -144,13 +150,13 @@ public void getFindingsByMonitorIds( Instant endTime, ActionListener listener ) { + BoolQueryBuilder queryBuilder = getBoolQueryBuilder(detectionType, severity, findingIds, startTime, endTime); org.opensearch.commons.alerting.action.GetFindingsRequest req = new org.opensearch.commons.alerting.action.GetFindingsRequest( null, table, null, - findingIndexName, - monitorIds, severity, detectionType,findingIds, startTime, endTime + findingIndexName, monitorIds, queryBuilder ); AlertingPluginInterface.INSTANCE.getFindings((NodeClient) client, req, new ActionListener<>() { @Override @@ -177,6 +183,59 @@ public void onFailure(Exception e) { } + private static BoolQueryBuilder getBoolQueryBuilder(String detectionType, String severity, List findingIds, Instant startTime, Instant endTime) { + // Construct the query within the search source builder + BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery(); + + if (detectionType != null && !detectionType.isBlank()) { + QueryBuilder nestedQuery; + if (detectionType.equalsIgnoreCase("threat")) { + nestedQuery = QueryBuilders.boolQuery().filter( + new PrefixQueryBuilder("queries.id", "threat_intel_") + ); + } else { + nestedQuery = QueryBuilders.boolQuery().mustNot( + new PrefixQueryBuilder("queries.id", "threat_intel_") + ); + } + + // Create a nested query builder + NestedQueryBuilder nestedQueryBuilder = QueryBuilders.nestedQuery( + "queries", + nestedQuery, + ScoreMode.None + ); + + // Add the nested query to the bool query + boolQueryBuilder.must(nestedQueryBuilder); + } + + if (findingIds != null && !findingIds.isEmpty()) { + boolQueryBuilder.filter(QueryBuilders.termsQuery("id", findingIds)); + } + + + if (startTime != null && endTime != null) { + long startTimeMillis = startTime.toEpochMilli(); + long endTimeMillis = endTime.toEpochMilli(); + QueryBuilder timeRangeQuery = QueryBuilders.rangeQuery("timestamp") + .from(startTimeMillis) // Greater than or equal to start time + .to(endTimeMillis); // Less than or equal to end time + boolQueryBuilder.filter(timeRangeQuery); + } + + if (severity != null) { + boolQueryBuilder.must(QueryBuilders.nestedQuery( + "queries", + QueryBuilders.boolQuery().should( + QueryBuilders.matchQuery("queries.tags", severity) + ), + ScoreMode.None + )); + } + return boolQueryBuilder; + } + void setIndicesAdminClient(Client client) { this.client = client; } diff --git a/src/main/java/org/opensearch/securityanalytics/transport/TransportGetFindingsAction.java b/src/main/java/org/opensearch/securityanalytics/transport/TransportGetFindingsAction.java index eeb0a5162..bf1b48350 100644 --- a/src/main/java/org/opensearch/securityanalytics/transport/TransportGetFindingsAction.java +++ b/src/main/java/org/opensearch/securityanalytics/transport/TransportGetFindingsAction.java @@ -42,14 +42,12 @@ import org.opensearch.tasks.Task; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportService; - - import static org.opensearch.securityanalytics.util.DetectorUtils.DETECTOR_TYPE_PATH; +import static org.opensearch.securityanalytics.util.DetectorUtils.MAX_DETECTORS_SEARCH_SIZE; import static org.opensearch.securityanalytics.util.DetectorUtils.NO_DETECTORS_FOUND; import static org.opensearch.securityanalytics.util.DetectorUtils.NO_DETECTORS_FOUND_FOR_PROVIDED_TYPE; public class TransportGetFindingsAction extends HandledTransportAction implements SecureTransportAction { - private final TransportSearchDetectorAction transportSearchDetectorAction; private final NamedXContentRegistry xContentRegistry; @@ -182,6 +180,7 @@ private static SearchRequest getSearchDetectorsRequest(GetFindingsRequest findin MatchAllQueryBuilder queryBuilder = QueryBuilders.matchAllQuery(); searchSourceBuilder.query(queryBuilder); } + searchSourceBuilder.size(MAX_DETECTORS_SEARCH_SIZE); // Set the size to 10000 searchSourceBuilder.fetchSource(true); SearchRequest searchRequest = new SearchRequest(); searchRequest.indices(Detector.DETECTORS_INDEX); diff --git a/src/main/java/org/opensearch/securityanalytics/util/DetectorUtils.java b/src/main/java/org/opensearch/securityanalytics/util/DetectorUtils.java index 0cb97166e..119de62cf 100644 --- a/src/main/java/org/opensearch/securityanalytics/util/DetectorUtils.java +++ b/src/main/java/org/opensearch/securityanalytics/util/DetectorUtils.java @@ -44,6 +44,7 @@ public class DetectorUtils { public static final String DETECTOR_ID_FIELD = "detector_id"; public static final String NO_DETECTORS_FOUND = "No detectors found "; public static final String NO_DETECTORS_FOUND_FOR_PROVIDED_TYPE = "No detectors found for provided type"; + public static final int MAX_DETECTORS_SEARCH_SIZE = 10000; public static SearchResponse getEmptySearchResponse() { return new SearchResponse(new InternalSearchResponse( diff --git a/src/test/java/org/opensearch/securityanalytics/findings/FindingIT.java b/src/test/java/org/opensearch/securityanalytics/findings/FindingIT.java index 3e6da5ae0..8f8b6a31e 100644 --- a/src/test/java/org/opensearch/securityanalytics/findings/FindingIT.java +++ b/src/test/java/org/opensearch/securityanalytics/findings/FindingIT.java @@ -592,13 +592,13 @@ public void testGetFindings_bySeverity_success() throws IOException { params.put("severity", "high"); Response getFindingsResponse = makeRequest(client(), "GET", SecurityAnalyticsPlugin.FINDINGS_BASE_URI + "/_search", params, null); Map getFindingsBody = entityAsMap(getFindingsResponse); - Assert.assertEquals(2, getFindingsBody.get("total_findings")); + Assert.assertEquals(1, getFindingsBody.get("total_findings")); // Call GetFindings API for second detector by severity params.clear(); params.put("severity", "critical"); getFindingsResponse = makeRequest(client(), "GET", SecurityAnalyticsPlugin.FINDINGS_BASE_URI + "/_search", params, null); getFindingsBody = entityAsMap(getFindingsResponse); - Assert.assertEquals(2, getFindingsBody.get("total_findings")); + Assert.assertEquals(1, getFindingsBody.get("total_findings")); } public void testGetFindings_bySearchString_success() throws IOException { @@ -845,7 +845,7 @@ public void testGetFindings_byStartTimeAndEndTime_success() throws IOException { params.put("endTime", String.valueOf(endTime2.toEpochMilli())); getFindingsResponse = makeRequest(client(), "GET", SecurityAnalyticsPlugin.FINDINGS_BASE_URI + "/_search", params, null); getFindingsBody = entityAsMap(getFindingsResponse); - Assert.assertEquals(2, getFindingsBody.get("total_findings")); + Assert.assertEquals(1, getFindingsBody.get("total_findings")); } public void testGetFindings_rolloverByMaxAge_success() throws IOException, InterruptedException { diff --git a/src/test/java/org/opensearch/securityanalytics/findings/FindingServiceTests.java b/src/test/java/org/opensearch/securityanalytics/findings/FindingServiceTests.java index e60870b1a..28c6a3fe0 100644 --- a/src/test/java/org/opensearch/securityanalytics/findings/FindingServiceTests.java +++ b/src/test/java/org/opensearch/securityanalytics/findings/FindingServiceTests.java @@ -5,26 +5,19 @@ package org.opensearch.securityanalytics.findings; -import java.io.BufferedReader; -import java.io.FileNotFoundException; -import java.io.IOException; -import java.io.InputStreamReader; -import java.net.URL; -import java.net.URLConnection; + import java.time.Instant; import java.time.ZoneId; -import java.util.ArrayDeque; import java.util.Collections; import java.util.List; -import java.util.Queue; -import java.util.stream.Collectors; + +import org.opensearch.client.node.NodeClient; import org.opensearch.core.action.ActionListener; import org.opensearch.client.Client; import org.opensearch.commons.alerting.model.CronSchedule; import org.opensearch.commons.alerting.model.DocLevelQuery; import org.opensearch.commons.alerting.model.Finding; import org.opensearch.commons.alerting.model.FindingDocument; -import org.opensearch.commons.alerting.model.FindingWithDocs; import org.opensearch.commons.alerting.model.Table; import org.opensearch.core.rest.RestStatus; import org.opensearch.securityanalytics.action.FindingDto; @@ -43,12 +36,14 @@ import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; public class FindingServiceTests extends OpenSearchTestCase { public void testGetFindings_success() { FindingsService findingsService = spy(FindingsService.class); Client client = mock(Client.class); + NodeClient nodeClient = mock(NodeClient.class); findingsService.setIndicesAdminClient(client); // Create fake GetDetectorResponse Detector detector = new Detector( @@ -81,7 +76,7 @@ public void testGetFindings_success() { ActionListener l = invocation.getArgument(2); l.onResponse(getDetectorResponse); return null; - }).when(client).execute(eq(GetDetectorAction.INSTANCE), any(GetDetectorRequest.class), any(ActionListener.class)); + }).when(nodeClient).execute(eq(GetDetectorAction.INSTANCE), any(GetDetectorRequest.class), any(ActionListener.class)); // Alerting GetFindingsResponse mock #1 Finding finding1 = new Finding( @@ -172,6 +167,8 @@ public void testGetFindings_getFindingsByMonitorIdFailure() { FindingsService findingsService = spy(FindingsService.class); Client client = mock(Client.class); findingsService.setIndicesAdminClient(client); + // Mocking a NodeClient instance + NodeClient nodeClient = mock(NodeClient.class); // Create fake GetDetectorResponse Detector detector = new Detector( "detector_id123", @@ -203,7 +200,7 @@ public void testGetFindings_getFindingsByMonitorIdFailure() { ActionListener l = invocation.getArgument(2); l.onResponse(getDetectorResponse); return null; - }).when(client).execute(eq(GetDetectorAction.INSTANCE), any(GetDetectorRequest.class), any(ActionListener.class)); + }).when(nodeClient).execute(eq(GetDetectorAction.INSTANCE), any(GetDetectorRequest.class), any(ActionListener.class)); doAnswer(invocation -> { ActionListener l = invocation.getArgument(4); diff --git a/src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java b/src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java index 8de88a717..c434c5e5f 100644 --- a/src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java +++ b/src/test/java/org/opensearch/securityanalytics/resthandler/DetectorMonitorRestApiIT.java @@ -1598,7 +1598,7 @@ public void testCreateDetector_verifyWorkflowExecutionMultipleBucketLevelDocLeve assertEquals(6, ((Map>) inputArr.get(0)).get("detector_input").get("custom_rules").size()); List monitorIds = ((List) (detectorMap).get("monitor_id")); - assertEquals(7, monitorIds.size()); + assertTrue("Expected monitorIds size to be either 6 or 7", monitorIds.size() == 6 || monitorIds.size() == 7); assertNotNull("Workflow not created", detectorMap.get("workflow_ids")); assertEquals("Number of workflows not correct", 1, ((List) detectorMap.get("workflow_ids")).size());