-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Entity Analytics] Show Asset Criticality in Risk Input Left Flyout #174638
Conversation
/ci |
...urity_solution/public/entity_analytics/components/asset_criticality/use_asset_criticality.ts
Show resolved
Hide resolved
/ci |
Pinging @elastic/security-entity-analytics (Team:Entity Analytics) |
/ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a minor question
...urity_solution/public/entity_analytics/components/entity_details_flyout/tabs/risk_inputs.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM!
Very good proposal 👏 👏 👏
It looks better without the extra tabs. Thank you!
I left some minor comments about the ContextsTable
component. Could you please take a look?
...urity_solution/public/entity_analytics/components/entity_details_flyout/tabs/risk_inputs.tsx
Outdated
Show resolved
Hide resolved
...urity_solution/public/entity_analytics/components/entity_details_flyout/tabs/risk_inputs.tsx
Outdated
Show resolved
Hide resolved
...urity_solution/public/entity_analytics/components/entity_details_flyout/tabs/risk_inputs.tsx
Outdated
Show resolved
Hide resolved
..._solution/public/entity_analytics/components/entity_details_flyout/tabs/risk_inputs.test.tsx
Show resolved
Hide resolved
@@ -67,6 +67,14 @@ describe( | |||
expandRiskInputsFlyoutPanel(); | |||
cy.get(RISK_INPUT_PANEL_HEADER).should('exist'); | |||
}); | |||
|
|||
it('should show asset criticality in the risk input panel', () => { | |||
waitForAlerts(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: You can move the waitForAlerts()
of all the tests to the beforeEach()
hook.
waitForAlerts(); | ||
expandFirstAlertUserFlyout(); | ||
expandRiskInputsFlyoutPanel(); | ||
cy.get(ASSET_CRITICALITY_BADGE).should('exist'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Below assertion is implicitly checking for the existence of the element, so this can be removed.
waitForAlerts(); | ||
expandFirstAlertHostFlyout(); | ||
expandRiskInputsFlyoutPanel(); | ||
cy.get(ASSET_CRITICALITY_BADGE).should('exist'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Below assertion is implicitly checking for the existence of the element, so this can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
security-engineering-productivity changes LGTM!
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @hop-dev |
Hey, great work! I see no obstacles in deploying this to production. However, for upcoming releases, I believe we can elevate it further by aligning these features more cohesively with other flyout designs. To achieve this, we should prioritize the asset criticality at the top of the flyout. Here's an example https://www.figma.com/file/IKQr6HbQPAdFws9i7Ikcx9/Entity-Analytics?type=design&node-id=473%3A25619&mode=design&t=juSE0rxLlaPUKbSk-1, Cheers! |
…lastic#174638) ## Summary Show the asset criticality as part of the risk inputs under a new contexts section: <img width="1517" alt="Screenshot 2024-01-10 at 16 45 29" src="https://github.com/elastic/kibana/assets/3315046/4ab7fd16-2849-4d9c-8f1c-f9cd9b677e8f"> If there is no criticality assigned here is what it looks like: <img width="1098" alt="Screenshot 2024-01-10 at 12 05 28" src="https://github.com/elastic/kibana/assets/3315046/817e4397-1a3f-4e65-be27-dbadb364e693"> this is based off the criticality_level on the risk score document not the current asset criticality of the entity. ## Test steps Assign asset criticality to a host or user and raise alerts + a risk score for them, view the host details flyout and then expand risk inputs, the asset criticality at the time of the score should be shown.
Summary
Show the asset criticality as part of the risk inputs under a new contexts section:

If there is no criticality assigned here is what it looks like:

this is based off the criticality_level on the risk score document not the current asset criticality of the entity.
Test steps
Assign asset criticality to a host or user and raise alerts + a risk score for them, view the host details flyout and then expand risk inputs, the asset criticality at the time of the score should be shown.