[SIEM][Detection Engine] Allow to edit actions for prepackaged rules

[patrykkopycinski]

Summary

• allow to edit actions for prepacked rules
• don't override actions when updating prepacked rules

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
• This was checked for cross-browser compatibility, including a check against IE11

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/siem (Team:SIEM)

[tsg]

Thank you, @patrykkopycinski, for finding a quick solution. I think I like this for 7.7 at least, but we should discuss the following two points:

• are we comfortable with mixing immutable and mutable data into the same saved object? Does this have the potential to cause issues with other features like import/export?

• is the UI experience acceptable? We are showing the Edit button for immutable rules, but then the only active tab is Alerts.

CC @FrankHassanabad @spong @MikePaquette @XavierM @marrasherrier for the discussion.

[MikePaquette]

@tsg @patrykkopycinski @FrankHassanabad @spong @XavierM

Amazing, this has the potential to save users hours of time whenever we update our prebuilt rules.

are we comfortable with mixing immutable and mutable data into the same saved object? Does this have the potential to cause issues with other features like import/export?

We also need to consider the common case of prebuilt rules being "updated" (e.g., each time we release updated rules). The expected experience is that the alerting settings (and in the future, allowlist settings) would be preserved independently, even though the rule contents (immutable parts) may change during updates.

is the UI experience acceptable? We are showing the Edit button for immutable rules, but then the only active tab is Alerts.

I'd say yes. @marrasherrier and I spoke about this yesterday with @lindseypoli, and while we may come up with a better experience in the future, this seems quite acceptable. In fact, we thought ahead to when we add support for rule exceptions, and this approach would be easily extensible to that. (i.e., if the user edits a pre-built rule, the only active tabs would be "Alerts" and "Exceptions.")

[patrykkopycinski]

• are we comfortable with mixing immutable and mutable data into the same saved object? Does this have the potential to cause issues with other features like import/export?

@tsg @MikePaquette we don't allow to export, so to import prepackaged rules. With my changes, we will not override actions for prepackaged rules during the update

[tsg]

We also need to consider the common case of prebuilt rules being "updated" (e.g., each time we release updated rules). The expected experience is that the alerting settings (and in the future, allowlist settings) would be preserved independently, even though the rule contents (immutable parts) may change during updates.

That should work with the current PR, if I understand it correctly. I'm just worried if this (technical) design is perhaps too brittle for the future. But we can always improve it as needed.

[XavierM]

Maybe, we should create rules_actions saved object, therefore we do not save action with the rules saved objects so it is independent of the rules and we can keep the immutable logic in rules for pre-packaged rules.
Also updating pre-packaged rules my overwrite the actions if it is set up in prepackaged rules.

[patrykkopycinski]

@elasticmachine merge upstream

[rylnd]

Ran things locally, two notes:

1. Adding actions doesn't affect a prepackaged rule's version, so 👍 there
2. When adding an action to a prepackaged rule, the rule was automatically enabled. This was unexpected to me so I'm thinking it might be a bug?

I just fixed the one test failure, so hopefully this'll be green in a little bit here. I'm babysitting some other stuff so I'll keep an eye on this one as well.

[rylnd]

Wouldn't every saved object have an attributes property? Ditto for the check of saved_objects below. If these are not guaranteed to find only the type in question, then these are functionally equivalent to just saying as [Type], and have all the same downsides.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 485e66f

History

• 💚 Build #37364 succeeded 55212dc
• 💔 Build #37324 failed 6c38780
• 💔 Build #37306 failed aad8054
• 💔 Build #37277 failed 61aa3b9
• 💔 Build #37254 failed b3793bf

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

[elasticmachine]

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