[security solution][Endpoint] Refactor host isolation exceptions list page to now use `<ArtifactListPage/>` component #129099

paul-tavares · 2022-03-31T16:21:56Z

Summary

Refactors the Host Isolation Exceptions page to use the new <ArtifactListPage> component. Deletes several files from under the page's directory that no longer apply
Refactor some policy details code to use generic artifact data hooks + Exceptions api client instad of custom hooks/services that were under the Host Isolations Exceptions
New generic <FormatError/> component for displaying formatted Error's
<EffectedPolicySelect> updated with new test utilities and support for disabled prop
<ArtifactListPage> component updated with additional props that allows for removing the Create, Edit or Delete actions
New generic getArtifactListPageUrlPath() url routing utility for use with pages that utilize the <ArtifactListPage/> component

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…host-isolation-exceptions-to-artifact-list-page-componet

paul-tavares · 2022-04-04T13:12:30Z

.../plugins/security_solution/public/management/components/effected_policy_select/test_utils.ts

+ * @param renderResult
+ * @param [atIndex=0]
+ */
+export const clickOnEffectedPolicy = async (


FYI: a reusable utility for clicking on policies based on the index of the policy entry. I had noticed that in many cases we are being forced to grab the ID of the policy in order to generate the data-test-subj (which includes the ID of policy) when really in some cases, we don't really care for about the UUID. Sos this utility just clicks on a entry based on its index in the list.

Further below there is another test utility that reports on whether a policy based on its index is selected or not.

This is super helpful. Thanks 🥇

…host-isolation-exceptions-to-artifact-list-page-componet

…ection store definition

…host-isolation-exceptions-to-artifact-list-page-componet # Conflicts: # x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/view/components/form.test.tsx # x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/view/components/form_flyout.test.tsx # x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/view/host_isolation_exceptions_list.test.tsx

…ct list page routing

elasticmachine · 2022-04-06T19:02:01Z

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

paul-tavares · 2022-04-06T19:04:05Z

x-pack/plugins/security_solution/public/management/common/url_routing/index.ts

+ * 2.0.
+ */
+
+export * from './utils';


FYI: This directory (url_routing) will be refactored and consolidated into a folder named routing that will contain multiple files. I did not want to do that in this PR because then I would have to move over the existing routing.ts module and that would have pulled in several others due to path changes. I will create a follow up PR to this one.

paul-tavares · 2022-04-06T19:08:15Z

x-pack/plugins/security_solution/public/management/common/url_routing/utils.ts

+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */


Some of these were copied over from the existing routing.ts. Once I refactor the existing routing.ts over to a new folder named routing, this util.ts module will likely hold all of the other utilities found there

paul-tavares · 2022-04-06T19:09:24Z

...ins/security_solution/public/management/components/artifact_list_page/artifact_list_page.tsx

@@ -68,6 +68,9 @@ export interface ArtifactListPageProps {
  searchableFields?: MaybeImmutable<string[]>;
  flyoutSize?: EuiFlyoutSize;
  'data-test-subj'?: string;
+  allowCardEditAction?: boolean;
+  allowCardDeleteAction?: boolean;


Although delete is not currently needed, I just backed it in since we will likely need it in the future with RBAC and the ability to have View only pages under Management

paul-tavares · 2022-04-06T19:11:00Z

...k/plugins/security_solution/public/management/components/formatted_error/formatted_error.tsx

+ * A general component for formatting errors. Recognizes different types of errors and displays
+ * their information.
+ */
+export const FormattedError = memo<FormattedErrorProps>(


We will likely improve upon this component moving forward. Example: if we ever introduce server/api provided error codes with response errors, this component could be improved to recognize those types of errors, and formatted them accordingly.

🔥 awesome

paul-tavares · 2022-04-06T19:12:56Z

x-pack/plugins/security_solution/public/management/pages/mocks/exceptions_list_http_mocks.ts

@@ -0,0 +1,259 @@
+/*


An exceptions list API HTTP set of mocks that currently uses the ExceptionsListItemGenerator. I will look to improve this in the future and add support allowing a custom generator to be passed in (ex. TrustedApps, EventFilters, etc)

…and mock resets

…host-isolation-exceptions-to-artifact-list-page-componet

…host-isolation-exceptions-to-artifact-list-page-componet # Conflicts: # x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/service.ts # x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/view/components/delete_modal.test.tsx # x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/view/components/form_flyout.test.tsx # x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/view/host_isolation_exceptions_list.test.tsx # x-pack/plugins/security_solution/public/management/pages/mocks/exceptions_list_http_mocks.ts # x-pack/plugins/security_solution/public/management/pages/policy/view/policy_details.test.tsx # x-pack/plugins/security_solution/public/management/pages/policy/view/tabs/policy_tabs.tsx

…host-isolation-exceptions-to-artifact-list-page-componet

…query)

…host-isolation-exceptions-to-artifact-list-page-componet

paul-tavares · 2022-04-18T21:15:30Z

@elasticmachine merge upstream

…-to-artifact-list-page-componet

kibana-ci · 2022-04-18T22:31:46Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2777	2775	-2

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.8MB	4.8MB	-13.8KB

Page load bundle

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

id	before	after	diff
`securitySolution`	248.0KB	248.1KB	+37.0B

Unknown metric groups

async chunk count

id	before	after	diff
`securitySolution`	23	24	+1

ESLint disabled line counts

id	before	after	diff
`securitySolution`	448	449	+1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	517	518	+1

History

💔 Build #38790 failed 2d5864c
💔 Build #38664 failed 53ae8de
💔 Build #38647 failed 1f6ca0a
💚 Build #37265 succeeded 176c48d
💚 Build #37005 succeeded 739f5ba

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

cc @paul-tavares

ashokaditya

I only have a few minor comments and suggestions but this is good to 🚢 . Thanks for the refactor and adding all the helper methods and utils for tests. 🙏

ashokaditya · 2022-04-07T12:45:51Z

...plugins/security_solution/public/management/common/url_routing/artifact_list_page_routing.ts

+const normalizeArtifactListPageUrlSearchParams = (
+  urlSearchParams: Partial<ArtifactListPageUrlParams> = {}
+): Partial<ArtifactListPageUrlParams> => {
+  return {
+    ...(!isDefaultOrMissing(urlSearchParams.page, 1) ? { page: urlSearchParams.page } : {}),
+    ...(!isDefaultOrMissing(urlSearchParams.pageSize, MANAGEMENT_DEFAULT_PAGE_SIZE)
+      ? { pageSize: urlSearchParams.pageSize }
+      : {}),
+    ...(!isDefaultOrMissing(urlSearchParams.show, undefined) ? { show: urlSearchParams.show } : {}),
+    ...(!isDefaultOrMissing(urlSearchParams.itemId, undefined)
+      ? { itemId: urlSearchParams.itemId }
+      : {}),
+    ...(!isDefaultOrMissing(urlSearchParams.filter, '') ? { filter: urlSearchParams.filter } : ''),
+    ...(!isDefaultOrMissing(urlSearchParams.includedPolicies, '')
+      ? { includedPolicies: urlSearchParams.includedPolicies }
+      : ''),
+  };
+};
+
+export const extractArtifactListPageUrlSearchParams = (
+  query: querystring.ParsedUrlQuery
+): ArtifactListPageUrlParams => {
+  const showParamValue = extractFirstParamValue(query, 'show') as ArtifactListPageUrlParams['show'];
+
+  return {
+    page: extractPageNumber(query),
+    pageSize: extractPageSizeNumber(query),
+    includedPolicies: extractFirstParamValue(query, 'includedPolicies'),
+    show:
+      showParamValue && SHOW_PARAM_ALLOWED_VALUES.includes(showParamValue)
+        ? showParamValue
+        : undefined,
+    itemId: extractFirstParamValue(query, 'itemId'),
+  };
+};


Nice ⭐ Looks like I had the same idea in my PR here.

I think I'll copy these changes into my own PR or wait for this to merge. After updating to use ArtifactListPageUrlParams for types some of these helper methods become obsolete.

ashokaditya · 2022-04-19T12:48:26Z

...rity_solution/public/management/components/effected_policy_select/effected_policy_select.tsx

@@ -158,11 +160,11 @@ export const EffectedPolicySelect = memo<EffectedPolicySelectProps>(
          ),
          policy,
          checked: isPolicySelected.has(policy.id) ? 'on' : undefined,
-          disabled: isGlobal || !isPlatinumPlus,
+          disabled: isGlobal || !isPlatinumPlus || disabled,


Should add a test for this change.

ashokaditya · 2022-04-19T13:59:14Z

...on/public/management/components/artifact_list_page/hooks/use_artifact_card_props_provider.ts

+      if (allowCardEditAction) {
+        cardActions.push({
+          icon: 'controlsHorizontal',
+          onClick: () => {
+            onAction({ type: 'edit', item: artifactItem });
+          },
+          'data-test-subj': getTestId('cardEditAction'),
+          children: cardActionEditLabel,
+        });
+      }
+
+      if (allowCardDeleteAction) {
+        cardActions.push({
+          icon: 'trash',
+          onClick: () => {
+            onAction({ type: 'delete', item: artifactItem });
+          },
+          'data-test-subj': getTestId('cardDeleteAction'),
+          children: cardActionDeleteLabel,
+        });
+      }
+


Nice these will be enabled with future RBAC controls. 👏

ashokaditya · 2022-04-19T14:05:55Z

...ck/plugins/security_solution/public/common/containers/query_client/query_client_provider.tsx

+export type ReactQueryClientProviderProps = PropsWithChildren<{
+  queryClient?: QueryClient;
+}>;


This is not used elsewhere, so no need to export.

ashokaditya · 2022-04-19T14:08:58Z

.../plugins/security_solution/public/management/components/effected_policy_select/test_utils.ts

+ * @param renderResult
+ * @param [atIndex=0]
+ */
+export const clickOnEffectedPolicy = async (


This is super helpful. Thanks 🥇

ashokaditya · 2022-04-19T14:10:22Z

...k/plugins/security_solution/public/management/components/formatted_error/formatted_error.tsx

+ * A general component for formatting errors. Recognizes different types of errors and displays
+ * their information.
+ */
+export const FormattedError = memo<FormattedErrorProps>(


🔥 awesome

ashokaditya · 2022-04-19T14:16:06Z

...ger_integration/endpoint_package_custom_extension/endpoint_package_custom_extension.test.tsx

@@ -38,7 +38,7 @@ describe('When displaying the EndpointPackageCustomExtension fleet UI extension'
  });

  afterEach(() => {
-    useEndpointPrivilegesMock.mockReturnValue(getEndpointPrivilegesInitialStateMock());
+    useEndpointPrivilegesMock.mockImplementation(getEndpointPrivilegesInitialStateMock);


ashokaditya · 2022-04-19T14:20:45Z

x-pack/plugins/security_solution/public/management/pages/host_isolation_exceptions/constants.ts

 export const HOST_ISOLATION_EXCEPTIONS_LIST_DEFINITION: CreateExceptionListSchema = {
  name: ENDPOINT_HOST_ISOLATION_EXCEPTIONS_LIST_NAME,
  namespace_type: 'agnostic',
  description: ENDPOINT_HOST_ISOLATION_EXCEPTIONS_LIST_DESCRIPTION,
  list_id: ENDPOINT_HOST_ISOLATION_EXCEPTIONS_LIST_ID,
-  type: HOST_ISOLATION_EXCEPTIONS_LIST_TYPE,
+  type: ExceptionListTypeEnum.ENDPOINT_HOST_ISOLATION_EXCEPTIONS,


This is nice! Although, I notice for some reason the trusted_apps list type is called endpoint😱 . We should update it to match the other names. Perhaps endpoint_trusted_apps. I'll do this in my PR if you agree.

Yeah, you can do that as long as it does not break anything. This custom type value was introduced at some point after TA and we never got to update TA to also have its own type.

refs elastic/pull/129099/commits/b688d505b460964117ba1a303b417760b13a2888

refs elastic/pull/129099/

…e` component (#129208) * move validations to artifacts fixes elastic/security-team/issues/3092 * use ArtifactsListPage fixes elastic/security-team/issues/3092 * Remove redundant files fixes elastic/security-team/issues/3092 * Update trusted app list ftr test fixes elastic/security-team/issues/3092 * fix test mock fixes elastic/security-team/issues/3092 * add trusted app form tests fixes elastic/security-team/issues/3092 * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * default page index to 1 when set to 0 refs /pull/129099/commits/b688d505b460964117ba1a303b417760b13a2888 * add tests for new routing methods refs /pull/129099/ * review changes fixes elastic/security-team/issues/3092 * update fleet integration test without any trusted app entries, it should see the trusted empty page * update translation again after merge refs 6a792fc * add a test for search field KQL review suggestions * remove redundant flyout size defaults to `m` Co-authored-by: kibanamachine <[email protected]>

… page to now use `<ArtifactListPage/>` component (elastic#129099) * Move Host Isolation Exceptions to `<ArtifactListPage>` * New Generic FormatError component for formatting error Objects * clean up un-used components ++ hooks * EffectedPolicySelect support for `disabled` prop * new ExceptionsList HTTP mocks * additional test utilities for EffectedPolicySelect component * New url routing utilities for Artifact List Page * Refactor `useCanSeeHostIsolationExceptionsMenu()` hook to use generic `useSummaryArtifact()` hook * add props to ArtifactListPage to allow for hiding actions like create, edit, delete * Split out useUserPrivileges() mock implementation function for reuse and mock resets * add generic `getFirstCard()` test utility to ArtifactListPage mocks

…e` component (elastic#129208) * move validations to artifacts fixes elastic/security-team/issues/3092 * use ArtifactsListPage fixes elastic/security-team/issues/3092 * Remove redundant files fixes elastic/security-team/issues/3092 * Update trusted app list ftr test fixes elastic/security-team/issues/3092 * fix test mock fixes elastic/security-team/issues/3092 * add trusted app form tests fixes elastic/security-team/issues/3092 * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * default page index to 1 when set to 0 refs elastic/pull/129099/commits/b688d505b460964117ba1a303b417760b13a2888 * add tests for new routing methods refs elastic/pull/129099/ * review changes fixes elastic/security-team/issues/3092 * update fleet integration test without any trusted app entries, it should see the trusted empty page * update translation again after merge refs 6a792fc * add a test for search field KQL review suggestions * remove redundant flyout size defaults to `m` Co-authored-by: kibanamachine <[email protected]>

paul-tavares added 3 commits March 31, 2022 10:57

initial commit wiht some cleanup

e7cc3b3

Adjust labels to match current implementation

937877c

add array of searchable fields

7ee5470

paul-tavares added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution OLM Sprint v8.3.0 labels Mar 31, 2022

paul-tavares self-assigned this Mar 31, 2022

paul-tavares added 15 commits March 31, 2022 16:13

Generic FormatError component for formatting error Objects

956b5f0

Adjust Host Isolation Exceptions Form to the ArtifactListPage component

48398ba

clean up un-used components ++ hooks

0ed0abb

Additional clean up of file content

12ac093

EffectedPolicySelect support for disabled prop

24c1f00

Ensure Form fields are disabled when API is in flight

cb56284

Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…

b696e92

…host-isolation-exceptions-to-artifact-list-page-componet

new ExceptionsList HTTP mocks

36bcdff

Tests for the List to validate search/filter

ef65a42

Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…

84e0aff

…host-isolation-exceptions-to-artifact-list-page-componet

additional test utilities for EffectedPolicySelect component

7840222

Form tests

5db499a

Tests for FormattedError

71854b0

Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…

7453c4a

…host-isolation-exceptions-to-artifact-list-page-componet

Fix i18n entries

c924f27

paul-tavares commented Apr 4, 2022

View reviewed changes

paul-tavares added 5 commits April 4, 2022 17:43

Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…

5812b67

…host-isolation-exceptions-to-artifact-list-page-componet

Delete host isolation exceptions store and adjust global Management s…

53a325d

…ection store definition

New url routing utilities for Artifact List Page

b688d50

Refactor Host Isolation Exceptions routing function to use new Artifa…

b4bf73b

…ct list page routing

paul-tavares commented Apr 6, 2022

View reviewed changes

paul-tavares added 4 commits April 7, 2022 11:43

Split out useUserPrivileges() mock implementation function for reuse …

6ce6b00

…and mock resets

add generic getFirstCard() test utility to ArtifactListPage mocks

f3d33f3

Fix form tests and add additional tests to list page

3ff6457

Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…

1163df0

…host-isolation-exceptions-to-artifact-list-page-componet

paul-tavares requested a review from a team as a code owner April 7, 2022 15:45

paul-tavares added 9 commits April 7, 2022 18:00

some adjustments to exports and jsdocs

739f5ba

Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…

176c48d

…host-isolation-exceptions-to-artifact-list-page-componet

fix stuff after merge from main

53ae8de

Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…

a2d1999

…host-isolation-exceptions-to-artifact-list-page-componet

Fix TS errors

5dba32d

Fix issue with test render keeping state from prior API calls (react-…

4d853ea

…query)

Fix tests for EndpointPackageCustomExtension

eb307c2

Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…

2d5864c

…host-isolation-exceptions-to-artifact-list-page-componet

Merge branch 'main' into task/olm-3094-move-host-isolation-exceptions…

b16c443

…-to-artifact-list-page-componet

ashokaditya approved these changes Apr 19, 2022

View reviewed changes

paul-tavares merged commit cc34125 into elastic:main Apr 19, 2022

paul-tavares deleted the task/olm-3094-move-host-isolation-exceptions-to-artifact-list-page-componet branch April 19, 2022 14:56

ashokaditya added a commit to ashokaditya/kibana that referenced this pull request Apr 20, 2022

default page index to 1 when set to 0

f52f60b

refs elastic/pull/129099/commits/b688d505b460964117ba1a303b417760b13a2888

ashokaditya added a commit to ashokaditya/kibana that referenced this pull request Apr 20, 2022

add tests for new routing methods

e82f9ac

refs elastic/pull/129099/

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[security solution][Endpoint] Refactor host isolation exceptions list page to now use `<ArtifactListPage/>` component #129099

[security solution][Endpoint] Refactor host isolation exceptions list page to now use `<ArtifactListPage/>` component #129099

paul-tavares commented Mar 31, 2022 •

edited

Loading

paul-tavares Apr 4, 2022

ashokaditya Apr 19, 2022

elasticmachine commented Apr 6, 2022

paul-tavares Apr 6, 2022

paul-tavares Apr 6, 2022

paul-tavares Apr 6, 2022

paul-tavares Apr 6, 2022

ashokaditya Apr 19, 2022

paul-tavares Apr 6, 2022

paul-tavares commented Apr 18, 2022

kibana-ci commented Apr 18, 2022

async chunk count

ESLint disabled line counts

Total ESLint disabled count

ashokaditya left a comment

ashokaditya Apr 7, 2022

ashokaditya Apr 19, 2022

ashokaditya Apr 19, 2022

ashokaditya Apr 19, 2022

ashokaditya Apr 19, 2022

ashokaditya Apr 19, 2022

ashokaditya Apr 19, 2022

ashokaditya Apr 19, 2022

ashokaditya Apr 19, 2022

paul-tavares Apr 19, 2022

[security solution][Endpoint] Refactor host isolation exceptions list page to now use <ArtifactListPage/> component #129099

[security solution][Endpoint] Refactor host isolation exceptions list page to now use <ArtifactListPage/> component #129099

Conversation

paul-tavares commented Mar 31, 2022 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 6, 2022

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

paul-tavares commented Apr 18, 2022

kibana-ci commented Apr 18, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

ashokaditya 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[security solution][Endpoint] Refactor host isolation exceptions list page to now use `<ArtifactListPage/>` component #129099

[security solution][Endpoint] Refactor host isolation exceptions list page to now use `<ArtifactListPage/>` component #129099

paul-tavares commented Mar 31, 2022 •

edited

Loading