[Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values #90634

yctercero · 2021-02-08T15:46:16Z

Summary

This PR is a follow-up to #89066 - which fixed the same issue occurring with indicator match lists UI. The lack of stable ids for exception item entries resulted in some funky business by where invalid values could overwrite other values when deleting entries in the builder.

Some interesting discussions came of it of whether it's tech debt from not adding such ids to begin with into the exceptions data structure. On the other side of the argument is that the ids have only really been needed for React specific purposes. The fix is to add a uuid.v4() to entry items at the API inbound boundary and a util to strip such id out on the outbound boundaries.

At first, thought of adding the id as close to the boundary as possible, so then anyone using the exceptions API on the UI would receive this added functionality. After a bit of discussion with team, found it made more sense to keep it to the hooks and leave the api calls themselves pure.

Bug

Notice I select start with a, c, d, e --> select to delete d --> remain with a, c, d

bug_1.mov

Notice I select start with a, c --> select to delete a --> all clear out

bug_2.mov

Fix

Notice I select start with [a,b], [c,d,e] --> select to delete d --> left with [a,b], [c, e]

Notice I select start with [b], [c,e] --> select to delete c --> left with [b], [e] --> select to delete b --> left with [e]

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

… yet

…t changes yet

yctercero · 2021-02-08T21:04:21Z

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

-        if (validatedEntry != null || validatedNestedEntry != null) {
-          return true;
-        }
+        if (singleEntry.type === 'nested') {


A bit more verbose but I think easier to understand.

…ceptions

elasticmachine · 2021-02-12T03:10:57Z

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

elasticmachine · 2021-02-12T03:10:58Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

x-pack/plugins/lists/public/exceptions/transforms.ts

yctercero · 2021-02-12T03:17:06Z

x-pack/test/security_solution_cypress/es_archives/exceptions/data.json

@@ -0,0 +1,23 @@
+{


For the modal cypress tests, it was unnecessary to load a large file such as auditbeat, needed a small simple nested a non-nested field to test with is all.

…ceptions

…ting lists plugin useApi hook (#92348) Doing a quick refactor to help with #90634 . While working on #90634 found that I would introduce a circular dependency if I didn't refactor the hook used by the exception builder component to make use of the existing useApi hook in the lists plugin. #90634 adds temporary ids to item entries to mitigate some React key requirements and the logic to remove/add these id's isn't at the boundary but in the hooks (so as not to pollute the data for everyone wanting to use the api. An upside is that it removed some of the looping and seemed to speed things up a bit. I briefly considered adding the bulk SO endpoints for exception items, but they are still experimental.

…ting lists plugin useApi hook (elastic#92348) Doing a quick refactor to help with elastic#90634 . While working on elastic#90634 found that I would introduce a circular dependency if I didn't refactor the hook used by the exception builder component to make use of the existing useApi hook in the lists plugin. elastic#90634 adds temporary ids to item entries to mitigate some React key requirements and the logic to remove/add these id's isn't at the boundary but in the hooks (so as not to pollute the data for everyone wanting to use the api. An upside is that it removed some of the looping and seemed to speed things up a bit. I briefly considered adding the bulk SO endpoints for exception items, but they are still experimental.

…ceptions

…ting lists plugin useApi hook (#92348) (#92534) Doing a quick refactor to help with #90634 . While working on #90634 found that I would introduce a circular dependency if I didn't refactor the hook used by the exception builder component to make use of the existing useApi hook in the lists plugin. #90634 adds temporary ids to item entries to mitigate some React key requirements and the logic to remove/add these id's isn't at the boundary but in the hooks (so as not to pollute the data for everyone wanting to use the api. An upside is that it removed some of the looping and seemed to speed things up a bit. I briefly considered adding the bulk SO endpoints for exception items, but they are still experimental.

…ting lists plugin useApi hook (#92348) (#92535) Doing a quick refactor to help with #90634 . While working on #90634 found that I would introduce a circular dependency if I didn't refactor the hook used by the exception builder component to make use of the existing useApi hook in the lists plugin. #90634 adds temporary ids to item entries to mitigate some React key requirements and the logic to remove/add these id's isn't at the boundary but in the hooks (so as not to pollute the data for everyone wanting to use the api. An upside is that it removed some of the looping and seemed to speed things up a bit. I briefly considered adding the bulk SO endpoints for exception items, but they are still experimental.

madirey · 2021-02-24T21:09:19Z

x-pack/plugins/lists/common/constants.mock.ts

+        operator: 'included',
+        type: 'match',
+        value: 'some value',
+      } as EntryMatch & { id: string },


Nit, but you could create an EntryId to reuse the { id: string } pattern that occurs frequently. May not be worth it.

fwiw, when I fixed mine id's up, I got the same feedback where the suggestion was I just do a cast and very carefully do the cast in a few areas from @rylnd as the lesser of the evils and only add the extra id in only a few typings and only where I needed it. That felt more right to get past this issue.

I did this type of thing very carefully only where I needed the id:

const maybeId: typeof item & { id?: string } = item;

Everywhere else I left the typings alone. I think @madirey 's suggestions are good as well, it's a bit of a balancing act and your choice in these areas.

Later if we push the id down to the data layer or in the REST routes we can make the id's part of the schema and be ok as well. This all looks good to me here as is though too.

Thanks, yea I could definitely see that it could maybe be cleaned up. I tried to add comments where there were casts to give an explanation of it.

There's a number of refactors that I'd like to do to follow up. If it's ok, I think it could be addressed then?

madirey · 2021-02-24T21:14:05Z

x-pack/plugins/lists/public/exceptions/hooks/persist_exception_item.test.ts

+
+  test('it invokes "addExceptionListItem" when payload has "id"', async () => {
+    const updateExceptionItem = jest.spyOn(api, 'updateExceptionListItem');
+    const createExceptionItem = jest.spyOn(api, 'addExceptionListItem');


The naming here is a little confusing because it's called addExceptionItem in the test above this one, yet it's mocking the same thing.

FrankHassanabad · 2021-02-24T21:39:06Z

x-pack/test/security_solution_cypress/es_archives/exceptions/mappings.json

+      }
+    }
+  }
+}


fwiw, something I wished I had done before was add this to some of my tests and mappings:

https://github.com/elastic/kibana/blob/master/x-pack/test/functional/es_archives/filebeat/default/mappings.json#L5936

Which is a "refresh_interval": "5s" it reduces flake and speeds things up if you're pushing data from es archiver into it. Optional as I forget to add it. Without it, I think the default of tests are 1 minute. They might have changed the default for tests that I don't know about, but I don't know the answer to that either.

madirey · 2021-02-24T22:52:46Z

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

@@ -282,6 +285,7 @@ export const getUpdatedEntriesOnDelete = (
      const { field } = itemOfInterest;
      const updatedItemOfInterest: EntryNested | EmptyNestedEntry = {
        field,
+        id: itemOfInterest.id ?? `${entryIndex}`,


Will this exceptional case ever happen?

id is mapped to string | undefined as I can't truly guarantee that the id will be there given it's not actually part of the schema so I hesitated to not use id?. The entries that are typed as empty entries have id set as just string (as opposed to string | undefined) because we're creating those client side and can guarantee we'll stick an id in there.

…ceptions

kibanamachine · 2021-02-25T02:12:48Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lists`	130	131	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	7.7MB	7.7MB	+1.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`lists`	143.5KB	145.2KB	+1.7KB
`securitySolution`	234.9KB	235.8KB	+948.0B
total			+2.6KB

History

💛 Build #109199 was flaky 7825c7b
💔 Build #108962 failed a4fd2f3
💔 Build #108949 failed a12f211
💔 Build #108631 failed d63064b
💔 Build #106329 failed 65ee43e

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

cc @yctercero

…nvalid values can cause overwrites of other values (elastic#90634) ### Summary This PR is a follow-up to elastic#89066 - which fixed the same issue occurring with indicator match lists UI. The lack of stable ids for exception item entries resulted in some funky business by where invalid values could overwrite other values when deleting entries in the builder.

…nvalid values can cause overwrites of other values (#90634) (#92749) ### Summary This PR is a follow-up to #89066 - which fixed the same issue occurring with indicator match lists UI. The lack of stable ids for exception item entries resulted in some funky business by where invalid values could overwrite other values when deleting entries in the builder. Co-authored-by: Kibana Machine <[email protected]>

* master: (38 commits) Fixes Cypress flake by adding pipe, click, and should (elastic#92762) [Discover] Fix filtering selected sidebar fields (elastic#91828) [ML] Fixes positions of calendar arrow buttons in start datafeed modal (elastic#92625) [dev/build_ts_refs] check that commit in outDirs matches mergeBase (elastic#92513) add dep on `@kbn/config` so it is built first [Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn (elastic#90481) [Lens] Fix Workspace hidden when using Safari (elastic#92616) [Lens] Fixes vertical alignment validation messages (elastic#91878) forbid x-elastic-product-origin header in elasticsearch configuration (elastic#92359) [Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules (elastic#92081) [ILM][Accessibility] Added A11y test for ILM new policy form. (elastic#92570) [Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values (elastic#90634) Automatically generated Api documentation (elastic#86232) Increase index pattern select limit to 1000 (elastic#92093) [core.logging] Add RewriteAppender for filtering LogMeta. (elastic#91492) [Security Solution][Detection Rules] Update prebuilt rule threats to match schema (elastic#92281) [Security Solutions][Detection Engine] Fixes bug with not being able to duplicate indicator matches (elastic#92565) [Dashboard] Export appropriate references from byValue panels (elastic#91567) [Upgrade Assistant] Align code between branches (elastic#91862) [Security Solution][Case] Fix alerts push (elastic#91638) ...

…nvalid values can cause overwrites of other values (#90634) (#92750) ### Summary This PR is a follow-up to #89066 - which fixed the same issue occurring with indicator match lists UI. The lack of stable ids for exception item entries resulted in some funky business by where invalid values could overwrite other values when deleting entries in the builder. Co-authored-by: Kibana Machine <[email protected]>

yctercero added 4 commits February 5, 2021 13:05

added ids to incoming exception item entries, no ui component changes…

7a35075

… yet

added code to strip ids from outgoing exception items, no ui componen…

ec75e51

…t changes yet

made updates to UI portion to now make use of the entry ids

76e2f62

finicky types, attempting to get types right

2320f62

yctercero commented Feb 8, 2021

View reviewed changes

yctercero changed the title ~~[Security Solution][Exceptions] - Fix React keys bug affecting exceptions builder UI~~ [Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values Feb 8, 2021

yctercero added 6 commits February 9, 2021 19:51

Merge branch 'master' of github.com:elastic/kibana into react_keys_ex…

520a662

…ceptions

Merge branch 'master' of github.com:elastic/kibana into react_keys_ex…

6c9d3ec

…ceptions

updating to work with nested

a76e1b2

Merge branch 'master' of github.com:elastic/kibana into react_keys_ex…

fe06f3a

…ceptions

updated types to remove so many castings

5c6b7a8

finalizing tests

65ee43e

yctercero self-assigned this Feb 12, 2021

yctercero added bug Fixes for quality problems that affect the customer experience release_note:fix Team:Detections and Resp Security Detection Response Team v7.12.0 v8.0.0 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Feb 12, 2021

yctercero marked this pull request as ready for review February 12, 2021 03:10

yctercero requested review from a team as code owners February 12, 2021 03:10

yctercero commented Feb 12, 2021

View reviewed changes

x-pack/plugins/lists/public/exceptions/transforms.ts Outdated Show resolved Hide resolved

yctercero commented Feb 12, 2021

View reviewed changes

yctercero added 4 commits February 12, 2021 23:09

Merge branch 'master' of github.com:elastic/kibana into react_keys_ex…

d640f50

…ceptions

Merge branch 'master' of github.com:elastic/kibana into react_keys_ex…

d63064b

…ceptions

Merge branch 'master' of github.com:elastic/kibana into react_keys_ex…

602cfb6

…ceptions

fixed up tests

259fc9d

yctercero mentioned this pull request Feb 23, 2021

[Security Solution][Exceptions] - Update exceptions modal to use existing lists plugin useApi hook #92348

Merged

1 task

yctercero added 2 commits February 23, 2021 15:02

Merge branch 'master' of github.com:elastic/kibana into react_keys_ex…

39933a3

…ceptions

merged with master and fixed tests

a12f211

yctercero added the v7.13.0 label Feb 23, 2021

yctercero added 3 commits February 23, 2021 17:53

cleanup of types

e679318

updated to use i18n in one throw

a4fd2f3

updated types to remove casting

7825c7b

madirey reviewed Feb 24, 2021

View reviewed changes

FrankHassanabad reviewed Feb 24, 2021

View reviewed changes

madirey reviewed Feb 24, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into react_keys_ex…

ffb1bd2

…ceptions

madirey approved these changes Feb 25, 2021

View reviewed changes

adding feedback

3e6e8b6

yctercero merged commit 9d2a7b8 into elastic:master Feb 25, 2021

yctercero mentioned this pull request Feb 25, 2021

[7.x] [Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values (#90634) #92749

Merged

yctercero mentioned this pull request Feb 25, 2021

[7.12] [Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values (#90634) #92750

Merged

yctercero deleted the react_keys_exceptions branch October 13, 2021 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values #90634

[Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values #90634

yctercero commented Feb 8, 2021 •

edited

Loading

yctercero Feb 8, 2021

elasticmachine commented Feb 12, 2021

elasticmachine commented Feb 12, 2021

yctercero Feb 12, 2021

madirey Feb 24, 2021

FrankHassanabad Feb 24, 2021

yctercero Feb 25, 2021

madirey Feb 24, 2021

FrankHassanabad Feb 24, 2021

madirey Feb 24, 2021

yctercero Feb 25, 2021

kibanamachine commented Feb 25, 2021

[Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values #90634

[Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values #90634

Conversation

yctercero commented Feb 8, 2021 • edited Loading

Summary

Bug

Fix

Checklist

Choose a reason for hiding this comment

elasticmachine commented Feb 12, 2021

elasticmachine commented Feb 12, 2021

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

kibanamachine commented Feb 25, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

yctercero commented Feb 8, 2021 •

edited

Loading