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] Remove exceptions_list, author and license from Diffable Rule #196561

Merged
merged 11 commits into from
Oct 18, 2024

Conversation

jpdjere
Copy link
Contributor

@jpdjere jpdjere commented Oct 16, 2024

Resolves: #196213

Summary

Excludes the fields exceptions_list, author and license from the DiffableRule definition.

This will:

  • prevent them from appearing in the Three Way Diff component
  • prevent them from being able to be passed as a value in the fields object of the /upgrade/_perform endpoint to set a specific pick_version for it (NOTE: the current logic already forces exceptions_list to upgrade to the CURRENT version, but removing it from DiffableRule, will completely remove the from the payload schema, and the endpoint will then throw a validation error if included, rather than silently ignoring it)

Screenshots

Before

image

After

image

For maintainers

@jpdjere jpdjere added 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:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) 8.16 candidate labels Oct 16, 2024
@jpdjere jpdjere self-assigned this Oct 16, 2024
@jpdjere jpdjere requested a review from a team as a code owner October 16, 2024 15:00
@jpdjere jpdjere requested a review from nikitaindik October 16, 2024 15:00
@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 requested a review from a team as a code owner October 16, 2024 17:56
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikitaindik Deleted the author and license components since they won't be needed, and the DiffableRule type doesn't allow it anymore anyways. Let me know if this makes sense.

@@ -10,10 +10,4 @@ import type { DiffableAllFields } from '../../../../../../../common/api/detectio
type NonEditableFields = Readonly<Set<keyof DiffableAllFields>>;

/* These fields are not visible in the comparison UI and are not editable */
export const HIDDEN_FIELDS: NonEditableFields = new Set([
'alert_suppression',
Copy link
Contributor Author

@jpdjere jpdjere Oct 16, 2024

Choose a reason for hiding this comment

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

@nikitaindik alert_suppression actually needs to be displayed in the diff and be able to be customized by the user. Can you make a note of that for the work that you and Maxim are currently doing?

Copy link
Contributor

@nikitaindik nikitaindik Oct 17, 2024

Choose a reason for hiding this comment

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

Will do.

Actually, can you please remove this whole file since these constants are not used anywhere. It's a duplicate of this constant. I thought I removed it but it looks like it got resurrected during a merge.

I wonder if I can just reuse your FIELDS_TO_UPGRADE_TO_TARGET_VERSION constant to hide non-editable fields from the "left side / right side" in UI?

@jpdjere jpdjere requested a review from a team as a code owner October 16, 2024 19:14
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.

Thank @jpdjere! The PR LGTM overall. Left a couple comments.

@jpdjere
Copy link
Contributor Author

jpdjere commented Oct 17, 2024

@nikitaindik

Removed the file with the constants.

I wonder if I can just reuse your FIELDS_TO_UPGRADE_TO_TARGET_VERSION constant to hide non-editable fields from the "left side / right side" in UI?

Yes. I think you can actually use both FIELDS_TO_UPGRADE_TO_TARGET_VERSION and FIELDS_TO_UPGRADE_TO_CURRENT_VERSION, since all of the fields listed there are "read only" during the upgrade and it doesn't make sense to display them in the UI.

@jpdjere jpdjere requested a review from nikitaindik October 17, 2024 15:28
@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #82 / Serverless Common UI - Management Index Management Indices can delete index

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 6038 6036 -2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 20.7MB 20.7MB -1.6KB

History

cc @jpdjere

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! Changes LGTM.

@jpdjere jpdjere merged commit 716fdb2 into elastic:main Oct 18, 2024
44 checks passed
@jpdjere jpdjere deleted the remove-diffable-fields-2 branch October 18, 2024 14:50
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 18, 2024
…from Diffable Rule (elastic#196561)

Resolves: elastic#196213

## Summary

Excludes the fields `exceptions_list`, `author` and `license` from the
`DiffableRule` definition.

This will:

- prevent them from appearing in the Three Way Diff component
- prevent them from being able to be passed as a value in the `fields`
object of the `/upgrade/_perform` endpoint to set a specific
`pick_version` for it (NOTE: the current logic already forces
`exceptions_list` to upgrade to the CURRENT version, but removing it
from DiffableRule, will completely remove the from the payload schema,
and the endpoint will then throw a validation error if included, rather
than silently ignoring it)

## Screenshots

### Before

![image](https://github.com/user-attachments/assets/aacd0b43-bb29-46d0-990d-c669224c1451)

### After

![image](https://github.com/user-attachments/assets/e568ca7f-03fc-42d6-8879-d3f23558ae9d)

### 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)

(cherry picked from commit 716fdb2)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 18, 2024
…from Diffable Rule (elastic#196561)

Resolves: elastic#196213

## Summary

Excludes the fields `exceptions_list`, `author` and `license` from the
`DiffableRule` definition.

This will:

- prevent them from appearing in the Three Way Diff component
- prevent them from being able to be passed as a value in the `fields`
object of the `/upgrade/_perform` endpoint to set a specific
`pick_version` for it (NOTE: the current logic already forces
`exceptions_list` to upgrade to the CURRENT version, but removing it
from DiffableRule, will completely remove the from the payload schema,
and the endpoint will then throw a validation error if included, rather
than silently ignoring it)

## Screenshots

### Before

![image](https://github.com/user-attachments/assets/aacd0b43-bb29-46d0-990d-c669224c1451)

### After

![image](https://github.com/user-attachments/assets/e568ca7f-03fc-42d6-8879-d3f23558ae9d)

### 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)

(cherry picked from commit 716fdb2)
@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

jpdjere added a commit that referenced this pull request Oct 18, 2024
…author&#x60; and &#x60;license&#x60; from Diffable Rule (#196561) (#196903)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[Security Solution] Remove &#x60;exceptions_list&#x60;,
&#x60;author&#x60; and &#x60;license&#x60; from Diffable Rule
(#196561)](#196561)

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

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

<!--BACKPORT [{"author":{"name":"Juan Pablo
Djeredjian","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-18T14:50:36Z","message":"[Security
Solution] Remove `exceptions_list`, `author` and `license` from Diffable
Rule (#196561)\n\nResolves:
https://github.com/elastic/kibana/issues/196213\r\n\r\n##
Summary\r\n\r\nExcludes the fields `exceptions_list`, `author` and
`license` from the\r\n`DiffableRule` definition.\r\n\r\nThis
will:\r\n\r\n- prevent them from appearing in the Three Way Diff
component\r\n- prevent them from being able to be passed as a value in
the `fields`\r\nobject of the `/upgrade/_perform` endpoint to set a
specific\r\n`pick_version` for it (NOTE: the current logic already
forces\r\n`exceptions_list` to upgrade to the CURRENT version, but
removing it\r\nfrom DiffableRule, will completely remove the from the
payload schema,\r\nand the endpoint will then throw a validation error
if included, rather\r\nthan silently ignoring it)\r\n\r\n##
Screenshots\r\n\r\n###
Before\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/aacd0b43-bb29-46d0-990d-c669224c1451)\r\n\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e568ca7f-03fc-42d6-8879-d3f23558ae9d)\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#kibana-release-notes-process)","sha":"716fdb23c7d42c9f7c29525af793ff1594ad67f0","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","backport:prev-minor","v8.16.0"],"title":"[Security Solution]
Remove `exceptions_list`, `author` and `license` from Diffable
Rule","number":196561,"url":"https://github.com/elastic/kibana/pull/196561","mergeCommit":{"message":"[Security
Solution] Remove `exceptions_list`, `author` and `license` from Diffable
Rule (#196561)\n\nResolves:
https://github.com/elastic/kibana/issues/196213\r\n\r\n##
Summary\r\n\r\nExcludes the fields `exceptions_list`, `author` and
`license` from the\r\n`DiffableRule` definition.\r\n\r\nThis
will:\r\n\r\n- prevent them from appearing in the Three Way Diff
component\r\n- prevent them from being able to be passed as a value in
the `fields`\r\nobject of the `/upgrade/_perform` endpoint to set a
specific\r\n`pick_version` for it (NOTE: the current logic already
forces\r\n`exceptions_list` to upgrade to the CURRENT version, but
removing it\r\nfrom DiffableRule, will completely remove the from the
payload schema,\r\nand the endpoint will then throw a validation error
if included, rather\r\nthan silently ignoring it)\r\n\r\n##
Screenshots\r\n\r\n###
Before\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/aacd0b43-bb29-46d0-990d-c669224c1451)\r\n\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e568ca7f-03fc-42d6-8879-d3f23558ae9d)\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#kibana-release-notes-process)","sha":"716fdb23c7d42c9f7c29525af793ff1594ad67f0"}},"sourceBranch":"main","suggestedTargetBranches":["8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196561","number":196561,"mergeCommit":{"message":"[Security
Solution] Remove `exceptions_list`, `author` and `license` from Diffable
Rule (#196561)\n\nResolves:
https://github.com/elastic/kibana/issues/196213\r\n\r\n##
Summary\r\n\r\nExcludes the fields `exceptions_list`, `author` and
`license` from the\r\n`DiffableRule` definition.\r\n\r\nThis
will:\r\n\r\n- prevent them from appearing in the Three Way Diff
component\r\n- prevent them from being able to be passed as a value in
the `fields`\r\nobject of the `/upgrade/_perform` endpoint to set a
specific\r\n`pick_version` for it (NOTE: the current logic already
forces\r\n`exceptions_list` to upgrade to the CURRENT version, but
removing it\r\nfrom DiffableRule, will completely remove the from the
payload schema,\r\nand the endpoint will then throw a validation error
if included, rather\r\nthan silently ignoring it)\r\n\r\n##
Screenshots\r\n\r\n###
Before\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/aacd0b43-bb29-46d0-990d-c669224c1451)\r\n\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e568ca7f-03fc-42d6-8879-d3f23558ae9d)\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#kibana-release-notes-process)","sha":"716fdb23c7d42c9f7c29525af793ff1594ad67f0"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Juan Pablo Djeredjian <[email protected]>
jpdjere added a commit that referenced this pull request Oct 18, 2024
…uthor&#x60; and &#x60;license&#x60; from Diffable Rule (#196561) (#196904)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Remove &#x60;exceptions_list&#x60;,
&#x60;author&#x60; and &#x60;license&#x60; from Diffable Rule
(#196561)](#196561)

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

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

<!--BACKPORT [{"author":{"name":"Juan Pablo
Djeredjian","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-18T14:50:36Z","message":"[Security
Solution] Remove `exceptions_list`, `author` and `license` from Diffable
Rule (#196561)\n\nResolves:
https://github.com/elastic/kibana/issues/196213\r\n\r\n##
Summary\r\n\r\nExcludes the fields `exceptions_list`, `author` and
`license` from the\r\n`DiffableRule` definition.\r\n\r\nThis
will:\r\n\r\n- prevent them from appearing in the Three Way Diff
component\r\n- prevent them from being able to be passed as a value in
the `fields`\r\nobject of the `/upgrade/_perform` endpoint to set a
specific\r\n`pick_version` for it (NOTE: the current logic already
forces\r\n`exceptions_list` to upgrade to the CURRENT version, but
removing it\r\nfrom DiffableRule, will completely remove the from the
payload schema,\r\nand the endpoint will then throw a validation error
if included, rather\r\nthan silently ignoring it)\r\n\r\n##
Screenshots\r\n\r\n###
Before\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/aacd0b43-bb29-46d0-990d-c669224c1451)\r\n\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e568ca7f-03fc-42d6-8879-d3f23558ae9d)\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#kibana-release-notes-process)","sha":"716fdb23c7d42c9f7c29525af793ff1594ad67f0","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Prebuilt Detection
Rules","backport:prev-minor","v8.16.0"],"title":"[Security Solution]
Remove `exceptions_list`, `author` and `license` from Diffable
Rule","number":196561,"url":"https://github.com/elastic/kibana/pull/196561","mergeCommit":{"message":"[Security
Solution] Remove `exceptions_list`, `author` and `license` from Diffable
Rule (#196561)\n\nResolves:
https://github.com/elastic/kibana/issues/196213\r\n\r\n##
Summary\r\n\r\nExcludes the fields `exceptions_list`, `author` and
`license` from the\r\n`DiffableRule` definition.\r\n\r\nThis
will:\r\n\r\n- prevent them from appearing in the Three Way Diff
component\r\n- prevent them from being able to be passed as a value in
the `fields`\r\nobject of the `/upgrade/_perform` endpoint to set a
specific\r\n`pick_version` for it (NOTE: the current logic already
forces\r\n`exceptions_list` to upgrade to the CURRENT version, but
removing it\r\nfrom DiffableRule, will completely remove the from the
payload schema,\r\nand the endpoint will then throw a validation error
if included, rather\r\nthan silently ignoring it)\r\n\r\n##
Screenshots\r\n\r\n###
Before\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/aacd0b43-bb29-46d0-990d-c669224c1451)\r\n\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e568ca7f-03fc-42d6-8879-d3f23558ae9d)\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#kibana-release-notes-process)","sha":"716fdb23c7d42c9f7c29525af793ff1594ad67f0"}},"sourceBranch":"main","suggestedTargetBranches":["8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196561","number":196561,"mergeCommit":{"message":"[Security
Solution] Remove `exceptions_list`, `author` and `license` from Diffable
Rule (#196561)\n\nResolves:
https://github.com/elastic/kibana/issues/196213\r\n\r\n##
Summary\r\n\r\nExcludes the fields `exceptions_list`, `author` and
`license` from the\r\n`DiffableRule` definition.\r\n\r\nThis
will:\r\n\r\n- prevent them from appearing in the Three Way Diff
component\r\n- prevent them from being able to be passed as a value in
the `fields`\r\nobject of the `/upgrade/_perform` endpoint to set a
specific\r\n`pick_version` for it (NOTE: the current logic already
forces\r\n`exceptions_list` to upgrade to the CURRENT version, but
removing it\r\nfrom DiffableRule, will completely remove the from the
payload schema,\r\nand the endpoint will then throw a validation error
if included, rather\r\nthan silently ignoring it)\r\n\r\n##
Screenshots\r\n\r\n###
Before\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/aacd0b43-bb29-46d0-990d-c669224c1451)\r\n\r\n\r\n###
After\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e568ca7f-03fc-42d6-8879-d3f23558ae9d)\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#kibana-release-notes-process)","sha":"716fdb23c7d42c9f7c29525af793ff1594ad67f0"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Juan Pablo Djeredjian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) 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. v8.16.0 v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Remove exceptions_list, author and license from Diffable Rule
4 participants