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] Top-level Cases feature under the Security #112980

Merged
merged 13 commits into from
Oct 1, 2021

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Sep 23, 2021

Summary

Create new top-level Cases feature under the Security category just for 8.x, we will have a follow up PR for 7.x to deprecate the sub feature of cases into the top level feature

#109158

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting-cases (Team:Threat Hunting:Cases)

@XavierM
Copy link
Contributor Author

XavierM commented Sep 23, 2021

@elasticmachine merge upstream

@XavierM XavierM requested a review from a team as a code owner September 24, 2021 14:37
@XavierM
Copy link
Contributor Author

XavierM commented Sep 27, 2021

@elasticmachine merge upstream

@@ -349,7 +350,7 @@ export function getDeepLinks(
(deepLink) =>
(deepLink.id !== SecurityPageName.case && deepLink.id !== SecurityPageName.ueba) || // is not cases or ueba
(deepLink.id === SecurityPageName.case &&
(capabilities == null || capabilities.siem.read_cases === true)) || // is cases with at least read only caps
(capabilities == null || capabilities[CASES_FEATURE_ID].read_cases === true)) || // is cases with at least read only caps
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a conflicting PR #113046 but no worries I will sync mine when this one is merged

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Code changes look good. I tested this locally too 👍

One thing I did notice that is slightly different behavior than before is that a user can have cases read or all and not have security and they will not be able to access cases because the entire security plugin will be disabled.

image

This is slightly different from how it worked as a sub feature because it was not possible to grant cases privileges when the security privilege was set to none

The toggle is grayed out:

image

@XavierM
Copy link
Contributor Author

XavierM commented Sep 27, 2021

@jonathan-buttner that's a valid point, let me see what I can do here and let me talk to @semd and see if we can register cases routes outside.

@semd
Copy link
Contributor

semd commented Sep 28, 2021

@XavierM
About that:

@jonathan-buttner that's a valid point, let me see what I can do here and let me talk to @semd and see if we can register cases routes outside.

Currently, the Cases routes are defined outside the plugin, the SecuritySolutions and O11y plugins define the Cases routes and deep links, and then they import and render the Cases pages accordingly. This is something we'd like to change and define all the routes within the Cases plugin, so there is only one entry point, but right now it is the parent plugin responsibility.
In addition, we would need to implement the main app components and logic in Cases plugin to render it as a "standalone" plugin, which does not exist right now.

So, maybe another option would be to register and mount SecuritySolutions plugin anyway (even I am not sure how to do it), and have only the Cases pages visible, hiding the rest of the sections.

Or otherwise, keep the same precedence and not show Cases if Security is disabled, while we prepare the Cases plugin to be rendered alone. 🤷‍♂️

What do you think? @cnasikas

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

LGTM, a couple minor nits below.
Discussed offline that a follow-on PR will be submitted to deal with a user who has access to Cases but not the Security Solution.

@XavierM XavierM force-pushed the upgrade-cases-security-privileges branch from 620e616 to 19897a7 Compare September 30, 2021 21:24
@XavierM XavierM enabled auto-merge (squash) September 30, 2021 21:26
@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Kibana Pipeline / general / Should have the same query and open the timeline modal.Create a timeline from a template Should have the same query and open the timeline modal

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

CypressError: Timed out retrying after 60050ms: `cy.click()` failed because this element:

`<button class="euiButtonIcon euiButtonIcon--text euiButtonIcon--empty euiButtonIcon--xSmall" type="button" aria-label="All actions" data-test-subj="euiCollapsedItemActionsButton">...</button>`

is being covered by another element:

`<div class="euiFlexGroup euiFlexGroup--alignItemsCenter euiFlexGroup--directionRow euiFlexGroup--responsive" aria-label="You are in group 1">...</div>`

Fix this problem, or use {force: true} to disable error checking.

https://on.cypress.io/element-cannot-be-interacted-with
    at $Cy.ensureDescendents (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:160669:87)
    at ensureDescendents (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:147543:8)
    at ensureDescendentsAndScroll (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:147550:14)
    at ensureElIsNotCovered (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:147684:5)
    at runAllChecks (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:147871:52)
    at retryActionability (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:147894:16)
    at tryCatcher (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:13212:23)
    at Function.Promise.attempt.Promise.try (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:10486:29)
    at tryFn (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:165329:61)
    at whenStable (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:165368:14)
    at http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:164855:18
    at tryCatcher (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:13212:23)
    at Promise._settlePromiseFromHandler (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:11147:31)
    at Promise._settlePromise (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:11204:18)
    at Promise._settlePromise0 (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:11249:10)
    at Promise._settlePromises (http://elastic:changeme@localhost:61161/__cypress/runner/cypress_runner.js:11329:18)
From Your Spec Code:
    at Object.expandEventAction (http://localhost:61161/__cypress/tests?p=cypress/integration/timelines/creation.spec.ts:17235:61)
    at Context.eval (http://localhost:61161/__cypress/tests?p=cypress/integration/timelines/creation.spec.ts:15613:24)

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 4.3MB 4.3MB -83.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 109.8KB 109.9KB +64.0B

History

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

@XavierM XavierM merged commit 3b958e7 into elastic:master Oct 1, 2021
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 5, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 112980 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 112980 or prevent reminders by adding the backport:skip label.

@jportner jportner added the backport:skip This commit does not require backporting label Oct 6, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:enhancement Team:Threat Hunting Security Solution Threat Hunting Team v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants