[Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule #191065

banderror · 2024-08-22T10:35:44Z

Fixes: #190628
Related to: #173595, #173594

Summary

As stated in the bug ticket, when duplicating a prebuilt rule, the "Related Integrations" and "Required Fields" values should be inherited from the original rule, as it was specified in the Acceptance Criteria for #173595 and #173594.

This PR:

Removes the logic that resets these fields to empty arrays for duplicated prebuilt rules - we needed this logic in the past because these fields were not editable in the UI, but we don't need it anymore.
Updates the corresponding unit tests.

Screenshots

These screenshots were taken after introducing the fixes.

Original prebuilt rule:

Duplicated prebuilt rule:

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

banderror · 2024-08-22T10:57:43Z

/ci

elasticmachine · 2024-08-23T11:40:46Z

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

elasticmachine · 2024-08-23T11:40:47Z

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

elasticmachine · 2024-08-23T11:40:48Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

maximpn

@banderror Thanks for handling this bug and extending test coverage 🙏

I left some comments which are rather nit.

maximpn · 2024-08-26T07:14:12Z

...ecurity_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.ts


  // Duplicated rules are always considered custom rules
-  const immutable = false;
+  const immutable: InternalRuleCreate['params']['immutable'] = false;


I'm curious what's the point to explicitly specify types here? Usually TS is able to infer types pretty accurately (especially such simple ones) and check wether these match return type (since it's specified for the function this also the reason why it's better to have function's return type). If something is wrong TS will complain on lines 51-52.

@maximpn There are sometimes instances when TS allows to return an object from a function if the object fulfills a certain type or interface, but has additional fields as well that are not part of this type. I've seen it multiple times but I can't give you a concrete example immediately.

Here maybe you're right that these type annotations are not necessary, but I figured I'd still add them because here we return a complex rule object from the function and it's probably better if we add extra type annotations. It shouldn't hurt, anyway.

maximpn · 2024-08-26T07:18:18Z

...ty_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.test.ts

          }),
        })
      );
    });

-    it('copies setup guide as is', async () => {
+    it('copies fields from the original rule', async () => {


Do I get it correctly that we wanna test for coping of ALL fields in that unit test? And it's a desired behavior to have this test failed when a new field got added for example by Response Ops team?

WDYT about changing the test name to copies all fields from the original rule?

Do I get it correctly that we wanna test for coping of ALL fields in that unit test?

@maximpn Strictly speaking, we don't copy all rule fields. We copy all parameters + some of the fields that Alerting Framework owns. Some fields get adjusted (e.g. name), some fields get regenerated. I don't think it would be correct to rename the test to copies all fields from the original rule.

And it's a desired behavior to have this test failed when a new field got added for example by Response Ops team?

Yes, because if they add a new top-level framework's field we need to make sure it is correctly handled by the rule duplication logic: copied, adjusted, or regenerated.

maximpn · 2024-08-26T07:20:48Z

...ty_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.test.ts


+    it('keeps it custom', async () => {
+      const rule = createCustomRule();
      const result = await duplicateRule({
        rule,
      });

      expect(result).toEqual(


nit: It should be simpler to assert result.params directly

Suggested change

expect(result).toEqual(

expect(result.params).toEqual(

expect.objectContaining({

requiredFields: rule.params.requiredFields,

immutable: false,

ruleSource: {

type: 'internal',

},

}),

})

);

maximpn · 2024-08-26T07:22:03Z

...ty_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.test.ts

-        },
-      ];
-
+    it('copies fields from the original rule', async () => {


nit: The same as before should it be renamed copies fields from the original rule -> copies all fields from the original rule?

#191065 (comment)

kibana-ci · 2024-08-26T09:31:57Z

💚 Build Succeeded

Buildkite Build
Commit: 0941f9d

Metrics [docs]

✅ unchanged

History

💔 Build #229428 failed 2c171c3d7abd58041a760baa57513c2dba12c663
💚 Build #229100 succeeded dbbf90dd3878c678d8c31ff377913c7273b04704

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

cc @banderror

nikitaindik

Thanks for the fix @banderror! I have checked the implementation and tested locally. Found no issues.

banderror · 2024-08-26T13:42:40Z

Thanks @maximpn and @nikitaindik 👍 I'll respond to the comments tomorrow and open a follow-up PR, if needed. Gonna merge this one now so we have the fix in main before the 8.15.1 FF.

kibanamachine · 2024-08-26T13:47:30Z

💔 All backports failed

Status	Branch	Result
❌	8.15	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 191065

Questions ?

Please refer to the Backport tool documentation

banderror · 2024-08-27T12:23:27Z

💚 All backports created successfully

Status	Branch	Result
✅	8.15

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ed integrations and required fields from the original rule (elastic#191065) **Fixes: elastic#190628 **Related to:** elastic#173595, elastic#173594 ## Summary As stated in the bug ticket, when duplicating a prebuilt rule, the "Related Integrations" and "Required Fields" values should be inherited from the original rule, as it was specified in the Acceptance Criteria for elastic#173595 and elastic#173594. This PR: - Removes the logic that resets these fields to empty arrays for duplicated prebuilt rules - we needed this logic in the past because these fields were not editable in the UI, but we don't need it anymore. - Updates the corresponding unit tests. ## Screenshots These screenshots were taken after introducing the fixes. **Original prebuilt rule:** <img width="1463" alt="Screenshot_2024-08-23_at_13_25_07" src="https://github.com/user-attachments/assets/ad8673f5-aba3-40c8-ae91-bbd7d334b119"> **Duplicated prebuilt rule:** <img width="1469" alt="Screenshot_2024-08-23_at_13_25_43" src="https://github.com/user-attachments/assets/03761a2b-6f53-4bab-bf4c-a71c6860802b"> ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 (cherry picked from commit b144c05) # Conflicts: # x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.test.ts # x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/actions/duplicate_rule.ts

…y related integrations and required fields from the original rule (#191065) (#191493) # Backport This will backport the following commits from `main` to `8.15`: - [[Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule (#191065)](#191065)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

banderror self-assigned this Aug 22, 2024

banderror changed the title ~~[Security Solution] Fix prebuilt rule duplication logic~~ [Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule Aug 22, 2024

banderror mentioned this pull request Aug 22, 2024

[Security Solution] When duplicating a prebuilt rule, 'Related Integrations' and 'Required Fields' values are not inherited from the original rule #190628

Closed

banderror force-pushed the fix-prebuilt-rule-duplication-logic branch from dbbf90d to 2c171c3 Compare August 23, 2024 10:59

banderror marked this pull request as ready for review August 23, 2024 11:40

banderror requested a review from a team as a code owner August 23, 2024 11:40

banderror requested a review from jpdjere August 23, 2024 11:40

banderror requested review from maximpn and nikitaindik and removed request for jpdjere August 23, 2024 11:41

maximpn approved these changes Aug 26, 2024

View reviewed changes

Fix prebuilt rule duplication logic

0941f9d

banderror force-pushed the fix-prebuilt-rule-duplication-logic branch from 2c171c3 to 0941f9d Compare August 26, 2024 08:11

nikitaindik approved these changes Aug 26, 2024

View reviewed changes

banderror merged commit b144c05 into elastic:main Aug 26, 2024
39 checks passed

banderror mentioned this pull request Aug 27, 2024

[8.15] [Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule (#191065) #191493

Merged

banderror deleted the fix-prebuilt-rule-duplication-logic branch August 27, 2024 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule #191065

[Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule #191065

banderror commented Aug 22, 2024 •

edited by kibanamachine

Loading

banderror commented Aug 22, 2024

elasticmachine commented Aug 23, 2024

elasticmachine commented Aug 23, 2024

elasticmachine commented Aug 23, 2024

maximpn left a comment

maximpn Aug 26, 2024

banderror Sep 12, 2024

maximpn Aug 26, 2024

banderror Sep 12, 2024

maximpn Aug 26, 2024

maximpn Aug 26, 2024

banderror Sep 12, 2024

kibana-ci commented Aug 26, 2024

nikitaindik left a comment

banderror commented Aug 26, 2024

kibanamachine commented Aug 26, 2024

banderror commented Aug 27, 2024

-      expect(result).toEqual(
+      expect(result.params).toEqual(
+        expect.objectContaining({
+            requiredFields: rule.params.requiredFields,
+            immutable: false,
+            ruleSource: {
+              type: 'internal',
+            },
+          }),
+        })
+     );

[Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule #191065

[Security Solution] Fix prebuilt rule duplication logic to copy related integrations and required fields from the original rule #191065

Conversation

banderror commented Aug 22, 2024 • edited by kibanamachine Loading

Summary

Screenshots

Checklist

banderror commented Aug 22, 2024

elasticmachine commented Aug 23, 2024

elasticmachine commented Aug 23, 2024

elasticmachine commented Aug 23, 2024

maximpn left a comment

Choose a reason for hiding this comment

maximpn Aug 26, 2024

Choose a reason for hiding this comment

banderror Sep 12, 2024

Choose a reason for hiding this comment

maximpn Aug 26, 2024

Choose a reason for hiding this comment

banderror Sep 12, 2024

Choose a reason for hiding this comment

maximpn Aug 26, 2024

Choose a reason for hiding this comment

maximpn Aug 26, 2024

Choose a reason for hiding this comment

banderror Sep 12, 2024

Choose a reason for hiding this comment

kibana-ci commented Aug 26, 2024

💚 Build Succeeded

Metrics [docs]

History

nikitaindik left a comment

Choose a reason for hiding this comment

banderror commented Aug 26, 2024

kibanamachine commented Aug 26, 2024

💔 All backports failed

Manual backport

Questions ?

banderror commented Aug 27, 2024

💚 All backports created successfully

Questions ?

banderror commented Aug 22, 2024 •

edited by kibanamachine

Loading