Skip to content

Commit

Permalink
[Security Solution] Unlock Prebuil Rules Customization workflow for r…
Browse files Browse the repository at this point in the history
…ules with missing base version (elastic#201301)

**Resolves: elastic#200904

## Summary

This PR unlocks Prebuilt Rules Customization workflow for rules with missing base version.

## Details

Each Prebuilt Rule update contains `version` diff. `version` is a special non-customizable field we use to track prebuilt rule version. It always gets target rule version's value after rule upgrade.

A generic `numberDiffAlgorithm` algorithm was used for `version` field. It produces a `SOLVABLE` conflict when rule's base version is missing. It blocked the workflow in UI. We check the number of field with conflicts versus resolved conflicts to decide when a rule is ready for upgrade. In case `version` field got a conflict user had no possibility to resolve it.

The fix adds a new `forceTargetVersionDiffAlgorithm` diff algorithm applied only for `version` field. It produces a non-conflict diff all the time even when base version is missing. The reason behind is that `version` always gets target rule's version.
  • Loading branch information
maximpn authored and CAWilson94 committed Dec 12, 2024
1 parent bb3b07e commit 45044e5
Show file tree
Hide file tree
Showing 13 changed files with 156 additions and 27 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import type { ThreeVersionsOf } from '../../../../../../../../common/api/detection_engine';
import {
ThreeWayMergeOutcome,
MissingVersion,
ThreeWayDiffConflict,
} from '../../../../../../../../common/api/detection_engine';
import { forceTargetVersionDiffAlgorithm } from './force_target_version_diff_algorithm';

describe('forceTargetVersionDiffAlgorithm', () => {
describe('when base version exists', () => {
it('returns a NON conflict diff', () => {
const mockVersions: ThreeVersionsOf<number> = {
base_version: 1,
current_version: 1,
target_version: 2,
};

const result = forceTargetVersionDiffAlgorithm(mockVersions);

expect(result).toMatchObject({
conflict: ThreeWayDiffConflict.NONE,
});
});

it('return merge outcome TARGET', () => {
const mockVersions: ThreeVersionsOf<number> = {
base_version: 1,
current_version: 1,
target_version: 2,
};

const result = forceTargetVersionDiffAlgorithm(mockVersions);

expect(result).toMatchObject({
has_base_version: true,
merge_outcome: ThreeWayMergeOutcome.Target,
});
});
});

describe('when base version missing', () => {
it('returns a NON conflict diff', () => {
const mockVersions: ThreeVersionsOf<number> = {
base_version: MissingVersion,
current_version: 1,
target_version: 2,
};

const result = forceTargetVersionDiffAlgorithm(mockVersions);

expect(result).toMatchObject({
conflict: ThreeWayDiffConflict.NONE,
});
});

it('return merge outcome TARGET', () => {
const mockVersions: ThreeVersionsOf<number> = {
base_version: MissingVersion,
current_version: 1,
target_version: 2,
};

const result = forceTargetVersionDiffAlgorithm(mockVersions);

expect(result).toMatchObject({
has_base_version: false,
merge_outcome: ThreeWayMergeOutcome.Target,
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import type {
ThreeVersionsOf,
ThreeWayDiff,
} from '../../../../../../../../common/api/detection_engine/prebuilt_rules';
import {
MissingVersion,
ThreeWayDiffConflict,
ThreeWayMergeOutcome,
determineDiffOutcome,
} from '../../../../../../../../common/api/detection_engine/prebuilt_rules';

/**
* Diff algorithm forcing target version. Useful for special fields like `version`.
*/
export const forceTargetVersionDiffAlgorithm = <TValue>(
versions: ThreeVersionsOf<TValue>
): ThreeWayDiff<TValue> => {
const {
base_version: baseVersion,
current_version: currentVersion,
target_version: targetVersion,
} = versions;
const hasBaseVersion = baseVersion !== MissingVersion;
const hasUpdate = targetVersion !== currentVersion;

return {
has_base_version: hasBaseVersion,
base_version: hasBaseVersion ? baseVersion : undefined,
current_version: currentVersion,
target_version: targetVersion,
merged_version: targetVersion,
merge_outcome: ThreeWayMergeOutcome.Target,

diff_outcome: determineDiffOutcome(baseVersion, currentVersion, targetVersion),
has_update: hasUpdate,
conflict: ThreeWayDiffConflict.NONE,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ export { kqlQueryDiffAlgorithm } from './kql_query_diff_algorithm';
export { eqlQueryDiffAlgorithm } from './eql_query_diff_algorithm';
export { esqlQueryDiffAlgorithm } from './esql_query_diff_algorithm';
export { ruleTypeDiffAlgorithm } from './rule_type_diff_algorithm';
export { forceTargetVersionDiffAlgorithm } from './force_target_version_diff_algorithm';
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import {
eqlQueryDiffAlgorithm,
esqlQueryDiffAlgorithm,
ruleTypeDiffAlgorithm,
forceTargetVersionDiffAlgorithm,
} from './algorithms';

const BASE_TYPE_ERROR = `Base version can't be of different rule type`;
Expand Down Expand Up @@ -179,7 +180,11 @@ const calculateCommonFieldsDiff = (

const commonFieldsDiffAlgorithms: FieldsDiffAlgorithmsFor<DiffableCommonFields> = {
rule_id: simpleDiffAlgorithm,
version: numberDiffAlgorithm,
/**
* `version` shouldn't have a conflict. It always get target value automatically.
* Diff has informational purpose.
*/
version: forceTargetVersionDiffAlgorithm,
name: singleLineStringDiffAlgorithm,
tags: scalarArrayDiffAlgorithm,
description: multiLineStringDiffAlgorithm,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -897,11 +897,11 @@ export default ({ getService }: FtrProviderContext): void => {
expect(fieldDiffObject.data_source).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // version is considered conflict
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -958,7 +958,7 @@ export default ({ getService }: FtrProviderContext): void => {
});

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2); // version + tags
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // tags
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,11 +382,11 @@ export default ({ getService }: FtrProviderContext): void => {
expect(fieldDiffObject.eql_query).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); // `version` is considered an updated field
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // `version` is considered conflict
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -451,7 +451,7 @@ export default ({ getService }: FtrProviderContext): void => {
});

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2); // version + query
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2); // version + query
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // query
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,11 @@ export default ({ getService }: FtrProviderContext): void => {
expect(fieldDiffObject.esql_query).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // version is considered conflict
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -420,7 +420,7 @@ export default ({ getService }: FtrProviderContext): void => {
});

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2); // version + query
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // query
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1042,11 +1042,11 @@ export default ({ getService }: FtrProviderContext): void => {
expect(fieldDiffObject.kql_query).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); // `version` is considered an updated field
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // `version` is considered conflict
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -1114,7 +1114,7 @@ export default ({ getService }: FtrProviderContext): void => {
});

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2); // version + query
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // query
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.rules[0].diff.fields.description).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -430,7 +430,7 @@ export default ({ getService }: FtrProviderContext): void => {
has_base_version: false,
});
expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,11 +274,11 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.rules[0].diff.fields.risk_score).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -322,7 +322,7 @@ export default ({ getService }: FtrProviderContext): void => {
has_base_version: false,
});
expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,11 +298,11 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.rules[0].diff.fields.type).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // version is considered a conflict
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -348,7 +348,7 @@ export default ({ getService }: FtrProviderContext): void => {
});

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(3);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(3); // type + version + query are all considered conflicts
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2); // type + query are all considered conflicts
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(1);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -423,11 +423,11 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.rules[0].diff.fields.tags).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // version is considered conflict
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -472,7 +472,7 @@ export default ({ getService }: FtrProviderContext): void => {
});

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2); // version + tags
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // tags
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,11 +277,11 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.rules[0].diff.fields.name).toBeUndefined();

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // version is considered a conflict
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0);
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(1);
expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0);
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});
Expand Down Expand Up @@ -326,7 +326,7 @@ export default ({ getService }: FtrProviderContext): void => {
});

expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(2);
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(2); // name + version are both considered conflicts
expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(1); // name is considered as a conflict
expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0);

expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1);
Expand Down

0 comments on commit 45044e5

Please sign in to comment.