Skip to content
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] Test plan for prebuilt rule customization #204888

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dplumlee
Copy link
Contributor

Summary

Addresses #202068

Adds test plan for rule customization features related to the milestone 3 prebuilt rule customization epic

@dplumlee dplumlee added test-plan v9.0.0 Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area backport:version Backport to applied version labels v8.18.0 labels Dec 19, 2024
@dplumlee dplumlee self-assigned this Dec 19, 2024
@dplumlee dplumlee requested a review from a team as a code owner December 19, 2024 06:17
@dplumlee dplumlee requested a review from maximpn December 19, 2024 06:17
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@dplumlee dplumlee added the release_note:skip Skip the PR/issue when compiling release notes label Dec 19, 2024
@banderror banderror requested a review from pborgonovi December 23, 2024 19:09
@banderror
Copy link
Contributor

@pborgonovi Please review this one

Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dplumlee Thanks for adding Prebuilt Rules Customization test plan 🙏

I reviewed the test plan and left suggestions to make the test plan more robust and improve readability.

It looks like this test plan should include the following scenarios as well

  • A test scenario (with examples) for each field affecting isCustomized
  • Custom rules shouldn't have "Modified" badge on rule details page
  • Custom rules shouldn't have "Modified" badge on rule management table
  • Scenarios covering ruleSource
  • Users can navigate to rule edit page from prebuilt rule details

Feel free to reach me to discuss it over Zoom in case of questions.


```Gherkin
Given a space with at least one prebuilt rule installed
And the rule is unmodified
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
And the rule is unmodified
And the rule is non-customized

```Gherkin
Given a space with at least one prebuilt rule installed
And the rule is unmodified
When a user edits the rule from the rule edit page to something different than the original version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When a user edits the rule from the rule edit page to something different than the original version
When user changes any rule field value (so it differs from the base version) in rule edit form
And saves the form

Editing a rule is possible only via rule editing form but the same terminology isn't applicable to CRUD API. Change rule field sounds more generic. WDYT?

Comment on lines +41 to +42
And the ruleSource should be "external"
And isCustomized should be true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ruleSource and isCustomized be explained in terminology?

**Automation**: 1 cypress test.

```Gherkin
Given a space with at least one customized prebuilt rule installed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Given a space with at least one customized prebuilt rule installed
Given a space with at least one prebuilt rule installed
And it is customized


```Gherkin
Given a space with at least one customized prebuilt rule installed
When a user edits the rule from the rule edit page to something different than the original version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When a user edits the rule from the rule edit page to something different than the original version
When user changes any rule field value (so it differs from the base version) in rule edit form
And saves the form


### Assumptions

- Rule package used will have all rule versions present (no missing base versions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Rule package used will have all rule versions present (no missing base versions)
- Rule package used will have all previous rule versions present (no missing base versions)


### Terminology

- **Base version**: The version of the rule we ship with the rule package, can be thought of as the "original" version of the rule.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **Base version**: The version of the rule we ship with the rule package, can be thought of as the "original" version of the rule.
- **Base version**: Prebuilt rule asset we ship in the rule package corresponding to the currently installed prebuilt rules. It represents "original" version of the rule. During prebuilt rules installation prebuilt rule assets data is copied over and becomes an installed prebuilt rule.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we say that it's a prebuilt rule asset whose "version" field matches the "version" of an installed rule? WDYT?


- **Base version**: The version of the rule we ship with the rule package, can be thought of as the "original" version of the rule.

- **Customized prebuilt rule**: A prebuilt rule that has been changed by the user from the base version of the rule. Also referred to as "Modified" in the UI.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **Customized prebuilt rule**: A prebuilt rule that has been changed by the user from the base version of the rule. Also referred to as "Modified" in the UI.
- **Customized prebuilt rule**: An installed prebuilt rule that has been changed by the user in the way rule fields semantically differ from the base version. Also referred to as "Modified" in the UI.


- **Customized prebuilt rule**: A prebuilt rule that has been changed by the user from the base version of the rule. Also referred to as "Modified" in the UI.

- **Non-customized prebuilt rule**: A prebuilt rule that has no change from the base version of the rule.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **Non-customized prebuilt rule**: A prebuilt rule that has no change from the base version of the rule.
- **Non-customized prebuilt rule**: An installed prebuilt rule that has rule fields values matching the base version.


- **Non-customized prebuilt rule**: A prebuilt rule that has no change from the base version of the rule.

- **Custom rules**: A rule created by the user themselves
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **Custom rules**: A rule created by the user themselves
- **Custom rule**: A rule created by the user themselves

@nikitaindik nikitaindik self-requested a review January 6, 2025 13:28
```Gherkin
Given a space with at least one customized prebuilt rule installed
When a user navigates to that rule's detail page
Then the rule's isCustomized value should be true
Copy link
Contributor

@nikitaindik nikitaindik Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be snake case, since it'll come from our API

Suggested change
Then the rule's isCustomized value should be true
Then the rule's `is_customized` value should be true

If you are going to incorporate this suggestion, please check other places as well

Copy link
Contributor

@nikitaindik nikitaindik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @dplumlee! Overall, the test coverage looks solid. I’ve left a couple of comments for you to consider.

One potential improvement: should we add a scenario to ensure that the “Edit” buttons in the rule details page and rule management table are clickable when the feature flag is enabled?

Additionally, do we want to include a test to verify that non-modifiable fields, such as version and id are uneditable?”

Looking forward to your thoughts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. test-plan v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants