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] Extend bulk edit rules API to return skipped rules in response #145093

Closed
jpdjere opened this issue Nov 14, 2022 · 4 comments · Fixed by #147345
Closed

[Security Solution] Extend bulk edit rules API to return skipped rules in response #145093

jpdjere opened this issue Nov 14, 2022 · 4 comments · Fixed by #147345
Assignees
Labels
8.7 candidate enhancement New value added to drive a business result Feature:Rule Management Security Solution Detection Rule Management area 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. v8.7.0

Comments

@jpdjere
Copy link
Contributor

jpdjere commented Nov 14, 2022

Needed for: #139802
Docs issue: elastic/security-docs#2684

Summary

Introduce a new skipped property in the API response of the /api/detection_engine/rules/_bulk_action endpoint that would contain rules that were intentionally skipped and not updated by the endpoint.

attributes:
  results:
    # new property:
    skipped: [
        { id: "rule id", name: "rule name", skip_reason: "RULE_NOT_MODIFIED" },
    ]
  summary:
    # new property:
    skipped: 1

Add support for skipping rules into the business logic of the RulesClient.bulkEdit() method.

Details

The current behavior of the endpoint is that, if operations on parameters or attributes of the rules result in a no-op, then the update "silently succeeds" and the response reports the rules as being updated. Also, rules saved objects get technically updated under the hood as well, just with the same parameters.

Two current examples of this are:

  1. Attempting to add an index_pattern to a rule which is configured to use a data_view, when the option to override data views is not selected.
  2. Attempting to add a tag to a rule which already contains that tag in its tag array attribute.

Since the bulk update API allows a user to update properties of a rule that are either attributes as well as params, in order for a rule to consider skipped, all operations attempted on a single rule have to be considered as skipped for the whole rule to be considered skipped. Only in that case should the rule be returned in the skipped property of the API response.

If a rule is considered skipped, then do not update its saved_object or invalidate its api keys.

Note that multiple operations on a single rule on a single API call are only possible when executing the call directly via the API. The usage of this endpoint via the UI does not allow for multiple operations as of yet.

If any one operation is applied to rule successfully, then the rule as a whole should be considered updated and returned in that property of the response.

The skipped rule object does not need to return the whole rule information, but it should contain its id, name and a reason why the rule was skipped. For example:

{ id: "RULE_1_ID", name: "RULE_1_NAME", skip_reason: "RULE_NOT_MODIFIED" }

The updated response of the endpoint should look like this (example):

attributes:
  results:
    created: []
    deleted: []
    updated: []
    skipped: [
        { id: "RULE_1_ID", name: "RULE_1_NAME", skip_reason: "RULE_NOT_MODIFIED" },
        { id: "RULE_2_ID", name: "RULE_2_NAME", skip_reason: "RULE_NOT_MODIFIED" },
    ]
  summary:
    failed: 0
    succeeded: 0
    skipped: 1
    total: 1
rules_count: 1
success: true

Cases for skipping rule updates

  1. Attempting to add an index_pattern to a rule which is configured to use a data_view, when the option to override data views is not selected.
  2. Attempting to add an index_pattern to a rule which already contains that index pattern in its index array parameter.
  3. Attempting to delete an index_pattern from a rule which doesn't contain that index pattern in its index array parameter.
  4. Attempting to add a tag to a rule which already contains that tag in its tag array attribute.
  5. Attempting to delete a tag from a rule which doesn't contain that tag in its tag array attribute.
@jpdjere jpdjere self-assigned this Nov 14, 2022
@botelastic botelastic bot added the needs-team Issues missing a team label label Nov 14, 2022
@jpdjere jpdjere added enhancement New value added to drive a business result Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team and removed needs-team Issues missing a team label labels Nov 14, 2022
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@banderror
Copy link
Contributor

@jpdjere I updated the ticket's description a little bit: added a summary and did a few minor adjustments.

@jpdjere
Copy link
Contributor Author

jpdjere commented Dec 19, 2022

Follow up work: as discussed with @spong in #147345 (comment)

we'd like to add additional checks for noop updated in the case of the Bulk Edit type being:

  1. set_schedule
  2. set_timeline

jpdjere added a commit that referenced this issue Dec 20, 2022
…#147345)

**Addresses:** #145093
**Related to:** #139802

## Summary

- Extends Bulk Edit API to return a new `skipped` property for rules
whose updating was skipped. See
[#145093](#145093) for details
on when a rule is skipped.
- In `x-pack/plugins/alerting/server/rules_client/methods/bulk_edit.ts`,
refactored the methods `bulkEdit` and `bulkEditOcc` to smaller methods,
following an immutable approach.
- Updated all related tests and expanded coverage. (unit, integration
and e2e)
- Update success toast message so that the user is informed if rules
were skipped.


https://user-images.githubusercontent.com/5354282/199806913-eb70e7a6-0435-486a-96f1-dd0e8abaffe2.mp4

### 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)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
  - elastic/security-docs#2684
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)



### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

Co-authored-by: Xavier Mouligneau <[email protected]>
banderror pushed a commit to elastic/security-docs that referenced this issue Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.7 candidate enhancement New value added to drive a business result Feature:Rule Management Security Solution Detection Rule Management area 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. v8.7.0
Projects
None yet
3 participants