Asset Criticality UI #173011

tiansivive · 2023-12-10T15:09:38Z

Summary

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

asset-criticality.demo.mov

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

tiansivive · 2023-12-11T10:36:20Z

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

+          <EuiSpacer size="m" />
+          <AssetCriticalitySelector entity={{ type: 'host', name: hostName }} />


Still not sure if this is the correct placement? @SourinPaul

After discussing with Pablo, we were wondering if it also needs to show up in the user flyout in the timeline?

Still not sure if this is the correct placement? @SourinPaul

Can we move the assignment component before displaying host metadata? The accordion can stay collapsed.

After discussing with Pablo, we were wondering if it also needs to show up in the user flyout in the timeline?

In short, yes. The users should be able to assign from every instance of the user flyout, including the timeline. Afaik, this user flyout component is reused everywhere in our app for consistency. I may be incorrect, so worth double checking that.

cc: @tiansivive @machadoum

Afaik, this user flyout component is reused everywhere in our app for consistency. I may be incorrect, so worth double checking that.

Not a direct answer, but regarding consistency, today the timeline view does not support the new expanded flyouts. This means that, today, even when we have the new users flyout enabled, the timeline view still shows the old flyout.

I'm working to find an answer for when the timeline view will support expanded flyouts, so that this discrepancy will be fixed.

@jaredburgettelastic The key here is that the timeline does not use flyouts, hence the discrepancy. Perhaps it should?

The key here is that the timeline does not use flyouts

Apologies for the lack of clarity, to make this more clear:

This means that, today, even when we have the new users flyout enabled, the timeline view still shows ~~the old flyout~~ a panel containing many of the same components used by the old host/user flyout, but embedded directly into the timeline view.

tiansivive · 2023-12-11T10:42:22Z

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

+  );
+};
+
+const useAssetCriticality = (entity: Entity): State => {


Not sure if this is the correct place for this hook?
This is under the components folder, maybe we want the hook logic somewhere else?

Maybe security_solution/public/entity_analytics/hooks/use_asset_criticality.ts?

use_asset_criticality looks like a single usage hook. You could create a folder for the component for asset_criticality inside components and encapsulate everything related to the component inside the folder.

done.
As discussed I separated the hook into one dealing with data fetching and another for the modal and placed them in a file under the new asset criticality directory

tiansivive · 2023-12-11T10:42:50Z

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

+  const QC = useQueryClient();
+
+  const privileges = useQuery({
+    queryKey: ['ASSET_CRITICALITY', 'PRIVILEGES', entity.name],


Do we have common place for react-query keys?

Moved this to a new file in the new asset criticality folder

tiansivive · 2023-12-11T10:44:02Z

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

+  type: 'host' | 'user';
+}
+
+const criticalityDisplayText: Record<AssetCriticalityRecord['criticality_level'], string> = {


Should this also be moved to a different file? It seems like this mapping might apply anywhere we deal with asset criticality levels?

This looks like a translation map, shouldn't we use i18n for the texts?

tiansivive · 2023-12-11T10:48:23Z

x-pack/plugins/security_solution/common/asset_criticality/constants.ts

 export const ASSET_CRITICALITY_INDEX_PATTERN = '.asset-criticality.asset-criticality-*';

 type AssetCriticalityIndexPrivilege = 'read' | 'write';
 export const ASSET_CRITICALITY_REQUIRED_ES_INDEX_PRIVILEGES = {
  [ASSET_CRITICALITY_INDEX_PATTERN]: ['read', 'write'] as AssetCriticalityIndexPrivilege[],
 };
+
+export const PICK_ASSET_CRITICALITY = i18n.translate(


I wonder if this is also the correct place to place this common stuff?

Not the best place, as a general rule of thumb:

security_solution/common/* -> shared between public and server directories (URLs, indexPatterns, privileges..)

security_solution/public/common/* -> shared between public "sub-plugin" directories (reusable components, reusable hooks, paths, constants...)

These translations, however, do not seem to be common stuff, they are only used in the security_solution/public/entity_analytics/components/asset_criticality_selector.tsx component.

So, it would be better to put them in a new translations file inside the same directory, like: security_solution/public/entity_analytics/components/translations.ts.

However, how things are organized inside the security_solution/public/entity_analytics directory is up to your team.

security_solution/public/entity_analytics/components/translations.ts is a good option. The docs recommend inlining the copy into the react component using <FormattedMessage>.

You can read more here: https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/GUIDELINE.md

Yeah, I think @hop-dev just had a PR that moved stuff around quite a bit within EA?

I moved stuff around, most of this stuff now lives along side the component in a new asset criticality folder within EA

x-pack/plugins/security_solution/public/entity_analytics/api/api.ts

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

x-pack/plugins/security_solution/common/asset_criticality/constants.ts

machadoum · 2023-12-11T12:22:36Z

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

+          fill
+          data-test-subj="asset-criticality-modal-save-btn"
+        >
+          {'Save'}


Please use i18n to allow translation.

machadoum · 2023-12-11T12:29:29Z

x-pack/plugins/security_solution/common/asset_criticality/constants.ts

 export const ASSET_CRITICALITY_INDEX_PATTERN = '.asset-criticality.asset-criticality-*';

 type AssetCriticalityIndexPrivilege = 'read' | 'write';
 export const ASSET_CRITICALITY_REQUIRED_ES_INDEX_PRIVILEGES = {
  [ASSET_CRITICALITY_INDEX_PATTERN]: ['read', 'write'] as AssetCriticalityIndexPrivilege[],
 };
+
+export const PICK_ASSET_CRITICALITY = i18n.translate(


security_solution/public/entity_analytics/components/translations.ts is a good option. The docs recommend inlining the copy into the react component using <FormattedMessage>.

You can read more here: https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/GUIDELINE.md

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

machadoum · 2023-12-11T12:37:30Z

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

+    <>
+      <EuiAccordion
+        id="asset-criticality-selector"
+        buttonContent="Asset Criticality"


machadoum · 2023-12-11T12:45:20Z

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

+
+const useAssetCriticality = (entity: Entity): State => {
+  const [visible, toggleModal] = useToggle(false);
+  const modal = { visible, toggle: (next: boolean) => () => toggleModal(next) };


This object will update on every hook execution.
I would rather move the toggle state to the component itself so we don't have to create this object.

Moved to a different hook

machadoum · 2023-12-11T12:49:28Z

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx

+  );
+};
+
+const useAssetCriticality = (entity: Entity): State => {


use_asset_criticality looks like a single usage hook. You could create a folder for the component for asset_criticality inside components and encapsulate everything related to the component inside the folder.

machadoum · 2023-12-12T13:24:25Z

...ugins/security_solution/public/entity_analytics/components/asset_criticality/translations.ts

+import type { CriticalityLevel } from './common';
+
+export const PICK_ASSET_CRITICALITY = i18n.translate(
+  'xpack.securitySolution.timeline.sidePanel.hostDetails.assetCriticality.pick',


nit: The id xpack.securitySolution.timeline.sidePanel.hostDetails.assetCriticality.pick looks outdated

machadoum · 2023-12-12T13:44:44Z

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

+/**
+ * Find the first alert row in the alerts table then click on the host name to open the flyout
+ */
+export const expandFirstAlertHostExpandableFlyout = () => {


I noticed that you created files and functions inside the cypress folder containing HostExpandableFlyout, but there isn't a host expandable flyout yet. We are going to create it soon, but for now, we only have a HostFlyout.

ex: expandable_flyout/host_details_right_panel.ts

Yeah, that whole thing is weird.
I was pairing with @jaredburgettelastic and we figured that was the best place to add the test. Do you have a better option?

A recently merged PR created this folder x-pack/test/security_solution_cypress/cypress/e2e/entity_analytics. I believe that we should put all entity_analytics tests there.

Please don't name it "expandable" if it is testing the old flyout.

machadoum · 2023-12-12T17:12:02Z

I noticed some small UI issues:

If we keep the asset criticality as the last item, the content might be hidden after the item is expanded

Dec-12-2023.18-08-36.mov

Duplicated CancelCancel text

adding footer btns to the modal improving tests

using react-query

kibana-ci · 2023-12-18T11:31:22Z

💔 Build Failed

Buildkite Build
Commit: fca815b384e36c3454f76fc3dcfc6c8bcbe43c67
Interpreting CI Failures

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #11 / Details Panel Component DetailsPanel:HostDetails: rendering it should render the Host Details view in the Details Panel when the panelView is hostDetail and the hostName is set
[job] [logs] Jest Tests #11 / Expandable Host Component ExpandableHostDetails: rendering it should set date range to anomaly date range
[job] [logs] Jest Tests #11 / Expandable Host Component ExpandableUserDetails: rendering it should set date range to anomaly date range
[job] [logs] Defend Workflows Cypress Tests #11 / Response console From Alerts "after all" hook for "should open responder from alert details flyout" "after all" hook for "should open responder from alert details flyout"
[job] [logs] Defend Workflows Cypress Tests #11 / Response console From Alerts "before all" hook for "should open responder from alert details flyout" "before all" hook for "should open responder from alert details flyout"
[job] [logs] Defend Workflows Cypress Tests #9 / Unenroll agent from fleet changing when agent tamper protection is enabled but then is switched to a policy with it disabled "after all" hook for "should unenroll from fleet without issues" "after all" hook for "should unenroll from fleet without issues"
[job] [logs] Defend Workflows Cypress Tests #9 / Unenroll agent from fleet changing when agent tamper protection is enabled but then is switched to a policy with it disabled "before each" hook for "should unenroll from fleet without issues" "before each" hook for "should unenroll from fleet without issues"
[job] [logs] Defend Workflows Cypress Tests #8 / Unenroll agent from fleet when agent tamper protection is disabled but then is switched to a policy with it enabled "after all" hook for "should unenroll from fleet without issues" "after all" hook for "should unenroll from fleet without issues"
[job] [logs] Defend Workflows Cypress Tests #8 / Unenroll agent from fleet when agent tamper protection is disabled but then is switched to a policy with it enabled "before each" hook for "should unenroll from fleet without issues" "before each" hook for "should unenroll from fleet without issues"
[job] [logs] Defend Workflows Cypress Tests #10 / Uninstall agent from host changing agent policy when agent tamper protection is disabled but then is switched to a policy with it enabled "after all" hook for "should uninstall from host without issues" "after all" hook for "should uninstall from host without issues"
[job] [logs] Defend Workflows Cypress Tests #10 / Uninstall agent from host changing agent policy when agent tamper protection is disabled but then is switched to a policy with it enabled "before each" hook for "should uninstall from host without issues" "before each" hook for "should uninstall from host without issues"

History

💔 Build #183868 failed c0db5ee
💔 Build #183866 failed f95165c
💔 Build #183821 failed daf8e91968b44569057be41ea8cba9e4616adb16
💔 Build #182978 failed bdb0e7f8d481b3637410807c0597742c03c20a84

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

cc @tiansivive

tiansivive added the Team:Entity Analytics Security Entity Analytics Team label Dec 10, 2023

tiansivive self-assigned this Dec 10, 2023

tiansivive commented Dec 11, 2023

View reviewed changes

tiansivive force-pushed the siem-ea-8086 branch 2 times, most recently from c448f27 to 65e5d20 Compare December 11, 2023 10:53

tiansivive marked this pull request as ready for review December 11, 2023 11:20

tiansivive requested review from a team as code owners December 11, 2023 11:20

tiansivive added release_note:feature Makes this part of the condensed release notes v8.13.0 labels Dec 11, 2023

semd reviewed Dec 11, 2023

View reviewed changes

x-pack/plugins/security_solution/public/entity_analytics/api/api.ts Outdated Show resolved Hide resolved

semd reviewed Dec 11, 2023

View reviewed changes

x-pack/plugins/security_solution/public/entity_analytics/api/api.ts Outdated Show resolved Hide resolved

semd reviewed Dec 11, 2023

View reviewed changes

.../plugins/security_solution/public/entity_analytics/components/asset_criticality_selector.tsx Outdated Show resolved Hide resolved

semd reviewed Dec 11, 2023

View reviewed changes

x-pack/plugins/security_solution/common/asset_criticality/constants.ts Outdated Show resolved Hide resolved

machadoum self-requested a review December 11, 2023 12:21

machadoum reviewed Dec 11, 2023

View reviewed changes

tiansivive force-pushed the siem-ea-8086 branch from 65e5d20 to dbb1a5a Compare December 11, 2023 15:05

tiansivive requested review from semd and machadoum December 12, 2023 11:40

machadoum reviewed Dec 12, 2023

View reviewed changes

tiansivive mentioned this pull request Dec 13, 2023

Replace KibanaServices singleton with useKibana hook #173071

Closed

tiansivive requested review from a team as code owners December 17, 2023 22:33

Introducing a selector

aee9394

tiansivive added 9 commits December 18, 2023 10:30

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

review

528ee30

missed translation

1f757db

improving tests

f95165c

tiansivive force-pushed the siem-ea-8086 branch from daf8e91 to f95165c Compare December 18, 2023 09:37

adapting to new api routes

c0db5ee

tiansivive requested a review from machadoum December 18, 2023 11:10

tiansivive marked this pull request as draft December 18, 2023 11:15

tiansivive force-pushed the siem-ea-8086 branch from fca815b to b02c9c4 Compare December 18, 2023 11:28

Polishing UI

a27f094

tiansivive force-pushed the siem-ea-8086 branch from b02c9c4 to a27f094 Compare December 18, 2023 11:33

tiansivive closed this Dec 18, 2023

tiansivive mentioned this pull request Dec 18, 2023

[Security Solution] Asset criticality UI #173512

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset Criticality UI #173011

Asset Criticality UI #173011

tiansivive commented Dec 10, 2023 •

edited

Loading

tiansivive Dec 11, 2023

tiansivive Dec 11, 2023

SourinPaul Dec 12, 2023

jaredburgettelastic Dec 16, 2023 •

edited

Loading

tiansivive Dec 17, 2023

jaredburgettelastic Dec 18, 2023 •

edited

Loading

tiansivive Dec 11, 2023

semd Dec 11, 2023

machadoum Dec 11, 2023

tiansivive Dec 11, 2023 •

edited

Loading

tiansivive Dec 11, 2023

tiansivive Dec 11, 2023 •

edited

Loading

tiansivive Dec 11, 2023

semd Dec 11, 2023

tiansivive Dec 11, 2023

tiansivive Dec 11, 2023

semd Dec 11, 2023

machadoum Dec 11, 2023

tiansivive Dec 11, 2023

tiansivive Dec 11, 2023

machadoum Dec 11, 2023

tiansivive Dec 11, 2023

machadoum Dec 11, 2023

machadoum Dec 11, 2023

tiansivive Dec 11, 2023

machadoum Dec 11, 2023

tiansivive Dec 11, 2023

machadoum Dec 11, 2023

machadoum Dec 12, 2023

machadoum Dec 12, 2023

tiansivive Dec 12, 2023

machadoum Dec 12, 2023 •

edited

Loading

machadoum commented Dec 12, 2023 •

edited

Loading

kibana-ci commented Dec 18, 2023 •

edited

Loading

		<EuiSpacer size="m" />
		<AssetCriticalitySelector entity={{ type: 'host', name: hostName }} />

Asset Criticality UI #173011

Asset Criticality UI #173011

Conversation

tiansivive commented Dec 10, 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

jaredburgettelastic Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredburgettelastic Dec 18, 2023 • edited Loading

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

tiansivive Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiansivive Dec 11, 2023 • edited Loading

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

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

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 Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

machadoum commented Dec 12, 2023 • edited Loading

kibana-ci commented Dec 18, 2023 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

History

tiansivive commented Dec 10, 2023 •

edited

Loading

jaredburgettelastic Dec 16, 2023 •

edited

Loading

jaredburgettelastic Dec 18, 2023 •

edited

Loading

tiansivive Dec 11, 2023 •

edited

Loading

tiansivive Dec 11, 2023 •

edited

Loading

machadoum Dec 12, 2023 •

edited

Loading

machadoum commented Dec 12, 2023 •

edited

Loading

kibana-ci commented Dec 18, 2023 •

edited

Loading