[Security Solution] Extend upgrade prebuilt rules context with conflict resolution functionality

[maximpn]

Addresses: #171520

Summary

This PR implements necessary UpgradePrebuiltRulesTableContext changes to provide uses a way to resolve conflicts manually by providing field's resolved value.

Details

During prebuilt rules upgrading users may encounter solvable and non-solvable conflicts between customized and target rule versions. Three-Way-Diff field component allow to specify a desired resolve value user expects to be in the rule after upgrading. It's also possible to customize rules during the upgrading process.

Current functionality is informational only without an ability to customize prebuilt rules. As the core part of that process it's required to manage the upgrading state and provide necessary data for downstream components rendering field diffs and accepting user input.

This PR extends UpgradePrebuiltRulesTableContext with rule upgrade state and provides it to ThreeWayDiffTab stub component. It's planned to add implementation to ThreeWayDiffTab in follow up PRs.

On top of that UpgradePrebuiltRulesTableContext and AddPrebuiltRulesTableContext were symmetrically refactored from architecture point of view to improve encapsulation by separation of concerns which leads to slight complexity reduction.

Feature flag prebuiltRulesCustomizationEnabled

ThreeWayDiffTab is hidden under a feature flag prebuiltRulesCustomizationEnabled. It accepts a finalDiffableRule which represents rule fields the user expects to see in the upgraded rule. finalDiffableRule is a combination of field resolved values and target rule fields where resolved values have precedence.

[elasticmachine]

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

[elasticmachine]

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

[elasticmachine]

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

[nikitaindik]

We need to allow passing dataTestSubj via props, because we used:

• dataTestSubj="installPrebuiltRulePreview" for installation
• dataTestSubj="updatePrebuiltRulePreview" for upgrade

We have a Cypress test that uses these, but it doesn't run because it has issues with tags. I have created a PR to make it run again. We'll need to run these changes through CI once that PR is merged.

[nikitaindik]

Also id={PREBUILT_RULE_UPDATE_FLYOUT_ANCHOR} is not needed for the installation use case. I'm thinking maybe we could have a generic prop like flyoutProps that we would spread over? wdyt?

[maximpn]

Yes, thanks for noticing. I planned to address data test subject ids in follow up PRs where we add tests. As you can see the current code doesn't cause any failures.

Adding flyoutProps right away looks as a good option.

[jpdjere]

Add a space after this line for clarity?

[jpdjere]

I'm not sure if this is WIP, but if we want to get the "final version" of a rule, we should get all diffable fields, and then overwrite all fields that have a merged_version within diff.fields.

The logic here implemented is correct, but it will only work when the user manually does a change in the edit component.

I think the return value of this method should look something like:

  return {
    ...convertRuleToDiffable(ruleUpgradeInfo.target_rule),
    ...getFieldsWithMergedVersions(ruleUpgradeInfo.diff), // loops through diff.fields and retrieves fields with a merged_version
    ...ruleResolvedConflicts,
  } as DiffableRule;

[jpdjere]

This will make sure to use the target version as the "initial" diffable rule, then overwrite with any values that were returned with a merged_version from /upgrade/_review, and finally overwrite any values that have been manually edited by the user.

[maximpn]

Thanks. It makes sense. I made proper adjustments in the code.

[jpdjere]

We spoke about this today, but posting here for transparency: this should include the fields with SOLVABLE conflicts, which should be accepted by the user as well in order to unblock the upgrade of the rule.

[maximpn]

Yes. I changed the implementation to match our discussion results.

[nikitaindik]

If understand it correctly, we should use ruleUpgradeInfo.rule_id here and when calling calcUnresolvedConflicts below. Otherwise it won't find an item in rulesResolvedConflicts.

[nikitaindik]

I wonder if we should also let the user revert a conflict back to unresolved. Let's say they set some resolved value and then decide that it's not good and they want to start over from the initial state (merge proposal). In current implementation even if you close and reopen the flyout the state will be preserved. And if we later add persistence in sessionStorage even page reload won't help.

Should we then think about adding buttons for reverting? @maximpn @jpdjere

[maximpn]

Yes. It might be helpful feature. But it's possible to implement this feature on top of the current functionality. I wouldn't address that in the current PR.

[nikitaindik]

Yep, I agree, it's for a separate PR. I'm curious what you folks think in general about reverts - is it needed or not. Because it's something that we haven't discussed when planning the feature.

[jpdjere]

We'll definitely need a way to "revert" conflict resolutions. Just as an example: a rule update has an ABC scenario conflict for its name field. The user manually resolves the conflict, but then manually deletes the input value in the edit component. Since name is required, we should unresolve the conflict and disable the "Update rule" button.

But yes, we can add this complexity incrementally, doesn't need to be part of this PR.

[jpdjere]

Just realized that my message above refers to unresolving a conflict from the state management point of view (which can be handled using the existing setRulesResolvedConflicts setter), but what @nikitaindik meant was allowing the user to unaccept a change from the UI/UX perspective.

In the original plan, for each field, we'd have a Edit/Save button that switches back and forth between those two values when clicking on it. When clicking on Save, the change is accepted and conflict resolved, when clicking on Edit, it is un-accepted and conflict un-resolved.

However, this has one limitation that we need to resolved. Copying here what I pasted on Slack yesterday:

There's one thing that I think we need to figure out: when planning out this component we decided to simplify the UI by just having a button that switches back and forth betwen Edit and Save. And by clicking on "Save", that would mark the state as "accepted".

However, since the "SOLVABLE" conflicts appear first as Read-Only components, they are already "saved" from the UI perspective (the Edit button is visible, no the Save button), but not from the state management perspective. It feels like we still need to add a separate button to accept the changes, that is independent from the Edit/Save switch. But I'm afraid this might explode the complexity of the component's state.

What do you guys think? cc @maximpn

[jpdjere]

Continued the discussion in the Slack channel pasted above.

[jpdjere]

Thanks for addressing my comments. Now I see the expected finalVersion with the merged_versions of the fields that have updated. 👍

[nikitaindik]

Thanks for the changes @maximpn! PR LGTM now.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: d72f1b4

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
securitySolution	5733	5744	+11

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
securitySolution	19.7MB	19.7MB	+8.4KB

History

• 💔 Build #233103 failed 7e61fad
• 💛 Build #232718 was flaky 2071dff351ab85b928862faf17b3ba102a3568c3
• 💛 Build #231411 was flaky 3425c1333b09885c5f1feae701517ccff56eaf8f
• 💛 Build #231335 was flaky 95030441730711eebb03a56d01df1567bad9d496
• 💛 Build #230855 was flaky ada8757c3e1bcfe5c66decdde87be55e191bfeb3

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

cc @maximpn