[Security Solution] Implement isCustomized calculation #186988

xcrzx · 2024-06-26T13:53:37Z

Resolves: #180145
Resolves: #184364

Note

This PR doesn't include isCustomized recalculation when bulk editing rules. This should be addressed separately as it might require first changing the RulesClient.bulkEdit method signature.

Summary

This PR implements the calculation of ruleSource.isCustomized inside the DetectionRulesClient. The recalculation of the isCustomized field is performed on every rule patch and update operation, including rule upgrades. See the ticket for more information: #180145 and detection_rules_client/mergers/rule_source/calculate_is_customized.ts for implementation details.

The isCustomized calculation is based on the calculateRuleFieldsDiff method used inside the prebuilt rules domain for calculating changed fields during rule upgrades. This ensures that the diff calculation logic is unified and reused to avoid any discrepancies in different paths of rule management.

The recalculation and saving of the field is done in the following endpoints:

Update Rule - PUT /rules
Patch Rule - PATCH /rules
Bulk Update Rules - PUT /rules/_bulk_update
Bulk Patch Rules - PATCH /rules/_bulk_update
Import Rules - POST /rules/_import
Perform Rule Upgrade - POST /prebuilt_rules/upgrade/_perform

This PR also partially addresses refactoring mentioned here: #184364. Namely:

Splits the rule converters into smaller single-responsibility functions.
- Separate methods to convert RuleResponse to AlertingRule and back
- Separate methods to apply rule patches, updates, or set defaults
- Separate case converters
Migrates methods to work with RuleResponse instead of alerting type wherever possible.
Adds new methods for fetching rules by id or rule id and deprecates the readRules. Although new methods are not exposed yet in the public client interface, this is something that needs to be addressed separately.

dhurley14

Holding until we can better understand how to resolve expected code conflicts brought up here: #183937 (review)

cc: @jpdjere

xcrzx · 2024-07-03T14:00:54Z

Holding until we can better understand how to resolve expected code conflicts brought up here: #183937 (review)

cc: @jpdjere

Hey @dhurley14, the rule customization epic is a super high priority right now. It would be great if you don't hold this PR for too long.

cc @banderror @peluja1012

dhurley14 · 2024-07-03T14:12:13Z

I agree, it would be great if we don't hold up these PR's for too long. You have done a lot of work to determine how best to migrate the logic from the rule converter code into the separate areas you created here. Considering this, you have the best knowledge for determining how, after the system actions code is merged, those changes can then also be added into the new files you created to maintain parity, as you have already done.

I think we can both work together to determine the best path forward to reduce code conflicts.

dhurley14 · 2024-07-03T14:22:27Z

Mentioned it here but posting for posterity, please let me know how I can make changes to my PR to lessen the impact it will have on yours!

setting up a meeting instead

banderror · 2024-07-03T16:44:33Z

@xcrzx Could you please add a PR description and open it for review? I don't want it to be perceived as a draft / something not very urgent for our team.

elasticmachine · 2024-07-03T16:50:43Z

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

elasticmachine · 2024-07-03T16:50:43Z

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

elasticmachine · 2024-07-03T16:50:43Z

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

xcrzx · 2024-07-03T17:06:49Z

buildkite test this

elasticmachine · 2024-07-03T18:34:43Z

⏳ Build in-progress, with failures

Buildkite Build
Commit: 8159534b093faff16996289a9b858c6f94ed904f

Failed CI Steps

cc @xcrzx

.../rule_management/logic/detection_rules_client/mergers/rule_source/calculate_is_customized.ts

...ver/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts

...b/detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts

...ib/detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_patch.ts

...ment/logic/detection_rules_client/converters/convert_prebuilt_rule_asset_to_rule_response.ts

jpdjere

Thanks for addressing my comments, and for the whole refactor! ✅

...management/logic/detection_rules_client/converters/convert_rule_response_to_alerting_rule.ts

nkhristinin · 2024-07-09T06:36:31Z

...rule_management/logic/detection_rules_client/converters/common_params_camel_to_snake.test.ts

+      },
+    });
+    expect(transformedParams).toEqual(
+      expect.objectContaining({


Should we in this type of converters tests for the whole objects and fields present?

I'm a bit opposed to creating snapshot tests for the whole function output because snapshots have proven to be too flaky without providing any extra quality assurance.

Anyway, this test existed before, and my goal here was not to provide additional test coverage of the existing code. During the refactoring, I transferred the already existing tests to new locations and added new tests only for the newly introduced functionality, like the isCustomized calculation.

nkhristinin · 2024-07-09T06:41:01Z

...tion_engine/exceptions/workflows/basic_license_essentials_tier/create_rule_exceptions_ess.ts

        const {
          hits: {
            hits: [{ _source: ruleSO }],
          },
        } = await getRuleSOById(es, ruleWithLegacyInvestigationField.id);
-        const isInvestigationFieldMigratedInSo = await checkInvestigationFieldSoValue(ruleSO, {


Hey, why this logic was removed?

Now, the legacy investigation field migration happens much earlier. With this refactoring, we convert data to our internal representation RuleResponse immediately after reading a rule from RulesClient. This means that instead of transforming the data only before returning it in the response, we transform it as soon as we read it.

This approach ensures that in our API handlers, the data is always in the new format and thus gets migrated on any write or update. This is much more straightforward than migrating the data only on certain writes as before.

nkhristinin · 2024-07-09T07:11:10Z

...lugins/security_solution/server/lib/detection_engine/rule_preview/api/preview_rules/route.ts

@@ -237,6 +239,8 @@ export const previewRulesRoute = (
              createdAt: new Date(),
              createdBy: username ?? 'preview-created-by',
              producer: 'preview-producer',
+              consumer: SERVER_APP_ID,


Does this consumer affect behaviour, or it's just type thing here?

This field was previously added with the convertCreateAPIToInternalSchema function. Now that I have split that function into multiple single-purpose functions, we have to add it manually here. There should be no changes in the resulting rule passed to Alerting.

nkhristinin

LGTM, thanks!

xcrzx self-assigned this Jun 26, 2024

xcrzx requested review from a team as code owners June 26, 2024 13:53

xcrzx requested review from e40pud and dplumlee June 26, 2024 13:53

xcrzx marked this pull request as draft June 26, 2024 13:53

xcrzx force-pushed the is-customized branch 10 times, most recently from 9805884 to 1e1fbea Compare July 3, 2024 09:31

elastic deleted a comment from kibana-ci Jul 3, 2024

xcrzx force-pushed the is-customized branch from 1e1fbea to ecdaa28 Compare July 3, 2024 10:55

jpdjere mentioned this pull request Jul 3, 2024

[Security Solution] [Detections] Adds support for system actions (and cases action) to detection rules #183937

Merged

3 tasks

xcrzx force-pushed the is-customized branch from ecdaa28 to 4033831 Compare July 3, 2024 13:07

dhurley14 previously requested changes Jul 3, 2024

View reviewed changes

elastic deleted a comment from elasticmachine Jul 3, 2024

xcrzx force-pushed the is-customized branch from 4033831 to 8159534 Compare July 3, 2024 16:42

xcrzx marked this pull request as ready for review July 3, 2024 16:49

xcrzx added Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area v8.16.0 labels Jul 3, 2024

banderror requested review from jpdjere and banderror July 4, 2024 08:38

jpdjere reviewed Jul 4, 2024

View reviewed changes

.../rule_management/logic/detection_rules_client/mergers/rule_source/calculate_is_customized.ts Outdated Show resolved Hide resolved

jpdjere reviewed Jul 4, 2024

View reviewed changes

...ver/lib/detection_engine/rule_management/logic/detection_rules_client/methods/import_rule.ts Outdated Show resolved Hide resolved

jpdjere reviewed Jul 4, 2024

View reviewed changes

...b/detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_update.ts Show resolved Hide resolved

jpdjere reviewed Jul 4, 2024

View reviewed changes

...ib/detection_engine/rule_management/logic/detection_rules_client/mergers/apply_rule_patch.ts Show resolved Hide resolved

banderror mentioned this pull request Jul 5, 2024

[Security Solution] DetectionRulesClient refactoring. Part 2 #184364

Closed

17 tasks

jpdjere reviewed Jul 5, 2024

View reviewed changes

...ment/logic/detection_rules_client/converters/convert_prebuilt_rule_asset_to_rule_response.ts Outdated Show resolved Hide resolved

xcrzx force-pushed the is-customized branch from 8159534 to ce556cb Compare July 8, 2024 09:54

jpdjere approved these changes Jul 8, 2024

View reviewed changes

banderror removed their request for review July 8, 2024 15:30

yctercero requested review from nkhristinin and removed request for e40pud July 8, 2024 15:49

nkhristinin reviewed Jul 9, 2024

View reviewed changes

Implement isCustomized calculation

eae1512

xcrzx force-pushed the is-customized branch from ce556cb to eae1512 Compare July 9, 2024 09:02

xcrzx requested a review from nkhristinin July 9, 2024 09:21

nkhristinin approved these changes Jul 9, 2024

View reviewed changes

xcrzx merged commit 045aafc into elastic:main Jul 9, 2024
40 of 41 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jul 9, 2024

xcrzx deleted the is-customized branch July 10, 2024 12:33

xcrzx restored the is-customized branch July 10, 2024 12:33

xcrzx deleted the is-customized branch July 10, 2024 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Implement isCustomized calculation #186988

[Security Solution] Implement isCustomized calculation #186988

xcrzx commented Jun 26, 2024 •

edited by banderror

Loading

dhurley14 left a comment

xcrzx commented Jul 3, 2024

dhurley14 commented Jul 3, 2024

dhurley14 commented Jul 3, 2024

banderror commented Jul 3, 2024 •

edited

Loading

elasticmachine commented Jul 3, 2024

elasticmachine commented Jul 3, 2024

elasticmachine commented Jul 3, 2024

xcrzx commented Jul 3, 2024

elasticmachine commented Jul 3, 2024

jpdjere left a comment

nkhristinin Jul 9, 2024

xcrzx Jul 9, 2024

nkhristinin Jul 9, 2024

xcrzx Jul 9, 2024

nkhristinin Jul 9, 2024

xcrzx Jul 9, 2024

nkhristinin left a comment

[Security Solution] Implement isCustomized calculation #186988

[Security Solution] Implement isCustomized calculation #186988

Conversation

xcrzx commented Jun 26, 2024 • edited by banderror Loading

Summary

dhurley14 left a comment

Choose a reason for hiding this comment

xcrzx commented Jul 3, 2024

dhurley14 commented Jul 3, 2024

dhurley14 commented Jul 3, 2024

banderror commented Jul 3, 2024 • edited Loading

elasticmachine commented Jul 3, 2024

elasticmachine commented Jul 3, 2024

elasticmachine commented Jul 3, 2024

xcrzx commented Jul 3, 2024

elasticmachine commented Jul 3, 2024

⏳ Build in-progress, with failures

Failed CI Steps

jpdjere left a comment

Choose a reason for hiding this comment

nkhristinin Jul 9, 2024

Choose a reason for hiding this comment

xcrzx Jul 9, 2024

Choose a reason for hiding this comment

nkhristinin Jul 9, 2024

Choose a reason for hiding this comment

xcrzx Jul 9, 2024

Choose a reason for hiding this comment

nkhristinin Jul 9, 2024

Choose a reason for hiding this comment

xcrzx Jul 9, 2024

Choose a reason for hiding this comment

nkhristinin left a comment

Choose a reason for hiding this comment

xcrzx commented Jun 26, 2024 •

edited by banderror

Loading

banderror commented Jul 3, 2024 •

edited

Loading