[Security Solution][Exceptions] - Exceptions modal pt 2 #70886

dplumlee · 2020-07-06T21:50:43Z

Summary

Part 2 of the exceptions modal feature. Adds on more functionality and tests to the exceptions modal, and switches certain patterns from a testing convention to what they will be in actual usage.

Coming in Part 3

In add exception modal, if the close checkbox is selected, refresh the alerts table on submit
Look into using lists plugin hooks like the persistExceptionItem hook
Filter out fields not included in the list of Endpoint "exceptionable" fields when the user tries to add an Endpoint Exception
Allow the user to add Exceptions from the Timeline
Disable ORs when editing an exception item. Need to coordinate with @dontcallmesherryli
Add tests for react components
Handle case when user deletes an item in the Edit Modal.
Implement bulk close. Currently the checkbox doesn't trigger a bulk close action on the server.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

dontcallmesherryli · 2020-07-08T16:19:29Z

Some things I observed that needs to be addressed that isn't in checklist above:

"Open alerts", "In progress alerts", and "Closed alerts" should loose the "alert" wording. --> Tabs named "Open", "In progress" and "Closed".
According to Marra's mocks, I thought the action overflow menu would also include "edit action" as option.
Mocks for Exception: https://www.figma.com/file/jcCKnGXvOlFxMOpUjlTMMz/All-Exceptions?node-id=1%3A2047
This is in the checklist: Disable "Add Endpoint Exception" option for non-Endpoint alerts. -- Please make sure to disable it both in the Alert List overflow menu, as well as in the rules exception page:
The spacing for Endpoint exception modal seems off. The "close alert" part on the bottom seems to be taking too much real estate.
Weird loading of the action overflow menu when user clicks on overflow menu before the page fully loads. It's just texts of the action options floating on top of the alert list.
I can add an Endpoint exception without any info entered in the current Endpoint exception modal. When viewing the exception in the rules page, it shows a much of empty fields. (Is that expected behavior?)
Within the Alert list, once user selects "select all 1,700 alerts", this selection doesn't auto unselect when user finishes taking action on the full selection. So when user wants to select 1 or 2 alerts and do things to it, it's still selecting 1,700 alerts. See screenshot:

.../plugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx

elasticmachine · 2020-07-09T15:17:25Z

Pinging @elastic/siem (Team:SIEM)

FrankHassanabad · 2020-07-09T16:36:42Z

x-pack/plugins/security_solution/public/common/components/exceptions/helpers.tsx

+
+  function doesFieldNameExist(exceptionEntry: Entry) {
+    return indexPatterns.fields.find(({ name }) => name === exceptionEntry.field) === undefined;
+  }


Is this named right? It looks like it seeing if there is nothing here through the check against undefined? As if it is saying "doesFieldNameNotExist"?

Some pointers/nits (nothing to hold this up), I would still suggest we follow the flow of most of the code base looking at == null rather than something as specific as undefined since == null will catch both null and undefined values. Most of the other plugins and core code base typically follow that pattern.

I would also use a .some vs. a .find() === undefined if that makes more semantic sense within arrays. Most of the times these days I rarely find a reason to use a for loop instead of one of the more semantic array methods such as .some, .every, .map, .reduce, .flat. etc.... I still sometimes need to use a .forEach or for of but I almost always use it as a last resort and try to see if there's a more semantic loop from the array that makes sense first.

Even looking above this at the loops above it, I mentally rewrite it in my mind as a series of .some() loops as it seems to be doing a series of loops looking for the first entry to become true to return true but if it cannot find it, then it returns false.

For pure functions, I would be careful of introducing functions within functions. They're harder to isolate and test vs pulling the function out and then testing outside of the original function. Since they're pure functions, I don't really worry about other people calling them if they want to as it doesn't have side effects.

Although we don't have strict linter rules around ES2015+ style fat arrows vs. function names, I would stick to one style within a file when you can and use for example the ES2015+ style.

I still recommend to define the return types within TypeScript for functions as it makes it easier to keep other maintainers from accidentally introducing a new return type by accident.

yctercero · 2020-07-09T16:57:20Z

.../plugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx

@@ -65,7 +66,7 @@ interface AddExceptionModalProps {
    nonEcsData: TimelineNonEcsData[];
  };
  onCancel: () => void;
-  onConfirm: () => void;
+  onConfirm: (didCloseAlert?: boolean) => void;


It seems from all the uses of onConfirm that the boolean argument is always present, do we need the ??

yctercero · 2020-07-09T16:59:55Z

.../plugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx

  const enrichExceptionItems = useCallback(() => {
    let enriched: Array<ExceptionListItemSchema | CreateExceptionListItemSchema> = [];
    enriched =
      comment !== ''
        ? enrichExceptionItemsWithComments(exceptionItemsToAdd, [{ comment }])
        : exceptionItemsToAdd;
    if (exceptionListType === 'endpoint') {
-      const osTypes = alertData ? ['windows'] : ['windows', 'macos', 'linux'];
+      const osTypes = alertData ? retrieveAlertOsTypes() : ['windows', 'macos', 'linux'];


Nit: Might be able to just clean up a bit and add the alertData check within retrieveAlertOsTypes so if someone else later uses it they don't need to remember to make this check?

yctercero · 2020-07-09T17:04:35Z

x-pack/plugins/security_solution/public/common/components/exceptions/helpers.test.tsx

@@ -248,6 +258,36 @@ describe('Exception helpers', () => {
    });
  });

+  describe('#getEntryValue', () => {


Thanks for adding these!

yctercero · 2020-07-09T17:10:55Z

x-pack/plugins/security_solution/public/common/components/exceptions/helpers.test.tsx

@@ -278,27 +318,33 @@ describe('Exception helpers', () => {

  describe('#getOperatingSystems', () => {


Should this be formatOperatingSystems?

yctercero · 2020-07-09T17:13:22Z

x-pack/plugins/security_solution/public/common/components/exceptions/helpers.tsx

  const item = data.find((d) => d.field === fieldName);
  if (item != null && item.value != null) {
    return item.value;
  }
-  return undefined;
+  return [];
 };

 export const entryHasListType = (
  exceptionItems: Array<ExceptionListItemSchema | CreateExceptionListItemSchema>
 ) => {
  for (const { entries } of exceptionItems) {
    for (const exceptionEntry of entries ?? []) {


entries gets defaulted to empty array, does Typescript show an error without the ?? [] because then I'm wondering if we missed something somewhere.

since we type CreateExceptionListItemSchema as a potential type within the array, it's doesn't hasn't been defaulted yet

Ah, gotcha. Is this being used to check exceptions created within the builder? If that's the case I would type it to ExceptionsBuilderExceptionItem[]. That uses ExceptionListItemBuilderSchema | CreateExceptionListItemBuilderSchema and CreateExceptionListItemBuilderSchema is

xport type CreateExceptionListItemBuilderSchema = Omit< CreateExceptionListItemSchema, 'meta' | 'entries' > & { meta: { temporaryUuid: string }; entries: BuilderEntry[]; };

But I guess, that's only if it's being used in the builder.

Nah, only being used in both versions of the exception modals so it's usually being passed that exceptionItemsToAdd we define with the same Array<ExceptionListItemSchema | CreateExceptionListItemSchema> type in component state

yctercero · 2020-07-09T17:18:39Z

x-pack/plugins/security_solution/public/detections/components/alerts_table/default_config.tsx

@@ -323,20 +338,20 @@ export const getAlertActions = ({
        }
      },
      id: 'addEndpointException',
-      isActionDisabled: () => !canUserCRUD || !hasIndexWrite,
+      isActionDisabled: () => !canUserCRUD || !hasIndexWrite || isEndpointAlert() === false,


Super nit: I see sometimes we check for falsiness using ! and other times we do === false, is there a specific reason for one over the other throughout? Same with checking for true. I think I've noticed that in the codebase when wanting to check if something exists we use != null to check for both null or undefined. That way when I see someone use ! I usually assume that value is a boolean.

yctercero · 2020-07-09T17:23:37Z

x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx

-    closeAddExceptionModal();
-  }, [closeAddExceptionModal]);
+  const onAddExceptionConfirm = useCallback(
+    (didCloseAlert?: boolean) => {


Nit: I might make this explicit and remove the ?

yctercero

LGTM. Thanks for the changes! I chatted with @peluja1012 about the index/autocomplete weirdness. I don't think it's related to any of the code, but maybe it's something we can circle back on.

FrankHassanabad

LGTM! Thanks for all the great typescript and work on this. This is really clean easy to read code.

kibanamachine · 2020-07-09T23:41:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0269167

Build metrics

✅ unchanged

History

💛 Build #60415 was flaky b5c13d4
💔 Build #60386 failed 4262c59
💔 Build #60262 failed 163701d
💔 Build #59908 failed 800d8b82308a4e3b09cd9949a70b760ac6211394
💔 Build #59896 failed 7a7d454758ebbb912872d436e466f29044ab4210

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

) * makes comment updates * adds tests * adds back non ecs data to timeline * comments * fixes jest tests * fixes typo Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>

…11y-overlay * 'master' of github.com:elastic/kibana: (33 commits) address index templates feedback (elastic#71353) Upgrade EUI to v26.3.1 (elastic#70243) [build] Creates Linux aarch64 archive (elastic#69165) [SIEM][Detection Engine] Fixes skipped tests (elastic#71347) [SIEM][Detection Engine][Lists] Adds read_privileges route for lists and list items [kbn/optimizer] implement "requiredBundles" property of KP plugins (elastic#70911) [Security Solution][Exceptions] - Exceptions modal pt 2 (elastic#70886) [ML] DF Analytics: stop status polling when job stopped (elastic#71159) [SIEM][CASE] IBM Resilient Connector (elastic#66385) jenkins_xpack_saved_objects_field_metrics.sh expects to be run from the KIBANA_DIR in CI Deduplication of entries and items before sending to endpoint (elastic#71297) [services/remote/webdriver] fix eslint error (elastic#71346) send slack notifications on visual baseline failures fix visual regression job (elastic#70999) [Ingest Manager] Add schema to usageCollector. (elastic#71219) [ftr] use typed chromeOptions object, adding TEST_BROWSER_BINARY_PATH (elastic#71279) [Ingest Manager] Fix limited packages incorrect response (elastic#71292) Support multiple features declaring same properties (elastic#71106) [Security_Solution][Resolver]Add beta badge to Resolver panel (elastic#71183) [DOCS] Clarify trial subscription levels (elastic#70636) ...

* master: (78 commits) Bump lodash package version (elastic#71392) refactor: 💡 use allow-list in AppArch codebase (elastic#71400) improve bugfix 7198 test stability (elastic#71250) [Security Solution][Ingest Manager][Endpoint] Optional ingest manager (elastic#71198) [Metrics UI] Round metric threshold time buckets to nearest unit (elastic#71172) [Security Solution][Endpoint] Policy creation callback fixes + Improved error handling in user manifest loop (elastic#71269) [Security Solution] Allow to configure Event Renderers settings (elastic#69693) Fix a11y keyboard overlay (elastic#71214) [APM] UI text updates (elastic#71333) [Logs UI] Limit `extendDatemath` to valid ranges (elastic#71113) [SIEM] fix tooltip of notes (elastic#71342) address index templates feedback (elastic#71353) Upgrade EUI to v26.3.1 (elastic#70243) [build] Creates Linux aarch64 archive (elastic#69165) [SIEM][Detection Engine] Fixes skipped tests (elastic#71347) [SIEM][Detection Engine][Lists] Adds read_privileges route for lists and list items [kbn/optimizer] implement "requiredBundles" property of KP plugins (elastic#70911) [Security Solution][Exceptions] - Exceptions modal pt 2 (elastic#70886) [ML] DF Analytics: stop status polling when job stopped (elastic#71159) [SIEM][CASE] IBM Resilient Connector (elastic#66385) ...

elasticmachine · 2021-09-23T14:31:54Z

Pinging @elastic/security-solution (Team: SecuritySolution)

dplumlee force-pushed the exception-modal-2 branch from 7a7d454 to 800d8b8 Compare July 8, 2020 19:30

peluja1012 reviewed Jul 8, 2020

View reviewed changes

.../plugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx Outdated Show resolved Hide resolved

dplumlee added 3 commits July 9, 2020 11:05

makes comment updates

9816522

adds tests

1459799

adds back non ecs data to timeline

163701d

dplumlee force-pushed the exception-modal-2 branch from ec7e94d to 163701d Compare July 9, 2020 15:06

dplumlee added release_note:enhancement Team:SIEM v7.9.0 v8.0.0 labels Jul 9, 2020

dplumlee changed the title ~~Exception modal 2~~ [Security Solution][Exceptions] - Exceptions modal pt 2 Jul 9, 2020

dplumlee marked this pull request as ready for review July 9, 2020 15:17

dplumlee requested review from a team as code owners July 9, 2020 15:17

FrankHassanabad reviewed Jul 9, 2020

View reviewed changes

yctercero reviewed Jul 9, 2020

View reviewed changes

dplumlee added 2 commits July 9, 2020 14:34

comments

4262c59

fixes jest tests

b5c13d4

yctercero approved these changes Jul 9, 2020

View reviewed changes

peluja1012 approved these changes Jul 9, 2020

View reviewed changes

fixes typo

0269167

FrankHassanabad approved these changes Jul 9, 2020

View reviewed changes

peluja1012 merged commit c1b2665 into elastic:master Jul 10, 2020

peluja1012 mentioned this pull request Jul 10, 2020

[7.x] [Security Solution][Exceptions] - Exceptions modal pt 2 (#70886) #71354

Merged

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Exceptions] - Exceptions modal pt 2 #70886

[Security Solution][Exceptions] - Exceptions modal pt 2 #70886

dplumlee commented Jul 6, 2020 •

edited

Loading

dontcallmesherryli commented Jul 8, 2020 •

edited

Loading

elasticmachine commented Jul 9, 2020

FrankHassanabad Jul 9, 2020

yctercero Jul 9, 2020

yctercero Jul 9, 2020

yctercero Jul 9, 2020

yctercero Jul 9, 2020

yctercero Jul 9, 2020

dplumlee Jul 9, 2020

yctercero Jul 9, 2020

dplumlee Jul 9, 2020

yctercero Jul 9, 2020 •

edited

Loading

yctercero Jul 9, 2020

yctercero left a comment

FrankHassanabad left a comment

kibanamachine commented Jul 9, 2020

elasticmachine commented Sep 23, 2021

		@@ -278,27 +318,33 @@ describe('Exception helpers', () => {

		describe('#getOperatingSystems', () => {

[Security Solution][Exceptions] - Exceptions modal pt 2 #70886

[Security Solution][Exceptions] - Exceptions modal pt 2 #70886

Conversation

dplumlee commented Jul 6, 2020 • edited Loading

Summary

Coming in Part 3

Checklist

For maintainers

dontcallmesherryli commented Jul 8, 2020 • edited Loading

elasticmachine commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yctercero Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yctercero left a comment

Choose a reason for hiding this comment

FrankHassanabad left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 9, 2020

💚 Build Succeeded

Build metrics

History

elasticmachine commented Sep 23, 2021

dplumlee commented Jul 6, 2020 •

edited

Loading

dontcallmesherryli commented Jul 8, 2020 •

edited

Loading

yctercero Jul 9, 2020 •

edited

Loading