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] Fixes required_fields being removed after rule PATCH calls #199901

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Nov 13, 2024

Fixes #199665

Summary

Fixes the required_fields field being removed from the existing rule when not present in the rule PATCH API call.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dplumlee dplumlee added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes v9.0.0 Feature:Detection Rules Security Solution rules and Detection Engine 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 v8.17.0 v8.16.1 labels Nov 13, 2024
@dplumlee dplumlee self-assigned this Nov 13, 2024
@dplumlee dplumlee requested a review from a team as a code owner November 13, 2024 03:11
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@dplumlee dplumlee added the backport:version Backport to applied version labels label Nov 13, 2024
@banderror banderror added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Nov 13, 2024
@banderror banderror requested review from xcrzx and banderror and removed request for nikitaindik and xcrzx November 13, 2024 11:24
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7397

[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_patch/basic_license_essentials_tier/configs/ess.config.ts: 100/100 tests passed.
[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_patch/basic_license_essentials_tier/configs/serverless.config.ts: 100/100 tests passed.

see run history

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Reviewed the diff and tested the fix locally in main with the feature flag turned OFF.

I installed a prebuilt rule and PATCHed its name. Everything worked as expected:

  • The name field was updated
  • The required_fields field remained unchanged
  • No other rule parameters were changed by mistake
  • A few technical fields were updated:
    • updated_at
    • revision was updated from 0 to 1
    • rule_source.is_customized was updated from false to true

Here's a diff between two revisions of the rule and a screenshot of the updated rule's Details page:

Screenshot 2024-11-14 at 15 53 51

So the bug has been fixed and we don't have any other similar bugs in the PATCH endpoint.

@dplumlee Thank you for the fix and adding that integration test! 🚀

@banderror banderror added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Nov 14, 2024
@banderror banderror enabled auto-merge (squash) November 14, 2024 14:56
@dplumlee
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #12 / discover/group5 discover tab field data shows top-level object keys

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [53c05a3]

History

cc @dplumlee

@dplumlee dplumlee force-pushed the rule-patch-required-fields-fix branch from d965d89 to aea65f4 Compare November 14, 2024 18:53
@elasticmachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #32 / serverless search UI Elasticsearch Start [Onboarding Empty State] developer should show the api key in code view

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [4d1cb0b]

History

cc @dplumlee

@banderror
Copy link
Contributor

@elasticmachine merge upstream

@banderror banderror merged commit f716948 into elastic:main Nov 15, 2024
44 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11858792233

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 15, 2024
…`PATCH` calls (elastic#199901)

**Fixes elastic#199665

## Summary

Fixes the `required_fields` field being removed from the existing rule
when not present in the rule `PATCH` API call.

### Checklist

Delete any items that are not applicable to this PR.

- [x] [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
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

### 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: Georgii Gorbachev <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit f716948)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 15, 2024
…`PATCH` calls (elastic#199901)

**Fixes elastic#199665

## Summary

Fixes the `required_fields` field being removed from the existing rule
when not present in the rule `PATCH` API call.

### Checklist

Delete any items that are not applicable to this PR.

- [x] [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
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

### 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: Georgii Gorbachev <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit f716948)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.16
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@dplumlee dplumlee deleted the rule-patch-required-fields-fix branch November 15, 2024 15:23
kibanamachine added a commit that referenced this pull request Nov 15, 2024
…moved after rule &#x60;PATCH&#x60; calls (#199901) (#200306)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[Security Solution] Fixes &#x60;required_fields&#x60; being removed
after rule &#x60;PATCH&#x60; calls
(#199901)](#199901)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-15T15:17:37Z","message":"[Security
Solution] Fixes `required_fields` being removed after rule `PATCH` calls
(#199901)\n\n**Fixes
https://github.com/elastic/kibana/issues/199665**\r\n\r\n##
Summary\r\n\r\nFixes the `required_fields` field being removed from the
existing rule\r\nwhen not present in the rule `PATCH` API
call.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f716948053c1b6f4a9f1dda27d4f5a501631b692","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","impact:medium","v9.0.0","Feature:Detection
Rules","Team:Detections and Resp","Team:
SecuritySolution","Team:Detection Rule
Management","backport:version","v8.17.0","v8.16.1"],"title":"[Security
Solution] Fixes `required_fields` being removed after rule `PATCH`
calls","number":199901,"url":"https://github.com/elastic/kibana/pull/199901","mergeCommit":{"message":"[Security
Solution] Fixes `required_fields` being removed after rule `PATCH` calls
(#199901)\n\n**Fixes
https://github.com/elastic/kibana/issues/199665**\r\n\r\n##
Summary\r\n\r\nFixes the `required_fields` field being removed from the
existing rule\r\nwhen not present in the rule `PATCH` API
call.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f716948053c1b6f4a9f1dda27d4f5a501631b692"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199901","number":199901,"mergeCommit":{"message":"[Security
Solution] Fixes `required_fields` being removed after rule `PATCH` calls
(#199901)\n\n**Fixes
https://github.com/elastic/kibana/issues/199665**\r\n\r\n##
Summary\r\n\r\nFixes the `required_fields` field being removed from the
existing rule\r\nwhen not present in the rule `PATCH` API
call.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f716948053c1b6f4a9f1dda27d4f5a501631b692"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <[email protected]>
kibanamachine added a commit that referenced this pull request Nov 15, 2024
…oved after rule &#x60;PATCH&#x60; calls (#199901) (#200307)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Fixes &#x60;required_fields&#x60; being removed
after rule &#x60;PATCH&#x60; calls
(#199901)](#199901)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-15T15:17:37Z","message":"[Security
Solution] Fixes `required_fields` being removed after rule `PATCH` calls
(#199901)\n\n**Fixes
https://github.com/elastic/kibana/issues/199665**\r\n\r\n##
Summary\r\n\r\nFixes the `required_fields` field being removed from the
existing rule\r\nwhen not present in the rule `PATCH` API
call.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f716948053c1b6f4a9f1dda27d4f5a501631b692","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","impact:medium","v9.0.0","Feature:Detection
Rules","Team:Detections and Resp","Team:
SecuritySolution","Team:Detection Rule
Management","backport:version","v8.17.0","v8.16.1"],"title":"[Security
Solution] Fixes `required_fields` being removed after rule `PATCH`
calls","number":199901,"url":"https://github.com/elastic/kibana/pull/199901","mergeCommit":{"message":"[Security
Solution] Fixes `required_fields` being removed after rule `PATCH` calls
(#199901)\n\n**Fixes
https://github.com/elastic/kibana/issues/199665**\r\n\r\n##
Summary\r\n\r\nFixes the `required_fields` field being removed from the
existing rule\r\nwhen not present in the rule `PATCH` API
call.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f716948053c1b6f4a9f1dda27d4f5a501631b692"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199901","number":199901,"mergeCommit":{"message":"[Security
Solution] Fixes `required_fields` being removed after rule `PATCH` calls
(#199901)\n\n**Fixes
https://github.com/elastic/kibana/issues/199665**\r\n\r\n##
Summary\r\n\r\nFixes the `required_fields` field being removed from the
existing rule\r\nwhen not present in the rule `PATCH` API
call.\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n\r\n\r\n### For maintainers\r\n\r\n-
[ ] This was checked for breaking API changes and was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels)\r\n-
[ ] This will appear in the **Release Notes** and follow
the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Georgii Gorbachev <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"f716948053c1b6f4a9f1dda27d4f5a501631b692"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.1","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <[email protected]>
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 bug Fixes for quality problems that affect the customer experience Feature:Detection Rules Security Solution rules and Detection Engine impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. release_note:fix 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.16.1 v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Required fields are getting erased on rule PATCH
4 participants