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] Handle conflicting rules in the Rule Upgrade table #180589

Closed
Tracked by #174168
jpdjere opened this issue Apr 11, 2024 · 5 comments · Fixed by #196776
Closed
Tracked by #174168

[Security Solution] Handle conflicting rules in the Rule Upgrade table #180589

jpdjere opened this issue Apr 11, 2024 · 5 comments · Fixed by #196776
Assignees
Labels
8.17 candidate Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules needs design 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.

Comments

@jpdjere
Copy link
Contributor

jpdjere commented Apr 11, 2024

Epics: https://github.com/elastic/security-team/issues/1974 (internal), #174168
Design Discussion context: #178211
Design: ?

Summary

The goal is to update the rules management table to handle conflicts during the rule upgrade workflow more effectively. Below are the key changes and improvements:

  1. Upgrade All Button Behavior:

    • The "Upgrade All" button should now only upgrade rules that do not contain conflicts. The system should automatically exclude rules with conflicts from the upgrade process.
    • The UI needs to be updated to clearly communicate this to users, ensuring they understand that only conflict-free rules will be upgraded when using this option.
  2. Bulk Upgrade Action:

    • When users select multiple rules for a bulk upgrade, only those without conflicts should be upgraded. The system should automatically exclude rules with conflicts from the upgrade process.
    • If a user selects rules that include conflicts, a modal window should appear (similar to the one used during bulk edits). This modal will inform the user that some selected rules contain conflicts and cannot be upgraded in bulk. The user will then be prompted to proceed with upgrading only the rules without conflicts.
  3. Reviewing Conflicts:

    • Rules that contain conflicts, including those with solvable conflicts, must be reviewed before they can be upgraded. Users need to open the upgrade preview, review the proposed changes, and then decide whether to accept or decline the changes.
    • The option to upgrade individual rules directly from the rules management table should be disabled for rules that have conflicts. Users must review the conflicts through the upgrade preview before proceeding with any upgrade.

This update aims to enhance the user experience by ensuring that conflict handling during rule upgrades is clear, intuitive, and prevents errors or unintended consequences.

Details

  • When saying "conflicts" above we mean both solvable and non-solvable conflicts. All of them should be excluded from bulk and single actions. Users will need to review such rules one-by-one using the Rule Upgrade flyout.
  • The bulk and single upgrade actions mentioned above should be updated to upgrade rules to the MERGED version instead of the TARGET one.
@jpdjere jpdjere added triage_needed 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 labels Apr 11, 2024
@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)

@jpdjere jpdjere added the Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules label Apr 11, 2024
@banderror banderror changed the title [Security Solution] Add ability to bulk update all rules that don't have conflicts [Security Solution] Add ability to bulk update all rules that don't have conflicts (DRAFT) Apr 17, 2024
@banderror banderror changed the title [Security Solution] Add ability to bulk update all rules that don't have conflicts (DRAFT) [Security Solution] Add ability to bulk update all rules that don't have conflicts Jul 12, 2024
@jpdjere
Copy link
Contributor Author

jpdjere commented Jul 12, 2024

Hi @ARWNightingale

We have a pretty clear idea of the functionality that we want to implement in this ticket, as is described above. But we'd like some product/design input to answer the following questions:

  • Where do we place this button? Should it replace our current "Upgrade all" button? (Should it be a button at all?)
  • How do we explain to the user that only rules with 0 conflicts will be updated? A pre-confirmation modal? A tooltip? What text should we use?

This is the current Rule Updates table, with the "Update All" button at the top-right:
image

@banderror banderror changed the title [Security Solution] Add ability to bulk update all rules that don't have conflicts [Security Solution] Handle conflicting rules in the Rule Upgrade table (upgrade to MERGED, exclude conflicts) Aug 13, 2024
@banderror banderror changed the title [Security Solution] Handle conflicting rules in the Rule Upgrade table (upgrade to MERGED, exclude conflicts) [Security Solution] Handle conflicting rules in the Rule Upgrade table (upgrade to MERGED, exclude conflicts) (DRAFT) Aug 13, 2024
@banderror banderror changed the title [Security Solution] Handle conflicting rules in the Rule Upgrade table (upgrade to MERGED, exclude conflicts) (DRAFT) [Security Solution] Handle conflicting rules in the Rule Upgrade table Aug 14, 2024
@banderror
Copy link
Contributor

Hey @approksiu and @ARWNightingale, we updated the description based on the conversations we had in the last couple of days. Please review. We will need some designs for that one.

@xcrzx xcrzx closed this as completed in 99160f5 Nov 11, 2024
xcrzx pushed a commit to xcrzx/kibana that referenced this issue Nov 11, 2024
Resolves: elastic#180589

## Summary

- Handles bulk updating of rules with conflicts in the Rule Upgrades
table. See detailed requirements implemented in ticket linked above.
- Changes default `pick_version` of both the `/upgrade/_perform`
endpoint, and of the request payloads for that endpoint from the
frontend, from `TARGET` to `MERGED`, when the
`isPrebuiltRulesCustomizationEnabled` is `true`.
- **Also:** handles issue in `/upgrade/_perform` endpoint with the
`index` and `data_view_id` fields. See file:
`x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/perform_rule_upgrade/diffable_rule_fields_mappings.ts`.

**See demo video:**
https://www.loom.com/share/90d94d2a8f16442b9a43a425eeab6697

**New copy in warning modal**
<img width="1660" alt="image"
src="https://github.com/user-attachments/assets/85632192-142a-4e12-b396-1eb2320ca3f4">

**Newly added tooltips:**

![image](https://github.com/user-attachments/assets/7ada117e-57a7-4699-ad08-312c734586d9)

![image](https://github.com/user-attachments/assets/c8ed80ac-c1c3-48f1-8f8e-2433415a6772)

![image](https://github.com/user-attachments/assets/d77ed6f0-5d65-4933-9012-f6cd153a9350)

### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)
- [ ] This will appear in the **Release Notes** and follow the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Nastasha Solomon <[email protected]>
Co-authored-by: Dmitrii <[email protected]>
(cherry picked from commit 99160f5)

# Conflicts:
#	.github/CODEOWNERS
tkajtoch pushed a commit to tkajtoch/kibana that referenced this issue Nov 12, 2024
Resolves: elastic#180589

## Summary

- Handles bulk updating of rules with conflicts in the Rule Upgrades
table. See detailed requirements implemented in ticket linked above.
- Changes default `pick_version` of both the `/upgrade/_perform`
endpoint, and of the request payloads for that endpoint from the
frontend, from `TARGET` to `MERGED`, when the
`isPrebuiltRulesCustomizationEnabled` is `true`.
- **Also:** handles issue in `/upgrade/_perform` endpoint with the
`index` and `data_view_id` fields. See file:
`x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/perform_rule_upgrade/diffable_rule_fields_mappings.ts`.

**See demo video:**
https://www.loom.com/share/90d94d2a8f16442b9a43a425eeab6697

**New copy in warning modal**
<img width="1660" alt="image"
src="https://github.com/user-attachments/assets/85632192-142a-4e12-b396-1eb2320ca3f4">

**Newly added tooltips:** 


![image](https://github.com/user-attachments/assets/7ada117e-57a7-4699-ad08-312c734586d9)

![image](https://github.com/user-attachments/assets/c8ed80ac-c1c3-48f1-8f8e-2433415a6772)

![image](https://github.com/user-attachments/assets/d77ed6f0-5d65-4933-9012-f6cd153a9350)


### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)
- [ ] This will appear in the **Release Notes** and follow the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Nastasha Solomon <[email protected]>
Co-authored-by: Dmitrii <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.17 candidate Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules needs design 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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants