[Security Solution][Detection Engine] Fixes indicator matches mapping UI where invalid list values can cause overwrites of other values by FrankHassanabad · Pull Request #89066 · elastic/kibana

FrankHassanabad · 2021-01-22T19:17:39Z

Summary

This fixes the ReactJS keys to not use array indexes for the ReactJS keys which fixes #84893 as well as a few other bugs that I will show below. The fix for the ReactJS keys is to add a unique id version 4 uuid.v4() to the incoming threat_mapping and the entities. On save out to elastic I remove the id. This is considered better practices for ReactJS keys

Down the road we might augment the arrays to have that id information but for now I add them when we get the data and then remove them as we save the data.

This PR also:

Fixes tech debt around the hooks to remove the disabling of the react-hooks/exhaustive-deps in a few areas
Fixes one React Hook misnamed that would not have triggered React linter rules (_useRuleAsyn)
Adds 23 new Cypress e2e tests
Adds a new pattern of dealing with on button clicks for the Cypress tests that are make it less flakey

cy.get(`button[title="${indexField}"]`)
      .should('be.visible')
      .then(([e]) => e.click());

Adds several new utilities to Cypress for testing rows for indicator matches and other Cypress utils to improve velocity and ergonomics

fillIndicatorMatchRow
getDefineContinueButton
getIndicatorInvalidationText
getIndicatorIndexComboField
getIndicatorDeleteButton
getIndicatorOrButton
getIndicatorAndButton

Bug 1

Deleting row 1 can cause row 2 to be cleared out or only partial data to stick around.

Before:

After:

Bug 2

Deleting row 2 in the middle of 3 rows did not shift the value up correctly

Before:

After:

Bug 3

When using OR with values it does not shift up correctly similar to AND

Before:

After:

Checklist

Delete any items that are not applicable to this PR.

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)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…ake test writing more efficient

elasticmachine · 2021-01-28T00:46:01Z

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

FrankHassanabad · 2021-01-28T19:21:14Z

@elasticmachine merge upstream

FrankHassanabad · 2021-01-28T20:18:53Z

@elasticmachine merge upstream

FrankHassanabad · 2021-01-29T00:37:14Z

@elasticmachine merge upstream

kibanamachine · 2021-01-29T00:37:25Z

expected head sha didn’t match current head ref.

rylnd

Tested this locally and the bugs look fixed! Thank you for the review notes, they were incredibly helpful ❤️ .

I had a few nits/hemming/hawing about the cypress stuff, but at the end of the day: it's more test coverage and it can be refactored later if need be. As long as CI is happy, this LGTM.

x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts

x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts

rylnd · 2021-01-29T01:01:11Z

...k/plugins/security_solution/cypress/integration/detection_rules/indicator_match_rule.spec.ts

+        );
+      });
+
+      it('Does NOT show invalidation text on initial page load', () => {


Broad comment: these narrowly-focused tests are great for legibility and test isolation but likely terrible for CI speed. My preference for these types of tests is to walk through a particular user workflow, making assertions along the way until we get to a terminal point, e.g.:

sign in, define rule, about rule, go back to define step and assert values are repopulated, create rule, assert rule created

This seems to be the predominant pattern in our cypress suite right now, so these new tests are a divergence from that. Neither's objectively correct, certainly, and I don't think these need to change, but I wanted to call out the difference. @MadameSheema may have a stronger opinion here 😉

...k/plugins/security_solution/cypress/integration/detection_rules/indicator_match_rule.spec.ts

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

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

rylnd · 2021-01-29T01:27:17Z

.../plugins/security_solution/public/detections/containers/detection_engine/rules/transforms.ts

+ * in the form of:
+ * flow(addIdToThreatMatchArray, myNewTransform)(rule)
+ *
+ * @param rule The rule to transform the output of


💯 on the JSDoc here, much appreciated 👍

… into fix-ui-react-keys

kibanamachine · 2021-01-30T02:08:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 181bce7

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2175	2177	+2

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.5MB	7.5MB	+3.7KB

History

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

… UI where invalid list values can cause overwrites of other values (elastic#89066) ## Summary This fixes the ReactJS keys to not use array indexes for the ReactJS keys which fixes elastic#84893 as well as a few other bugs that I will show below. The fix for the ReactJS keys is to add a unique id version 4 `uuid.v4()` to the incoming threat_mapping and the entities. On save out to elastic I remove the id. This is considered [better practices for ReactJS keys](https://reactjs.org/docs/lists-and-keys.html) Down the road we might augment the arrays to have that id information but for now I add them when we get the data and then remove them as we save the data. This PR also: * Fixes tech debt around the hooks to remove the disabling of the `react-hooks/exhaustive-deps` in a few areas * Fixes one React Hook misnamed that would not have triggered React linter rules (_useRuleAsyn) * Adds 23 new Cypress e2e tests * Adds a new pattern of dealing with on button clicks for the Cypress tests that are make it less flakey ```ts cy.get(`button[title="${indexField}"]`) .should('be.visible') .then(([e]) => e.click()); ``` * Adds several new utilities to Cypress for testing rows for indicator matches and other Cypress utils to improve velocity and ergonomics ```ts fillIndicatorMatchRow getDefineContinueButton getIndicatorInvalidationText getIndicatorIndexComboField getIndicatorDeleteButton getIndicatorOrButton getIndicatorAndButton ``` ## Bug 1 Deleting row 1 can cause row 2 to be cleared out or only partial data to stick around. Before: ![im_bug_1](https://user-images.githubusercontent.com/1151048/105916137-c57b1d80-5fed-11eb-95b7-ad25b71cf4b8.gif) After: ![im_fix_1_1](https://user-images.githubusercontent.com/1151048/105917509-9fef1380-5fef-11eb-98eb-025c226f79fe.gif) ## Bug 2 Deleting row 2 in the middle of 3 rows did not shift the value up correctly Before: ![im_bug_2](https://user-images.githubusercontent.com/1151048/105917584-c01ed280-5fef-11eb-8c5b-fefb36f81008.gif) After: ![im_fix_2](https://user-images.githubusercontent.com/1151048/105917650-e0e72800-5fef-11eb-9fd3-020d52e4e3b1.gif) ## Bug 3 When using OR with values it does not shift up correctly similar to AND Before: ![im_bug_3](https://user-images.githubusercontent.com/1151048/105917691-f2303480-5fef-11eb-9368-b11d23159606.gif) After: ![im_fix_3](https://user-images.githubusercontent.com/1151048/105917714-f9574280-5fef-11eb-9be4-1f56c207525a.gif) ### Checklist Delete any items that are not applicable to this PR. - [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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

… UI where invalid list values can cause overwrites of other values (#89066) (#89817) ## Summary This fixes the ReactJS keys to not use array indexes for the ReactJS keys which fixes #84893 as well as a few other bugs that I will show below. The fix for the ReactJS keys is to add a unique id version 4 `uuid.v4()` to the incoming threat_mapping and the entities. On save out to elastic I remove the id. This is considered [better practices for ReactJS keys](https://reactjs.org/docs/lists-and-keys.html) Down the road we might augment the arrays to have that id information but for now I add them when we get the data and then remove them as we save the data. This PR also: * Fixes tech debt around the hooks to remove the disabling of the `react-hooks/exhaustive-deps` in a few areas * Fixes one React Hook misnamed that would not have triggered React linter rules (_useRuleAsyn) * Adds 23 new Cypress e2e tests * Adds a new pattern of dealing with on button clicks for the Cypress tests that are make it less flakey ```ts cy.get(`button[title="${indexField}"]`) .should('be.visible') .then(([e]) => e.click()); ``` * Adds several new utilities to Cypress for testing rows for indicator matches and other Cypress utils to improve velocity and ergonomics ```ts fillIndicatorMatchRow getDefineContinueButton getIndicatorInvalidationText getIndicatorIndexComboField getIndicatorDeleteButton getIndicatorOrButton getIndicatorAndButton ``` ## Bug 1 Deleting row 1 can cause row 2 to be cleared out or only partial data to stick around. Before: ![im_bug_1](https://user-images.githubusercontent.com/1151048/105916137-c57b1d80-5fed-11eb-95b7-ad25b71cf4b8.gif) After: ![im_fix_1_1](https://user-images.githubusercontent.com/1151048/105917509-9fef1380-5fef-11eb-98eb-025c226f79fe.gif) ## Bug 2 Deleting row 2 in the middle of 3 rows did not shift the value up correctly Before: ![im_bug_2](https://user-images.githubusercontent.com/1151048/105917584-c01ed280-5fef-11eb-8c5b-fefb36f81008.gif) After: ![im_fix_2](https://user-images.githubusercontent.com/1151048/105917650-e0e72800-5fef-11eb-9fd3-020d52e4e3b1.gif) ## Bug 3 When using OR with values it does not shift up correctly similar to AND Before: ![im_bug_3](https://user-images.githubusercontent.com/1151048/105917691-f2303480-5fef-11eb-9368-b11d23159606.gif) After: ![im_fix_3](https://user-images.githubusercontent.com/1151048/105917714-f9574280-5fef-11eb-9be4-1f56c207525a.gif) ### Checklist Delete any items that are not applicable to this PR. - [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 - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

…nvalid values can cause overwrites of other values (#90634) ### 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.

…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 <42973632+kibanamachine@users.noreply.github.com>

…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 <42973632+kibanamachine@users.noreply.github.com>

FrankHassanabad added 5 commits January 22, 2021 11:55

Reworked keys and tech debt issues

f05425a

Merge branch 'master' into fix-ui-react-keys

d897c0d

Adds cypress tests and optimizes the cypress tests and functions to m…

cdd905c

…ake test writing more efficient

Merge branch 'master' into fix-ui-react-keys

3b14b2c

Fixed types, added unit tests for this particular test

25e4d4e

FrankHassanabad changed the title ~~Fix UI react keys~~ [Security Solution][Detection Engine] Fixes indicator matches mapping UI where invalid list values can cause overwrites Jan 26, 2021

FrankHassanabad self-assigned this Jan 26, 2021

FrankHassanabad added v8.0.0 v7.12.0 Feature:Indicator Match Rule Security Solution Indicator Match rule type release_note:skip Skip the PR/issue when compiling release notes release_note:fix labels Jan 26, 2021

FrankHassanabad marked this pull request as ready for review January 26, 2021 23:47

FrankHassanabad requested review from a team as code owners January 26, 2021 23:47

FrankHassanabad requested a review from rylnd January 27, 2021 20:05

FrankHassanabad added 2 commits January 27, 2021 13:14

Merge branch 'master' into fix-ui-react-keys

0e0f6d7

Merge branch 'master' into fix-ui-react-keys

9ee21c5

FrankHassanabad added the Team:Detections and Resp Security Detection Response Team label Jan 28, 2021

FrankHassanabad added 2 commits January 28, 2021 09:48

Merge branch 'master' into fix-ui-react-keys

839e7d7

Merge branch 'master' into fix-ui-react-keys

eaeb74d

Merge branch 'master' into fix-ui-react-keys

ed42b99

Merge branch 'master' into fix-ui-react-keys

0ab3dca

Merge branch 'master' into fix-ui-react-keys

4e83f1b

rylnd approved these changes Jan 29, 2021

View reviewed changes

rylnd requested a review from MadameSheema January 29, 2021 01:33

FrankHassanabad added 4 commits January 29, 2021 14:11

Merge branch 'fix-ui-react-keys' of github.com:FrankHassanabad/kibana…

6932cb4

… into fix-ui-react-keys

Merge branch 'master' into fix-ui-react-keys

1fd7158

Merge branch 'master' into fix-ui-react-keys

13d0ff9

Updated from code review

181bce7

FrankHassanabad merged commit 2f80e44 into elastic:master Jan 30, 2021

FrankHassanabad deleted the fix-ui-react-keys branch January 30, 2021 02:16

FrankHassanabad mentioned this pull request Jan 30, 2021

[7.x] [Security Solution][Detection Engine] Fixes indicator matches mapping UI where invalid list values can cause overwrites of other values (#89066) #89817

Merged

FrankHassanabad mentioned this pull request Feb 5, 2021

[Security Solution][Detections] Indicator Rules cannot be duplicated from the Rule Details page #90356

Closed

yctercero mentioned this pull request Feb 9, 2021

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

Merged

8 tasks

FrankHassanabad removed the release_note:skip Skip the PR/issue when compiling release notes label Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detection Engine] Fixes indicator matches mapping UI where invalid list values can cause overwrites of other values#89066

[Security Solution][Detection Engine] Fixes indicator matches mapping UI where invalid list values can cause overwrites of other values#89066
FrankHassanabad merged 16 commits intoelastic:masterfrom
FrankHassanabad:fix-ui-react-keys

FrankHassanabad commented Jan 22, 2021 •

edited

Loading

Uh oh!

elasticmachine commented Jan 28, 2021

Uh oh!

FrankHassanabad commented Jan 28, 2021

Uh oh!

FrankHassanabad commented Jan 28, 2021

Uh oh!

FrankHassanabad commented Jan 29, 2021

Uh oh!

kibanamachine commented Jan 29, 2021

Uh oh!

rylnd left a comment

Uh oh!

Uh oh!

Uh oh!

rylnd Jan 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rylnd Jan 29, 2021

Uh oh!

kibanamachine commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

FrankHassanabad commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Bug 1

Bug 2

Bug 3

Checklist

Uh oh!

elasticmachine commented Jan 28, 2021

Uh oh!

FrankHassanabad commented Jan 28, 2021

Uh oh!

FrankHassanabad commented Jan 28, 2021

Uh oh!

FrankHassanabad commented Jan 29, 2021

Uh oh!

kibanamachine commented Jan 29, 2021

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rylnd Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rylnd Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 30, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FrankHassanabad commented Jan 22, 2021 •

edited

Loading