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

Closed
wants to merge 8 commits into from
Closed

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

wants to merge 8 commits into from

Conversation

michaelolo24
Copy link
Contributor

@michaelolo24 michaelolo24 commented Oct 24, 2023 •
edited
Loading

Summary

Addresses #169684

The alert privileges need 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 added bug Fixes for quality problems that affect the customer experience fixed Team:Threat Hunting:Investigations Security Solution Investigations Team v8.11.0 v8.9.3 v8.10.5 labels Oct 24, 2023
@michaelolo24 michaelolo24 mentioned this pull request Oct 24, 2023
Closed
@michaelolo24 michaelolo24 force-pushed the fix-alert-status-vieweronly branch from f5fde4e to 2010f05 Compare October 25, 2023 15:39
michaelolo24
michaelolo24 commented Oct 25, 2023
View reviewed changes
...security_solution_cypress/cypress/e2e/detection_response/detection_alerts/alert_status.cy.ts
});
});

context('Marking alerts as acknowledged', () => {
context('User is readonly', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the newly added tests, the other changes are just moving code to an encapsulated space

@michaelolo24 michaelolo24 force-pushed the fix-alert-status-vieweronly branch 2 times, most recently from bef0656 to d24ef5e Compare October 25, 2023 17:53
@jonathan-buttner
Copy link
Contributor

Excellent PR Mike!

@michaelolo24 michaelolo24 force-pushed the fix-alert-status-vieweronly branch 2 times, most recently from 0c04ab9 to 749b3cd Compare October 25, 2023 19:56
@michaelolo24 michaelolo24 changed the title [Security Solution][Investigations] - Add privileges check for changing alert status from alert table [Security Solution][Investigations] - Add check for changing alert status from bulk options Oct 25, 2023
@michaelolo24 michaelolo24 marked this pull request as ready for review October 25, 2023 19:59
@michaelolo24 michaelolo24 requested review from a team as code owners October 25, 2023 19:59
@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@michaelolo24 michaelolo24 force-pushed the fix-alert-status-vieweronly branch from 950578f to 077760d Compare October 27, 2023 12:42
logeekal
logeekal approved these changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

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

Thanks @michaelolo24 for these changes. PR looks good. I just add one question.

...security_solution_cypress/cypress/e2e/detection_response/detection_alerts/alert_status.cy.ts
});

it('should not allow users to change a single alert status', () => {
cy.get(TIMELINE_CONTEXT_MENU_BTN).should('not.exist');
Copy link
Contributor

Choose a reason for hiding this comment

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

@michaelolo24

This will remove complete context menu btn? Are other actions such as Run Osquery , Add to Case, etc are also not allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to update the role used. The Roles.reader affects those as well. Wanted to avoid creating another user, but maybe it's worth it... 🤷🏾‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Once this is merged - #169017 - you can test with the appropriate roles!

@maximpn

Copy link
Contributor Author

@michaelolo24 michaelolo24 Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@yctercero looking at it now. Reader may still be the most accurate. I don't know if there's really a defined role that has cases and osquery write access, but not alert status access?

@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

@michaelolo24 michaelolo24 force-pushed the fix-alert-status-vieweronly branch 2 times, most recently from 201e57f to a67e86a Compare November 2, 2023 13:09
yctercero
yctercero approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

Pulled down and confirmed change. Thanks! LGTM

@michaelolo24 michaelolo24 force-pushed the fix-alert-status-vieweronly branch from f30cff8 to 0a4a80b Compare November 3, 2023 14:33
@michaelolo24 michaelolo24 requested a review from a team as a code owner November 3, 2023 14:33
@michaelolo24 michaelolo24 requested review from a team as code owners November 3, 2023 14:33
@michaelolo24 michaelolo24 force-pushed the fix-alert-status-vieweronly branch from 0a4a80b to 338d041 Compare November 3, 2023 21:45
@kibana-ci

This comment was marked as outdated.

Sign in to view
@michaelolo24
Copy link
Contributor Author

Closing this PR for now as there seems to be issues with utilizing the viewer role. Will re-open a more trimmed down PR. Sorry for the noise!

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

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

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]>
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 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 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yctercero yctercero yctercero approved these changes

@logeekal logeekal logeekal approved these changes

@jbudz jbudz jbudz approved these changes

@tomsonpl tomsonpl Awaiting requested review from tomsonpl tomsonpl was automatically assigned from elastic/security-defend-workflows

@paul-tavares paul-tavares Awaiting requested review from paul-tavares paul-tavares was automatically assigned from elastic/security-defend-workflows

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience release_note:fix Team:Threat Hunting:Investigations Security Solution Investigations Team v8.9.3 v8.10.5 v8.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@michaelolo24 @jonathan-buttner @kibana-ci @jbudz @logeekal @yctercero