From 67378b93fe9f6709e8960338a3d976553618f206 Mon Sep 17 00:00:00 2001 From: Frank Hassanabad Date: Fri, 15 Oct 2021 16:00:12 -0600 Subject: [PATCH 1/7] Fixes Cypress flake cypress test (#115270) ## Summary Fixes flake cypress test Fixes https://github.com/elastic/kibana/pull/115245 See also: https://github.com/elastic/kibana/pull/114075, https://github.com/elastic/kibana/pull/115245 ### 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 --- x-pack/plugins/security_solution/cypress/screens/alerts.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/security_solution/cypress/screens/alerts.ts b/x-pack/plugins/security_solution/cypress/screens/alerts.ts index 0a815705f5b21..c9660668f488b 100644 --- a/x-pack/plugins/security_solution/cypress/screens/alerts.ts +++ b/x-pack/plugins/security_solution/cypress/screens/alerts.ts @@ -58,7 +58,7 @@ export const TAKE_ACTION_POPOVER_BTN = '[data-test-subj="selectedShowBulkActions export const TIMELINE_CONTEXT_MENU_BTN = '[data-test-subj="timeline-context-menu-button"]'; -export const ATTACH_ALERT_TO_CASE_BUTTON = '[data-test-subj="attach-alert-to-case-button"]'; +export const ATTACH_ALERT_TO_CASE_BUTTON = '[data-test-subj="add-existing-case-menu-item"]'; export const ALERT_COUNT_TABLE_FIRST_ROW_COUNT = '[data-test-subj="alertsCountTable"] tr:nth-child(1) td:nth-child(2) .euiTableCellContent__text'; From d29aad4357bd4919e3b48f841daabb318e663194 Mon Sep 17 00:00:00 2001 From: Tyler Smalley Date: Fri, 15 Oct 2021 15:05:37 -0700 Subject: [PATCH 2/7] [build] Dockerfile update (#115237) Signed-off-by: Tyler Smalley --- .../os_packages/docker_generator/templates/base/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dev/build/tasks/os_packages/docker_generator/templates/base/Dockerfile b/src/dev/build/tasks/os_packages/docker_generator/templates/base/Dockerfile index 078741a0d0f6c..b1d9fafffab57 100644 --- a/src/dev/build/tasks/os_packages/docker_generator/templates/base/Dockerfile +++ b/src/dev/build/tasks/os_packages/docker_generator/templates/base/Dockerfile @@ -110,7 +110,7 @@ COPY --chown=1000:0 config/kibana.yml /usr/share/kibana/config/kibana.yml # Add the launcher/wrapper script. It knows how to interpret environment # variables and translate them to Kibana CLI options. -COPY --chown=1000:0 bin/kibana-docker /usr/local/bin/ +COPY bin/kibana-docker /usr/local/bin/ # Ensure gid 0 write permissions for OpenShift. RUN chmod g+ws /usr/share/kibana && \ From 55235c61e5a75e6cb2dc4c65c265a59873957e6b Mon Sep 17 00:00:00 2001 From: Frank Hassanabad Date: Fri, 15 Oct 2021 18:37:00 -0600 Subject: [PATCH 3/7] [Security Solutions] Fixes the newer notification system throttle resets and enabling immediate execution on first detection of a signal (#114214) ## Summary Fixes: * Resets happening by adding the throttle to the else switches and error catching. We have to call throttle on every rule execution or we will cause a reset. * Fixes a case where we were not firing the signal immediately by pushing down the alerts detected. This can cause a reset or a delay of MTTD. * Adds unit tests for the conditions * Changes some of the logic to clean things up. ### 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 --- ...dule_throttle_notification_actions.test.ts | 422 +++++++++++++++++- .../schedule_throttle_notification_actions.ts | 97 +++- .../notifications/utils.test.ts | 388 +++++++++++++++- .../detection_engine/notifications/utils.ts | 53 +++ .../create_security_rule_type_wrapper.ts | 45 +- .../signals/signal_rule_alert_type.test.ts | 27 +- .../signals/signal_rule_alert_type.ts | 45 +- 7 files changed, 1035 insertions(+), 42 deletions(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts index 2e5e331b71b00..81f229c636bd8 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { elasticsearchServiceMock } from 'src/core/server/mocks'; +import { elasticsearchServiceMock, loggingSystemMock } from 'src/core/server/mocks'; import { alertsMock } from '../../../../../alerting/server/mocks'; import { scheduleThrottledNotificationActions } from './schedule_throttle_notification_actions'; import { @@ -19,8 +19,10 @@ jest.mock('./schedule_notification_actions', () => ({ describe('schedule_throttle_notification_actions', () => { let notificationRuleParams: NotificationRuleTypeParams; + let logger: ReturnType; beforeEach(() => { + logger = loggingSystemMock.createLogger(); (scheduleNotificationActions as jest.Mock).mockReset(); notificationRuleParams = { author: ['123'], @@ -82,6 +84,38 @@ describe('schedule_throttle_notification_actions', () => { ), alertInstance: alertsMock.createAlertInstanceFactory(), notificationRuleParams, + logger, + signals: [], + }); + + expect(scheduleNotificationActions as jest.Mock).toHaveBeenCalled(); + }); + + it('should call "scheduleNotificationActions" if the signals length is 1 or greater', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [], + total: 0, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _id: '123', + index: '123', + }, + ], }); expect(scheduleNotificationActions as jest.Mock).toHaveBeenCalled(); @@ -105,6 +139,8 @@ describe('schedule_throttle_notification_actions', () => { ), alertInstance: alertsMock.createAlertInstanceFactory(), notificationRuleParams, + logger, + signals: [], }); expect(scheduleNotificationActions as jest.Mock).not.toHaveBeenCalled(); @@ -132,6 +168,8 @@ describe('schedule_throttle_notification_actions', () => { ), alertInstance: alertsMock.createAlertInstanceFactory(), notificationRuleParams, + logger, + signals: [], }); expect(scheduleNotificationActions as jest.Mock).not.toHaveBeenCalled(); @@ -161,6 +199,8 @@ describe('schedule_throttle_notification_actions', () => { ), alertInstance: alertsMock.createAlertInstanceFactory(), notificationRuleParams, + logger, + signals: [], }); expect((scheduleNotificationActions as jest.Mock).mock.calls[0][0].resultsLink).toMatch( @@ -174,4 +214,384 @@ describe('schedule_throttle_notification_actions', () => { }) ); }); + + it('should log debug information when passing through in expected format and no error messages', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _source: {}, + }, + ], + total: 1, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [], + }); + // We only test the first part since it has date math using math + expect(logger.debug.mock.calls[0][0]).toMatch( + /The notification throttle resultsLink created is/ + ); + expect(logger.debug.mock.calls[1][0]).toEqual( + 'The notification throttle query result size before deconflicting duplicates is: 1. The notification throttle passed in signals size before deconflicting duplicates is: 0. The deconflicted size and size of the signals sent into throttle notification is: 1. The signals count from results size is: 1. The final signals count being sent to the notification is: 1.' + ); + // error should not have been called in this case. + expect(logger.error).not.toHaveBeenCalled(); + }); + + it('should log error information if "throttle" is an invalid string', async () => { + await scheduleThrottledNotificationActions({ + throttle: 'invalid', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _source: {}, + }, + ], + total: 1, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [], + }); + + expect(logger.error).toHaveBeenCalledWith( + 'The notification throttle "from" and/or "to" range values could not be constructed as valid. Tried to construct the values of "from": now-invalid "to": 2021-08-24T19:19:22.094Z. This will cause a reset of the notification throttle. Expect either missing alert notifications or alert notifications happening earlier than expected.' + ); + }); + + it('should count correctly if it does a deconflict', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + { + _index: 'index-456', + _id: 'id-456', + _source: { + test: 456, + }, + }, + ], + total: 2, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-456', + _id: 'id-456', + test: 456, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 2, + signals: [ + { + _id: 'id-456', + _index: 'index-456', + test: 456, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); + + it('should count correctly if it does not do a deconflict', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + { + _index: 'index-456', + _id: 'id-456', + _source: { + test: 456, + }, + }, + ], + total: 2, + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-789', + _id: 'id-789', + test: 456, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 3, + signals: [ + { + _id: 'id-789', + _index: 'index-789', + test: 456, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + { + _id: 'id-456', + _index: 'index-456', + test: 456, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); + + it('should count total hit with extra total elements', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + ], + total: 20, // total can be different from the actual return values so we have to ensure we count these. + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-789', + _id: 'id-789', + test: 456, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 21, + signals: [ + { + _id: 'id-789', + _index: 'index-789', + test: 456, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); + + it('should count correctly if it does a deconflict and the total has extra values', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + { + _index: 'index-456', + _id: 'id-456', + _source: { + test: 456, + }, + }, + ], + total: 20, // total can be different from the actual return values so we have to ensure we count these. + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-456', + _id: 'id-456', + test: 456, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 20, + signals: [ + { + _id: 'id-456', + _index: 'index-456', + test: 456, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); + + it('should add extra count element if it has signals added', async () => { + await scheduleThrottledNotificationActions({ + throttle: '1d', + startedAt: new Date('2021-08-24T19:19:22.094Z'), + id: '123', + kibanaSiemAppUrl: 'http://www.example.com', + outputIndex: 'output-123', + ruleId: 'rule-123', + esClient: elasticsearchServiceMock.createElasticsearchClient( + elasticsearchServiceMock.createSuccessTransportRequestPromise({ + hits: { + hits: [ + { + _index: 'index-123', + _id: 'id-123', + _source: { + test: 123, + }, + }, + { + _index: 'index-456', + _id: 'id-456', + _source: { + test: 456, + }, + }, + ], + total: 20, // total can be different from the actual return values so we have to ensure we count these. + }, + }) + ), + alertInstance: alertsMock.createAlertInstanceFactory(), + notificationRuleParams, + logger, + signals: [ + { + _index: 'index-789', + _id: 'id-789', + test: 789, + }, + ], + }); + expect(scheduleNotificationActions).toHaveBeenCalledWith( + expect.objectContaining({ + signalsCount: 21, // should be 1 more than the total since we pushed in an extra signal + signals: [ + { + _id: 'id-789', + _index: 'index-789', + test: 789, + }, + { + _id: 'id-123', + _index: 'index-123', + test: 123, + }, + { + _id: 'id-456', + _index: 'index-456', + test: 456, + }, + ], + ruleParams: notificationRuleParams, + }) + ); + }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts index 5dd583d47b403..5bf18496e6375 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts @@ -5,11 +5,11 @@ * 2.0. */ -import { ElasticsearchClient, SavedObject } from 'src/core/server'; +import { ElasticsearchClient, SavedObject, Logger } from 'src/core/server'; import { parseScheduleDates } from '@kbn/securitysolution-io-ts-utils'; import { AlertInstance } from '../../../../../alerting/server'; import { RuleParams } from '../schemas/rule_schemas'; -import { getNotificationResultsLink } from '../notifications/utils'; +import { deconflictSignalsAndResults, getNotificationResultsLink } from '../notifications/utils'; import { DEFAULT_RULE_NOTIFICATION_QUERY_SIZE } from '../../../../common/constants'; import { getSignals } from '../notifications/get_signals'; import { @@ -18,8 +18,25 @@ import { } from './schedule_notification_actions'; import { AlertAttributes } from '../signals/types'; +interface ScheduleThrottledNotificationActionsOptions { + id: SavedObject['id']; + startedAt: Date; + throttle: AlertAttributes['throttle']; + kibanaSiemAppUrl: string | undefined; + outputIndex: RuleParams['outputIndex']; + ruleId: RuleParams['ruleId']; + esClient: ElasticsearchClient; + alertInstance: AlertInstance; + notificationRuleParams: NotificationRuleTypeParams; + signals: unknown[]; + logger: Logger; +} + /** * Schedules a throttled notification action for executor rules. + * NOTE: It's important that since this is throttled that you call this in _ALL_ cases including error conditions or results being empty or not a success. + * If you do not call this within your rule executor then this will cause a "reset" and will stop "throttling" and the next call will cause an immediate action + * to be sent through the system. * @param throttle The throttle which is the alerting saved object throttle * @param startedAt When the executor started at * @param id The id the alert which caused the notifications @@ -40,17 +57,9 @@ export const scheduleThrottledNotificationActions = async ({ esClient, alertInstance, notificationRuleParams, -}: { - id: SavedObject['id']; - startedAt: Date; - throttle: AlertAttributes['throttle']; - kibanaSiemAppUrl: string | undefined; - outputIndex: RuleParams['outputIndex']; - ruleId: RuleParams['ruleId']; - esClient: ElasticsearchClient; - alertInstance: AlertInstance; - notificationRuleParams: NotificationRuleTypeParams; -}): Promise => { + signals, + logger, +}: ScheduleThrottledNotificationActionsOptions): Promise => { const fromInMs = parseScheduleDates(`now-${throttle}`); const toInMs = parseScheduleDates(startedAt.toISOString()); @@ -62,6 +71,22 @@ export const scheduleThrottledNotificationActions = async ({ kibanaSiemAppUrl, }); + logger.debug( + [ + `The notification throttle resultsLink created is: ${resultsLink}.`, + ' Notification throttle is querying the results using', + ` "from:" ${fromInMs.valueOf()}`, + ' "to":', + ` ${toInMs.valueOf()}`, + ' "size":', + ` ${DEFAULT_RULE_NOTIFICATION_QUERY_SIZE}`, + ' "index":', + ` ${outputIndex}`, + ' "ruleId":', + ` ${ruleId}`, + ].join('') + ); + const results = await getSignals({ from: `${fromInMs.valueOf()}`, to: `${toInMs.valueOf()}`, @@ -71,18 +96,56 @@ export const scheduleThrottledNotificationActions = async ({ esClient, }); - const signalsCount = + // This will give us counts up to the max of 10k from tracking total hits. + const signalsCountFromResults = typeof results.hits.total === 'number' ? results.hits.total : results.hits.total.value; - const signals = results.hits.hits.map((hit) => hit._source); - if (results.hits.hits.length !== 0) { + const resultsFlattened = results.hits.hits.map((hit) => { + return { + _id: hit._id, + _index: hit._index, + ...hit._source, + }; + }); + + const deconflicted = deconflictSignalsAndResults({ + logger, + signals, + querySignals: resultsFlattened, + }); + + // Difference of how many deconflicted results we have to subtract from our signals count. + const deconflictedDiff = resultsFlattened.length + signals.length - deconflicted.length; + + // Subtract any deconflicted differences from the total count. + const signalsCount = signalsCountFromResults + signals.length - deconflictedDiff; + logger.debug( + [ + `The notification throttle query result size before deconflicting duplicates is: ${resultsFlattened.length}.`, + ` The notification throttle passed in signals size before deconflicting duplicates is: ${signals.length}.`, + ` The deconflicted size and size of the signals sent into throttle notification is: ${deconflicted.length}.`, + ` The signals count from results size is: ${signalsCountFromResults}.`, + ` The final signals count being sent to the notification is: ${signalsCount}.`, + ].join('') + ); + + if (deconflicted.length !== 0) { scheduleNotificationActions({ alertInstance, signalsCount, - signals, + signals: deconflicted, resultsLink, ruleParams: notificationRuleParams, }); } + } else { + logger.error( + [ + 'The notification throttle "from" and/or "to" range values could not be constructed as valid. Tried to construct the values of', + ` "from": now-${throttle}`, + ` "to": ${startedAt.toISOString()}.`, + ' This will cause a reset of the notification throttle. Expect either missing alert notifications or alert notifications happening earlier than expected.', + ].join('') + ); } }; diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts index 5a667616a9a39..2da7a0398bd3f 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts @@ -5,18 +5,384 @@ * 2.0. */ -import { getNotificationResultsLink } from './utils'; +import { SearchHit } from '@elastic/elasticsearch/api/types'; +import { loggingSystemMock } from 'src/core/server/mocks'; +import { SignalSource } from '../signals/types'; +import { deconflictSignalsAndResults, getNotificationResultsLink } from './utils'; describe('utils', () => { - it('getNotificationResultsLink', () => { - const resultLink = getNotificationResultsLink({ - kibanaSiemAppUrl: 'http://localhost:5601/app/security', - id: 'notification-id', - from: '00000', - to: '1111', - }); - expect(resultLink).toEqual( - `http://localhost:5601/app/security/detections/rules/id/notification-id?timerange=(global:(linkTo:!(timeline),timerange:(from:00000,kind:absolute,to:1111)),timeline:(linkTo:!(global),timerange:(from:00000,kind:absolute,to:1111)))` - ); + let logger = loggingSystemMock.create().get('security_solution'); + + beforeEach(() => { + logger = loggingSystemMock.create().get('security_solution'); + }); + + describe('getNotificationResultsLink', () => { + test('it returns expected link', () => { + const resultLink = getNotificationResultsLink({ + kibanaSiemAppUrl: 'http://localhost:5601/app/security', + id: 'notification-id', + from: '00000', + to: '1111', + }); + expect(resultLink).toEqual( + `http://localhost:5601/app/security/detections/rules/id/notification-id?timerange=(global:(linkTo:!(timeline),timerange:(from:00000,kind:absolute,to:1111)),timeline:(linkTo:!(global),timerange:(from:00000,kind:absolute,to:1111)))` + ); + }); + }); + + describe('deconflictSignalsAndResults', () => { + type FuncReturn = ReturnType; + + test('given no signals and no query results it returns an empty array', () => { + expect( + deconflictSignalsAndResults({ logger, querySignals: [], signals: [] }) + ).toEqual([]); + }); + + test('given an empty signal and a single query result it returns the query result in the array', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + expect( + deconflictSignalsAndResults({ logger, querySignals, signals: [] }) + ).toEqual(querySignals); + }); + + test('given a single signal and an empty query result it returns the query result in the array', () => { + const signals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + expect( + deconflictSignalsAndResults({ logger, querySignals: [], signals }) + ).toEqual(signals); + }); + + test('given a signal and a different query result it returns both combined together', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + const signals: Array> = [ + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + }); + + test('given a duplicate in querySignals it returns both combined together without the duplicate', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', // This should only show up once and not be duplicated twice + _source: { + test: '123', + }, + }, + { + _index: 'index-890', + _id: 'id-890', + _source: { + test: '890', + }, + }, + ]; + const signals: Array> = [ + { + _id: 'id-123', // This should only show up once and not be duplicated twice + _index: 'index-123', + _source: { + test: '123', + }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + { + _id: 'id-890', + _index: 'index-890', + _source: { + test: '890', + }, + }, + ]); + }); + + test('given a duplicate in signals it returns both combined together without the duplicate', () => { + const signals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', // This should only show up once and not be duplicated twice + _source: { + test: '123', + }, + }, + { + _index: 'index-890', + _id: 'id-890', + _source: { + test: '890', + }, + }, + ]; + const querySignals: Array> = [ + { + _id: 'id-123', // This should only show up once and not be duplicated twice + _index: 'index-123', + _source: { + test: '123', + }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + { + _id: 'id-123', + _index: 'index-123', + _source: { test: '123' }, + }, + { + _id: 'id-890', + _index: 'index-890', + _source: { test: '890' }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { test: '456' }, + }, + ]); + }); + + test('does not give a duplicate in signals if they are only different by their index', () => { + const signals: Array> = [ + { + _id: 'id-123', + _index: 'index-123-a', // This is only different by index + _source: { + test: '123', + }, + }, + { + _index: 'index-890', + _id: 'id-890', + _source: { + test: '890', + }, + }, + ]; + const querySignals: Array> = [ + { + _id: 'id-123', // This is only different by index + _index: 'index-123-b', + _source: { + test: '123', + }, + }, + { + _id: 'id-789', + _index: 'index-456', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + }); + + test('it logs a debug statement when it sees a duplicate and returns nothing if both are identical', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + const signals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]); + expect(logger.debug).toHaveBeenCalledWith( + 'Notification throttle removing duplicate signal and query result found of "_id": id-123, "_index": index-123' + ); + }); + + test('it logs an error statement if it sees a signal missing an "_id" for an uncommon reason and returns both documents', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + const signals: unknown[] = [ + { + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + expect(logger.error).toHaveBeenCalledWith( + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index". Expect possible duplications in your alerting actions. Passed in signals "_id": undefined. Passed in signals "_index": index-123. Passed in query "result._id": id-123. Passed in query "result._index": index-123.' + ); + }); + + test('it logs an error statement if it sees a signal missing a "_index" for an uncommon reason and returns both documents', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '123', + }, + }, + ]; + const signals: unknown[] = [ + { + _id: 'id-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + expect(logger.error).toHaveBeenCalledWith( + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index". Expect possible duplications in your alerting actions. Passed in signals "_id": id-123. Passed in signals "_index": undefined. Passed in query "result._id": id-123. Passed in query "result._index": index-123.' + ); + }); + + test('it logs an error statement if it sees a querySignals missing an "_id" for an uncommon reason and returns both documents', () => { + const querySignals: Array> = [ + { + _index: 'index-123', + _source: { + test: '123', + }, + }, + ] as unknown[] as Array>; + const signals: unknown[] = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + expect(logger.error).toHaveBeenCalledWith( + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index". Expect possible duplications in your alerting actions. Passed in signals "_id": id-123. Passed in signals "_index": index-123. Passed in query "result._id": undefined. Passed in query "result._index": index-123.' + ); + }); + + test('it logs an error statement if it sees a querySignals missing a "_index" for an uncommon reason and returns both documents', () => { + const querySignals: Array> = [ + { + _id: 'id-123', + _source: { + test: '123', + }, + }, + ] as unknown[] as Array>; + const signals: unknown[] = [ + { + _id: 'id-123', + _index: 'index-123', + _source: { + test: '456', + }, + }, + ]; + expect(deconflictSignalsAndResults({ logger, querySignals, signals })).toEqual([ + ...signals, + ...querySignals, + ]); + expect(logger.error).toHaveBeenCalledWith( + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index". Expect possible duplications in your alerting actions. Passed in signals "_id": id-123. Passed in signals "_index": index-123. Passed in query "result._id": id-123. Passed in query "result._index": undefined.' + ); + }); }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.ts index 4c4bac7da6a62..c8fc6febe4d0f 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.ts @@ -5,7 +5,9 @@ * 2.0. */ +import { Logger } from 'src/core/server'; import { APP_PATH } from '../../../../common/constants'; +import { SignalSearchResponse } from '../signals/types'; export const getNotificationResultsLink = ({ kibanaSiemAppUrl = APP_PATH, @@ -22,3 +24,54 @@ export const getNotificationResultsLink = ({ return `${kibanaSiemAppUrl}/detections/rules/id/${id}?timerange=(global:(linkTo:!(timeline),timerange:(from:${from},kind:absolute,to:${to})),timeline:(linkTo:!(global),timerange:(from:${from},kind:absolute,to:${to})))`; }; + +interface DeconflictOptions { + signals: unknown[]; + querySignals: SignalSearchResponse['hits']['hits']; + logger: Logger; +} + +/** + * Given a signals array of unknown that at least has a '_id' and '_index' this will deconflict it with a results. + * @param signals The signals array to deconflict with results + * @param results The results to deconflict with the signals + * @param logger The logger to log results + */ +export const deconflictSignalsAndResults = ({ + signals, + querySignals, + logger, +}: DeconflictOptions): unknown[] => { + const querySignalsFiltered = querySignals.filter((result) => { + return !signals.find((signal) => { + const { _id, _index } = signal as { _id?: string; _index?: string }; + if (_id == null || _index == null || result._id == null || result._index == null) { + logger.error( + [ + 'Notification throttle cannot determine if we can de-conflict as either the passed in signal or the results query has a null value for either "_id" or "_index".', + ' Expect possible duplications in your alerting actions.', + ` Passed in signals "_id": ${_id}.`, + ` Passed in signals "_index": ${_index}.`, + ` Passed in query "result._id": ${result._id}.`, + ` Passed in query "result._index": ${result._index}.`, + ].join('') + ); + return false; + } else { + if (result._id === _id && result._index === _index) { + logger.debug( + [ + 'Notification throttle removing duplicate signal and query result found of', + ` "_id": ${_id},`, + ` "_index": ${_index}`, + ].join('') + ); + return true; + } else { + return false; + } + } + }); + }); + return [...signals, ...querySignalsFiltered]; +}; diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts index 77981d92b2ba7..0ad416e86e31a 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/create_security_rule_type_wrapper.ts @@ -111,6 +111,12 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = let result = createResultObject(state); + const notificationRuleParams: NotificationRuleTypeParams = { + ...params, + name: name as string, + id: ruleSO.id as string, + } as unknown as NotificationRuleTypeParams; + // check if rule has permissions to access given index pattern // move this collection of lines into a function in utils // so that we can use it in create rules route, bulk, etc. @@ -296,12 +302,6 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = const createdSignalsCount = result.createdSignals.length; if (actions.length) { - const notificationRuleParams: NotificationRuleTypeParams = { - ...params, - name: name as string, - id: ruleSO.id as string, - } as unknown as NotificationRuleTypeParams; - const fromInMs = parseScheduleDates(`now-${interval}`)?.format('x'); const toInMs = parseScheduleDates('now')?.format('x'); const resultsLink = getNotificationResultsLink({ @@ -328,6 +328,8 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = ruleId, esClient: services.scopedClusterClient.asCurrentUser, notificationRuleParams, + signals: result.createdSignals, + logger, }); } else if (createdSignalsCount) { const alertInstance = services.alertInstanceFactory(alertId); @@ -372,6 +374,21 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = ) ); } else { + // NOTE: Since this is throttled we have to call it even on an error condition, otherwise it will "reset" the throttle and fire early + await scheduleThrottledNotificationActions({ + alertInstance: services.alertInstanceFactory(alertId), + throttle: ruleSO.attributes.throttle, + startedAt, + id: ruleSO.id, + kibanaSiemAppUrl: (meta as { kibana_siem_app_url?: string } | undefined) + ?.kibana_siem_app_url, + outputIndex: ruleDataClient.indexName, + ruleId, + esClient: services.scopedClusterClient.asCurrentUser, + notificationRuleParams, + signals: result.createdSignals, + logger, + }); const errorMessage = buildRuleMessage( 'Bulk Indexing of signals failed:', truncateMessageList(result.errors).join() @@ -389,6 +406,22 @@ export const createSecurityRuleTypeWrapper: CreateSecurityRuleTypeWrapper = }); } } catch (error) { + // NOTE: Since this is throttled we have to call it even on an error condition, otherwise it will "reset" the throttle and fire early + await scheduleThrottledNotificationActions({ + alertInstance: services.alertInstanceFactory(alertId), + throttle: ruleSO.attributes.throttle, + startedAt, + id: ruleSO.id, + kibanaSiemAppUrl: (meta as { kibana_siem_app_url?: string } | undefined) + ?.kibana_siem_app_url, + outputIndex: ruleDataClient.indexName, + ruleId, + esClient: services.scopedClusterClient.asCurrentUser, + notificationRuleParams, + signals: result.createdSignals, + logger, + }); + const errorMessage = error.message ?? '(no error message given)'; const message = buildRuleMessage( 'An error occurred during rule execution:', diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts index c2923b566175e..88b276358a705 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts @@ -35,6 +35,7 @@ import { allowedExperimentalValues } from '../../../../common/experimental_featu import { scheduleNotificationActions } from '../notifications/schedule_notification_actions'; import { ruleExecutionLogClientMock } from '../rule_execution_log/__mocks__/rule_execution_log_client'; import { RuleExecutionStatus } from '../../../../common/detection_engine/schemas/common/schemas'; +import { scheduleThrottledNotificationActions } from '../notifications/schedule_throttle_notification_actions'; import { eventLogServiceMock } from '../../../../../event_log/server/mocks'; import { createMockConfig } from '../routes/__mocks__'; @@ -58,7 +59,7 @@ jest.mock('@kbn/securitysolution-io-ts-utils', () => { parseScheduleDates: jest.fn(), }; }); - +jest.mock('../notifications/schedule_throttle_notification_actions'); const mockRuleExecutionLogClient = ruleExecutionLogClientMock.create(); jest.mock('../rule_execution_log/rule_execution_log_client', () => ({ @@ -200,6 +201,7 @@ describe('signal_rule_alert_type', () => { }); mockRuleExecutionLogClient.logStatusChange.mockClear(); + (scheduleThrottledNotificationActions as jest.Mock).mockClear(); }); describe('executor', () => { @@ -520,5 +522,28 @@ describe('signal_rule_alert_type', () => { }) ); }); + + it('should call scheduleThrottledNotificationActions if result is false to prevent the throttle from being reset', async () => { + const result: SearchAfterAndBulkCreateReturnType = { + success: false, + warning: false, + searchAfterTimes: [], + bulkCreateTimes: [], + lastLookBackDate: null, + createdSignalsCount: 0, + createdSignals: [], + warningMessages: [], + errors: ['Error that bubbled up.'], + }; + (queryExecutor as jest.Mock).mockResolvedValue(result); + await alert.executor(payload); + expect(scheduleThrottledNotificationActions).toHaveBeenCalledTimes(1); + }); + + it('should call scheduleThrottledNotificationActions if an error was thrown to prevent the throttle from being reset', async () => { + (queryExecutor as jest.Mock).mockRejectedValue({}); + await alert.executor(payload); + expect(scheduleThrottledNotificationActions).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts index 2094264cbf15f..4e98bee83aeb5 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts @@ -172,6 +172,12 @@ export const signalRulesAlertType = ({ newStatus: RuleExecutionStatus['going to run'], }); + const notificationRuleParams: NotificationRuleTypeParams = { + ...params, + name, + id: savedObject.id, + }; + // check if rule has permissions to access given index pattern // move this collection of lines into a function in utils // so that we can use it in create rules route, bulk, etc. @@ -396,12 +402,6 @@ export const signalRulesAlertType = ({ if (result.success) { if (actions.length) { - const notificationRuleParams: NotificationRuleTypeParams = { - ...params, - name, - id: savedObject.id, - }; - const fromInMs = parseScheduleDates(`now-${interval}`)?.format('x'); const toInMs = parseScheduleDates('now')?.format('x'); const resultsLink = getNotificationResultsLink({ @@ -426,8 +426,10 @@ export const signalRulesAlertType = ({ ?.kibana_siem_app_url, outputIndex, ruleId, + signals: result.createdSignals, esClient: services.scopedClusterClient.asCurrentUser, notificationRuleParams, + logger, }); } else if (result.createdSignalsCount) { const alertInstance = services.alertInstanceFactory(alertId); @@ -471,6 +473,22 @@ export const signalRulesAlertType = ({ ) ); } else { + // NOTE: Since this is throttled we have to call it even on an error condition, otherwise it will "reset" the throttle and fire early + await scheduleThrottledNotificationActions({ + alertInstance: services.alertInstanceFactory(alertId), + throttle: savedObject.attributes.throttle, + startedAt, + id: savedObject.id, + kibanaSiemAppUrl: (meta as { kibana_siem_app_url?: string } | undefined) + ?.kibana_siem_app_url, + outputIndex, + ruleId, + signals: result.createdSignals, + esClient: services.scopedClusterClient.asCurrentUser, + notificationRuleParams, + logger, + }); + const errorMessage = buildRuleMessage( 'Bulk Indexing of signals failed:', truncateMessageList(result.errors).join() @@ -488,6 +506,21 @@ export const signalRulesAlertType = ({ }); } } catch (error) { + // NOTE: Since this is throttled we have to call it even on an error condition, otherwise it will "reset" the throttle and fire early + await scheduleThrottledNotificationActions({ + alertInstance: services.alertInstanceFactory(alertId), + throttle: savedObject.attributes.throttle, + startedAt, + id: savedObject.id, + kibanaSiemAppUrl: (meta as { kibana_siem_app_url?: string } | undefined) + ?.kibana_siem_app_url, + outputIndex, + ruleId, + signals: result.createdSignals, + esClient: services.scopedClusterClient.asCurrentUser, + notificationRuleParams, + logger, + }); const errorMessage = error.message ?? '(no error message given)'; const message = buildRuleMessage( 'An error occurred during rule execution:', From 95e412b4a139b8fc4a92a2669e34a54810a37aae Mon Sep 17 00:00:00 2001 From: Frank Hassanabad Date: Fri, 15 Oct 2021 18:37:36 -0600 Subject: [PATCH 4/7] Fixes migration bug where I was deleting attributes (#115098) ## Summary During the work here: https://github.com/elastic/kibana/pull/113577 I accidentally have introduced a bug where on migration I was deleting the attributes of `ruleThrottle` and `alertThrottle` because I was not using splat correctly. Added unit and e2e tests to fix this. ### 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 --- .../rule_actions/legacy_migrations.test.ts | 4 ++++ .../rule_actions/legacy_migrations.ts | 2 +- .../security_and_spaces/tests/migrations.ts | 21 +++++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions/legacy_migrations.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions/legacy_migrations.test.ts index 8414aa93c7984..7dd05c5122a61 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions/legacy_migrations.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions/legacy_migrations.test.ts @@ -20,6 +20,8 @@ describe('legacy_migrations', () => { test('it migrates both a "ruleAlertId" and a actions array with 1 element into the references array', () => { const doc = { attributes: { + ruleThrottle: '1d', + alertThrottle: '1d', ruleAlertId: '123', actions: [ { @@ -37,6 +39,8 @@ describe('legacy_migrations', () => { ) ).toEqual({ attributes: { + ruleThrottle: '1d', + alertThrottle: '1d', actions: [ { actionRef: 'action_0', diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions/legacy_migrations.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions/legacy_migrations.ts index 8a52d3a13f065..aa85898e94a33 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions/legacy_migrations.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions/legacy_migrations.ts @@ -245,7 +245,7 @@ export const legacyMigrateRuleAlertId = ( return { ...doc, attributes: { - ...attributesWithoutRuleAlertId.attributes, + ...attributesWithoutRuleAlertId, actions: actionsWithRef, }, references: [...existingReferences, ...alertReferences, ...actionsReferences], diff --git a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/migrations.ts b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/migrations.ts index d25fb5bfa5899..4c0f21df8c0ff 100644 --- a/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/migrations.ts +++ b/x-pack/test/detection_engine_api_integration/security_and_spaces/tests/migrations.ts @@ -65,6 +65,27 @@ export default ({ getService }: FtrProviderContext): void => { undefined ); }); + + it('migrates legacy siem-detection-engine-rule-actions and retains "ruleThrottle" and "alertThrottle" as the same attributes as before', async () => { + const response = await es.get<{ + 'siem-detection-engine-rule-actions': { + ruleThrottle: string; + alertThrottle: string; + }; + }>({ + index: '.kibana', + id: 'siem-detection-engine-rule-actions:fce024a0-0452-11ec-9b15-d13d79d162f3', + }); + expect(response.statusCode).to.eql(200); + + // "alertThrottle" and "ruleThrottle" should still exist + expect(response.body._source?.['siem-detection-engine-rule-actions'].alertThrottle).to.eql( + '7d' + ); + expect(response.body._source?.['siem-detection-engine-rule-actions'].ruleThrottle).to.eql( + '7d' + ); + }); }); }); }; From bf17898753cc05b778870e09d075a2bd1be95109 Mon Sep 17 00:00:00 2001 From: Frank Hassanabad Date: Fri, 15 Oct 2021 18:38:00 -0600 Subject: [PATCH 5/7] one line remove assert (#115127) ## Summary Removes one liner non-null-assert. Instead of this line: ```ts if (rule != null && spacesApi && outcome === 'conflict') { ``` We just check using the `?` operator and type narrowing to remove the possibility of an error ```ts if (rule?.alias_target_id != null && spacesApi && rule.outcome === 'conflict') { ``` The `rule?.alias_target_id != null` ensures that both `rule` and `alias_target_id` are not `null/undefined` --- .../pages/detection_engine/rules/details/index.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx index 7167b07c7da5d..774b9463bed69 100644 --- a/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx +++ b/x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx @@ -300,10 +300,8 @@ const RuleDetailsPageComponent: React.FC = ({ }, [rule, spacesApi]); const getLegacyUrlConflictCallout = useMemo(() => { - const outcome = rule?.outcome; - if (rule != null && spacesApi && outcome === 'conflict') { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const aliasTargetId = rule?.alias_target_id!; // This is always defined if outcome === 'conflict' + if (rule?.alias_target_id != null && spacesApi && rule.outcome === 'conflict') { + const aliasTargetId = rule.alias_target_id; // We have resolved to one rule, but there is another one with a legacy URL associated with this page. Display a // callout with a warning for the user, and provide a way for them to navigate to the other rule. const otherRulePath = `rules/id/${aliasTargetId}${window.location.search}${window.location.hash}`; From d98bf0c2452f711f8c180e618dec4c47be6e5ffc Mon Sep 17 00:00:00 2001 From: Jonathan Budzenski Date: Fri, 15 Oct 2021 20:21:41 -0500 Subject: [PATCH 6/7] skip flaky suite. #115130 --- .../functional/apps/monitoring/elasticsearch/node_detail.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/test/functional/apps/monitoring/elasticsearch/node_detail.js b/x-pack/test/functional/apps/monitoring/elasticsearch/node_detail.js index 6b1658dd9ed0e..07fda7a143a99 100644 --- a/x-pack/test/functional/apps/monitoring/elasticsearch/node_detail.js +++ b/x-pack/test/functional/apps/monitoring/elasticsearch/node_detail.js @@ -14,7 +14,8 @@ export default function ({ getService, getPageObjects }) { const nodesList = getService('monitoringElasticsearchNodes'); const nodeDetail = getService('monitoringElasticsearchNodeDetail'); - describe('Elasticsearch node detail', () => { + // FLAKY https://github.com/elastic/kibana/issues/115130 + describe.skip('Elasticsearch node detail', () => { describe('Active Nodes', () => { const { setup, tearDown } = getLifecycleMethods(getService, getPageObjects); From 16320cc249d5fd4df3255cbe8b2b499bfdd52d5a Mon Sep 17 00:00:00 2001 From: Andrew Goldstein Date: Sat, 16 Oct 2021 12:44:19 -0600 Subject: [PATCH 7/7] [Security Solution] Restores Alerts table local storage persistence and the Remove Column action (#114742) ## [Security Solution] Restores Alerts table local storage persistence and the Remove Column action This PR implements the following changes summarized below to address , as proposed [here](https://github.com/elastic/kibana/issues/113090#issuecomment-935143690): - Configures the `Columns` popover to be consistent with `Discover` - Changes the `Hide column` action to `Remove column`, to be consistent with `Discover` - Persists updates to the `Columns` popover order in `local storage` - Restores the feature to persist column widths in `local storage` ### Configures the `Columns` popover to be consistent with `Discover` - We now pass `false` to the `allowHide` [EuiDataGrid API](https://elastic.github.io/eui/#/tabular-content/data-grid): ![allow_hide](https://user-images.githubusercontent.com/4459398/136114714-02f25b97-86af-47e5-9adc-1177d5a2c715.png) This makes all `EuiDataGrid`-based views in the Security Solution consistent with `Discover`'s use of the `EuiDataGrid` `Columns` popover. In `7.15`, the `Columns` popover includes the _hide column_ toggle, as shown in the screenshot below: ![alerts_columns_popover_7_15](https://user-images.githubusercontent.com/4459398/136112441-455ddbeb-dea3-4837-81ad-32d6c82c11fe.png) _Above: The `Columns` popover in the `7.15` `Alerts` table_ The `Columns` popover in `Discover`'s `EuiDataGrid`-based table does not display the hide column toggle, as shown the screenshot below: ![columns_popover_discover](https://user-images.githubusercontent.com/4459398/136112856-7e42c822-2260-4759-ac78-5bea63a171c7.png) _Above: The `EuiDataGrid` `Columns` popover in `Discover`, in `master`_ Passing `false` to the `allowHide` [EuiDataGrid API](https://elastic.github.io/eui/#/tabular-content/data-grid) API makes the `Columns` popover in all `EuiDataGrid`-based views in the Security Solution consistent with `Discover`, as illustrated by the screenshot below: ![alerts_columns_popover_no_hide](https://user-images.githubusercontent.com/4459398/136112980-d4219fbd-1443-4612-8cdb-b97bee8b97ef.png) _Above: The `Columns` popover is now consistent with `Discover`_ ## Changes the `Hide column` action to `Remove column`, to be consistent with `Discover` - The `Hide column` action shown in the `7.15` alerts table is changed to `Remove column`, making it consistent with `Discover`'s use of `EuiDataGrid` In `7.15`, the `Alerts` table has a `Hide column` action, as shown in the screenshot below: ![hide_column](https://user-images.githubusercontent.com/4459398/136115681-9e0da144-a981-4352-8092-9368d74cd153.png) _Above: The `Hide Column` action in the `7.15` `Alerts` table_ In `7.15`, clicking the `Hide Column` action shown in the screenshot above hides the column, but does not remove it. In `7.15`, columns may only be removed by un-checking them in the `Fields` browser, or by un-toggling them in the Alerts / Events details popover. Both of those methods require multiple clicks, and require uses to re-find the field in the modal or popover before it may be toggled for removal. In `Discover`, users don't hide columns. In `Discover`, users directly remove columns by clicking the `Remove column` action, shown in the screenshot below: ![discover_remove_column](https://user-images.githubusercontent.com/4459398/136114295-f018a561-f9ee-4ce4-a9c6-0fcd7f71e67b.png) _Above: The `Remove column` action in `Discover`'s use of `EuiDataGrid` in `master`_ All `EuiDataGrid`-based views in the Security Solution were made consistent with `Discover` by replacing the `Hide column` action with `Remove column`, per the screenshot below: ![remove_column_after](https://user-images.githubusercontent.com/4459398/137047582-3c4d6cb0-ac12-4c50-9c34-0c4ef5536550.png) _Above: The `Remove column` action in the Alerts table_ Note: the `Remove column` action shown above appears as the last item in the popover because it's specified via the `EuiDataGrid` `EuiDataGridColumnActions` > `additonal` API, which appends additonal actions to the end of popover, after the built-in actions: ![additional](https://user-images.githubusercontent.com/4459398/137047825-625002b3-5cd6-4b3e-87da-e76dbaf2a827.png) ## Persists updates to the `Columns` popover order in `local storage` - Persist column order updates to `local storage` when users update the order of columns via the `Columns` popover The following PR restored partial support for persisting columns across page refreshes via `local storage`, but the Redux store was not updated when users sort columns via the `Columns` popover, an shown in the animated gif below: ![ordering_via_columns](https://user-images.githubusercontent.com/4459398/136119497-65f76f49-091c-4a45-b8d3-1e5ef80ccbb2.gif) _Above: Ordering via the `Columns` popover is not persisted to `local storage` in `7.15`_ This PR utilizes the `setVisibleColumns` [EuiDataGrid API](https://elastic.github.io/eui/#/tabular-content/data-grid) API as a callback to update Redux when the columns are sorted, which will in-turn update `local storage` to persist the new order across page refreshes: ![setVisibleColumns](https://user-images.githubusercontent.com/4459398/136117249-628bb147-a860-4ccf-811a-0e57a99296fb.png) ## Restores the feature to persist column widths in `local storage` In previous releases, resized column widths were peristed in `local storage` to persist across page refreshes, as documented in : ``` { "detections-page":{ "id":"detections-page", "activeTab":"query", "prevActiveTab":"query", "columns":[ { "category":"base", "columnHeaderType":"not-filtered", "description":"Date/time when the event originated. This is the date/time extracted from the event, typically representing when the event was generated by the source. If the event source has no original timestamp, this value is typically populated by the first time the event was received by the pipeline. Required field for all events.", "example":"2016-05-23T08:05:34.853Z", "id":"@timestamp", "type":"date", "aggregatable":true, "width":190 }, { "category":"cloud", "columnHeaderType":"not-filtered", "description":"The cloud account or organization id used to identify different entities in a multi-tenant environment. Examples: AWS account id, Google Cloud ORG Id, or other unique identifier.", "example":"666777888999", "id":"cloud.account.id", "type":"string", "aggregatable":true, "width":180 }, { "category":"cloud", "columnHeaderType":"not-filtered", "description":"Availability zone in which this host is running.", "example":"us-east-1c", "id":"cloud.availability_zone", "type":"string", "aggregatable":true, "width":180 }, // ... } ], // ... } } ``` _Above: column widths were persisted to `local storage` in previous release, (going at least back to `7.12`)_ In this PR, we utilize the `onColumnResize` [EuiDataGrid API](https://elastic.github.io/eui/#/tabular-content/data-grid) API as a callback to update Redux when the columns are sorted via the `Columns` popover. Updating Redux will in-turn update `local storage`, so resized columns widths will persist across page refreshes: ![onColumnResize](https://user-images.githubusercontent.com/4459398/136120062-3b0bebce-9c44-47fc-9956-48fe07a30f83.png) ### Other changes The Alerts page `Trend` chart and table were updated to include the following additional `Stack by` fields (CC @paulewing): ``` process.name file.name hash.sha256 ``` per the before / after screenshots below: ![alerts-trend-before](https://user-images.githubusercontent.com/4459398/137045011-7da4530b-0259-4fd4-b903-9eee6c26d02f.png) _Above: The Alerts `Trend` Stack by fields in `7.15` (before)_ ![alerts-trend-after](https://user-images.githubusercontent.com/4459398/137045023-d0ae987c-a474-4123-a05b-a6ad2fc52922.png) _Above: The Alerts `Trend` `Stack by` fields (after the addition of the `process.name`, `file.name`, and `hash.sha256` fields)_ CC: @monina-n @paulewing --- .../components/alerts_kpis/common/config.ts | 3 + .../components/alerts_kpis/common/types.ts | 5 +- .../timelines/store/timeline/actions.ts | 2 + .../timeline/epic_local_storage.test.tsx | 33 +++++ .../store/timeline/epic_local_storage.ts | 4 + .../body/column_headers/column_header.tsx | 4 +- .../body/column_headers/helpers.test.tsx | 1 + .../t_grid/body/column_headers/helpers.tsx | 1 + .../body/column_headers/translations.ts | 4 - .../components/t_grid/body/index.test.tsx | 55 +++++++++ .../public/components/t_grid/body/index.tsx | 52 ++++++-- .../timelines/public/store/t_grid/actions.ts | 11 ++ .../public/store/t_grid/helpers.test.tsx | 115 +++++++++++++++++- .../timelines/public/store/t_grid/helpers.ts | 58 +++++++++ .../timelines/public/store/t_grid/reducer.ts | 21 ++++ .../translations/translations/ja-JP.json | 1 - .../translations/translations/zh-CN.json | 1 - 17 files changed, 352 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/config.ts b/x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/config.ts index cb5a23e711974..a835628fae6cf 100644 --- a/x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/config.ts +++ b/x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/config.ts @@ -19,6 +19,9 @@ export const alertsStackByOptions: AlertsStackByOption[] = [ { text: 'signal.rule.name', value: 'signal.rule.name' }, { text: 'source.ip', value: 'source.ip' }, { text: 'user.name', value: 'user.name' }, + { text: 'process.name', value: 'process.name' }, + { text: 'file.name', value: 'file.name' }, + { text: 'hash.sha256', value: 'hash.sha256' }, ]; export const DEFAULT_STACK_BY_FIELD = 'signal.rule.name'; diff --git a/x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/types.ts b/x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/types.ts index 833c05bfc7a79..f561c3f6faa21 100644 --- a/x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/types.ts +++ b/x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/types.ts @@ -21,4 +21,7 @@ export type AlertsStackByField = | 'signal.rule.type' | 'signal.rule.name' | 'source.ip' - | 'user.name'; + | 'user.name' + | 'process.name' + | 'file.name' + | 'hash.sha256'; diff --git a/x-pack/plugins/security_solution/public/timelines/store/timeline/actions.ts b/x-pack/plugins/security_solution/public/timelines/store/timeline/actions.ts index 3750bc22ddc69..95ad6c5d44ca3 100644 --- a/x-pack/plugins/security_solution/public/timelines/store/timeline/actions.ts +++ b/x-pack/plugins/security_solution/public/timelines/store/timeline/actions.ts @@ -37,7 +37,9 @@ export const { setSelected, setTGridSelectAll, toggleDetailPanel, + updateColumnOrder, updateColumns, + updateColumnWidth, updateIsLoading, updateItemsPerPage, updateItemsPerPageOptions, diff --git a/x-pack/plugins/security_solution/public/timelines/store/timeline/epic_local_storage.test.tsx b/x-pack/plugins/security_solution/public/timelines/store/timeline/epic_local_storage.test.tsx index 01bc589393d2e..131f255b5a7a7 100644 --- a/x-pack/plugins/security_solution/public/timelines/store/timeline/epic_local_storage.test.tsx +++ b/x-pack/plugins/security_solution/public/timelines/store/timeline/epic_local_storage.test.tsx @@ -25,7 +25,9 @@ import { removeColumn, upsertColumn, applyDeltaToColumnWidth, + updateColumnOrder, updateColumns, + updateColumnWidth, updateItemsPerPage, updateSort, } from './actions'; @@ -168,4 +170,35 @@ describe('epicLocalStorage', () => { ); await waitFor(() => expect(addTimelineInStorageMock).toHaveBeenCalled()); }); + + it('persists updates to the column order to local storage', async () => { + shallow( + + + + ); + store.dispatch( + updateColumnOrder({ + columnIds: ['event.severity', '@timestamp', 'event.category'], + id: 'test', + }) + ); + await waitFor(() => expect(addTimelineInStorageMock).toHaveBeenCalled()); + }); + + it('persists updates to the column width to local storage', async () => { + shallow( + + + + ); + store.dispatch( + updateColumnWidth({ + columnId: 'event.severity', + id: 'test', + width: 123, + }) + ); + await waitFor(() => expect(addTimelineInStorageMock).toHaveBeenCalled()); + }); }); diff --git a/x-pack/plugins/security_solution/public/timelines/store/timeline/epic_local_storage.ts b/x-pack/plugins/security_solution/public/timelines/store/timeline/epic_local_storage.ts index 9a889e9ec1af8..6c4ebf91b7adf 100644 --- a/x-pack/plugins/security_solution/public/timelines/store/timeline/epic_local_storage.ts +++ b/x-pack/plugins/security_solution/public/timelines/store/timeline/epic_local_storage.ts @@ -19,6 +19,8 @@ import { applyDeltaToColumnWidth, setExcludedRowRendererIds, updateColumns, + updateColumnOrder, + updateColumnWidth, updateItemsPerPage, updateSort, } from './actions'; @@ -30,6 +32,8 @@ const timelineActionTypes = [ upsertColumn.type, applyDeltaToColumnWidth.type, updateColumns.type, + updateColumnOrder.type, + updateColumnWidth.type, updateItemsPerPage.type, updateSort.type, setExcludedRowRendererIds.type, diff --git a/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/column_header.tsx b/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/column_header.tsx index 033292711c5af..4e6db10cc8bce 100644 --- a/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/column_header.tsx +++ b/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/column_header.tsx @@ -161,8 +161,8 @@ const ColumnHeaderComponent: React.FC = ({ id: 0, items: [ { - icon: , - name: i18n.HIDE_COLUMN, + icon: , + name: i18n.REMOVE_COLUMN, onClick: () => { dispatch(tGridActions.removeColumn({ id: timelineId, columnId: header.id })); handleClosePopOverTrigger(); diff --git a/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.test.tsx b/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.test.tsx index 2e684b9eda989..47fcb8c8e1509 100644 --- a/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.test.tsx +++ b/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.test.tsx @@ -98,6 +98,7 @@ describe('helpers', () => { describe('getColumnHeaders', () => { // additional properties used by `EuiDataGrid`: const actions = { + showHide: false, showSortAsc: true, showSortDesc: true, }; diff --git a/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.tsx b/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.tsx index c658000e6d331..66ec3ec1c399f 100644 --- a/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.tsx +++ b/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.tsx @@ -27,6 +27,7 @@ import { allowSorting } from '../helpers'; const defaultActions: EuiDataGridColumnActions = { showSortAsc: true, showSortDesc: true, + showHide: false, }; const getAllBrowserFields = (browserFields: BrowserFields): Array> => diff --git a/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/translations.ts b/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/translations.ts index 2d4fbcbd54cfa..202eef8d675b8 100644 --- a/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/translations.ts +++ b/x-pack/plugins/timelines/public/components/t_grid/body/column_headers/translations.ts @@ -23,10 +23,6 @@ export const FULL_SCREEN = i18n.translate('xpack.timelines.timeline.fullScreenBu defaultMessage: 'Full screen', }); -export const HIDE_COLUMN = i18n.translate('xpack.timelines.timeline.hideColumnLabel', { - defaultMessage: 'Hide column', -}); - export const SORT_AZ = i18n.translate('xpack.timelines.timeline.sortAZLabel', { defaultMessage: 'Sort A-Z', }); diff --git a/x-pack/plugins/timelines/public/components/t_grid/body/index.test.tsx b/x-pack/plugins/timelines/public/components/t_grid/body/index.test.tsx index 50764af3c7f2f..5a7ae6e407b0b 100644 --- a/x-pack/plugins/timelines/public/components/t_grid/body/index.test.tsx +++ b/x-pack/plugins/timelines/public/components/t_grid/body/index.test.tsx @@ -6,9 +6,11 @@ */ import React from 'react'; +import { fireEvent, render, screen } from '@testing-library/react'; import { BodyComponent, StatefulBodyProps } from '.'; import { Sort } from './sort'; +import { REMOVE_COLUMN } from './column_headers/translations'; import { Direction } from '../../../../common/search_strategy'; import { useMountAppended } from '../../utils/use_mount_appended'; import { defaultHeaders, mockBrowserFields, mockTimelineData, TestProviders } from '../../../mock'; @@ -273,4 +275,57 @@ describe('Body', () => { .find((c) => c.id === 'signal.rule.risk_score')?.cellActions ).toBeUndefined(); }); + + test('it does NOT render switches for hiding columns in the `EuiDataGrid` `Columns` popover', async () => { + render( + + + + ); + + // Click the `EuidDataGrid` `Columns` button to open the popover: + fireEvent.click(screen.getByTestId('dataGridColumnSelectorButton')); + + // `EuiDataGrid` renders switches for hiding in the `Columns` popover when `showColumnSelector.allowHide` is `true` + const switches = await screen.queryAllByRole('switch'); + + expect(switches.length).toBe(0); // no switches are rendered, because `allowHide` is `false` + }); + + test('it dispatches the `REMOVE_COLUMN` action when a user clicks `Remove column` in the column header popover', async () => { + render( + + + + ); + + // click the `@timestamp` column header to display the popover + fireEvent.click(screen.getByText('@timestamp')); + + // click the `Remove column` action in the popover + fireEvent.click(await screen.getByText(REMOVE_COLUMN)); + + expect(mockDispatch).toBeCalledWith({ + payload: { columnId: '@timestamp', id: 'timeline-test' }, + type: 'x-pack/timelines/t-grid/REMOVE_COLUMN', + }); + }); + + test('it dispatches the `UPDATE_COLUMN_WIDTH` action when a user resizes a column', async () => { + render( + + + + ); + + // simulate resizing the column + fireEvent.mouseDown(screen.getAllByTestId('dataGridColumnResizer')[0]); + fireEvent.mouseMove(screen.getAllByTestId('dataGridColumnResizer')[0]); + fireEvent.mouseUp(screen.getAllByTestId('dataGridColumnResizer')[0]); + + expect(mockDispatch).toBeCalledWith({ + payload: { columnId: '@timestamp', id: 'timeline-test', width: NaN }, + type: 'x-pack/timelines/t-grid/UPDATE_COLUMN_WIDTH', + }); + }); }); diff --git a/x-pack/plugins/timelines/public/components/t_grid/body/index.tsx b/x-pack/plugins/timelines/public/components/t_grid/body/index.tsx index 619571a0c8e81..9e43c16fd5e6f 100644 --- a/x-pack/plugins/timelines/public/components/t_grid/body/index.tsx +++ b/x-pack/plugins/timelines/public/components/t_grid/body/index.tsx @@ -75,6 +75,7 @@ import { ViewSelection } from '../event_rendered_view/selector'; import { EventRenderedView } from '../event_rendered_view'; import { useDataGridHeightHack } from './height_hack'; import { Filter } from '../../../../../../../src/plugins/data/public'; +import { REMOVE_COLUMN } from './column_headers/translations'; const StatefulAlertStatusBulkActions = lazy( () => import('../toolbar/bulk_actions/alert_status_bulk_actions') @@ -497,7 +498,7 @@ export const BodyComponent = React.memo( showFullScreenSelector: false, } : { - showColumnSelector: { allowHide: true, allowReorder: true }, + showColumnSelector: { allowHide: false, allowReorder: true }, showSortSelector: true, showFullScreenSelector: true, }), @@ -559,13 +560,32 @@ export const BodyComponent = React.memo( [columnHeaders, dispatch, id, loadPage] ); - const [visibleColumns, setVisibleColumns] = useState(() => - columnHeaders.map(({ id: cid }) => cid) - ); // initializes to the full set of columns + const visibleColumns = useMemo(() => columnHeaders.map(({ id: cid }) => cid), [columnHeaders]); // the full set of columns - useEffect(() => { - setVisibleColumns(columnHeaders.map(({ id: cid }) => cid)); - }, [columnHeaders]); + const onColumnResize = useCallback( + ({ columnId, width }: { columnId: string; width: number }) => { + dispatch( + tGridActions.updateColumnWidth({ + columnId, + id, + width, + }) + ); + }, + [dispatch, id] + ); + + const onSetVisibleColumns = useCallback( + (newVisibleColumns: string[]) => { + dispatch( + tGridActions.updateColumnOrder({ + columnIds: newVisibleColumns, + id, + }) + ); + }, + [dispatch, id] + ); const setEventsLoading = useCallback( ({ eventIds, isLoading: loading }) => { @@ -654,6 +674,19 @@ export const BodyComponent = React.memo( return { ...header, + actions: { + ...header.actions, + additional: [ + { + iconType: 'cross', + label: REMOVE_COLUMN, + onClick: () => { + dispatch(tGridActions.removeColumn({ id, columnId: header.id })); + }, + size: 'xs', + }, + ], + }, ...(hasCellActions(header.id) ? { cellActions: @@ -663,7 +696,7 @@ export const BodyComponent = React.memo( : {}), }; }), - [columnHeaders, defaultCellActions, browserFields, data, pageSize, id] + [columnHeaders, defaultCellActions, browserFields, data, pageSize, id, dispatch] ); const renderTGridCellValue = useMemo(() => { @@ -761,7 +794,7 @@ export const BodyComponent = React.memo( data-test-subj="body-data-grid" aria-label={i18n.TGRID_BODY_ARIA_LABEL} columns={columnsWithCellActions} - columnVisibility={{ visibleColumns, setVisibleColumns }} + columnVisibility={{ visibleColumns, setVisibleColumns: onSetVisibleColumns }} gridStyle={gridStyle} leadingControlColumns={leadingTGridControlColumns} trailingControlColumns={trailingTGridControlColumns} @@ -769,6 +802,7 @@ export const BodyComponent = React.memo( rowCount={totalItems} renderCellValue={renderTGridCellValue} sorting={{ columns: sortingColumns, onSort }} + onColumnResize={onColumnResize} pagination={{ pageIndex: activePage, pageSize, diff --git a/x-pack/plugins/timelines/public/store/t_grid/actions.ts b/x-pack/plugins/timelines/public/store/t_grid/actions.ts index a039a236fb186..feab12b616c78 100644 --- a/x-pack/plugins/timelines/public/store/t_grid/actions.ts +++ b/x-pack/plugins/timelines/public/store/t_grid/actions.ts @@ -32,6 +32,17 @@ export const applyDeltaToColumnWidth = actionCreator<{ delta: number; }>('APPLY_DELTA_TO_COLUMN_WIDTH'); +export const updateColumnOrder = actionCreator<{ + columnIds: string[]; + id: string; +}>('UPDATE_COLUMN_ORDER'); + +export const updateColumnWidth = actionCreator<{ + columnId: string; + id: string; + width: number; +}>('UPDATE_COLUMN_WIDTH'); + export type ToggleDetailPanel = TimelineExpandedDetailType & { tabType?: TimelineTabs; timelineId: string; diff --git a/x-pack/plugins/timelines/public/store/t_grid/helpers.test.tsx b/x-pack/plugins/timelines/public/store/t_grid/helpers.test.tsx index 121e5bda78ed8..1e1fbe290a115 100644 --- a/x-pack/plugins/timelines/public/store/t_grid/helpers.test.tsx +++ b/x-pack/plugins/timelines/public/store/t_grid/helpers.test.tsx @@ -7,7 +7,11 @@ import { SortColumnTimeline } from '../../../common'; import { tGridDefaults } from './defaults'; -import { setInitializeTgridSettings } from './helpers'; +import { + setInitializeTgridSettings, + updateTGridColumnOrder, + updateTGridColumnWidth, +} from './helpers'; import { mockGlobalState } from '../../mock/global_state'; import { TGridModelSettings } from '.'; @@ -57,3 +61,112 @@ describe('setInitializeTgridSettings', () => { expect(result).toBe(timelineById); }); }); + +describe('updateTGridColumnOrder', () => { + test('it returns the columns in the new expected order', () => { + const originalIdOrder = defaultTimelineById.test.columns.map((x) => x.id); // ['@timestamp', 'event.severity', 'event.category', '...'] + + // the new order swaps the positions of the first and second columns: + const newIdOrder = [originalIdOrder[1], originalIdOrder[0], ...originalIdOrder.slice(2)]; // ['event.severity', '@timestamp', 'event.category', '...'] + + expect( + updateTGridColumnOrder({ + columnIds: newIdOrder, + id: 'test', + timelineById: defaultTimelineById, + }) + ).toEqual({ + ...defaultTimelineById, + test: { + ...defaultTimelineById.test, + columns: [ + defaultTimelineById.test.columns[1], // event.severity + defaultTimelineById.test.columns[0], // @timestamp + ...defaultTimelineById.test.columns.slice(2), // all remaining columns + ], + }, + }); + }); + + test('it omits unknown column IDs when re-ordering columns', () => { + const originalIdOrder = defaultTimelineById.test.columns.map((x) => x.id); // ['@timestamp', 'event.severity', 'event.category', '...'] + const unknownColumId = 'does.not.exist'; + const newIdOrder = [originalIdOrder[0], unknownColumId, ...originalIdOrder.slice(1)]; // ['@timestamp', 'does.not.exist', 'event.severity', 'event.category', '...'] + + expect( + updateTGridColumnOrder({ + columnIds: newIdOrder, + id: 'test', + timelineById: defaultTimelineById, + }) + ).toEqual({ + ...defaultTimelineById, + test: { + ...defaultTimelineById.test, + }, + }); + }); + + test('it returns an empty collection of columns if none of the new column IDs are found', () => { + const newIdOrder = ['this.id.does.NOT.exist', 'this.id.also.does.NOT.exist']; // all unknown IDs + + expect( + updateTGridColumnOrder({ + columnIds: newIdOrder, + id: 'test', + timelineById: defaultTimelineById, + }) + ).toEqual({ + ...defaultTimelineById, + test: { + ...defaultTimelineById.test, + columns: [], // <-- empty, because none of the new column IDs match the old IDs + }, + }); + }); +}); + +describe('updateTGridColumnWidth', () => { + test("it updates (only) the specified column's width", () => { + const columnId = '@timestamp'; + const width = 1234; + + const expectedUpdatedColumn = { + ...defaultTimelineById.test.columns[0], // @timestamp + initialWidth: width, + }; + + expect( + updateTGridColumnWidth({ + columnId, + id: 'test', + timelineById: defaultTimelineById, + width, + }) + ).toEqual({ + ...defaultTimelineById, + test: { + ...defaultTimelineById.test, + columns: [expectedUpdatedColumn, ...defaultTimelineById.test.columns.slice(1)], + }, + }); + }); + + test('it is a noop if the the specified column is unknown', () => { + const unknownColumId = 'does.not.exist'; + + expect( + updateTGridColumnWidth({ + columnId: unknownColumId, + id: 'test', + timelineById: defaultTimelineById, + width: 90210, + }) + ).toEqual({ + ...defaultTimelineById, + test: { + ...defaultTimelineById.test, + }, + }); + }); +}); diff --git a/x-pack/plugins/timelines/public/store/t_grid/helpers.ts b/x-pack/plugins/timelines/public/store/t_grid/helpers.ts index f7b0d86f88621..34de86d32a9b2 100644 --- a/x-pack/plugins/timelines/public/store/t_grid/helpers.ts +++ b/x-pack/plugins/timelines/public/store/t_grid/helpers.ts @@ -8,6 +8,7 @@ import { omit, union } from 'lodash/fp'; import { isEmpty } from 'lodash'; +import { EuiDataGridColumn } from '@elastic/eui'; import type { ToggleDetailPanel } from './actions'; import { TGridPersistInput, TimelineById, TimelineId } from './types'; import type { TGridModel, TGridModelSettings } from './model'; @@ -232,6 +233,63 @@ export const applyDeltaToTimelineColumnWidth = ({ }; }; +type Columns = Array< + Pick & ColumnHeaderOptions +>; + +export const updateTGridColumnOrder = ({ + columnIds, + id, + timelineById, +}: { + columnIds: string[]; + id: string; + timelineById: TimelineById; +}): TimelineById => { + const timeline = timelineById[id]; + + const columns = columnIds.reduce((acc, cid) => { + const columnIndex = timeline.columns.findIndex((c) => c.id === cid); + + return columnIndex !== -1 ? [...acc, timeline.columns[columnIndex]] : acc; + }, []); + + return { + ...timelineById, + [id]: { + ...timeline, + columns, + }, + }; +}; + +export const updateTGridColumnWidth = ({ + columnId, + id, + timelineById, + width, +}: { + columnId: string; + id: string; + timelineById: TimelineById; + width: number; +}): TimelineById => { + const timeline = timelineById[id]; + + const columns = timeline.columns.map((x) => ({ + ...x, + initialWidth: x.id === columnId ? width : x.initialWidth, + })); + + return { + ...timelineById, + [id]: { + ...timeline, + columns, + }, + }; +}; + interface UpdateTimelineColumnsParams { id: string; columns: ColumnHeaderOptions[]; diff --git a/x-pack/plugins/timelines/public/store/t_grid/reducer.ts b/x-pack/plugins/timelines/public/store/t_grid/reducer.ts index d29240d5658db..d3af1dc4e9b30 100644 --- a/x-pack/plugins/timelines/public/store/t_grid/reducer.ts +++ b/x-pack/plugins/timelines/public/store/t_grid/reducer.ts @@ -23,7 +23,9 @@ import { setSelected, setTimelineUpdatedAt, toggleDetailPanel, + updateColumnOrder, updateColumns, + updateColumnWidth, updateIsLoading, updateItemsPerPage, updateItemsPerPageOptions, @@ -40,6 +42,8 @@ import { setDeletedTimelineEvents, setLoadingTimelineEvents, setSelectedTimelineEvents, + updateTGridColumnOrder, + updateTGridColumnWidth, updateTimelineColumns, updateTimelineItemsPerPage, updateTimelinePerPageOptions, @@ -91,6 +95,23 @@ export const tGridReducer = reducerWithInitialState(initialTGridState) timelineById: state.timelineById, }), })) + .case(updateColumnOrder, (state, { id, columnIds }) => ({ + ...state, + timelineById: updateTGridColumnOrder({ + columnIds, + id, + timelineById: state.timelineById, + }), + })) + .case(updateColumnWidth, (state, { id, columnId, width }) => ({ + ...state, + timelineById: updateTGridColumnWidth({ + columnId, + id, + timelineById: state.timelineById, + width, + }), + })) .case(removeColumn, (state, { id, columnId }) => ({ ...state, timelineById: removeTimelineColumn({ diff --git a/x-pack/plugins/translations/translations/ja-JP.json b/x-pack/plugins/translations/translations/ja-JP.json index 6a97078082117..d67126fdad4bb 100644 --- a/x-pack/plugins/translations/translations/ja-JP.json +++ b/x-pack/plugins/translations/translations/ja-JP.json @@ -24458,7 +24458,6 @@ "xpack.timelines.timeline.fieldTooltip": "フィールド", "xpack.timelines.timeline.flyout.pane.removeColumnButtonLabel": "列を削除", "xpack.timelines.timeline.fullScreenButton": "全画面", - "xpack.timelines.timeline.hideColumnLabel": "列を非表示", "xpack.timelines.timeline.openedAlertFailedToastMessage": "アラートを開けませんでした", "xpack.timelines.timeline.openSelectedTitle": "選択した項目を開く", "xpack.timelines.timeline.properties.timelineToggleButtonAriaLabel": "タイムライン {title} を{isOpen, select, false {開く} true {閉じる} other {切り替える}}", diff --git a/x-pack/plugins/translations/translations/zh-CN.json b/x-pack/plugins/translations/translations/zh-CN.json index 6e09814d015cc..598a5c24bdee2 100644 --- a/x-pack/plugins/translations/translations/zh-CN.json +++ b/x-pack/plugins/translations/translations/zh-CN.json @@ -24874,7 +24874,6 @@ "xpack.timelines.timeline.fieldTooltip": "字段", "xpack.timelines.timeline.flyout.pane.removeColumnButtonLabel": "移除列", "xpack.timelines.timeline.fullScreenButton": "全屏", - "xpack.timelines.timeline.hideColumnLabel": "隐藏列", "xpack.timelines.timeline.openedAlertFailedToastMessage": "无法打开告警", "xpack.timelines.timeline.openedAlertSuccessToastMessage": "已成功打开 {totalAlerts} 个{totalAlerts, plural, other {告警}}。", "xpack.timelines.timeline.openSelectedTitle": "打开所选",