Skip to content

Commit

Permalink
[Security Solution][Detections] Rule Execution Log Feedback and Fixes (
Browse files Browse the repository at this point in the history
…#129003)

## Summary

Addresses feedback and fixes identified in #126215

Feedback addressed includes:
* Adds route validation via io-ts decode and schema tests
* Fixed styling of max execution events error by wrapping text (#129321)
* Fixed types within `view alerts for execution` action onClick
* Caps auto-refresh minimum to `1min` (#129332)
* Adds cardinality aggs to initial `execution_uuid` query to properly report total counts when filtering by status
* Disabled `View Alerts from Execution` action as current UX was too cumbersome with erasing users existing filters

---
Additional follow-ups for another PR:
- [ ] UI Unit tests
- [ ] Finalize API Integration tests for gap remediation events
- [ ] Persist table state (DatePicker/StatusFilter/SortField/SortOrder/Pagination) when navigating to other tabs on the same page
- [ ] Update global DatePicker to daterange of execution for `view alerts for execution` action (and clear all other filters)
- [ ] Support `disabled rule` platform error #126215 (comment)
- [ ] Verify StatusFilter issue #126215 (comment)

---
### Checklist

Delete any items that are not applicable to this PR.

- [X] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [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] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [X] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
  • Loading branch information
spong authored Apr 5, 2022
1 parent e5b8807 commit bc413c6
Show file tree
Hide file tree
Showing 20 changed files with 356 additions and 114 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ export enum RuleExecutionStatus {

export const ruleExecutionStatus = enumeration('RuleExecutionStatus', RuleExecutionStatus);

export type RuleExecutionStatusType = t.TypeOf<typeof ruleExecutionStatus>;

export const ruleExecutionStatusOrder = PositiveInteger;
export type RuleExecutionStatusOrder = t.TypeOf<typeof ruleExecutionStatusOrder>;

Expand Down Expand Up @@ -134,3 +136,14 @@ export const aggregateRuleExecutionEvent = t.type({
});

export type AggregateRuleExecutionEvent = t.TypeOf<typeof aggregateRuleExecutionEvent>;

export const executionLogTableSortColumns = t.keyof({
timestamp: IsoDateString,
duration_ms: t.number,
gap_duration_ms: t.number,
indexing_duration_ms: t.number,
search_duration_ms: t.number,
schedule_delay_ms: t.number,
});

export type ExecutionLogTableSortColumns = t.TypeOf<typeof executionLogTableSortColumns>;
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
/*
* 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 { pipe } from 'fp-ts/lib/pipeable';
import { left } from 'fp-ts/lib/Either';
import { foldLeftRight, getPaths } from '@kbn/securitysolution-io-ts-utils';
import {
DefaultSortField,
DefaultSortOrder,
DefaultStatusFiltersStringArray,
} from './get_rule_execution_events_schema';

describe('get_rule_execution_events_schema', () => {
describe('DefaultStatusFiltersStringArray', () => {
test('it should validate a single ruleExecutionStatus', () => {
const payload = 'succeeded';
const decoded = DefaultStatusFiltersStringArray.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual([payload]);
});
test('it should validate an array of ruleExecutionStatus joined by "\'"', () => {
const payload = ['succeeded', 'failed'];
const decoded = DefaultStatusFiltersStringArray.decode(payload.join(','));
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual(payload);
});

test('it should not validate an invalid ruleExecutionStatus', () => {
const payload = ['value 1', 5].join(',');
const decoded = DefaultStatusFiltersStringArray.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([
'Invalid value "value 1" supplied to "DefaultStatusFiltersStringArray"',
'Invalid value "5" supplied to "DefaultStatusFiltersStringArray"',
]);
expect(message.schema).toEqual({});
});

test('it should return a default array entry', () => {
const payload = null;
const decoded = DefaultStatusFiltersStringArray.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual([]);
});
});
describe('DefaultSortField', () => {
test('it should validate a valid sort field', () => {
const payload = 'duration_ms';
const decoded = DefaultSortField.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual(payload);
});

test('it should not validate an invalid sort field', () => {
const payload = 'es_search_duration_ms';
const decoded = DefaultSortField.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([
'Invalid value "es_search_duration_ms" supplied to "DefaultSortField"',
]);
expect(message.schema).toEqual({});
});

test('it should return the default sort field "timestamp"', () => {
const payload = null;
const decoded = DefaultSortField.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual('timestamp');
});
});
describe('DefaultSortOrder', () => {
test('it should validate a valid sort order', () => {
const payload = 'asc';
const decoded = DefaultSortOrder.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual(payload);
});

test('it should not validate an invalid sort order', () => {
const payload = 'behind_you';
const decoded = DefaultSortOrder.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([
'Invalid value "behind_you" supplied to "DefaultSortOrder"',
]);
expect(message.schema).toEqual({});
});

test('it should return the default sort order "desc"', () => {
const payload = null;
const decoded = DefaultSortOrder.decode(payload);
const message = pipe(decoded, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual('desc');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,96 @@

import * as t from 'io-ts';

import { sortFieldOrUndefined, sortOrderOrUndefined } from '../common';
import { DefaultPerPage, DefaultPage } from '@kbn/securitysolution-io-ts-alerting-types';
import { DefaultEmptyString, IsoDateString } from '@kbn/securitysolution-io-ts-types';

import { Either } from 'fp-ts/lib/Either';
import {
ExecutionLogTableSortColumns,
executionLogTableSortColumns,
ruleExecutionStatus,
RuleExecutionStatusType,
} from '../common';

/**
* Types the DefaultStatusFiltersStringArray as:
* - If undefined, then a default array will be set
* - If an array is sent in, then the array will be validated to ensure all elements are a ruleExecutionStatus
*/
export const DefaultStatusFiltersStringArray = new t.Type<
RuleExecutionStatusType[],
RuleExecutionStatusType[],
unknown
>(
'DefaultStatusFiltersStringArray',
t.array(ruleExecutionStatus).is,
(input, context): Either<t.Errors, RuleExecutionStatusType[]> => {
if (input == null) {
return t.success([]);
} else if (typeof input === 'string') {
return t.array(ruleExecutionStatus).validate(input.split(','), context);
} else {
return t.array(ruleExecutionStatus).validate(input, context);
}
},
t.identity
);

/**
* Types the DefaultSortField as:
* - If undefined, then a default sort field of 'timestamp' will be set
* - If a string is sent in, then the string will be validated to ensure it is as valid sortFields
*/
export const DefaultSortField = new t.Type<
ExecutionLogTableSortColumns,
ExecutionLogTableSortColumns,
unknown
>(
'DefaultSortField',
executionLogTableSortColumns.is,
(input, context): Either<t.Errors, ExecutionLogTableSortColumns> =>
input == null ? t.success('timestamp') : executionLogTableSortColumns.validate(input, context),
t.identity
);

const sortOrder = t.keyof({ asc: null, desc: null });
type SortOrder = t.TypeOf<typeof sortOrder>;

/**
* Types the DefaultSortOrder as:
* - If undefined, then a default sort order of 'desc' will be set
* - If a string is sent in, then the string will be validated to ensure it is as valid sortOrder
*/
export const DefaultSortOrder = new t.Type<SortOrder, SortOrder, unknown>(
'DefaultSortOrder',
sortOrder.is,
(input, context): Either<t.Errors, SortOrder> =>
input == null ? t.success('desc') : sortOrder.validate(input, context),
t.identity
);

/**
* Route Request Params
*/
export const GetRuleExecutionEventsRequestParams = t.exact(
t.type({
ruleId: t.string,
})
);

/**
* Route Query Params (as constructed from the above codecs)
*/
export const GetRuleExecutionEventsQueryParams = t.exact(
t.type({
start: t.string,
end: t.string,
query_text: t.union([t.string, t.undefined]),
status_filters: t.union([t.string, t.undefined]),
per_page: t.union([t.string, t.undefined]),
page: t.union([t.string, t.undefined]),
sort_field: sortFieldOrUndefined,
sort_order: sortOrderOrUndefined,
start: IsoDateString,
end: IsoDateString,
query_text: DefaultEmptyString, // default to "" if not sent in during decode
status_filters: DefaultStatusFiltersStringArray, // defaults to empty array if not sent in during decode
per_page: DefaultPerPage, // defaults to "20" if not sent in during decode
page: DefaultPage, // defaults to "1" if not sent in during decode
sort_field: DefaultSortField, // defaults to "desc" if not sent in during decode
sort_order: DefaultSortOrder, // defaults to "timestamp" if not sent in during decode
})
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,12 @@ BarGroup.displayName = 'BarGroup';

export const BarText = styled.p.attrs({
className: 'siemUtilityBar__text',
})`
${({ theme }) => css`
})<{ shouldWrap: boolean }>`
${({ shouldWrap, theme }) => css`
color: ${theme.eui.euiTextSubduedColor};
font-size: ${theme.eui.euiFontSizeXS};
line-height: ${theme.eui.euiLineHeight};
white-space: nowrap;
white-space: ${shouldWrap ? 'normal' : 'nowrap'};
`}
`;
BarText.displayName = 'BarText';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,15 @@ import { BarText } from './styles';
export interface UtilityBarTextProps {
children: string | JSX.Element;
dataTestSubj?: string;
shouldWrap?: boolean;
}

export const UtilityBarText = React.memo<UtilityBarTextProps>(({ children, dataTestSubj }) => (
<BarText data-test-subj={dataTestSubj}>{children}</BarText>
));
export const UtilityBarText = React.memo<UtilityBarTextProps>(
({ children, dataTestSubj, shouldWrap = false }) => (
<BarText data-test-subj={dataTestSubj} shouldWrap={shouldWrap}>
{children}
</BarText>
)
);

UtilityBarText.displayName = 'UtilityBarText';
Original file line number Diff line number Diff line change
Expand Up @@ -360,8 +360,8 @@ export const fetchRuleExecutionEvents = async ({
query: {
start: startDate?.utc().toISOString(),
end: endDate?.utc().toISOString(),
query_text: queryText?.trim(),
status_filters: statusFilters?.trim(),
query_text: queryText,
status_filters: statusFilters,
page,
per_page: perPage,
sort_field: sortField,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,17 @@ export const useRuleExecutionEvents = ({
return useQuery<GetAggregateRuleExecutionEventsResponse>(
[
'ruleExecutionEvents',
ruleId,
start,
end,
queryText,
statusFilters,
page,
perPage,
sortField,
sortOrder,
{
ruleId,
start,
end,
queryText,
statusFilters,
page,
perPage,
sortField,
sortOrder,
},
],
async ({ signal }) => {
return fetchRuleExecutionEvents({
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,14 @@ import React from 'react';
import { ExecutionLogSearchBar } from './execution_log_search_bar';
import { noop } from 'lodash/fp';

// TODO: Replace snapshot test with base test cases
/**
* NOTE: This component is currently not shown in the UI as custom search queries
* are not yet fully supported by the Rule Execution Log aggregation API since
* certain queries could result in missing data or inclusion of wrong events.
* Please see this comment for history/details: https://github.com/elastic/kibana/pull/127339/files#r825240516
*
* Not expanding test coverage until component is complete/in-use.
*/

describe('ExecutionLogSearchBar', () => {
describe('snapshots', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ interface ExecutionLogTableSearchProps {
onStatusFilterChange: (statusFilters: string[]) => void;
}

/**
* SearchBar + StatusFilters component to be used with the Rule Execution Log table
* NOTE: This component is currently not shown in the UI as custom search queries
* are not yet fully supported by the Rule Execution Log aggregation API since
* certain queries could result in missing data or inclusion of wrong events.
* Please see this comment for history/details: https://github.com/elastic/kibana/pull/127339/files#r825240516
*/
export const ExecutionLogSearchBar = React.memo<ExecutionLogTableSearchProps>(
({ onlyShowFilters, onSearch, onStatusFilterChange }) => {
const [isPopoverOpen, setIsPopoverOpen] = useState(false);
Expand Down
Loading

0 comments on commit bc413c6

Please sign in to comment.