Skip to content

Commit

Permalink
[SecuritySolution] Fix building block alert highlighting (elastic#155497
Browse files Browse the repository at this point in the history
)

## Summary

As described in elastic#152318, we
noticed that building block alerts were not highlighted anymore after
the migration to the new alerts table.

A preferred implementation of building block alert highlighting would
follow the [`EUIDataGrid` approach of row
highlighting](https://eui.elastic.co/#/tabular-content/data-grid-style-display#grid-row-classes).
The `DataGrid` allows you to pass custom CSS class names for each row
(`gridStyle.rowClasses`). That would allow us to highlight table rows
with building block alerts.

However, without access to the underlying data, we would not be able
generate the correct `rowClasses` for rows with building block alerts.
So simply passing `gridStyle.rowClasses` to the `AlertsStateTable` was
not an option.

Therefore in this PR we're introducing a new prop on the `AlertsTable`,
the `highlightedRowMapper`. It's a callback function that receives the
alert data and when it returns true, that row will be highlighted.

This allows for highlighting of rows from the outside without exposing
too many details about the underlying data structures.

**Screenshot of the alerts table with a highlightedRowMapper that
highlights building block alerts**

<img width="1259" alt="Screenshot 2023-04-21 at 13 03 54"
src="https://user-images.githubusercontent.com/68591/233620704-a56204c0-e285-4289-897a-58481f440446.png">

### Additional notes

- Since the alerts table has default grid styles, it allows to pass
`gridStyle` and it computes its own `rowClasses` for "active row"
highlighting, the logic for merging all those styles looks intimidating.
I tried my best to comment that part of code to make it clear why the
merges are necessary and how they work.
- While working on the issue, I noticed that active rows are not
highlighted anymore (related bug:
elastic#155487). The changes in this PR
fix that behaviour as well as you can see in the screenshot below:

<img width="936" alt="Screenshot 2023-04-21 at 13 04 15"
src="https://user-images.githubusercontent.com/68591/233620752-d752dada-9c97-4f00-933a-5425e19a5793.png">

### 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

---------

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit ce71264)
  • Loading branch information
janmonschke committed Apr 28, 2023
1 parent e2ebd66 commit dfe6897
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import { getBuildingBlockRule } from '../../objects/rule';
import { OVERVIEW_ALERTS_HISTOGRAM_EMPTY } from '../../screens/overview';
import { HIGHLIGHTED_ROWS_IN_TABLE } from '../../screens/rule_details';
import { OVERVIEW } from '../../screens/security_header';
import { goToRuleDetails } from '../../tasks/alerts_detection_rules';
import { createRule } from '../../tasks/api_calls/rules';
Expand Down Expand Up @@ -40,6 +41,9 @@ describe('Alerts generated by building block rules', () => {
// Check that generated events are visible on the Details page
waitForAlertsToPopulate(EXPECTED_NUMBER_OF_ALERTS);

// Make sure rows are highlighted
cy.get(HIGHLIGHTED_ROWS_IN_TABLE).should('exist');

navigateFromHeaderTo(OVERVIEW);

// Check that generated events are hidden on the Overview page
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,6 @@ export const THREAT_TECHNIQUE = '[data-test-subj="threatTechniqueLink"]';
export const THREAT_SUBTECHNIQUE = '[data-test-subj="threatSubtechniqueLink"]';

export const BACK_TO_RULES_TABLE = '[data-test-subj="breadcrumb"][title="Rules"]';

export const HIGHLIGHTED_ROWS_IN_TABLE =
'[data-test-subj="euiDataGridBody"] .alertsTableHighlightedRow';
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import type { FC } from 'react';
import React, { useRef, useEffect, useState, useCallback, useMemo } from 'react';
import { Storage } from '@kbn/kibana-utils-plugin/public';
import type { AlertsTableStateProps } from '@kbn/triggers-actions-ui-plugin/public/application/sections/alerts_table/alerts_table_state';
import type { Alert } from '@kbn/triggers-actions-ui-plugin/public/types';
import { ALERT_BUILDING_BLOCK_TYPE } from '@kbn/rule-data-utils';
import styled from 'styled-components';
import { useDispatch, useSelector } from 'react-redux';
import { getEsQueryConfig } from '@kbn/data-plugin/public';
Expand Down Expand Up @@ -49,6 +51,9 @@ import * as i18n from './translations';

const { updateIsLoading, updateTotalCount } = dataTableActions;

// Highlight rows with building block alerts
const shouldHighlightRow = (alert: Alert) => !!alert[ALERT_BUILDING_BLOCK_TYPE];

const storage = new Storage(localStorage);

interface GridContainerProps {
Expand Down Expand Up @@ -255,6 +260,7 @@ export const AlertsTableComponent: FC<DetectionEngineAlertTableProps> = ({
query: finalBoolQuery,
showExpandToDetails: false,
gridStyle,
shouldHighlightRow,
rowHeightsOptions,
columns: finalColumns,
browserFields: finalBrowserFields,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
.alertsTableHighlightedRow {
background-color: $euiColorHighlight;
}

.alertsTableActiveRow {
background-color: tintOrShade($euiColorLightShade, 50%, 0);
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,8 @@ import { SystemCellId } from './types';
import { SystemCellFactory, systemCells } from './cells';
import { triggersActionsUiQueriesKeys } from '../../hooks/constants';

export const ACTIVE_ROW_CLASS = 'alertsTableActiveRow';

const AlertsFlyout = lazy(() => import('./alerts_flyout'));
const GridStyles: EuiDataGridStyle = {
const DefaultGridStyle: EuiDataGridStyle = {
border: 'none',
header: 'underline',
fontSize: 's',
Expand All @@ -63,7 +61,9 @@ const isSystemCell = (columnId: string): columnId is SystemCellId => {

const AlertsTable: React.FunctionComponent<AlertsTableProps> = (props: AlertsTableProps) => {
const dataGridRef = useRef<EuiDataGridRefProps>(null);
const [_, setRowClasses] = useState<EuiDataGridStyle['rowClasses']>({});
const [activeRowClasses, setActiveRowClasses] = useState<
NonNullable<EuiDataGridStyle['rowClasses']>
>({});
const alertsData = props.useFetchAlertsData();
const {
activePage,
Expand Down Expand Up @@ -286,11 +286,30 @@ const AlertsTable: React.FunctionComponent<AlertsTableProps> = (props: AlertsTab
useEffect(() => {
// Row classes do not deal with visible row indices, so we need to handle page offset
const rowIndex = flyoutAlertIndex + pagination.pageIndex * pagination.pageSize;
setRowClasses({
[rowIndex]: ACTIVE_ROW_CLASS,
setActiveRowClasses({
[rowIndex]: 'alertsTableActiveRow',
});
}, [flyoutAlertIndex, pagination.pageIndex, pagination.pageSize]);

// Update highlighted rows when alerts or pagination changes
const highlightedRowClasses = useMemo(() => {
let mappedRowClasses: EuiDataGridStyle['rowClasses'] = {};
const shouldHighlightRowCheck = props.shouldHighlightRow;
if (shouldHighlightRowCheck) {
mappedRowClasses = alerts.reduce<NonNullable<EuiDataGridStyle['rowClasses']>>(
(rowClasses, alert, index) => {
if (shouldHighlightRowCheck(alert)) {
rowClasses[index + pagination.pageIndex * pagination.pageSize] =
'alertsTableHighlightedRow';
}
return rowClasses;
},
{}
);
}
return mappedRowClasses;
}, [props.shouldHighlightRow, alerts, pagination.pageIndex, pagination.pageSize]);

const handleFlyoutClose = useCallback(() => setFlyoutAlertIndex(-1), [setFlyoutAlertIndex]);

const renderCellValue = useCallback(
Expand Down Expand Up @@ -377,6 +396,47 @@ const AlertsTable: React.FunctionComponent<AlertsTableProps> = (props: AlertsTab
return props.columns;
}, [getCellActions, disabledCellActions, props.columns, visibleCellActions]);

// Merges the default grid style with the grid style that comes in through props.
const actualGridStyle = useMemo(() => {
const propGridStyle: NonNullable<EuiDataGridStyle> = props.gridStyle ?? {};
// Merges default row classes, custom ones and adds the active row class style
const mergedGridStyle: EuiDataGridStyle = {
...DefaultGridStyle,
...propGridStyle,
rowClasses: {
// We're spreadind the highlighted row classes first, so that the active
// row classed can override the highlighted row classes.
...highlightedRowClasses,
...activeRowClasses,
},
};

// If ANY additional rowClasses have been provided, we need to merge them with our internal ones
if (propGridStyle.rowClasses) {
// Get all row indices with a rowClass.
const mergedKeys = [
...Object.keys(mergedGridStyle.rowClasses || {}),
...Object.keys(propGridStyle.rowClasses || {}),
];
// Deduplicate keys to avoid extra iterations
const dedupedKeys = Array.from(new Set(mergedKeys));

// For each index, merge row classes
const mergedRowClasses = dedupedKeys.reduce<NonNullable<EuiDataGridStyle['rowClasses']>>(
(rowClasses, key) => {
const intKey = parseInt(key, 10);
// Use internal row classes over custom row classes.
rowClasses[intKey] =
mergedGridStyle.rowClasses?.[intKey] || propGridStyle.rowClasses?.[intKey] || '';
return rowClasses;
},
{}
);
mergedGridStyle.rowClasses = mergedRowClasses;
}
return mergedGridStyle;
}, [activeRowClasses, highlightedRowClasses, props.gridStyle]);

return (
<InspectButtonContainer>
<section style={{ width: '100%' }} data-test-subj={props['data-test-subj']}>
Expand Down Expand Up @@ -404,7 +464,7 @@ const AlertsTable: React.FunctionComponent<AlertsTableProps> = (props: AlertsTab
leadingControlColumns={leadingControlColumns}
rowCount={alertsCount}
renderCellValue={handleRenderCellValue}
gridStyle={{ ...GridStyles, ...(props.gridStyle ?? {}) }}
gridStyle={actualGridStyle}
sorting={{ columns: sortingColumns, onSort }}
toolbarVisibility={toolbarVisibility}
pagination={{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ export type AlertsTableStateProps = {
onUpdate?: (args: TableUpdateHandlerArgs) => void;
showAlertStatusWithFlapping?: boolean;
toolbarVisibility?: EuiDataGridToolBarVisibilityOptions;
/**
* Allows to consumers of the table to decide to highlight a row based on the current alert.
*/
shouldHighlightRow?: (alert: Alert) => boolean;
} & Partial<EuiDataGridProps>;

export interface AlertsTableStorage {
Expand Down Expand Up @@ -138,6 +142,7 @@ const AlertsTableStateWithQueryProvider = ({
onUpdate,
showAlertStatusWithFlapping,
toolbarVisibility,
shouldHighlightRow,
}: AlertsTableStateProps) => {
const { cases: casesService } = useKibana<{ cases?: CasesService }>().services;

Expand Down Expand Up @@ -353,6 +358,7 @@ const AlertsTableStateWithQueryProvider = ({
controls: persistentControls,
showInspectButton,
toolbarVisibility,
shouldHighlightRow,
}),
[
alertsTableConfiguration,
Expand Down Expand Up @@ -380,6 +386,7 @@ const AlertsTableStateWithQueryProvider = ({
persistentControls,
showInspectButton,
toolbarVisibility,
shouldHighlightRow,
]
);

Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/triggers_actions_ui/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,10 @@ export type AlertsTableProps = {
controls?: EuiDataGridToolBarAdditionalControlsOptions;
showInspectButton?: boolean;
toolbarVisibility?: EuiDataGridToolBarVisibilityOptions;
/**
* Allows to consumers of the table to decide to highlight a row based on the current alert.
*/
shouldHighlightRow?: (alert: Alert) => boolean;
} & Partial<Pick<EuiDataGridProps, 'gridStyle' | 'rowHeightsOptions'>>;

// TODO We need to create generic type between our plugin, right now we have different one because of the old alerts table
Expand Down

0 comments on commit dfe6897

Please sign in to comment.