[Security Solution] Fix coverage overview endpoint (#164749)

## Summary

This PR fixes a bug found by @jamesspi when it's Coverage Overview page doesn't show the cards but applying individual filters makes the page rendering.

## Steps to reproduce the problem

- create a non security rule in your environment
- navigate to the Coverage Overview page

Expected behavior
Coverage Overview page is loaded and displays the cards.

Actual behavior
Coverage Overview page does not display the cards.

## Details

`rulesClient.find()` was used directly instead of `findRules()` helper function defined in Security Solution. The subtle different between these two is that `findRules()` passes filter to `enrichFilterWithRuleTypeMapping()` to make sure only security rules are fetched and processed. As `rulesClient.find()` returned all the rules there was a non security rule in the list with missing `rule.params` field causing the endpoint to fail. This has been fixed by using `findRules()`.

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/coverage_overview/handle_coverage_overview_request.test.ts

@@ -7,17 +7,14 @@
 
 import type { Rule } from '@kbn/alerting-plugin/common';
 import { rulesClientMock } from '@kbn/alerting-plugin/server/mocks';
+import { findRules } from '../../../logic/search/find_rules';
 import { handleCoverageOverviewRequest } from './handle_coverage_overview_request';
 
-describe('handleCoverageOverviewRequest', () => {
-  let rulesClient: ReturnType<typeof rulesClientMock.create>;
-
-  beforeEach(() => {
-    rulesClient = rulesClientMock.create();
-  });
+jest.mock('../../../logic/search/find_rules');
 
-  it('does not request more than 10k', async () => {
-    rulesClient.find
+describe('handleCoverageOverviewRequest', () => {
+  it('does not request more than 10k rules', async () => {
+    (findRules as jest.Mock)
       .mockResolvedValueOnce({
         total: 25555,
         page: 1,
@@ -40,17 +37,17 @@ describe('handleCoverageOverviewRequest', () => {
     await handleCoverageOverviewRequest({
       params: {},
       deps: {
-        rulesClient,
+        rulesClient: rulesClientMock.create(),
       },
     });
 
-    expect(rulesClient.find).toHaveBeenCalledTimes(1);
-    expect(rulesClient.find).toHaveBeenCalledWith({
-      options: expect.objectContaining({
+    expect(findRules).toHaveBeenCalledTimes(1);
+    expect(findRules).toHaveBeenCalledWith(
+      expect.objectContaining({
         page: 1,
         perPage: 10000,
-      }),
-    });
+      })
+    );
   });
 });
 

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/coverage_overview/handle_coverage_overview_request.ts

@@ -17,6 +17,7 @@ import {
   CoverageOverviewRuleActivity,
 } from '../../../../../../../common/api/detection_engine';
 import type { RuleParams } from '../../../../rule_schema';
+import { findRules } from '../../../logic/search/find_rules';
 
 type CoverageOverviewRuleParams = Pick<RuleParams, 'threat'>;
 
@@ -34,26 +35,25 @@ export async function handleCoverageOverviewRequest({
   deps: { rulesClient },
 }: HandleCoverageOverviewRequestArgs): Promise<CoverageOverviewResponse> {
   const activitySet = new Set(filter?.activity);
-  const kqlFilter = filter
-    ? convertRulesFilterToKQL({
-        filter: filter.search_term,
-        showCustomRules: filter.source?.includes(CoverageOverviewRuleSource.Custom) ?? false,
-        showElasticRules: filter.source?.includes(CoverageOverviewRuleSource.Prebuilt) ?? false,
-        enabled: getIsEnabledFilter(activitySet),
-      })
-    : undefined;
+  const kqlFilter = convertRulesFilterToKQL({
+    filter: filter?.search_term,
+    showCustomRules: filter?.source?.includes(CoverageOverviewRuleSource.Custom) ?? false,
+    showElasticRules: filter?.source?.includes(CoverageOverviewRuleSource.Prebuilt) ?? false,
+    enabled: getIsEnabledFilter(activitySet),
+  });
 
   // rulesClient.find uses ES Search API to fetch the rules. It has some limitations when the number of rules exceeds
   // index.max_result_window (set to 10K by default) Kibana fails. A proper way to handle it is via ES PIT API.
   // This way the endpoint handles max 10K rules for now while support for the higher number of rules will be addressed
   // in https://github.com/elastic/kibana/issues/160698
-  const rules = await rulesClient.find<CoverageOverviewRuleParams>({
-    options: {
-      filter: kqlFilter,
-      fields: ['name', 'enabled', 'params.threat'],
-      page: 1,
-      perPage: 10000,
-    },
+  const rules = await findRules({
+    rulesClient,
+    filter: kqlFilter,
+    fields: ['name', 'enabled', 'params.threat'],
+    page: 1,
+    perPage: 10000,
+    sortField: undefined,
+    sortOrder: undefined,
   });
 
   return rules.data.reduce(appendRuleToResponse, {

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/search/enrich_filter_with_rule_type_mappings.test.ts

@@ -17,22 +17,26 @@ import {
 
 import { enrichFilterWithRuleTypeMapping } from './enrich_filter_with_rule_type_mappings';
 
-const allAlertTypeIds = `(alert.attributes.alertTypeId: ${EQL_RULE_TYPE_ID}
+const allAlertTypeIds = `alert.attributes.alertTypeId: ${EQL_RULE_TYPE_ID}
  OR alert.attributes.alertTypeId: ${ML_RULE_TYPE_ID}
  OR alert.attributes.alertTypeId: ${QUERY_RULE_TYPE_ID}
  OR alert.attributes.alertTypeId: ${SAVED_QUERY_RULE_TYPE_ID}
  OR alert.attributes.alertTypeId: ${INDICATOR_RULE_TYPE_ID}
  OR alert.attributes.alertTypeId: ${THRESHOLD_RULE_TYPE_ID}
- OR alert.attributes.alertTypeId: ${NEW_TERMS_RULE_TYPE_ID})`.replace(/[\n\r]/g, '');
+ OR alert.attributes.alertTypeId: ${NEW_TERMS_RULE_TYPE_ID}`.replace(/[\n\r]/g, '');
 
 describe('enrichFilterWithRuleTypeMapping', () => {
   test('it returns a full filter with an AND if sent down', () => {
     expect(enrichFilterWithRuleTypeMapping('alert.attributes.enabled: true')).toEqual(
-      `${allAlertTypeIds} AND alert.attributes.enabled: true`
+      `(${allAlertTypeIds}) AND (alert.attributes.enabled: true)`
     );
   });
 
   test('it returns existing filter with no AND when not set [rule registry enabled: %p]', () => {
     expect(enrichFilterWithRuleTypeMapping(null)).toEqual(allAlertTypeIds);
   });
+
+  test('it returns existing filter with no AND when filter is an empty string', () => {
+    expect(enrichFilterWithRuleTypeMapping('')).toEqual(allAlertTypeIds);
+  });
 });

x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/search/enrich_filter_with_rule_type_mappings.ts

@@ -7,23 +7,25 @@
 
 import { ruleTypeMappings } from '@kbn/securitysolution-rules';
 
-const alertTypeFilter = `(${Object.values(ruleTypeMappings)
+const alertTypeFilter = Object.values(ruleTypeMappings)
   .map((type) => `alert.attributes.alertTypeId: ${type}`)
   .filter((type, i, arr) => type != null && arr.indexOf(type) === i)
-  .join(' OR ')})`;
+  .join(' OR ');
 
 /**
  * updates filter to restrict search results to only Security Solution rule types (siem.eqlRule, siem.mlRule, etc..)
  * @example
  * filter BEFORE: "alert.attributes.enabled: true"
- * modified filter AFTER: "(alert.attributes.alertTypeId: siem.eqlRule OR alert.attributes.alertTypeId: siem.mlRule OR alert.attributes.alertTypeId: siem.queryRule OR alert.attributes.alertTypeId: siem.savedQueryRule OR alert.attributes.alertTypeId: siem.indicatorRule OR alert.attributes.alertTypeId: siem.thresholdRule) AND alert.attributes.enabled: true"
+ * modified filter AFTER: "(alert.attributes.alertTypeId: siem.eqlRule OR alert.attributes.alertTypeId: siem.mlRule OR alert.attributes.alertTypeId: siem.queryRule OR alert.attributes.alertTypeId: siem.savedQueryRule OR alert.attributes.alertTypeId: siem.indicatorRule OR alert.attributes.alertTypeId: siem.thresholdRule) AND (alert.attributes.enabled: true")
+ *
  * @param filter
  * @returns modified filter
  */
 export const enrichFilterWithRuleTypeMapping = (filter: string | null | undefined) => {
-  if (filter == null) {
+  if (filter == null || filter.length === 0) {
     return alertTypeFilter;
-  } else {
-    return `${alertTypeFilter} AND ${filter}`;
   }
+
+  // Just in case of statements OR'ed in the filter wrap it in parentheses
+  return `(${alertTypeFilter}) AND (${filter})`;
 };

x-pack/test/detection_engine_api_integration/basic/tests/coverage_overview.ts

@@ -20,6 +20,7 @@ import {
   getSimpleRule,
   installPrebuiltRulesAndTimelines,
 } from '../../utils';
+import { createNonSecurityRule } from '../../utils/create_non_security_rule';
 
 // eslint-disable-next-line import/no-default-export
 export default ({ getService }: FtrProviderContext): void => {
@@ -32,6 +33,36 @@ export default ({ getService }: FtrProviderContext): void => {
       await deleteAllRules(supertest, log);
     });
 
+    it('does NOT error when there are no security rules', async () => {
+      await createNonSecurityRule(supertest);
+      const rule1 = await createRule(supertest, log, {
+        ...getSimpleRule(),
+        threat: generateThreatArray(1),
+      });
+
+      const { body } = await supertest
+        .post(RULE_MANAGEMENT_COVERAGE_OVERVIEW_URL)
+        .set('kbn-xsrf', 'true')
+        .set('elastic-api-version', '1')
+        .send({})
+        .expect(200);
+
+      expect(body).to.eql({
+        coverage: {
+          T001: [rule1.id],
+          TA001: [rule1.id],
+          'T001.001': [rule1.id],
+        },
+        unmapped_rule_ids: [],
+        rules_data: {
+          [rule1.id]: {
+            activity: 'disabled',
+            name: 'Simple Rule Query',
+          },
+        },
+      });
+    });
+
     describe('without filters', () => {
       it('returns an empty response if there are no rules', async () => {
         const { body } = await supertest

x-pack/test/detection_engine_api_integration/utils/create_non_security_rule.ts

@@ -0,0 +1,41 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+import type SuperTest from 'supertest';
+
+const SIMPLE_APM_RULE_DATA = {
+  name: 'Test rule',
+  rule_type_id: 'apm.anomaly',
+  enabled: false,
+  consumer: 'alerts',
+  tags: [],
+  actions: [],
+  params: {
+    windowSize: 30,
+    windowUnit: 'm',
+    anomalySeverityType: 'critical',
+    environment: 'ENVIRONMENT_ALL',
+  },
+  schedule: {
+    interval: '10m',
+  },
+};
+
+/**
+ * Created a non security rule. Helpful in tests to verify functionality works with presence of non security rules.
+ * @param supertest The supertest deps
+ */
+export async function createNonSecurityRule(
+  supertest: SuperTest.SuperTest<SuperTest.Test>
+): Promise<void> {
+  await supertest
+    .post('/api/alerting/rule')
+    .set('kbn-xsrf', 'true')
+    .set('elastic-api-version', '2023-10-31')
+    .send(SIMPLE_APM_RULE_DATA)
+    .expect(200);
+}