Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] The values for Field & Indicator index field are not mapped correctly under the indicator mapping section. #84893

Closed
muskangulati-qasource opened this issue Dec 3, 2020 · 4 comments · Fixed by #89066
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Indicator Match Rule Security Solution Indicator Match rule type impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. QA:Validated Issue has been validated by QA Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.12.0

Comments

@muskangulati-qasource
Copy link

muskangulati-qasource commented Dec 3, 2020

Describe the bug
The values for Field & Indicator index field are not mapped correctly under the indicator mapping section.

Build Details:

Version : 7.10.1 BC2
Commit:608c5e5dd32659e8afadd520d0cdc58766ba505b
Build number : 36063
Artifact: https://staging.elastic.co/7.10.1-928e89a5/summary-7.10.1.html

Browser Details
All

Preconditions

  1. Elastic Cloud 7.10.0 environment should be deployed
  2. Auditbeat should be running.
  3. Alert should be generated.

Steps to Reproduce

  1. Navigate to Detections Tab and click on 'Manage Detection Rules'
  2. Click on 'Create new Rule'.
  3. Select 'Indicator Match' and go to the 'Indicator mapping' section.
  4. Fill in multiple entries using 'And'/'Or' operators.

Impacted Test case(s)
N/A

Actual Result
When we delete any row(except the last), the data in the 'Indicator index field' is retained for the deleted row and this results in the data for the next row being overwritten by the deleted row's data.

Expected Result
Data for any row should not be overwritten without the User's consent.

What's Working
N/A

What's not Working
N/A

Screenshot

  • Entered 02 field values:
    details1

  • Deleted the first one and observed that data for the 2nd row is overwritten and updated.
    details2

@muskangulati-qasource muskangulati-qasource added bug Fixes for quality problems that affect the customer experience Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.10.2 labels Dec 3, 2020
@muskangulati-qasource
Copy link
Author

@manishgupta-qasource Please review!!

@manishgupta-qasource
Copy link

Reviewed & assigned to @peluja1012

@manishgupta-qasource manishgupta-qasource added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Dec 3, 2020
@peluja1012 peluja1012 added Team:Detections and Resp Security Detection Response Team Feature:Indicator Match Rule Security Solution Indicator Match rule type labels Dec 9, 2020
@spong spong added v7.12.0 and removed v7.10.2 labels Dec 10, 2020
@peluja1012 peluja1012 removed their assignment Jan 13, 2021
@FrankHassanabad FrankHassanabad self-assigned this Jan 19, 2021
FrankHassanabad added a commit that referenced this issue Jan 30, 2021
… UI where invalid list values can cause overwrites of other values (#89066)

## 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)
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this issue Jan 30, 2021
… 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 added a commit that referenced this issue Jan 30, 2021
… 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)
@ghost
Copy link

ghost commented Mar 1, 2021

Hi @MadameSheema

We have validated this ticket on 7.12.0 BC2 and found that issue is Fixed. Data for any row is not overwritten under indicator match

Build Details:

Version: 7.12.0 BC2
Build: 39000
Commit: 4f65a5a1268fa78f1af9117d12312e1cee433376
Artifacts: https://staging.elastic.co/7.12.0-37f40745/summary-7.12.0.html

Screenshot:
Entered 02 field values:
ind_match

Deleted the first one:
After_delete

Please let us know if anything else is required from our end.

Thanks!!

@ghost ghost added the QA:Validated Issue has been validated by QA label Mar 1, 2021
@ghost
Copy link

ghost commented Mar 26, 2021

Bug Conversion:

Created 01 Test-Case for this Ticket
https://elastic.testrail.io/index.php?/cases/view/76926

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Indicator Match Rule Security Solution Indicator Match rule type impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. QA:Validated Issue has been validated by QA Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.12.0
Projects
None yet
5 participants