[Security Solution] Return user-customized fields from the POST /prebuilt_rules/upgrade/_review API endpoint even if they haven't been updated by Elastic in the target version

[jpdjere]

Epics: https://github.com/elastic/security-team/issues/1974 (internal), #174168

Summary

• Return user-customized fields from the POST /prebuilt_rules/upgrade/_review API endpoint even if they haven't been updated by Elastic in the target version.
• The diff should show a diff for fields that are user customized, even if the field hasn't been changed in the target version.
• The idea is to be fully transparent and "remind" the user which fields have been customized.

Background

• We currently have the 5 following field diff outcomes:

export enum ThreeWayDiffOutcome {
  /** Stock rule, the value hasn't changed in the target version. */
  StockValueNoUpdate = 'BASE=A, CURRENT=A, TARGET=A',

  /** Stock rule, the value has changed in the target version. */
  StockValueCanUpdate = 'BASE=A, CURRENT=A, TARGET=B',

  /** Customized rule, the value hasn't changed in the target version comparing to the base one. */
  CustomizedValueNoUpdate = 'BASE=A, CURRENT=B, TARGET=A',

  /** Customized rule, the value has changed in the target version exactly the same way as in the user customization. */
  CustomizedValueSameUpdate = 'BASE=A, CURRENT=B, TARGET=B',

  /** Customized rule, the value has changed in the target version and is not equal to the current version. */
  CustomizedValueCanUpdate = 'BASE=A, CURRENT=B, TARGET=C',
}

• From these outcomes, a field is only returned in the diff.fields property of the endpoint's response if the outcome of the diff for that field is:

1. StockValueCanUpdate = 'BASE=A, CURRENT=A, TARGET=B'
2. CustomizedValueCanUpdate = 'BASE=A, CURRENT=B, TARGET=C',

• We need to expand these scenarios so that also a field diff is returned in diff.fields if the diff outcome is, additionally: CustomizedValueNoUpdate = 'BASE=A, CURRENT=B, TARGET=A' and 'CustomizedValueSameUpdate = 'BASE=A, CURRENT=B, TARGET=B'

• NOTE: in the Upgrade Workflow UI, these fields will appear auto-accepted and collapsed, with the accepted version of the field being the current value of the rule.

[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)

[dplumlee]

I've added this in #184889 for future diffing logic to adhere to when full prebuilt rule customization is implemented. In the meantime, this is the way the current per-field diff UI handles returning the CustomizedValueNoUpdate and CustomizedValueSameUpdate(are we adding this in too @banderror?) scenarios:

CustomizedValueNoUpdate('BASE=A, CURRENT=B, TARGET=A')

Here we have a rule diff with a max_signals field that has a

• base_version = 100
• current_version = 99
• target_version = 100
• merged_version = 99

I would argue with the current UI, it is not clear at all to the user that the current version is being retained. If anything, it looks like we're replacing it with the target version

CustomizedValueSameUpdate('BASE=A, CURRENT=B, TARGET=B')

Here we have a rule diff with a setup field that has a

• base_version = "Some long string"
• current_version = "Some new, updated long string"
• target_version = "Some new, updated long string"
• merged_version = "Some new, updated long string"

In this scenario, since we're using a package that calculates diffs based on current and target versions, and they are the same, we return nothing, but still have the accordion object. This is obviously also confusing UI and should be fixed alongside these changes.

Summary

In summary, I don't think we can use the current UI workflows with this either of this ticket's mentioned return logic, we need to either add in something alongside the change to display both of the scenarios in a more informative way, or skip displaying them at all.

Currently we have the entire diff object to work with which includes a lot of information from the backend about the scenario and diff and we could filter these diff situations (CustomizedValueNoUpdate and CustomizedValueSameUpdate) out of the front end UI as we don't return them from the API now anyways - effectively nothing to the user would change.

Since it's still a relatively rare use case for users to PATCH the prebuilt rules via the API, my vote would be to just skip displaying these scenarios for now (keeping the UI view the same) and just return them in the API so that the new diff algorithms and related tests can make use of them, as well as be present for the interactive UI we'll be building for three way diffs later on down the road.

Any thoughts? cc: @approksiu @banderror

[banderror]

@dplumlee Great update and questions, thank you. I agree that the best way to proceed would be:

• Return the CustomizedValueNoUpdate and CustomizedValueSameUpdate fields from the API in the diff object.
• Filter them out and skip displaying them in the per-field diff UI.

As far as I understand, we already have the same issue in the JSON diff UI. Since we don't use the diff object there, and instead text-diffing the current and target rule versions, in case of CustomizedValueNoUpdate the JSON diff would show a confusing diff too. We could try to fix this issue for the JSON diff component as well, but I feel it would require a more complex code to write, so I'm not suggesting this due to a low value/effort ratio.

[banderror]

@dplumlee @jpdjere @nikitaindik @xcrzx

As a follow-up to our chat yesterday, I'm posting an update and closing this ticket. The issue was addressed in #184889:

Also changes logic in the upgrade/_review API endpoint to return user customized fields in the diffs even if there was not an update for that field. This new logic is described in #180154. We filter out the fields that fall under this new logic so that they are only returned from the API but not displayed in the per-field rule diff flyout as the current UI cannot support them. They are utilized in testing logic and will be implemented in the UI at a later date

We now return all the diffs from the API except ThreeWayDiffOutcome.StockValueNoUpdate:

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

Lines 123 to 127 in 74c4d3a

	diff: {
	fields: pickBy<ThreeWayDiff<unknown>>(
	ruleDiff.fields,
	(fieldDiff) => fieldDiff.diff_outcome !== ThreeWayDiffOutcome.StockValueNoUpdate
	),

And CustomizedValueNoUpdate and CustomizedValueSameUpdate diffs are filtered out (hidden) in the UI:

kibana/x-pack/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/helpers.ts

Lines 42 to 58 in 74c4d3a

	/**
	* Filters out any fields that have a `diff_outcome` of `CustomizedValueNoUpdate`
	* or `CustomizedValueSameUpdate` as they are not supported for display in the
	* current per-field rule diff flyout
	*/
	export const filterUnsupportedDiffOutcomes = (
	fields: Partial<RuleFieldsDiff>
	): Partial<RuleFieldsDiff> =>
	Object.fromEntries(
	Object.entries(fields).filter(([key, value]) => {
	const diff = value as ThreeWayDiff<unknown>;
	return (
	diff.diff_outcome !== ThreeWayDiffOutcome.CustomizedValueNoUpdate &&
	diff.diff_outcome !== ThreeWayDiffOutcome.CustomizedValueSameUpdate
	);
	})
	);

NOTE: in the Upgrade Workflow UI, these fields will appear auto-accepted and collapsed, with the accepted version of the field being the current value of the rule.

We will address this in #171520.