[Security Solution] Asset criticality UI #173512

tiansivive · 2023-12-18T11:50:40Z

Summary

This PR adds the UI to assign asset criticality in the old host flyout. #8086

demoUI.mov

NOTE:
This introduces the changes from the closed PR #173011

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

adding footer btns to the modal improving tests

using react-query

machadoum · 2023-12-18T13:08:34Z

...t/security_solution_cypress/cypress/e2e/entity_analytics/asset_criticality/host_flyout.cy.ts

+    tags: ['@ess', '@serverless'],
+    env: {
+      ftrConfig: {
+        enableExperimental: ['entityAnalyticsAssetCriticalityEnabled'],


Quick questions: I noticed that you enabled entityAnalyticsAssetCriticalityEnabled on Serverless and ESS cypress configs files.

Do you also need to enable it here?

Ah!
I'm not sure exactly.
I talked with Mark about this. I think this flag here is required for running in CI. But locally, it doesnt get picked up, so I need the other one. I'm not sure if the other one is required for CI or if it's just local

As discussed in this thread, I think @elastic/security-engineering-productivity would prefer that you remove the configuration from the broader config files before merging this.

machadoum · 2023-12-18T14:30:40Z

x-pack/plugins/security_solution/public/timelines/components/side_panel/host_details/index.tsx

@@ -80,7 +82,9 @@ export const HostDetailsPanel: React.FC<HostDetailsProps> = React.memo(
        <StyledEuiFlyoutBody>
          <EuiSpacer size="m" />
          <ExpandableHostDetailsPageLink hostName={hostName} />
-          <EuiSpacer size="m" />
+          <EuiHorizontalRule />


Both EuiHorizontalRule will render when asset criticality flag is disabled, or the user doesn't have the privileges.

machadoum · 2023-12-18T14:30:56Z

...ecurity_solution/public/timelines/components/side_panel/user_details/user_details_flyout.tsx

@@ -43,7 +44,9 @@ export const UserDetailsFlyout = ({
    <StyledEuiFlyoutBody>
      <EuiSpacer size="m" />
      <ExpandableUserDetailsPageLink userName={userName} />
-      <EuiSpacer size="m" />
+      <EuiHorizontalRule />


Here too: https://github.com/elastic/kibana/pull/173512/files#r1430216024

machadoum

I desk-tested the feature and it looks good to me.
Thank you for the amazing job 🔥 🚀

tiansivive · 2023-12-19T14:21:28Z

@elasticmachine merge upstream

PhilippeOberti

LGTM for the Threat Hunting Investigations team

MadameSheema · 2023-12-21T14:39:09Z

...t/security_solution_cypress/cypress/e2e/entity_analytics/asset_criticality/host_flyout.cy.ts

+
+    describe('Host flyout', () => {
+      beforeEach(() => {
+        expandFirstAlertHostFlyout();


NIT: Why not moving this to the above beforeEach?

ah, good one! Originally it was because there were other flyouts to test, but that got scrapped. I should move it yes, thanks!

MadameSheema · 2023-12-21T14:40:55Z

...t/security_solution_cypress/cypress/e2e/entity_analytics/asset_criticality/host_flyout.cy.ts

+        cy.log('asset criticality update');
+
+        toggleAssetCriticalityModal();
+        cy.get(HOST_DETAILS_FLYOUT_ASSET_CRITICALITY_MODAL_SELECT).should('be.visible').click();


NIT: Wrap it into a meaningful task method. You don't need to check for the element to be visible, cypress makes sure the element is actionable before performing the click.

MadameSheema · 2023-12-21T14:41:05Z

...t/security_solution_cypress/cypress/e2e/entity_analytics/asset_criticality/host_flyout.cy.ts

+          .contains('Important')
+          .click();
+
+        cy.get(HOST_DETAILS_FLYOUT_ASSET_CRITICALITY_MODAL_SAVE_BTN).should('be.visible').click();


NIT: Wrap it into a meaningful task method. You don't need to check for the element to be visible, cypress makes sure the element is actionable before performing the click.

MadameSheema · 2023-12-21T14:41:21Z

...t/security_solution_cypress/cypress/e2e/entity_analytics/asset_criticality/host_flyout.cy.ts

+        toggleAssetCriticalityModal();
+        cy.get(HOST_DETAILS_FLYOUT_ASSET_CRITICALITY_MODAL_SELECT).should('be.visible').click();
+
+        cy.get(HOST_DETAILS_FLYOUT_ASSET_CRITICALITY_MODAL_SELECT_OPTION)


NIT: Wrap it into a meaningful task method.

MadameSheema · 2023-12-21T14:50:57Z

x-pack/test/security_solution_cypress/cypress/tasks/asset_criticality/common.ts

+ */
+export const toggleAssetCriticalityAccordion = () => {
+  cy.get(HOST_DETAILS_FLYOUT_ASSET_CRITICALITY_SELECTOR).scrollIntoView();
+  cy.get(HOST_DETAILS_FLYOUT_ASSET_CRITICALITY_SELECTOR).should('be.visible').click();


NIT: Cypress automatically performs different checks and actions before doing a click into an element when we are not forcing it as it is the case:

Scroll the element into view.

Ensure the element is not hidden.

Ensure the element is not disabled.

Ensure the element is not detached

Ensure the element is not readonly.

Ensure the element is not animating.

Ensure the element is not covered

Scroll the page if still covered by an element with fixed position.

So it should not be needed to do the the scrollIntoView or the implicit check on visibility.

MadameSheema · 2023-12-21T14:51:15Z

x-pack/test/security_solution_cypress/cypress/tasks/asset_criticality/common.ts

+export const toggleAssetCriticalityModal = () => {
+  toggleAssetCriticalityAccordion();
+
+  cy.get(HOST_DETAILS_FLYOUT_ASSET_CRITICALITY_BUTTON).should('be.visible').click();


NIT: No need for checking that the element is visible before clicking on it.

MadameSheema

I left some nit comments that would be great to follow up when you have the chance :)

tiansivive · 2023-12-21T16:04:15Z

@MadameSheema Done!
Thanks for the help

tiansivive · 2023-12-21T16:13:08Z

@elasticmachine merge upstream

kibanamachine · 2023-12-21T16:13:10Z

merge conflict between base and head

kibana-ci · 2023-12-27T12:51:26Z

💚 Build Succeeded

Buildkite Build
Commit: 4e53bae

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	4838	4842	+4

Async chunks

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

id	before	after	diff
`securitySolution`	11.3MB	11.3MB	+12.0KB

Page load bundle

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

id	before	after	diff
`securitySolution`	64.2KB	64.2KB	+69.0B

History

💚 Build #184826 succeeded 55067cd
💚 Build #184723 succeeded 5740116
💔 Build #184274 failed 002d482
💔 Build #183968 failed ec2c1da

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

cc @machadoum

tiansivive added 7 commits December 18, 2023 10:30

Introducing a selector

aee9394

functional tests

94116ad

adding footer btns to the modal improving tests

adding api call

580b3a5

using react-query

checking privileges

4f44826

removing story

b660fef

replacing with super selector and adding color badges

86fda7c

moving stuff around

8d8c28b

tiansivive assigned machadoum Dec 18, 2023

tiansivive added Team:Entity Analytics Security Entity Analytics Team release_note:feature Makes this part of the condensed release notes v8.13.0 labels Dec 18, 2023

review

dc41441

tiansivive force-pushed the asset-criticality-ui branch from 8374236 to 6fa437d Compare December 18, 2023 11:53

Polishing UI

1532e07

tiansivive force-pushed the asset-criticality-ui branch from 6fa437d to 1532e07 Compare December 18, 2023 12:19

machadoum reviewed Dec 18, 2023

View reviewed changes

tiansivive marked this pull request as ready for review December 18, 2023 14:25

tiansivive requested review from a team as code owners December 18, 2023 14:25

Merge branch 'main' into asset-criticality-ui

a27e3b3

machadoum reviewed Dec 18, 2023

View reviewed changes

tiansivive added 2 commits December 18, 2023 15:43

review

2374e50

test fix

ec2c1da

machadoum approved these changes Dec 18, 2023

View reviewed changes

Merge branch 'main' into asset-criticality-ui

002d482

PhilippeOberti approved these changes Dec 19, 2023

View reviewed changes

Merge branch 'main' into asset-criticality-ui

c503034

allowing feature flag in cypress tests

5740116

MadameSheema reviewed Dec 21, 2023

View reviewed changes

MadameSheema approved these changes Dec 21, 2023

View reviewed changes

cavokz approved these changes Dec 21, 2023

View reviewed changes

allowing feature flag in cypress tests

55067cd

Merge branch 'main' into asset-criticality-ui

4e53bae

tiansivive merged commit 121fb3a into elastic:main Dec 27, 2023
37 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Dec 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Asset criticality UI #173512

[Security Solution] Asset criticality UI #173512

tiansivive commented Dec 18, 2023 •

edited

Loading

machadoum Dec 18, 2023

tiansivive Dec 18, 2023

rylnd Dec 20, 2023

machadoum Dec 18, 2023

tiansivive Dec 18, 2023

machadoum Dec 18, 2023

tiansivive Dec 18, 2023

machadoum left a comment

tiansivive commented Dec 19, 2023

PhilippeOberti left a comment

MadameSheema Dec 21, 2023

tiansivive Dec 21, 2023

MadameSheema Dec 21, 2023

MadameSheema Dec 21, 2023

MadameSheema Dec 21, 2023

MadameSheema Dec 21, 2023

MadameSheema Dec 21, 2023

MadameSheema left a comment

tiansivive commented Dec 21, 2023

tiansivive commented Dec 21, 2023

kibanamachine commented Dec 21, 2023

kibana-ci commented Dec 27, 2023

[Security Solution] Asset criticality UI #173512

[Security Solution] Asset criticality UI #173512

Conversation

tiansivive commented Dec 18, 2023 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machadoum left a comment

Choose a reason for hiding this comment

tiansivive commented Dec 19, 2023

PhilippeOberti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadameSheema left a comment

Choose a reason for hiding this comment

tiansivive commented Dec 21, 2023

tiansivive commented Dec 21, 2023

kibanamachine commented Dec 21, 2023

kibana-ci commented Dec 27, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

tiansivive commented Dec 18, 2023 •

edited

Loading