Skip to content

[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 merged 1 commit intoelastic:7.xfrom
FrankHassanabad:backport/7.x/pr-89066
Jan 30, 2021

Conversation

@FrankHassanabad
Copy link
Copy Markdown
Contributor

Backports the following commits to 7.x:

… 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)
@FrankHassanabad FrankHassanabad added the backport This PR is a backport of another PR label Jan 30, 2021
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

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

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

@FrankHassanabad FrankHassanabad merged commit 55d4b74 into elastic:7.x Jan 30, 2021
@FrankHassanabad FrankHassanabad deleted the backport/7.x/pr-89066 branch January 30, 2021 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport This PR is a backport of another PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants