[Security Solution] JSON diff view for prebuilt rule upgrade flow

[nikitaindik]

Summary

Resolves: #169160
Resolves: #166164
Docs issue: elastic/security-docs#4371

This PR adds a new "Updates" tab to the prebuilt rules upgrade flyout. This tab shows a diff between the installed and updated rule JSON representations.

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
• Functional changes are communicated to the Docs team. A ticket or PR is opened in https://github.com/elastic/security-docs. The following information is included: any feature flags used, affected environments (Serverless, ESS, or both). (Docs issue)
• Documentation was added for features that require explanation or tutorials (Docs issue)
• Unit or functional tests were updated or added to match the most common scenarios (will be added in a follow-up PR)
• Functional changes are covered with a test plan and automated tests (will be added in a follow-up PR)
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• This renders correctly on smaller devices using a responsive layout. (Doesn't look great on phone screen, because viewing diff requires a lot of horizontal space. Tablets are fine though.)
• This was checked for cross-browser compatibility
• Functional changes are hidden behind a feature flag. If not hidden, the PR explains why these changes are being implemented in a long-living feature branch.
• Comprehensive manual testing is done by two engineers: the PR author and one of the PR reviewers. Changes are tested in both ESS and Serverless.

[elasticmachine]

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

[elasticmachine]

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

[jpdjere]

I feel the logic for this function is somewhat complicated, since we pass this function to RuleDetailsFlyout and expect the defaultTabs to be created and passed there, with no clear understanding here of what they are.

I think we can simplify and make this logic less tightly coupled by making getRuleTabs only return the diffTab if there's an active rule, or nothing if not:

  const getRuleTabs = useCallback(
    (rule: RuleResponse,): EuiTabbedContentTab[] => {
      const activeRule = filteredRules.find(({ id }) => id === rule.id);

      if (!activeRule || !isJsonPrebuiltRulesDiffingEnabled) {
        return;
      }

      const diffTab = {
        id: 'updates',
        name: ruleDetailsI18n.UPDATES_TAB_LABEL,
        content: (
          <TabContentPadding>
            <RuleDiffTab oldRule={activeRule.current_rule} newRule={activeRule.target_rule} />
          </TabContentPadding>
        ),
      };

      return diffTab;
    },
    [filteredRules]
  );

And then within RuleDetailsFlyout, tabs can have a clearer logic:

  const tabs = useMemo(() => {
    let tabs = [overviewTab];
    if (rule.note) {
      tabs.push(investigationGuideTab);
    }

    const diffTab = getRuleTabs(rule);
    if (diffTab) {
      tabs = [diffTab, ...tabs];
    }

    return tabs;
  }, [overviewTab, investigationGuideTab, rule, getRuleTabs]);

[jpdjere]

And in that case, I would actually rename getRuleTabs to something like getRuleDiffTab. It has a single responsibility now that is checking and getting the diff tab, completely independently from the other tabs.

[jpdjere]

One additional nit: To create the tab here you had to export TabContentPadding from rule_details_flyout.tsx into this parent component, which look anti-pattern-ny to me.

I think here you could just return:

      const diffTab = {  
        id: 'updates',  
        name: ruleDetailsI18n.UPDATES_TAB_LABEL,  
        content: <RuleDiffTab oldRule={activeRule.current_rule} newRule={activeRule.target_rule} />  
      };  

And then within RuleDetailsFlyout, wrap the component there so you don't have to export it.

  const tabs = useMemo(() => {  
    let tabs = [overviewTab];  
    if (rule.note) {  
      tabs.push(investigationGuideTab);  
    }  

    const diffTab = getRuleTabs(rule);  
    if (diffTab) {
      const paddedDiffTab = (
        <TabContentPadding>
          {diffTab}
        </TabContentPadding>
      )
      tabs = [paddedDiffTab, ...tabs];  
    }  

    return tabs;  
  }, [overviewTab, investigationGuideTab, rule, getRuleTabs]);  

[dplumlee]

++ we have a similar logic structure in the alerts table with default columns and it makes it very difficult to understand out of context and build upon.

[nikitaindik]

@dplumlee Just had a conversation with @jpdjere about this. We ended up deciding to refactor this part in a way that's easier to understand but still keeping RuleDetailsFlyout agnostic of which extra tabs it contains. Ideally we'd want to keep RuleDetailsFlyout unaware of the diffs if we want to reuse it on the MITRE page. I pushed the changes. Does this look fine to you?

[jpdjere]

One issue I'm seeing is that, when I patch/update a rule to test the diff, the revision is always displayed on the diff, as updating to 1.

When the rule is updated, the revision increases by 1, which is the correct behaviour (so, in this case, increases to 21 instead of resetting to 1 as the diff suggests).

I'm trying to see if this is something we should fix in the /upgrade/_review endpoint or something we should force the diff view not to display.

Same with the created_by, created_at, updated_by and updated_at fields.

[jpdjere]

One issue I'm seeing is that, when I patch/update a rule to test the diff, the revision is always displayed on the diff, as updating to 1.

When the rule is updated, the revision increases by 1, which is the correct behaviour (so, in this case, increases to 21 instead of resetting to 1 as the diff suggests).

I'm trying to see if this is something we should fix in the /upgrade/_review endpoint or something we should force the diff view not to display.

Same with the created_by, created_at, updated_by and updated_at fields.

So the issue is actually in the /upgrade/_review endpoint: can you please add the fix in this PR?

In the file x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/review_rule_upgrade/review_rule_upgrade_route.ts, the targetRule of the response is calculated so:

    const targetRule: RuleResponse = {
      ...convertPrebuiltRuleAssetToRuleResponse(targetVersion),
      id: installedCurrentVersion.id,
    };

and should be:

    const targetRule: RuleResponse = {
      ...convertPrebuiltRuleAssetToRuleResponse(targetVersion),
      id: installedCurrentVersion.id,
      revision: installedCurrentVersion.revision + 1,
    };

since convertPrebuiltRuleAssetToRuleResponse sets the revision to 1, which makes sense for rule installation but not for upgrade.

With this fix, the diff will show an update of revision from, for example, 22 to 23. This is technically correct; the question is if we still want to show this "technical" and not editable field diff to the user.

My opinion is that we should hide it from the user (although not necessarily in this PR due to time constraints, but as a follow-up). Maybe @banderror has an opinion?

[jpdjere]

Actually, we need a slightly larger fix in:

x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/review_rule_upgrade/review_rule_upgrade_route.ts

targetRule should be:

    const targetRule: RuleResponse = {
      ...convertPrebuiltRuleAssetToRuleResponse(targetVersion),
      id: installedCurrentVersion.id,
      revision: installedCurrentVersion.revision + 1,
      created_at: installedCurrentVersion.created_at,
      updated_at: new Date().toISOString(),
      updated_by: installedCurrentVersion.updated_by,
      created_by: installedCurrentVersion.created_by,
    };

because:

• revision: should be incremented by one on each update
• created_at: should remain unchanged
• created_by: should remain unchanged
• updated_at: should be updated to the current date
• updated_by: this I think should probably be updated to reflect the user that is updating it, but I think it's out of scope of this PR and we can do it as a follow up. Leaving this unchanged hides it from the diff for now.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 04750b5

Failed CI Steps

• FTR Configs #48
• Detection Engine - Security Solution Cypress Tests #6

Test Failures

• [job] [logs] Detection Engine - Security Solution Cypress Tests #6 / Alert user assignment - ESS & Serverless Basic rendering alert with many assignees (collapsed into badge) in alert's details flyout alert with many assignees (collapsed into badge) in alert's details flyout
• [job] [logs] FTR Configs #48 / serverless search UI Rule details Header should disable the rule

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
securitySolution	4816	4833	+17

Async chunks

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

id	before	after	diff
securitySolution	13.0MB	13.1MB	+108.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
securitySolution	66.9KB	66.9KB	+35.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
securitySolution	463	468	+5

Total ESLint disabled count

id	before	after	diff
securitySolution	533	538	+5

History

• 💛 Build #182558 was flaky da2b4ec
• 💚 Build #182451 succeeded 907c6fc
• 💚 Build #182174 succeeded e07509b
• 💚 Build #182040 succeeded 9c9b407
• 💛 Build #181842 was flaky 3176b1d
• 💔 Build #181776 failed 267694c

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

cc @nikitaindik

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.12

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

Questions ?

Please refer to the Backport tool documentation