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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Investigations] - Add check for changing alert status from bulk options #170584

Merged
merged 4 commits into from
Nov 7, 2023
Merged

[Security Solution][Investigations] - Add check for changing alert status from bulk options #170584

merged 4 commits into from
Nov 7, 2023

Conversation

michaelolo24
Copy link
Contributor

@michaelolo24 michaelolo24 commented Nov 3, 2023 •
edited by kibanamachine
Loading

Summary

Addresses #169684

This PR is a re-do of: #169723 (With cypress tests currently skipped until proper role is available).
The alert privileges needs to be added for the alert table as it wasn't added when the migration took place. An example of the privileges elsewhere is below:

kibana/x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/use_alerts_actions.tsx

Line 33 in 75e9d46

const { hasIndexWrite } = useAlertsPrivileges();

Fix:

Screen.Recording.2023-10-25.at.12.24.18.PM.mov

@michaelolo24 michaelolo24 requested review from a team as code owners November 3, 2023 22:39
@michaelolo24 michaelolo24 requested a review from rylnd November 3, 2023 22:39
@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@michaelolo24 michaelolo24 force-pushed the fix-alert-status-readonly branch from 6fbe850 to 9fb8b1a Compare November 6, 2023 16:46
janmonschke
janmonschke reviewed Nov 6, 2023
View reviewed changes
...security_solution_cypress/cypress/e2e/detection_response/detection_alerts/alert_status.cy.ts Outdated
// This test is unable to be run in serverless as `reader` is not available and viewer is currently reserved
// https://github.com/elastic/kibana/pull/169723#issuecomment-1793191007
// https://github.com/elastic/kibana/issues/170583
context.skip('User is readonly', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is only broken in serverless, you could add these tags to enabled the test on ESS: { tags: ['@ess', '@brokenInServerless'] }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I'm not sure if I like the idea of skipping the entire suite on serverless, but hopefully the roles issue is resolved sooner than later

Copy link
Contributor

Choose a reason for hiding this comment

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

you don't have to skip the entire suite! You can just have the tags at the test level so only skip the problematic test(s).

Look at this file for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Silly me, thanks. Updated!

@michaelolo24 michaelolo24 force-pushed the fix-alert-status-readonly branch from ab314db to 3007a40 Compare November 6, 2023 19:06
@michaelolo24 michaelolo24 force-pushed the fix-alert-status-readonly branch from 87c1f81 to 36ff150 Compare November 6, 2023 20:02
PhilippeOberti
PhilippeOberti approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

LGTM!

@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 13.1MB 13.1MB +130.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

janmonschke
janmonschke approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@janmonschke janmonschke left a comment

Choose a reason for hiding this comment

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

🎉

e40pud
e40pud approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

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

DE changes LGTM

...ecurity_solution/public/common/components/toolbar/bulk_actions/use_bulk_alert_tags_items.tsx
disableOnQuery: true,
},
];
const alertTagsItems = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I will need to do the same in my PR here https://github.com/elastic/kibana/pull/170579/files#diff-0ae8cbf7c5c94adb495f9faff8a4ace978f69f2e048018ae8c1b9767d0b738d8R39 for alert assignments.

@michaelolo24 michaelolo24 merged commit 3651571 into elastic:main Nov 7, 2023
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.10 Backport failed because of merge conflicts
❌ 8.11 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.11:
- [ResponseOps] Add target to Connectors documentation link (#170677)

Manual backport

To create the backport manually run:

node scripts/backport --pr 170584

Questions ?

Please refer to the Backport tool documentation

@michaelolo24 michaelolo24 mentioned this pull request Nov 7, 2023
Merged
michaelolo24 added a commit to michaelolo24/kibana that referenced this pull request Nov 7, 2023
@michaelolo24
[Security Solution][Investigations] - Add check for changing alert st…
293e88c 
…atus from bulk options (elastic#170584)

## Summary
Addresses elastic#169684

This PR is a re-do of: elastic#169723
(With cypress tests currently skipped until proper role is available).
The alert privileges needs to be added for the alert table as it wasn't
added when the migration took place. An example of the privileges
elsewhere is below:

https://github.com/elastic/kibana/blob/75e9d46b4b3a6ff5be4ffc324ba282cea0faea0c/x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/use_alerts_actions.tsx#L33

Fix:

https://github.com/elastic/kibana/assets/17211684/7b354906-9b96-4ba8-b30f-4080cf7e7c2f

---------

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 3651571)

# Conflicts:
#	x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_alerts/alert_status.cy.ts
@michaelolo24 michaelolo24 mentioned this pull request Nov 7, 2023
Closed
@michaelolo24
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
✅ 8.11
✅ 8.10

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

Questions ?

Please refer to the Backport tool documentation

michaelolo24 added a commit to michaelolo24/kibana that referenced this pull request Nov 7, 2023
@michaelolo24
[Security Solution][Investigations] - Add check for changing alert st…
d1d6852 
…atus from bulk options (elastic#170584)

## Summary
Addresses elastic#169684

This PR is a re-do of: elastic#169723
(With cypress tests currently skipped until proper role is available).
The alert privileges needs to be added for the alert table as it wasn't
added when the migration took place. An example of the privileges
elsewhere is below:

https://github.com/elastic/kibana/blob/75e9d46b4b3a6ff5be4ffc324ba282cea0faea0c/x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/use_alerts_actions.tsx#L33

Fix:

https://github.com/elastic/kibana/assets/17211684/7b354906-9b96-4ba8-b30f-4080cf7e7c2f

---------

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 3651571)

# Conflicts:
#	x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_alerts/alert_status.cy.ts
michaelolo24 added a commit that referenced this pull request Nov 9, 2023
@michaelolo24 @kibanamachine
[8.11] [Security Solution][Investigations] - Add check for changing a…
507566e 
…lert status from bulk options (#170584) (#170780)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[Security Solution][Investigations] - Add check for changing alert
status from bulk options
(#170584)](#170584)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Michael
Olorunnisola","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-11-07T17:01:23Z","message":"[Security
Solution][Investigations] - Add check for changing alert status from
bulk options (#170584)\n\n## Summary\r\nAddresses
https://github.com/elastic/kibana/issues/169684\r\n\r\nThis PR is a
re-do of: https://github.com/elastic/kibana/pull/169723\r\n(With cypress
tests currently skipped until proper role is available).\r\nThe alert
privileges needs to be added for the alert table as it wasn't\r\nadded
when the migration took place. An example of the privileges\r\nelsewhere
is
below:\r\n\r\nhttps://github.com/elastic/kibana/blob/75e9d46b4b3a6ff5be4ffc324ba282cea0faea0c/x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/use_alerts_actions.tsx#L33\r\n\r\n\r\nFix:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17211684/7b354906-9b96-4ba8-b30f-4080cf7e7c2f\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<[email protected]>","sha":"36515713a69f6021db8b959b95f7a8ff851b0aa7","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Threat
Hunting:Investigations","v8.12.0","v8.11.1","v8.10.5"],"number":170584,"url":"https://github.com/elastic/kibana/pull/170584","mergeCommit":{"message":"[Security
Solution][Investigations] - Add check for changing alert status from
bulk options (#170584)\n\n## Summary\r\nAddresses
https://github.com/elastic/kibana/issues/169684\r\n\r\nThis PR is a
re-do of: https://github.com/elastic/kibana/pull/169723\r\n(With cypress
tests currently skipped until proper role is available).\r\nThe alert
privileges needs to be added for the alert table as it wasn't\r\nadded
when the migration took place. An example of the privileges\r\nelsewhere
is
below:\r\n\r\nhttps://github.com/elastic/kibana/blob/75e9d46b4b3a6ff5be4ffc324ba282cea0faea0c/x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/use_alerts_actions.tsx#L33\r\n\r\n\r\nFix:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17211684/7b354906-9b96-4ba8-b30f-4080cf7e7c2f\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<[email protected]>","sha":"36515713a69f6021db8b959b95f7a8ff851b0aa7"}},"sourceBranch":"main","suggestedTargetBranches":["8.11","8.10"],"targetPullRequestStates":[{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/170584","number":170584,"mergeCommit":{"message":"[Security
Solution][Investigations] - Add check for changing alert status from
bulk options (#170584)\n\n## Summary\r\nAddresses
https://github.com/elastic/kibana/issues/169684\r\n\r\nThis PR is a
re-do of: https://github.com/elastic/kibana/pull/169723\r\n(With cypress
tests currently skipped until proper role is available).\r\nThe alert
privileges needs to be added for the alert table as it wasn't\r\nadded
when the migration took place. An example of the privileges\r\nelsewhere
is
below:\r\n\r\nhttps://github.com/elastic/kibana/blob/75e9d46b4b3a6ff5be4ffc324ba282cea0faea0c/x-pack/plugins/security_solution/public/detections/components/alerts_table/timeline_actions/use_alerts_actions.tsx#L33\r\n\r\n\r\nFix:\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/17211684/7b354906-9b96-4ba8-b30f-4080cf7e7c2f\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<[email protected]>","sha":"36515713a69f6021db8b959b95f7a8ff851b0aa7"}},{"branch":"8.11","label":"v8.11.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.10","label":"v8.10.5","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Kibana Machine <[email protected]>
@michaelolo24 michaelolo24 mentioned this pull request Nov 14, 2023
Closed
@semd semd mentioned this pull request Nov 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilippeOberti PhilippeOberti PhilippeOberti approved these changes

@e40pud e40pud e40pud approved these changes

@janmonschke janmonschke janmonschke approved these changes

@rylnd rylnd Awaiting requested review from rylnd rylnd was automatically assigned from elastic/security-detection-engine

Assignees
No one assigned
Labels
release_note:fix Team:Threat Hunting:Investigations Security Solution Investigations Team v8.10.5 v8.11.1 v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@michaelolo24 @kibana-ci @kibanamachine @janmonschke @e40pud @PhilippeOberti