-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security Solution] Rule type
field diff algorithm
#193369
Merged
+264
−0
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
fe40d7a
adds diff algorithm
dplumlee 521f7e2
Merge remote-tracking branch 'upstream/main' into rule-type-diff-algo…
dplumlee f1d6b91
fixes tests
dplumlee 6a39b5f
Merge remote-tracking branch 'upstream/main' into rule-type-diff-algo…
dplumlee b943c49
Merge remote-tracking branch 'upstream/main' into rule-type-diff-algo…
dplumlee 448899b
adds comments
dplumlee File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
165 changes: 165 additions & 0 deletions
165
..._engine/prebuilt_rules/logic/diff/calculation/algorithms/rule_type_diff_algorithm.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
/* | ||
* 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 { | ||
DiffableRuleTypes, | ||
ThreeVersionsOf, | ||
} from '../../../../../../../../common/api/detection_engine'; | ||
import { | ||
ThreeWayDiffOutcome, | ||
ThreeWayMergeOutcome, | ||
MissingVersion, | ||
ThreeWayDiffConflict, | ||
} from '../../../../../../../../common/api/detection_engine'; | ||
import { ruleTypeDiffAlgorithm } from './rule_type_diff_algorithm'; | ||
|
||
describe('ruleTypeDiffAlgorithm', () => { | ||
it('returns current_version as merged output if there is no update - scenario AAA', () => { | ||
const mockVersions: ThreeVersionsOf<DiffableRuleTypes> = { | ||
base_version: 'query', | ||
current_version: 'query', | ||
target_version: 'query', | ||
}; | ||
|
||
const result = ruleTypeDiffAlgorithm(mockVersions); | ||
|
||
expect(result).toEqual( | ||
expect.objectContaining({ | ||
merged_version: mockVersions.target_version, | ||
diff_outcome: ThreeWayDiffOutcome.StockValueNoUpdate, | ||
merge_outcome: ThreeWayMergeOutcome.Target, | ||
conflict: ThreeWayDiffConflict.NONE, | ||
}) | ||
); | ||
}); | ||
|
||
it('returns current_version as merged output if current_version is different and there is no update - scenario ABA', () => { | ||
// User can change rule type field between `query` and `saved_query` in the UI, no other rule types | ||
const mockVersions: ThreeVersionsOf<DiffableRuleTypes> = { | ||
base_version: 'query', | ||
current_version: 'saved_query', | ||
target_version: 'query', | ||
}; | ||
|
||
const result = ruleTypeDiffAlgorithm(mockVersions); | ||
|
||
expect(result).toEqual( | ||
expect.objectContaining({ | ||
merged_version: mockVersions.target_version, | ||
diff_outcome: ThreeWayDiffOutcome.CustomizedValueNoUpdate, | ||
merge_outcome: ThreeWayMergeOutcome.Target, | ||
conflict: ThreeWayDiffConflict.NON_SOLVABLE, | ||
}) | ||
); | ||
}); | ||
|
||
it('returns target_version as merged output if current_version is the same and there is an update - scenario AAB', () => { | ||
// User can change rule type field between `query` and `saved_query` in the UI, no other rule types | ||
const mockVersions: ThreeVersionsOf<DiffableRuleTypes> = { | ||
base_version: 'query', | ||
current_version: 'query', | ||
target_version: 'saved_query', | ||
}; | ||
|
||
const result = ruleTypeDiffAlgorithm(mockVersions); | ||
|
||
expect(result).toEqual( | ||
expect.objectContaining({ | ||
merged_version: mockVersions.target_version, | ||
diff_outcome: ThreeWayDiffOutcome.StockValueCanUpdate, | ||
merge_outcome: ThreeWayMergeOutcome.Target, | ||
conflict: ThreeWayDiffConflict.NON_SOLVABLE, | ||
}) | ||
); | ||
}); | ||
|
||
it('returns current_version as merged output if current version is different but it matches the update - scenario ABB', () => { | ||
// User can change rule type field between `query` and `saved_query` in the UI, no other rule types | ||
const mockVersions: ThreeVersionsOf<DiffableRuleTypes> = { | ||
base_version: 'query', | ||
current_version: 'saved_query', | ||
target_version: 'saved_query', | ||
}; | ||
|
||
const result = ruleTypeDiffAlgorithm(mockVersions); | ||
|
||
expect(result).toEqual( | ||
expect.objectContaining({ | ||
merged_version: mockVersions.target_version, | ||
diff_outcome: ThreeWayDiffOutcome.CustomizedValueSameUpdate, | ||
merge_outcome: ThreeWayMergeOutcome.Target, | ||
conflict: ThreeWayDiffConflict.NON_SOLVABLE, | ||
}) | ||
); | ||
}); | ||
|
||
it('returns current_version as merged output if all three versions are different - scenario ABC', () => { | ||
// User can change rule type field between `query` and `saved_query` in the UI, no other rule types | ||
// NOTE: This test case scenario is currently inaccessible via normal UI or API workflows, but the logic is covered just in case | ||
const mockVersions: ThreeVersionsOf<DiffableRuleTypes> = { | ||
base_version: 'query', | ||
current_version: 'eql', | ||
target_version: 'saved_query', | ||
}; | ||
|
||
const result = ruleTypeDiffAlgorithm(mockVersions); | ||
|
||
expect(result).toEqual( | ||
expect.objectContaining({ | ||
merged_version: mockVersions.target_version, | ||
diff_outcome: ThreeWayDiffOutcome.CustomizedValueCanUpdate, | ||
merge_outcome: ThreeWayMergeOutcome.Target, | ||
conflict: ThreeWayDiffConflict.NON_SOLVABLE, | ||
}) | ||
); | ||
}); | ||
|
||
describe('if base_version is missing', () => { | ||
it('returns current_version as merged output if current_version and target_version are the same - scenario -AA', () => { | ||
const mockVersions: ThreeVersionsOf<DiffableRuleTypes> = { | ||
base_version: MissingVersion, | ||
current_version: 'query', | ||
target_version: 'query', | ||
}; | ||
|
||
const result = ruleTypeDiffAlgorithm(mockVersions); | ||
|
||
expect(result).toEqual( | ||
expect.objectContaining({ | ||
has_base_version: false, | ||
base_version: undefined, | ||
merged_version: mockVersions.target_version, | ||
diff_outcome: ThreeWayDiffOutcome.MissingBaseNoUpdate, | ||
merge_outcome: ThreeWayMergeOutcome.Target, | ||
conflict: ThreeWayDiffConflict.NONE, | ||
}) | ||
); | ||
}); | ||
|
||
it('returns target_version as merged output if current_version and target_version are different - scenario -AB', () => { | ||
// User can change rule type field between `query` and `saved_query` in the UI, no other rule types | ||
const mockVersions: ThreeVersionsOf<DiffableRuleTypes> = { | ||
base_version: MissingVersion, | ||
current_version: 'query', | ||
target_version: 'saved_query', | ||
}; | ||
|
||
const result = ruleTypeDiffAlgorithm(mockVersions); | ||
|
||
expect(result).toEqual( | ||
expect.objectContaining({ | ||
has_base_version: false, | ||
base_version: undefined, | ||
merged_version: mockVersions.target_version, | ||
diff_outcome: ThreeWayDiffOutcome.MissingBaseCanUpdate, | ||
merge_outcome: ThreeWayMergeOutcome.Target, | ||
conflict: ThreeWayDiffConflict.NON_SOLVABLE, | ||
}) | ||
); | ||
}); | ||
}); | ||
}); |
98 changes: 98 additions & 0 deletions
98
...ction_engine/prebuilt_rules/logic/diff/calculation/algorithms/rule_type_diff_algorithm.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
/* | ||
* 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 { assertUnreachable } from '../../../../../../../../common/utility_types'; | ||
import type { | ||
DiffableRuleTypes, | ||
ThreeVersionsOf, | ||
ThreeWayDiff, | ||
} from '../../../../../../../../common/api/detection_engine/prebuilt_rules'; | ||
import { | ||
determineDiffOutcome, | ||
determineIfValueCanUpdate, | ||
MissingVersion, | ||
ThreeWayDiffConflict, | ||
ThreeWayDiffOutcome, | ||
ThreeWayMergeOutcome, | ||
} from '../../../../../../../../common/api/detection_engine/prebuilt_rules'; | ||
|
||
export const ruleTypeDiffAlgorithm = <TValue extends DiffableRuleTypes>( | ||
versions: ThreeVersionsOf<TValue> | ||
): ThreeWayDiff<TValue> => { | ||
const { | ||
base_version: baseVersion, | ||
current_version: currentVersion, | ||
target_version: targetVersion, | ||
} = versions; | ||
|
||
const diffOutcome = determineDiffOutcome(baseVersion, currentVersion, targetVersion); | ||
const valueCanUpdate = determineIfValueCanUpdate(diffOutcome); | ||
|
||
const hasBaseVersion = baseVersion !== MissingVersion; | ||
|
||
const { mergeOutcome, conflict, mergedVersion } = mergeVersions({ | ||
targetVersion, | ||
diffOutcome, | ||
}); | ||
|
||
return { | ||
has_base_version: hasBaseVersion, | ||
base_version: hasBaseVersion ? baseVersion : undefined, | ||
current_version: currentVersion, | ||
target_version: targetVersion, | ||
merged_version: mergedVersion, | ||
merge_outcome: mergeOutcome, | ||
|
||
diff_outcome: diffOutcome, | ||
has_update: valueCanUpdate, | ||
conflict, | ||
}; | ||
}; | ||
|
||
interface MergeResult<TValue> { | ||
mergeOutcome: ThreeWayMergeOutcome; | ||
mergedVersion: TValue; | ||
conflict: ThreeWayDiffConflict; | ||
} | ||
|
||
interface MergeArgs<TValue> { | ||
targetVersion: TValue; | ||
diffOutcome: ThreeWayDiffOutcome; | ||
} | ||
|
||
const mergeVersions = <TValue>({ | ||
targetVersion, | ||
diffOutcome, | ||
}: MergeArgs<TValue>): MergeResult<TValue> => { | ||
switch (diffOutcome) { | ||
// Scenario -AA is treated as scenario AAA: | ||
// https://github.com/elastic/kibana/pull/184889#discussion_r1636421293 | ||
case ThreeWayDiffOutcome.MissingBaseNoUpdate: | ||
case ThreeWayDiffOutcome.StockValueNoUpdate: | ||
return { | ||
conflict: ThreeWayDiffConflict.NONE, | ||
mergedVersion: targetVersion, | ||
mergeOutcome: ThreeWayMergeOutcome.Target, | ||
}; | ||
case ThreeWayDiffOutcome.CustomizedValueNoUpdate: | ||
case ThreeWayDiffOutcome.CustomizedValueSameUpdate: | ||
case ThreeWayDiffOutcome.StockValueCanUpdate: | ||
// NOTE: This scenario is currently inaccessible via normal UI or API workflows, but the logic is covered just in case | ||
case ThreeWayDiffOutcome.CustomizedValueCanUpdate: | ||
// Scenario -AB is treated as scenario ABC: | ||
// https://github.com/elastic/kibana/pull/184889#discussion_r1636421293 | ||
case ThreeWayDiffOutcome.MissingBaseCanUpdate: { | ||
return { | ||
mergedVersion: targetVersion, | ||
mergeOutcome: ThreeWayMergeOutcome.Target, | ||
conflict: ThreeWayDiffConflict.NON_SOLVABLE, | ||
}; | ||
} | ||
default: | ||
return assertUnreachable(diffOutcome); | ||
} | ||
}; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove the scenarios which are not possible, since the user cannot edit the rule type, or change the type of the installed rule in any way, not even by API:
Not possible:
Possible:
I think the not-possible scenarios shouldn't have test cases here cause it makes it confusing, maybe we can just add it as a comment at the top of this file. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user can change between
query
andsaved_query
rule types in the UI. Not saying that's a super common use case but it can technically be done. I think that would be enough to keep those other scenarios in here and maybe just update the tests to reflect that instead ofeql
? I just used that for mocking simplicity since it'd be testing the same logic under the hoodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dplumlee You are right about
query
andsaved_query
, I hadn't realized that.Then, considering that, and what Paula mentioned in #193372 (comment), I think we should leave the tests cases as they are and add explanations in comments, detailing that such scenarios are only possible for that particular rule change; and other rule type changes should not be possible in the app.