From b634e15626cd573304696257e4fcaaf82b9e1a70 Mon Sep 17 00:00:00 2001 From: Andrew Goldstein Date: Fri, 20 Aug 2021 18:34:33 -0600 Subject: [PATCH] [RAC] [Security Solution] Hides the `Show top ` action in chart legends ## Summary Fixes , which allowed users to launch a new `Top ` popover from an existing popover, to infinity (and beyond) The `Show top ` action is now hidden in the `Top ` popover's chart legend, and also: - In the legend items of charts throughout the Security Solution (e.g. on the `Overview` page, and in the `Trend` chart on the `Alerts` page) - For items in the `Count` aggregation table on the `Alerts` page ## Screenshots ### Before (Top `signal.rule.name` popover) ![before-top-signal-rule-name](https://user-images.githubusercontent.com/4459398/130302784-00a6c24d-17c8-4361-979e-01b8467f100e.png) _Above: It was possible to launch another `Top ` popover from the legend of an existing popover_ ### After (Top `signal.rule.name` popover) ![after-top-signal-rule-name](https://user-images.githubusercontent.com/4459398/130302925-d5aaa1ff-9565-4374-aa87-bde5880cb64f.png) _Above: It is no longer possible to launch another `Top ` popover from the legend of an existing popover_ ### Before (Chart legends) ![before-overview](https://user-images.githubusercontent.com/4459398/130303169-dc6c6de3-a2ba-40fe-a1f0-fe0d78b9638c.png) _Above: It was possible to launch a `Top ` popover from chart legends_ ### After (Chart legends) ![after-overview](https://user-images.githubusercontent.com/4459398/130303519-2eb0a60e-c6cd-4659-b6b2-d5ba234f668f.png) _Above: It is no longer possible to launch a `Top ` popover from chart legends_ ### Before (`Count` items) ![before-count](https://user-images.githubusercontent.com/4459398/130304111-b37373cf-1afb-41b8-9f38-b5d9b37cdb2d.png) _Above: It was possible to launch a `Top ` popover from `Count` items_ ### After (`Count` items) ![after-count](https://user-images.githubusercontent.com/4459398/130304166-fb641fa2-b52e-44ff-8210-0e228a43330c.png) _Above: It is no longer possible to launch a `Top ` popover from `Count` items_ cc @mdefazio --- .../charts/draggable_legend_item.test.tsx | 6 ++ .../charts/draggable_legend_item.tsx | 1 + .../drag_and_drop/draggable_wrapper.tsx | 6 ++ .../components/drag_and_drop/helpers.test.ts | 17 ++++ .../components/drag_and_drop/helpers.ts | 6 ++ .../__snapshots__/index.test.tsx.snap | 1 + .../common/components/draggables/index.tsx | 4 + .../common/components/hover_actions/index.tsx | 7 +- .../use_hover_action_items.test.tsx | 18 ++++ .../hover_actions/use_hover_action_items.tsx | 6 +- .../hover_actions/use_hover_actions.tsx | 4 + .../common/components/hover_actions/utils.ts | 97 ------------------- .../lib/cell_actions/default_cell_actions.tsx | 1 + .../alerts_count_panel/alerts_count.tsx | 1 + .../field_renderers.test.tsx.snap | 2 + 15 files changed, 78 insertions(+), 99 deletions(-) diff --git a/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.test.tsx b/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.test.tsx index cc272e568bce70..de4d348bfb8f5f 100644 --- a/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.test.tsx +++ b/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.test.tsx @@ -54,4 +54,10 @@ describe('DraggableLegendItem', () => { wrapper.find(`[data-test-subj="legend-item-${legendItem.dataProviderId}"]`).first().text() ).toEqual(legendItem.value); }); + + it('always hides the Top N action for legend items', () => { + expect( + wrapper.find(`[data-test-subj="legend-item-${legendItem.dataProviderId}"]`).prop('hideTopN') + ).toEqual(true); + }); }); diff --git a/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.tsx b/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.tsx index b4b12437f8660b..0cf580db672373 100644 --- a/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.tsx +++ b/x-pack/plugins/security_solution/public/common/components/charts/draggable_legend_item.tsx @@ -36,6 +36,7 @@ const DraggableLegendItemComponent: React.FC<{ = ({ dataProvider, + hideTopN = false, onFilterAdded, render, timelineId, @@ -147,6 +149,7 @@ const DraggableOnWrapperComponent: React.FC = ({ showTopN, } = useHoverActions({ dataProvider, + hideTopN, onFilterAdded, render, timelineId, @@ -304,6 +307,7 @@ const DraggableOnWrapperComponent: React.FC = ({ const DraggableWrapperComponent: React.FC = ({ dataProvider, + hideTopN = false, isDraggable = false, onFilterAdded, render, @@ -319,6 +323,7 @@ const DraggableWrapperComponent: React.FC = ({ showTopN, } = useHoverActions({ dataProvider, + hideTopN, isDraggable, onFilterAdded, render, @@ -363,6 +368,7 @@ const DraggableWrapperComponent: React.FC = ({ return ( { allowTopN({ browserField: aggregatableAllowedType, fieldName: aggregatableAllowedType.name, + hideTopN: false, }) ).toBe(true); }); @@ -664,6 +665,7 @@ describe('helpers', () => { allowTopN({ browserField: undefined, fieldName: 'signal.rule.name', + hideTopN: false, }) ).toBe(true); }); @@ -678,6 +680,7 @@ describe('helpers', () => { allowTopN({ browserField: nonAggregatableAllowedType, fieldName: nonAggregatableAllowedType.name, + hideTopN: false, }) ).toBe(false); }); @@ -692,6 +695,7 @@ describe('helpers', () => { allowTopN({ browserField: aggregatableNotAllowedType, fieldName: aggregatableNotAllowedType.name, + hideTopN: false, }) ).toBe(false); }); @@ -703,6 +707,7 @@ describe('helpers', () => { allowTopN({ browserField: missingAggregatable, fieldName: missingAggregatable.name, + hideTopN: false, }) ).toBe(false); }); @@ -714,6 +719,7 @@ describe('helpers', () => { allowTopN({ browserField: missingType, fieldName: missingType.name, + hideTopN: false, }) ).toBe(false); }); @@ -723,6 +729,17 @@ describe('helpers', () => { allowTopN({ browserField: undefined, fieldName: 'non-allowlisted', + hideTopN: false, + }) + ).toBe(false); + }); + + test('it returns false when hideTopN is true', () => { + expect( + allowTopN({ + browserField: aggregatableAllowedType, + fieldName: aggregatableAllowedType.name, + hideTopN: true, // <-- the Top N action shall not be shown for this (otherwise valid) field }) ).toBe(false); }); diff --git a/x-pack/plugins/security_solution/public/common/components/drag_and_drop/helpers.ts b/x-pack/plugins/security_solution/public/common/components/drag_and_drop/helpers.ts index 9717e1e1eda911..bca6c15d861408 100644 --- a/x-pack/plugins/security_solution/public/common/components/drag_and_drop/helpers.ts +++ b/x-pack/plugins/security_solution/public/common/components/drag_and_drop/helpers.ts @@ -92,9 +92,11 @@ export const addProviderToTimeline = ({ export const allowTopN = ({ browserField, fieldName, + hideTopN, }: { browserField: Partial | undefined; fieldName: string; + hideTopN: boolean; }): boolean => { const isAggregatable = browserField?.aggregatable ?? false; const fieldType = browserField?.type ?? ''; @@ -181,5 +183,9 @@ export const allowTopN = ({ 'signal.status', ].includes(fieldName); + if (hideTopN) { + return false; + } + return isAllowlistedNonBrowserField || (isAggregatable && isAllowedType); }; diff --git a/x-pack/plugins/security_solution/public/common/components/draggables/__snapshots__/index.test.tsx.snap b/x-pack/plugins/security_solution/public/common/components/draggables/__snapshots__/index.test.tsx.snap index 6b27cf5969f1aa..3cbb0d27a0e2f6 100644 --- a/x-pack/plugins/security_solution/public/common/components/draggables/__snapshots__/index.test.tsx.snap +++ b/x-pack/plugins/security_solution/public/common/components/draggables/__snapshots__/index.test.tsx.snap @@ -36,6 +36,7 @@ exports[`draggables rendering it renders the default DefaultDraggable 1`] = ` }, } } + hideTopN={false} isDraggable={true} render={[Function]} /> diff --git a/x-pack/plugins/security_solution/public/common/components/draggables/index.tsx b/x-pack/plugins/security_solution/public/common/components/draggables/index.tsx index 6ac1746d77709b..e33a8e42e6a397 100644 --- a/x-pack/plugins/security_solution/public/common/components/draggables/index.tsx +++ b/x-pack/plugins/security_solution/public/common/components/draggables/index.tsx @@ -19,6 +19,7 @@ import { import { Provider } from '../../../timelines/components/timeline/data_providers/provider'; export interface DefaultDraggableType { + hideTopN?: boolean; id: string; isDraggable?: boolean; field: string; @@ -88,9 +89,11 @@ Content.displayName = 'Content'; * @param tooltipContent - defaults to displaying `field`, pass `null` to * prevent a tooltip from being displayed, or pass arbitrary content * @param queryValue - defaults to `value`, this query overrides the `queryMatch.value` used by the `DataProvider` that represents the data + * @param hideTopN - defaults to `false`, when true, the option to aggregate this field will be hidden */ export const DefaultDraggable = React.memo( ({ + hideTopN = false, id, isDraggable = true, field, @@ -137,6 +140,7 @@ export const DefaultDraggable = React.memo( return ( ` - min-width: 138px; + min-width: ${({ $hideTopN }) => `${$hideTopN ? '112px' : '138px'}`}; padding: ${(props) => `0 ${props.theme.eui.paddingSizes.s}`}; display: flex; @@ -91,6 +92,7 @@ interface Props { enableOverflowButton?: boolean; field: string; goGetTimelineId?: (args: boolean) => void; + hideTopN?: boolean; isObjectArray: boolean; onFilterAdded?: () => void; ownFocus: boolean; @@ -129,6 +131,7 @@ export const HoverActions: React.FC = React.memo( field, goGetTimelineId, isObjectArray, + hideTopN = false, onFilterAdded, ownFocus, showOwnFocus = true, @@ -207,6 +210,7 @@ export const HoverActions: React.FC = React.memo( enableOverflowButton, field, handleHoverActionClicked, + hideTopN, isObjectArray, isOverflowPopoverOpen, onFilterAdded, @@ -231,6 +235,7 @@ export const HoverActions: React.FC = React.memo( onKeyDown={onKeyDown} $showTopN={showTopN} $showOwnFocus={showOwnFocus} + $hideTopN={hideTopN} $isActive={isActive} className={isActive ? 'hoverActions-active' : ''} > diff --git a/x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.test.tsx b/x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.test.tsx index 2ef72571cf3075..b70d520f142190 100644 --- a/x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.test.tsx +++ b/x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.test.tsx @@ -22,6 +22,7 @@ describe('useHoverActionItems', () => { defaultFocusedButtonRef: null, field: 'signal.rule.name', handleHoverActionClicked: jest.fn(), + hideTopN: false, isObjectArray: true, ownFocus: false, showTopN: false, @@ -112,4 +113,21 @@ describe('useHoverActionItems', () => { ); }); }); + + test('it should hide the Top N action when hideTopN is true', async () => { + await act(async () => { + const { result, waitForNextUpdate } = renderHook(() => { + const testProps = { + ...defaultProps, + hideTopN: true, // <-- hide the Top N action + }; + return useHoverActionItems(testProps); + }); + await waitForNextUpdate(); + + result.current.allActionItems.forEach((item) => { + expect(item.props['data-test-subj']).not.toEqual('hover-actions-show-top-n'); + }); + }); + }); }); diff --git a/x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.tsx b/x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.tsx index a7e4a528ca1b8d..ad91e1f56d4755 100644 --- a/x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.tsx +++ b/x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.tsx @@ -13,7 +13,7 @@ import { isEmpty } from 'lodash'; import { useKibana } from '../../lib/kibana'; import { getAllFieldsByName } from '../../containers/source'; -import { allowTopN } from './utils'; +import { allowTopN } from '../drag_and_drop/helpers'; import { useDeepEqualSelector } from '../../hooks/use_selector'; import { ColumnHeaderOptions, DataProvider, TimelineId } from '../../../../common/types/timeline'; import { SourcererScopeName } from '../../store/sourcerer/model'; @@ -29,6 +29,7 @@ export interface UseHoverActionItemsProps { enableOverflowButton?: boolean; field: string; handleHoverActionClicked: () => void; + hideTopN: boolean; isObjectArray: boolean; isOverflowPopoverOpen?: boolean; itemsToShow?: number; @@ -56,6 +57,7 @@ export const useHoverActionItems = ({ enableOverflowButton, field, handleHoverActionClicked, + hideTopN, isObjectArray, isOverflowPopoverOpen, itemsToShow = 2, @@ -182,6 +184,7 @@ export const useHoverActionItems = ({ allowTopN({ browserField: getAllFieldsByName(browserFields)[field], fieldName: field, + hideTopN, }) ? ( | undefined; - fieldName: string; -}): boolean => { - const isAggregatable = browserField?.aggregatable ?? false; - const fieldType = browserField?.type ?? ''; - const isAllowedType = [ - 'boolean', - 'geo-point', - 'geo-shape', - 'ip', - 'keyword', - 'number', - 'numeric', - 'string', - ].includes(fieldType); - - // TODO: remove this explicit allowlist when the ECS documentation includes alerts - const isAllowlistedNonBrowserField = [ - 'signal.ancestors.depth', - 'signal.ancestors.id', - 'signal.ancestors.rule', - 'signal.ancestors.type', - 'signal.original_event.action', - 'signal.original_event.category', - 'signal.original_event.code', - 'signal.original_event.created', - 'signal.original_event.dataset', - 'signal.original_event.duration', - 'signal.original_event.end', - 'signal.original_event.hash', - 'signal.original_event.id', - 'signal.original_event.kind', - 'signal.original_event.module', - 'signal.original_event.original', - 'signal.original_event.outcome', - 'signal.original_event.provider', - 'signal.original_event.risk_score', - 'signal.original_event.risk_score_norm', - 'signal.original_event.sequence', - 'signal.original_event.severity', - 'signal.original_event.start', - 'signal.original_event.timezone', - 'signal.original_event.type', - 'signal.original_time', - 'signal.parent.depth', - 'signal.parent.id', - 'signal.parent.index', - 'signal.parent.rule', - 'signal.parent.type', - 'signal.rule.created_by', - 'signal.rule.description', - 'signal.rule.enabled', - 'signal.rule.false_positives', - 'signal.rule.filters', - 'signal.rule.from', - 'signal.rule.id', - 'signal.rule.immutable', - 'signal.rule.index', - 'signal.rule.interval', - 'signal.rule.language', - 'signal.rule.max_signals', - 'signal.rule.name', - 'signal.rule.note', - 'signal.rule.output_index', - 'signal.rule.query', - 'signal.rule.references', - 'signal.rule.risk_score', - 'signal.rule.rule_id', - 'signal.rule.saved_id', - 'signal.rule.severity', - 'signal.rule.size', - 'signal.rule.tags', - 'signal.rule.threat', - 'signal.rule.threat.tactic.id', - 'signal.rule.threat.tactic.name', - 'signal.rule.threat.tactic.reference', - 'signal.rule.threat.technique.id', - 'signal.rule.threat.technique.name', - 'signal.rule.threat.technique.reference', - 'signal.rule.timeline_id', - 'signal.rule.timeline_title', - 'signal.rule.to', - 'signal.rule.type', - 'signal.rule.updated_by', - 'signal.rule.version', - 'signal.status', - ].includes(fieldName); - - return isAllowlistedNonBrowserField || (isAggregatable && isAllowedType); -}; diff --git a/x-pack/plugins/security_solution/public/common/lib/cell_actions/default_cell_actions.tsx b/x-pack/plugins/security_solution/public/common/lib/cell_actions/default_cell_actions.tsx index 085b2098cde352..745c7d5a2e9b03 100644 --- a/x-pack/plugins/security_solution/public/common/lib/cell_actions/default_cell_actions.tsx +++ b/x-pack/plugins/security_solution/public/common/lib/cell_actions/default_cell_actions.tsx @@ -137,6 +137,7 @@ export const defaultCellActions: TGridCellAction[] = [ {allowTopN({ browserField: getAllFieldsByName(browserFields)[columnId], fieldName: columnId, + hideTopN: false, }) && ( diff --git a/x-pack/plugins/security_solution/public/timelines/components/field_renderers/__snapshots__/field_renderers.test.tsx.snap b/x-pack/plugins/security_solution/public/timelines/components/field_renderers/__snapshots__/field_renderers.test.tsx.snap index 496877c8279159..b1c2652d278ee8 100644 --- a/x-pack/plugins/security_solution/public/timelines/components/field_renderers/__snapshots__/field_renderers.test.tsx.snap +++ b/x-pack/plugins/security_solution/public/timelines/components/field_renderers/__snapshots__/field_renderers.test.tsx.snap @@ -60,6 +60,7 @@ exports[`Field Renderers #hostIdRenderer it renders correctly against snapshot 1 }, } } + hideTopN={false} isDraggable={false} render={[Function]} /> @@ -82,6 +83,7 @@ exports[`Field Renderers #hostNameRenderer it renders correctly against snapshot }, } } + hideTopN={false} isDraggable={false} render={[Function]} />