From 2788dca390fd8cef4119f4e8038f4fda7818f608 Mon Sep 17 00:00:00 2001 From: Yara Tercero <yctercero@users.noreply.github.com> Date: Tue, 27 Oct 2020 20:28:48 -0400 Subject: [PATCH] [Security Solutions][Detections] - Fix bug, last response not showing for disabled rules (#81783) ## Summary **Bug fix addressed in this PR:** - fixes https://github.com/elastic/kibana/issues/63203 - in the backend, we were only fetching status data for enabled rules, changed to fetch status data regardles of whether rule is enabled or disabled --- .../cypress/screens/alerts_detection_rules.ts | 4 +- .../__snapshots__/index.test.tsx.snap | 20 --- .../rules/rule_switch/index.test.tsx | 167 +++++++++++++++++- .../components/rules/rule_switch/index.tsx | 29 +-- .../routes/rules/patch_rules_bulk_route.ts | 7 +- .../routes/rules/update_rules_bulk_route.ts | 8 +- .../routes/rules/validate.test.ts | 68 ++++++- .../detection_engine/routes/rules/validate.ts | 3 +- 8 files changed, 243 insertions(+), 63 deletions(-) delete mode 100644 x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/__snapshots__/index.test.tsx.snap diff --git a/x-pack/plugins/security_solution/cypress/screens/alerts_detection_rules.ts b/x-pack/plugins/security_solution/cypress/screens/alerts_detection_rules.ts index f68ad88f578c7..0d0ea8460edf1 100644 --- a/x-pack/plugins/security_solution/cypress/screens/alerts_detection_rules.ts +++ b/x-pack/plugins/security_solution/cypress/screens/alerts_detection_rules.ts @@ -47,9 +47,9 @@ export const RULE_CHECKBOX = '.euiTableRow .euiCheckbox__input'; export const RULE_NAME = '[data-test-subj="ruleName"]'; -export const RULE_SWITCH = '[data-test-subj="rule-switch"]'; +export const RULE_SWITCH = '[data-test-subj="ruleSwitch"]'; -export const RULE_SWITCH_LOADER = '[data-test-subj="rule-switch-loader"]'; +export const RULE_SWITCH_LOADER = '[data-test-subj="ruleSwitchLoader"]'; export const RULES_TABLE = '[data-test-subj="rules-table"]'; diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/__snapshots__/index.test.tsx.snap b/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/__snapshots__/index.test.tsx.snap deleted file mode 100644 index 604f86866d565..0000000000000 --- a/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/__snapshots__/index.test.tsx.snap +++ /dev/null @@ -1,20 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`RuleSwitch renders correctly against snapshot 1`] = ` -<EuiFlexGroup - alignItems="center" - justifyContent="spaceAround" -> - <EuiFlexItem - grow={false} - > - <StaticSwitch - checked={true} - data-test-subj="rule-switch" - label="rule-switch" - onChange={[Function]} - showLabel={true} - /> - </EuiFlexItem> -</EuiFlexGroup> -`; diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.test.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.test.tsx index 104eff34c91b3..910a28927fd93 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.test.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.test.tsx @@ -4,16 +4,173 @@ * you may not use this file except in compliance with the Elastic License. */ -import { shallow } from 'enzyme'; +import { mount } from 'enzyme'; import React from 'react'; +import { ThemeProvider } from 'styled-components'; +import euiLightVars from '@elastic/eui/dist/eui_theme_light.json'; +import { waitFor } from '@testing-library/react'; +import { enableRules } from '../../../containers/detection_engine/rules'; +import { enableRulesAction } from '../../../pages/detection_engine/rules/all/actions'; import { RuleSwitchComponent } from './index'; +import { getRulesSchemaMock } from '../../../../../common/detection_engine/schemas/response/rules_schema.mocks'; +import { RulesSchema } from '../../../../../common/detection_engine/schemas/response/rules_schema'; +import { useStateToaster, displayErrorToast } from '../../../../common/components/toasters'; + +jest.mock('../../../../common/components/toasters'); +jest.mock('../../../containers/detection_engine/rules'); +jest.mock('../../../pages/detection_engine/rules/all/actions'); describe('RuleSwitch', () => { - test('renders correctly against snapshot', () => { - const wrapper = shallow( - <RuleSwitchComponent optionLabel="rule-switch" enabled={true} id={'7'} isLoading={false} /> + beforeEach(() => { + (useStateToaster as jest.Mock).mockImplementation(() => [[], jest.fn()]); + (enableRules as jest.Mock).mockResolvedValue([getRulesSchemaMock()]); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + test('it renders loader if "isLoading" is true', () => { + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent optionLabel="rule-switch" enabled={true} id={'7'} isLoading /> + </ThemeProvider> + ); + + expect(wrapper.find('[data-test-subj="ruleSwitchLoader"]').exists()).toBeTruthy(); + expect(wrapper.find('[data-test-subj="ruleSwitch"]').exists()).toBeFalsy(); + }); + + test('it renders switch disabled if "isDisabled" is true', () => { + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent optionLabel="rule-switch" enabled={true} id={'7'} isDisabled /> + </ThemeProvider> + ); + + expect(wrapper.find('[data-test-subj="ruleSwitch"]').at(0).props().disabled).toBeTruthy(); + }); + + test('it renders switch enabled if "enabled" is true', () => { + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent optionLabel="rule-switch" enabled id={'7'} /> + </ThemeProvider> + ); + expect(wrapper.find('[data-test-subj="ruleSwitch"]').at(0).props().checked).toBeTruthy(); + }); + + test('it renders switch disabled if "enabled" is false', () => { + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent optionLabel="rule-switch" enabled={false} id={'7'} /> + </ThemeProvider> + ); + expect(wrapper.find('[data-test-subj="ruleSwitch"]').at(0).props().checked).toBeFalsy(); + }); + + test('it renders an off switch enabled on click', async () => { + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent + optionLabel="rule-switch" + enabled={false} + isDisabled={false} + id={'7'} + /> + </ThemeProvider> + ); + wrapper.find('[data-test-subj="ruleSwitch"]').at(2).simulate('click'); + + await waitFor(() => { + wrapper.update(); + expect(wrapper.find('[data-test-subj="ruleSwitch"]').at(1).props().checked).toBeTruthy(); + }); + }); + + test('it renders an on switch off on click', async () => { + const rule: RulesSchema = { ...getRulesSchemaMock(), enabled: false }; + + (enableRules as jest.Mock).mockResolvedValue([rule]); + + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent optionLabel="rule-switch" enabled isDisabled={false} id={'7'} /> + </ThemeProvider> + ); + wrapper.find('[data-test-subj="ruleSwitch"]').at(2).simulate('click'); + + await waitFor(() => { + wrapper.update(); + expect(wrapper.find('[data-test-subj="ruleSwitch"]').at(1).props().checked).toBeFalsy(); + }); + }); + + test('it dispatches error toaster if "enableRules" call rejects', async () => { + const mockError = new Error('uh oh'); + (enableRules as jest.Mock).mockRejectedValue(mockError); + + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent + optionLabel="rule-switch" + enabled={false} + isDisabled={false} + id={'7'} + /> + </ThemeProvider> ); - expect(wrapper).toMatchSnapshot(); + wrapper.find('[data-test-subj="ruleSwitch"]').at(2).simulate('click'); + + await waitFor(() => { + wrapper.update(); + expect(displayErrorToast).toHaveBeenCalledTimes(1); + }); + }); + + test('it dispatches error toaster if "enableRules" call resolves with some errors', async () => { + (enableRules as jest.Mock).mockResolvedValue([ + getRulesSchemaMock(), + { error: { status_code: 400, message: 'error' } }, + { error: { status_code: 400, message: 'error' } }, + ]); + + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent + optionLabel="rule-switch" + enabled={false} + isDisabled={false} + id={'7'} + /> + </ThemeProvider> + ); + wrapper.find('[data-test-subj="ruleSwitch"]').at(2).simulate('click'); + + await waitFor(() => { + wrapper.update(); + expect(displayErrorToast).toHaveBeenCalledTimes(1); + }); + }); + + test('it invokes "enableRulesAction" if dispatch is passed through', async () => { + const wrapper = mount( + <ThemeProvider theme={() => ({ eui: euiLightVars, darkMode: false })}> + <RuleSwitchComponent + optionLabel="rule-switch" + enabled + isDisabled={false} + id={'7'} + dispatch={jest.fn()} + /> + </ThemeProvider> + ); + wrapper.find('[data-test-subj="ruleSwitch"]').at(2).simulate('click'); + + await waitFor(() => { + wrapper.update(); + expect(enableRulesAction).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.tsx index 73d66bf024a62..1a9bcca7eb601 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/rule_switch/index.tsx @@ -13,7 +13,7 @@ import { } from '@elastic/eui'; import { isEmpty } from 'lodash/fp'; import styled from 'styled-components'; -import React, { useCallback, useState, useEffect } from 'react'; +import React, { useMemo, useCallback, useState, useEffect } from 'react'; import * as i18n from '../../../pages/detection_engine/rules/translations'; import { enableRules } from '../../../containers/detection_engine/rules'; @@ -63,8 +63,11 @@ export const RuleSwitchComponent = ({ if (dispatch != null) { await enableRulesAction([id], event.target.checked!, dispatch, dispatchToaster); } else { + const enabling = event.target.checked!; + const title = enabling + ? i18n.BATCH_ACTION_ACTIVATE_SELECTED_ERROR(1) + : i18n.BATCH_ACTION_DEACTIVATE_SELECTED_ERROR(1); try { - const enabling = event.target.checked!; const response = await enableRules({ ids: [id], enabled: enabling, @@ -73,9 +76,7 @@ export const RuleSwitchComponent = ({ if (errors.length > 0) { setMyIsLoading(false); - const title = enabling - ? i18n.BATCH_ACTION_ACTIVATE_SELECTED_ERROR(1) - : i18n.BATCH_ACTION_DEACTIVATE_SELECTED_ERROR(1); + displayErrorToast( title, errors.map((e) => e.error.message), @@ -88,8 +89,9 @@ export const RuleSwitchComponent = ({ onChange(rule.enabled); } } - } catch { + } catch (err) { setMyIsLoading(false); + displayErrorToast(title, err.message, dispatchToaster); } } setMyIsLoading(false); @@ -105,21 +107,22 @@ export const RuleSwitchComponent = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [enabled]); - useEffect(() => { + const showLoader = useMemo((): boolean => { if (myIsLoading !== isLoading) { - setMyIsLoading(isLoading ?? false); + return isLoading ?? false; } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [isLoading]); + + return myIsLoading; + }, [myIsLoading, isLoading]); return ( <EuiFlexGroup alignItems="center" justifyContent="spaceAround"> <EuiFlexItem grow={false}> - {myIsLoading ? ( - <EuiLoadingSpinner size="m" data-test-subj="rule-switch-loader" /> + {showLoader ? ( + <EuiLoadingSpinner size="m" data-test-subj="ruleSwitchLoader" /> ) : ( <StaticSwitch - data-test-subj="rule-switch" + data-test-subj="ruleSwitch" label={optionLabel ?? ''} showLabel={!isEmpty(optionLabel)} disabled={isDisabled} diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts index 081e804cf7356..1875e0cd83cf8 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/patch_rules_bulk_route.ts @@ -183,12 +183,7 @@ export const patchRulesBulkRoute = (router: IRouter, ml: SetupPlugins['ml']) => search: rule.id, searchFields: ['alertId'], }); - return transformValidateBulkError( - rule.id, - rule, - ruleActions, - ruleStatuses.saved_objects[0] - ); + return transformValidateBulkError(rule.id, rule, ruleActions, ruleStatuses); } else { return getIdBulkError({ id, ruleId }); } diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts index 8828bbe6c9826..ddc2ade9b5ac9 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/update_rules_bulk_route.ts @@ -50,7 +50,6 @@ export const updateRulesBulkRoute = (router: IRouter, ml: SetupPlugins['ml']) => if (!siemClient || !alertsClient) { return siemResponse.error({ statusCode: 404 }); } - const mlAuthz = buildMlAuthz({ license: context.licensing.license, ml, request }); const ruleStatusClient = ruleStatusSavedObjectsClientFactory(savedObjectsClient); const rules = await Promise.all( @@ -192,12 +191,7 @@ export const updateRulesBulkRoute = (router: IRouter, ml: SetupPlugins['ml']) => search: rule.id, searchFields: ['alertId'], }); - return transformValidateBulkError( - rule.id, - rule, - ruleActions, - ruleStatuses.saved_objects[0] - ); + return transformValidateBulkError(rule.id, rule, ruleActions, ruleStatuses); } else { return getIdBulkError({ id, ruleId }); } diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.test.ts index 06ec22b2f61b4..6bdbfedf625dd 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.test.ts @@ -9,13 +9,13 @@ import { transformValidateFindAlerts, transformValidateBulkError, } from './validate'; -import { getResult } from '../__mocks__/request_responses'; import { FindResult } from '../../../../../../alerts/server'; import { BulkError } from '../utils'; -import { RulesSchema } from '../../../../../common/detection_engine/schemas/response/rules_schema'; +import { RulesSchema } from '../../../../../common/detection_engine/schemas/response'; +import { getResult, getFindResultStatus } from '../__mocks__/request_responses'; import { getListArrayMock } from '../../../../../common/detection_engine/schemas/types/lists.mock'; -export const ruleOutput: RulesSchema = { +export const ruleOutput = (): RulesSchema => ({ actions: [], author: ['Elastic'], created_at: '2019-12-13T16:40:33.400Z', @@ -80,14 +80,14 @@ export const ruleOutput: RulesSchema = { note: '# Investigative notes', timeline_title: 'some-timeline-title', timeline_id: 'some-timeline-id', -}; +}); describe('validate', () => { describe('transformValidate', () => { test('it should do a validation correctly of a partial alert', () => { const ruleAlert = getResult(); const [validated, errors] = transformValidate(ruleAlert); - expect(validated).toEqual(ruleOutput); + expect(validated).toEqual(ruleOutput()); expect(errors).toEqual(null); }); @@ -103,14 +103,35 @@ describe('validate', () => { describe('transformValidateFindAlerts', () => { test('it should do a validation correctly of a find alert', () => { - const findResult: FindResult = { data: [getResult()], page: 1, perPage: 0, total: 0 }; + const findResult: FindResult = { + data: [getResult()], + page: 1, + perPage: 0, + total: 0, + }; const [validated, errors] = transformValidateFindAlerts(findResult, []); - expect(validated).toEqual({ data: [ruleOutput], page: 1, perPage: 0, total: 0 }); + const expected: { + page: number; + perPage: number; + total: number; + data: Array<Partial<RulesSchema>>; + } | null = { + data: [ruleOutput()], + page: 1, + perPage: 0, + total: 0, + }; + expect(validated).toEqual(expected); expect(errors).toEqual(null); }); test('it should do an in-validation correctly of a partial alert', () => { - const findResult: FindResult = { data: [getResult()], page: 1, perPage: 0, total: 0 }; + const findResult: FindResult = { + data: [getResult()], + page: 1, + perPage: 0, + total: 0, + }; // @ts-expect-error delete findResult.page; const [validated, errors] = transformValidateFindAlerts(findResult, []); @@ -123,7 +144,7 @@ describe('validate', () => { test('it should do a validation correctly of a rule id', () => { const ruleAlert = getResult(); const validatedOrError = transformValidateBulkError('rule-1', ruleAlert); - expect(validatedOrError).toEqual(ruleOutput); + expect(validatedOrError).toEqual(ruleOutput()); }); test('it should do an in-validation correctly of a rule id', () => { @@ -140,5 +161,34 @@ describe('validate', () => { }; expect(validatedOrError).toEqual(expected); }); + + test('it should do a validation correctly of a rule id with ruleStatus passed in', () => { + const ruleStatus = getFindResultStatus(); + const ruleAlert = getResult(); + const validatedOrError = transformValidateBulkError('rule-1', ruleAlert, null, ruleStatus); + const expected: RulesSchema = { + ...ruleOutput(), + status: 'succeeded', + status_date: '2020-02-18T15:26:49.783Z', + last_success_at: '2020-02-18T15:26:49.783Z', + last_success_message: 'succeeded', + }; + expect(validatedOrError).toEqual(expected); + }); + + test('it should return error object if "alert" is not expected alert type', () => { + const ruleAlert = getResult(); + // @ts-expect-error + delete ruleAlert.alertTypeId; + const validatedOrError = transformValidateBulkError('rule-1', ruleAlert); + const expected: BulkError = { + error: { + message: 'Internal error transforming', + status_code: 500, + }, + rule_id: 'rule-1', + }; + expect(validatedOrError).toEqual(expected); + }); }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.ts index 983382b28ab38..27100eaebea15 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/validate.ts @@ -22,6 +22,7 @@ import { isAlertType, IRuleSavedAttributesSavedObjectAttributes, isRuleStatusFindType, + IRuleStatusSOAttributes, } from '../../rules/types'; import { createBulkErrorObject, BulkError } from '../utils'; import { transformFindAlerts, transform, transformAlertToRule } from './utils'; @@ -74,7 +75,7 @@ export const transformValidateBulkError = ( ruleId: string, alert: PartialAlert, ruleActions?: RuleActions | null, - ruleStatus?: unknown + ruleStatus?: SavedObjectsFindResponse<IRuleStatusSOAttributes> ): RulesSchema | BulkError => { if (isAlertType(alert)) { if (isRuleStatusFindType(ruleStatus) && ruleStatus?.saved_objects.length > 0) {