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.

Sign up for GitHub

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

Jump to bottom

[Security Solutions][Detection Engine] Fixes bug with not being able to duplicate indicator matches #92565

Merged
merged 7 commits into from
Feb 24, 2021
Merged

[Security Solutions][Detection Engine] Fixes bug with not being able to duplicate indicator matches #92565

merged 7 commits into from
Feb 24, 2021

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Feb 24, 2021 •
edited
Loading

Summary

Fixes an unreleased regression bug where indicator rules could not be be duplicated.
#90356

@FrankHassanabad FrankHassanabad requested a review from a team as a code owner February 24, 2021 07:00
@FrankHassanabad FrankHassanabad self-assigned this Feb 24, 2021
@FrankHassanabad FrankHassanabad linked an issue Feb 24, 2021 that may be closed by this pull request
[Security Solution][Detections] Indicator Rules cannot be duplicated from the Rule Details page #90356
Closed
@FrankHassanabad FrankHassanabad added v8.0.0 v7.13.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Indicator Match Rule Security Solution Indicator Match Rule feature bug Fixes for quality problems that affect the customer experience auto-backport Deprecated - use backport:version if exact versions are needed labels Feb 24, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

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 +80.0B

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

cc @FrankHassanabad

yctercero
yctercero reviewed Feb 24, 2021
View reviewed changes
...k/plugins/security_solution/cypress/integration/detection_rules/indicator_match_rule.spec.ts
@@ -465,5 +470,30 @@ describe('indicator match', () => {
cy.get(ALERT_RULE_RISK_SCORE).first().should('have.text', newThreatIndicatorRule.riskScore);
});
});

describe('Duplicates the indicator rule', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 nice, this always feels great to add more tests here.

rylnd
rylnd approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix and the added coverage!

yctercero
yctercero reviewed Feb 24, 2021
View reviewed changes
x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/actions.tsx
const response = await duplicateRules({
// We cast this back and forth here as the front end types are not really the right io-ts ones
// and the two types conflict with each other.
rules: rules.map((rule) => transformOutput(rule as CreateRulesSchema) as Rule),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into the same thing with exceptions. I ended up creating two functions to deal with the two type scenarios, but thought about doing the as as well.

export const transformOutput = (
  exceptionItem: UpdateExceptionListItemSchema | ExceptionListItemSchema
): UpdateExceptionListItemSchema | ExceptionListItemSchema =>
  flow(removeIdFromExceptionItemsEntries)(exceptionItem);

export const transformNewItemOutput = (
  exceptionItem: CreateExceptionListItemSchema
): CreateExceptionListItemSchema => flow(removeIdFromExceptionItemsEntries)(exceptionItem);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting me know

yctercero
yctercero approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - awesome tests! Curious to see how much pipe helps.

@FrankHassanabad FrankHassanabad merged commit ebb0435 into elastic:master Feb 24, 2021
@FrankHassanabad FrankHassanabad deleted the fix-duplicate-indicator branch February 24, 2021 21:32
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 24, 2021
@FrankHassanabad @kibanamachine
[Security Solutions][Detection Engine] Fixes bug with not being able …
1a25478 
…to duplicate indicator matches (elastic#92565)

## Summary

Fixes an unreleased regression bug where indicator rules could not be be duplicated.
elastic#90356

- [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
@kibanamachine kibanamachine mentioned this pull request Feb 24, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 24, 2021
@FrankHassanabad @kibanamachine
[Security Solutions][Detection Engine] Fixes bug with not being able …
9cd13bc 
…to duplicate indicator matches (elastic#92565)

## Summary

Fixes an unreleased regression bug where indicator rules could not be be duplicated.
elastic#90356

- [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
@kibanamachine kibanamachine mentioned this pull request Feb 24, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

✅ 7.12 / #92716
✅ 7.x / #92717

Successful backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Feb 25, 2021
@kibanamachine @FrankHassanabad
[Security Solutions][Detection Engine] Fixes bug with not being able …
044de1e 
…to duplicate indicator matches (#92565) (#92717)

## Summary

Fixes an unreleased regression bug where indicator rules could not be be duplicated.
#90356

- [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

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 25, 2021
@gmmorris
Merge branch 'master' into task-manager/docs-monitoring
226deae 
* 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)
  ...
kibanamachine added a commit that referenced this pull request Feb 25, 2021
@kibanamachine @FrankHassanabad
[Security Solutions][Detection Engine] Fixes bug with not being able …
025ac2c 
…to duplicate indicator matches (#92565) (#92716)

## Summary

Fixes an unreleased regression bug where indicator rules could not be be duplicated.
#90356

- [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

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rylnd rylnd rylnd approved these changes

@yctercero yctercero yctercero approved these changes

Assignees

@FrankHassanabad FrankHassanabad

Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:Indicator Match Rule Security Solution Indicator Match Rule feature release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v7.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Security Solution][Detections] Indicator Rules cannot be duplicated from the Rule Details page
4 participants
@FrankHassanabad @kibanamachine @rylnd @yctercero