Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Detections][Alerts] fixes saved_query rule validation issue #142602

Merged
merged 10 commits into from
Oct 10, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export const REQUEST_NAMES = {
SECURITY_DASHBOARDS: `${APP_UI_ID} fetch security dashboards`,
ANOMALIES_TABLE: `${APP_UI_ID} fetch anomalies table data`,
GET_RISK_SCORE_DEPRECATED: `${APP_UI_ID} fetch is risk score deprecated`,
GET_SAVED_QUERY: `${APP_UI_ID} fetch saved query`,
ENABLE_RISK_SCORE: `${APP_UI_ID} fetch enable risk score`,
REFRESH_RISK_SCORE: `${APP_UI_ID} fetch refresh risk score`,
UPGRADE_RISK_SCORE: `${APP_UI_ID} fetch upgrade risk score`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export interface QueryBarDefineRuleProps {
* called when fetching of saved query fails
*/
onSavedQueryError?: () => void;
defaultSavedQuery?: SavedQuery | undefined;
}

const actionTimelineToHide: ActionTimelineToShow[] = ['duplicate', 'createFrom'];
Expand All @@ -74,6 +75,7 @@ const savedQueryToFieldValue = (savedQuery: SavedQuery): FieldValueQueryBar => (
});

export const QueryBarDefineRule = ({
defaultSavedQuery,
browserFields,
dataTestSubj,
field,
Expand All @@ -91,7 +93,7 @@ export const QueryBarDefineRule = ({
const { value: fieldValue, setValue: setFieldValue } = field as FieldHook<FieldValueQueryBar>;
const [originalHeight, setOriginalHeight] = useState(-1);
const [loadingTimeline, setLoadingTimeline] = useState(false);
const [savedQuery, setSavedQuery] = useState<SavedQuery | undefined>(undefined);
const [savedQuery, setSavedQuery] = useState<SavedQuery | undefined>(defaultSavedQuery);
const [isSavedQueryFailedToLoad, setIsSavedQueryFailedToLoad] = useState(false);
const { isInvalid, errorMessage } = getFieldValidityAndErrorMessage(field);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { isEqual, isEmpty, omit } from 'lodash';
import type { FieldSpec } from '@kbn/data-views-plugin/common';
import usePrevious from 'react-use/lib/usePrevious';

import type { SavedQuery } from '@kbn/data-plugin/public';
import type { DataViewBase } from '@kbn/es-query';
import { FormattedMessage } from '@kbn/i18n-react';
import { isMlRule } from '../../../../../common/machine_learning/helpers';
Expand Down Expand Up @@ -88,6 +89,7 @@ interface StepDefineRuleProps extends RuleStepProps {
defaultValues: DefineStepRule;
onRuleDataChange?: (data: DefineStepRule) => void;
onPreviewDisabledStateChange?: (isDisabled: boolean) => void;
defaultSavedQuery?: SavedQuery;
}

export const MyLabelButton = styled(EuiButtonEmpty)`
Expand Down Expand Up @@ -124,6 +126,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
threatIndicesConfig,
onRuleDataChange,
onPreviewDisabledStateChange,
defaultSavedQuery,
}) => {
const mlCapabilities = useMlCapabilities();
const [openTimelineSearch, setOpenTimelineSearch] = useState(false);
Expand Down Expand Up @@ -615,6 +618,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
onValidityChange: setIsQueryBarValid,
onCloseTimelineSearch: handleCloseTimelineSearch,
onSavedQueryError: handleSavedQueryError,
defaultSavedQuery,
} as QueryBarDefineRuleProps
}
/>
Expand All @@ -629,6 +633,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
openTimelineSearch,
formShouldLoadQueryDynamically,
handleSavedQueryError,
defaultSavedQuery,
]
);
const onOptionsChange = useCallback((field: FieldsEqlOptions, value: string | undefined) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ import { ExecutionLogTable } from './execution_log_table/execution_log_table';
import * as detectionI18n from '../../translations';
import * as ruleI18n from '../translations';
import { RuleDetailsContextProvider } from './rule_details_context';
import { useGetSavedQuery } from './use_get_saved_query';
import { useGetSavedQuery } from '../use_get_saved_query';
import * as i18n from './translations';
import { NeedAdminForUpdateRulesCallOut } from '../../../../components/callouts/need_admin_for_update_callout';
import { MissingPrivilegesCallOut } from '../../../../components/callouts/missing_privileges_callout';
Expand Down Expand Up @@ -306,11 +306,9 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
const [filterGroup, setFilterGroup] = useState<Status>(FILTER_OPEN);
const [dataViewOptions, setDataViewOptions] = useState<{ [x: string]: DataViewListItem }>({});

// load saved query only if rule type === 'saved_query', as other rule types still can have saved_id property that is not used
// Rule schema allows to save any rule with saved_id property, but it only used for saved_query rule type
// In future we might look in possibility to restrict rule schema (breaking change!) and remove saved_id from the rest of rules through migration
const savedQueryId = rule?.type === 'saved_query' ? rule?.saved_id : undefined;
const { isSavedQueryLoading, savedQueryBar } = useGetSavedQuery(savedQueryId);
const { isSavedQueryLoading, savedQueryBar } = useGetSavedQuery(rule?.saved_id, {
ruleType: rule?.type,
});

const [indicesConfig] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY);
const [threatIndicesConfig] = useUiSetting$<string[]>(DEFAULT_THREAT_INDEX_KEY);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,3 @@ export const DELETED_RULE = i18n.translate(
defaultMessage: 'Deleted rule',
}
);

export const SAVED_QUERY_LOAD_ERROR_TOAST = i18n.translate(
'xpack.securitySolution.hooks.useGetSavedQuery.errorToastMessage',
{
defaultMessage: 'Failed to load the saved query',
}
);

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ jest.mock('react-router-dom', () => {
};
});
jest.mock('../../../../../common/hooks/use_app_toasts');
jest.mock('../use_get_saved_query', () => ({
__esModule: true,
useGetSavedQuery: jest.fn().mockReturnValue({}),
}));

describe('EditRulePage', () => {
let appToastsMock: jest.Mocked<ReturnType<typeof useAppToastsMock.create>>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { FormattedMessage } from '@kbn/i18n-react';
import type { FC } from 'react';
import React, { memo, useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { useParams } from 'react-router-dom';
import { noop } from 'lodash';

import type { DataViewListItem } from '@kbn/data-views-plugin/common';
import type { UpdateRulesSchema } from '../../../../../../common/detection_engine/schemas/request';
Expand Down Expand Up @@ -75,6 +76,7 @@ import { HeaderPage } from '../../../../../common/components/header_page';
import { useStartTransaction } from '../../../../../common/lib/apm/use_start_transaction';
import { SINGLE_RULE_ACTIONS } from '../../../../../common/lib/apm/user_actions';
import { PreviewFlyout } from '../preview';
import { useGetSavedQuery } from '../use_get_saved_query';

const formHookNoop = async (): Promise<undefined> => undefined;

Expand All @@ -98,6 +100,11 @@ const EditRulePageComponent: FC = () => {
const [ruleLoading, rule] = useRule(ruleId);
const loading = ruleLoading || userInfoLoading || listsConfigLoading;

const { isSavedQueryLoading, savedQueryBar, savedQuery } = useGetSavedQuery(rule?.saved_id, {
ruleType: rule?.type,
onError: noop,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onError: noop, reduces straightness of the code since it requires knowledge what's going on under the hood and suppressing of that behaviour when necessary. I'd recommend to consider removing the default error handling functionality inside, exposing of showErrorToastHandler and passing it directly when necessary. Though it uses toasts then it makes sense to expose a hook like useShowErrorToastHandler() which can be composable and reusable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maximpn
Toast is not displayed in this case because error is displayed on edit form when saved query is not loaded. Thus it's prevents showing duplicated notifications

Screenshot 2022-10-10 at 11 58 27

});

const formHooks = useRef<RuleStepsFormHooks>({
[RuleStep.defineRule]: formHookNoop,
[RuleStep.aboutRule]: formHookNoop,
Expand Down Expand Up @@ -195,6 +202,17 @@ const EditRulePageComponent: FC = () => {
const [indicesConfig] = useUiSetting$<string[]>(DEFAULT_INDEX_KEY);
const [threatIndicesConfig] = useUiSetting$<string[]>(DEFAULT_THREAT_INDEX_KEY);

const defineStepDataWithSavedQuery = useMemo(
() =>
defineStep.data
? {
...defineStep.data,
queryBar: savedQueryBar ?? defineStep.data.queryBar,
}
: defineStep.data,
[defineStep.data, savedQueryBar]
);

const tabs = useMemo(
() => [
{
Expand All @@ -205,19 +223,20 @@ const EditRulePageComponent: FC = () => {
content: (
<>
<EuiSpacer />
<StepPanel loading={loading} title={ruleI18n.DEFINITION}>
{defineStep.data != null && (
<StepPanel loading={loading || isSavedQueryLoading} title={ruleI18n.DEFINITION}>
{defineStepDataWithSavedQuery != null && !isSavedQueryLoading && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that defineStepDataWithSavedQuery is either a specific object or undefined but there is a non strict check for null

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non strict check for null eliminates undefined, so the value of defineStepDataWithSavedQuery will be object of DefineStepRule type.

Fairly common pattern across Kibana codebase (example from previous implementation):

            <StepPanel loading={loading} title={ruleI18n.DEFINITION}>
              {defineStep.data != null && (

<StepDefineRule
isReadOnlyView={false}
isLoading={isLoading}
isLoading={isLoading || isSavedQueryLoading}
isUpdateView
defaultValues={defineStep.data}
defaultValues={defineStepDataWithSavedQuery}
setForm={setFormHook}
kibanaDataViews={dataViewOptions}
indicesConfig={indicesConfig}
threatIndicesConfig={threatIndicesConfig}
onRuleDataChange={onDataChange}
onPreviewDisabledStateChange={setIsPreviewDisabled}
defaultSavedQuery={savedQuery}
/>
)}
<EuiSpacer />
Expand Down Expand Up @@ -305,6 +324,8 @@ const EditRulePageComponent: FC = () => {
loading,
defineStep.data,
isLoading,
isSavedQueryLoading,
defineStepDataWithSavedQuery,
setFormHook,
dataViewOptions,
indicesConfig,
Expand All @@ -314,6 +335,7 @@ const EditRulePageComponent: FC = () => {
scheduleStep.data,
actionsStep.data,
actionMessageParams,
savedQuery,
]
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1105,3 +1105,10 @@ export const CANCEL_BUTTON_LABEL = i18n.translate(
defaultMessage: 'Cancel',
}
);

export const SAVED_QUERY_LOAD_ERROR_TOAST = i18n.translate(
'xpack.securitySolution.hooks.useGetSavedQuery.errorToastMessage',
{
defaultMessage: 'Failed to load the saved query',
}
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* 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 { useMemo } from 'react';
import { useQuery } from '@tanstack/react-query';

import type { Type } from '@kbn/securitysolution-io-ts-alerting-types';

import { useSavedQueryServices } from '../../../../common/utils/saved_query_services';
import type { DefineStepRule } from './types';

import { useAppToasts } from '../../../../common/hooks/use_app_toasts';

import { SAVED_QUERY_LOAD_ERROR_TOAST } from './translations';

interface UseGetSavedQuerySettings {
onError?: (e: unknown) => void;
ruleType: Type | undefined;
}

export const useGetSavedQuery = (
savedQueryId: string | undefined,
{ ruleType, onError }: UseGetSavedQuerySettings
) => {
const savedQueryServices = useSavedQueryServices();
const { addError } = useAppToasts();

const defaultErrorHandler = (e: unknown) => {
addError(e, { title: SAVED_QUERY_LOAD_ERROR_TOAST });
};

const query = useQuery(
['detectionEngine', 'rule', 'savedQuery', savedQueryId],
async () => {
if (!savedQueryId) {
return null;
}

return savedQueryServices.getSavedQuery(savedQueryId);
},
{
onError: onError ?? defaultErrorHandler,
// load saved query only if rule type === 'saved_query', as other rule types still can have saved_id property that is not used
// Rule schema allows to save any rule with saved_id property, but it only used for saved_query rule type
// In future we might look in possibility to restrict rule schema (breaking change!) and remove saved_id from the rest of rules through migration
enabled: Boolean(savedQueryId) && ruleType === 'saved_query',
retry: false,
}
);

const savedQueryBar = useMemo<DefineStepRule['queryBar'] | null>(
() =>
query.data
? {
saved_id: query.data.id,
filters: query.data.attributes.filters ?? [],
query: query.data.attributes.query,
title: query.data.attributes.title,
}
: null,
[query.data]
);

return {
isSavedQueryLoading: savedQueryId ? query.isLoading : false,
savedQueryBar,
savedQuery: query.data ?? undefined,
};
};