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][Endpoint] Refactor host isolation exceptions list page to now use <ArtifactListPage/> component #129099

Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
e7cc3b3
initial commit wiht some cleanup
paul-tavares Mar 31, 2022
937877c
Adjust labels to match current implementation
paul-tavares Mar 31, 2022
7ee5470
add array of searchable fields
paul-tavares Mar 31, 2022
956b5f0
Generic FormatError component for formatting error Objects
paul-tavares Mar 31, 2022
48398ba
Adjust Host Isolation Exceptions Form to the ArtifactListPage component
paul-tavares Mar 31, 2022
0ed0abb
clean up un-used components ++ hooks
paul-tavares Mar 31, 2022
12ac093
Additional clean up of file content
paul-tavares Mar 31, 2022
24c1f00
EffectedPolicySelect support for `disabled` prop
paul-tavares Mar 31, 2022
cb56284
Ensure Form fields are disabled when API is in flight
paul-tavares Mar 31, 2022
b696e92
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 1, 2022
36bcdff
new ExceptionsList HTTP mocks
paul-tavares Apr 1, 2022
ef65a42
Tests for the List to validate search/filter
paul-tavares Apr 1, 2022
84e0aff
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 1, 2022
7840222
additional test utilities for EffectedPolicySelect component
paul-tavares Apr 1, 2022
5db499a
Form tests
paul-tavares Apr 1, 2022
71854b0
Tests for FormattedError
paul-tavares Apr 1, 2022
7453c4a
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 4, 2022
c924f27
Fix i18n entries
paul-tavares Apr 4, 2022
5812b67
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 4, 2022
53a325d
Delete host isolation exceptions store and adjust global Management s…
paul-tavares Apr 4, 2022
722b8d7
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 5, 2022
b688d50
New url routing utilities for Artifact List Page
paul-tavares Apr 5, 2022
b4bf73b
Refactor Host Isolation Exceptions routing function to use new Artifa…
paul-tavares Apr 5, 2022
3ac4b84
Remove custom host isolation exceptions hook and use generic one
paul-tavares Apr 5, 2022
bb375e1
Remove unnecessary service functions and adjust tests in policy details
paul-tavares Apr 5, 2022
2ae32df
Add http mock for exceptions summary api
paul-tavares Apr 5, 2022
e7518f5
Refactor `useCanSeeHostIsolationExceptionsMenu()` hook to use generic…
paul-tavares Apr 5, 2022
ec07ea2
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 6, 2022
acc0983
add props to ArtifactListPage to allow for hiding actions like create…
paul-tavares Apr 6, 2022
3ca47ed
hide create/edit actions on host isolation list page if user can't is…
paul-tavares Apr 6, 2022
7529671
Additional tests for ArtifactLIstPage
paul-tavares Apr 6, 2022
344b2d0
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 6, 2022
6ce6b00
Split out useUserPrivileges() mock implementation function for reuse …
paul-tavares Apr 7, 2022
f3d33f3
add generic `getFirstCard()` test utility to ArtifactListPage mocks
paul-tavares Apr 7, 2022
3ff6457
Fix form tests and add additional tests to list page
paul-tavares Apr 7, 2022
1163df0
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 7, 2022
739f5ba
some adjustments to exports and jsdocs
paul-tavares Apr 7, 2022
176c48d
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 11, 2022
1f6ca0a
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 18, 2022
53ae8de
fix stuff after merge from main
paul-tavares Apr 18, 2022
a2d1999
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 18, 2022
5dba32d
Fix TS errors
paul-tavares Apr 18, 2022
4d853ea
Fix issue with test render keeping state from prior API calls (react-…
paul-tavares Apr 18, 2022
eb307c2
Fix tests for EndpointPackageCustomExtension
paul-tavares Apr 18, 2022
2d5864c
Merge remote-tracking branch 'upstream/main' into task/olm-3094-move-…
paul-tavares Apr 18, 2022
b16c443
Merge branch 'main' into task/olm-3094-move-host-isolation-exceptions…
kibanamachine Apr 18, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export type EffectedPolicySelectProps = Omit<
description?: string;
onChange: (selection: EffectedPolicySelection) => void;
selected?: PolicyData[];
disabled?: boolean;
};
export const EffectedPolicySelect = memo<EffectedPolicySelectProps>(
({
Expand All @@ -99,6 +100,7 @@ export const EffectedPolicySelect = memo<EffectedPolicySelectProps>(
listProps,
options,
selected = [],
disabled = false,
'data-test-subj': dataTestSubj,
...otherSelectableProps
}) => {
Expand Down Expand Up @@ -140,7 +142,7 @@ export const EffectedPolicySelect = memo<EffectedPolicySelectProps>(
id={htmlIdGenerator()()}
onChange={NOOP}
checked={isPolicySelected.has(policy.id)}
disabled={isGlobal || !isPlatinumPlus}
disabled={isGlobal || !isPlatinumPlus || disabled}
data-test-subj={`policy-${policy.id}-checkbox`}
/>
),
Expand All @@ -158,11 +160,11 @@ export const EffectedPolicySelect = memo<EffectedPolicySelectProps>(
),
policy,
checked: isPolicySelected.has(policy.id) ? 'on' : undefined,
disabled: isGlobal || !isPlatinumPlus,
disabled: isGlobal || !isPlatinumPlus || disabled,
Copy link
Member

Choose a reason for hiding this comment

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

Should add a test for this change.

'data-test-subj': `policy-${policy.id}`,
}))
.sort(({ label: labelA }, { label: labelB }) => labelA.localeCompare(labelB));
}, [getAppUrl, isGlobal, isPlatinumPlus, options, selected]);
}, [disabled, getAppUrl, isGlobal, isPlatinumPlus, options, selected]);

const handleOnPolicySelectChange = useCallback<
Required<EuiSelectableProps<OptionPolicyData>>['onChange']
Expand Down Expand Up @@ -223,14 +225,15 @@ export const EffectedPolicySelect = memo<EffectedPolicySelectProps>(
</EuiText>
</EuiFlexItem>
<StyledEuiFlexItemButtonGroup grow={1}>
<EuiFormRow fullWidth>
<EuiFormRow fullWidth isDisabled={disabled}>
<StyledButtonGroup
legend="Global Policy Toggle"
options={toggleGlobal}
idSelected={isGlobal ? 'globalPolicy' : 'perPolicy'}
onChange={handleGlobalButtonChange}
color="primary"
data-test-subj={getTestId('byPolicyGlobalButtonGroup')}
isDisabled={disabled}
/>
</EuiFormRow>
</StyledEuiFlexItemButtonGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* 2.0.
*/

import userEvent from '@testing-library/user-event';
import { AppContextTestRender } from '../../../common/mock/endpoint';

/**
* Forces the `offsetWidth` of `HTMLElement` to a given value. Needed due to the use of
* `react-virtualized-auto-sizer` by the eui `Selectable` component
Expand Down Expand Up @@ -42,3 +45,51 @@ export const forceHTMLElementOffsetWidth = (width: number = 100): (() => void) =
}
};
};

/**
* Clicks on a policy being displayed when `per policy` is selected.
* NOTE: ensure that per-policy is selected first. This utility will
* not do that.
* @param renderResult
* @param [atIndex=0]
*/
export const clickOnEffectedPolicy = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

This is super helpful. Thanks 🥇

renderResult: ReturnType<AppContextTestRender['render']>,
atIndex: number = 0
): Promise<Element> => {
const policiesHolderElement = await renderResult.findByTestId(
'effectedPolicies-select-policiesSelectable'
);
const policyElements = policiesHolderElement.querySelectorAll('li.euiSelectableListItem');
const item = policyElements.item(atIndex);

if (item) {
userEvent.click(item);
}

return item;
};

/**
* Returns true or false as to whether an effect policy at a given index in the list is selected or not
* @param renderResult
* @param atIndex
*/
export const isEffectedPolicySelected = async (
renderResult: ReturnType<AppContextTestRender['render']>,
atIndex: number = 0
): Promise<boolean> => {
const policiesHolderElement = await renderResult.findByTestId(
'effectedPolicies-select-policiesSelectable'
);
const policyElements = policiesHolderElement.querySelectorAll<HTMLLIElement>(
'li.euiSelectableListItem'
);
const item = policyElements.item(atIndex);

if (!item) {
throw new Error(`No policy found in EffectedPolicySelect at index position ${atIndex}`);
}

return item.dataset.testSelected === 'true';
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* 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.
*/

import { AppContextTestRender, createAppRootMockRenderer } from '../../../common/mock/endpoint';
import { FormattedError } from './formatted_error';
import React from 'react';
import type { HttpFetchError } from 'kibana/public';

describe('When using the `FormattedError` component', () => {
let render: () => ReturnType<AppContextTestRender['render']>;
let renderResult: ReturnType<AppContextTestRender['render']>;
let error: Error;

beforeEach(() => {
const mockedContext = createAppRootMockRenderer();

render = () =>
(renderResult = mockedContext.render(<FormattedError error={error} data-test-subj="test" />));
error = new Error('foo');
});

it('should display the error message for normal errors', () => {
render();

expect(renderResult.getByTestId('test').textContent).toEqual('foo');
});

it('should display status code, message and API response body for HttpFetchError', () => {
const httpError = new Error('api foo') as HttpFetchError;
Object.assign(httpError, {
name: 'foo',
req: {} as Request,
res: {} as Response,
response: {
status: '400',
statusText: 'some 400 error',
} as unknown as Response,
body: { message: 'something bad happen', at: 'some place' },
});
error = httpError;
render();

expect(renderResult.getByTestId('test').textContent).toEqual(
'400: some 400 errormessage: something bad happenat: some place'
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* 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.
*/

import React, { memo, useMemo } from 'react';
import { EuiText } from '@elastic/eui';
import type { HttpFetchError } from 'kibana/public';

const isHttpFetchError = (error: Error | HttpFetchError): error is HttpFetchError => {
return 'body' in error && 'req' in error && 'res' in error && 'response' in error;
};

export interface ObjectContentProps {
data: object;
}

export const ObjectContent = memo<ObjectContentProps>(({ data }) => {
return (
<EuiText size="s">
{Object.entries(data).map(([key, value]) => {
return (
<div key={key} className="eui-textBreakAll">
<strong>{key}</strong>
{': '}
{value}
</div>
);
})}
</EuiText>
);
});
ObjectContent.displayName = 'ObjectContent';

export interface FormattedErrorProps {
error: Error;
'data-test-subj'?: string;
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

🔥 awesome

({ error, 'data-test-subj': dataTestSubj }) => {
return useMemo(() => {
let content: JSX.Element = <>{error.message}</>;

if (isHttpFetchError(error)) {
content = (
<>
<div>{`${error.response?.status}: ${error.response?.statusText}`}</div>
{error.body && <ObjectContent data={error.body} />}
</>
);
}

return <EuiText data-test-subj={dataTestSubj}>{content}</EuiText>;
}, [dataTestSubj, error]);
}
);
FormattedError.displayName = 'FormattedError';
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* 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.
*/

export { FormattedError } from './formatted_error';
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* 2.0.
*/
import {
ExceptionListType,
ExceptionListTypeEnum,
CreateExceptionListSchema,
} from '@kbn/securitysolution-io-ts-list-types';
Expand All @@ -22,13 +21,10 @@ export const SEARCHABLE_FIELDS: Readonly<string[]> = [
`entries.value`,
];

export const HOST_ISOLATION_EXCEPTIONS_LIST_TYPE: ExceptionListType =
ExceptionListTypeEnum.ENDPOINT_HOST_ISOLATION_EXCEPTIONS;

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,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

};
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,11 @@
*/

import {
CreateExceptionListItemSchema,
ExceptionListItemSchema,
ExceptionListSummarySchema,
FoundExceptionListItemSchema,
UpdateExceptionListItemSchema,
} from '@kbn/securitysolution-io-ts-list-types';
import { ENDPOINT_HOST_ISOLATION_EXCEPTIONS_LIST_ID } from '@kbn/securitysolution-list-constants';
import { transformNewItemOutput, transformOutput } from '@kbn/securitysolution-list-hooks';
import { HttpStart } from 'kibana/public';
import { EXCEPTION_LIST_ITEM_URL, EXCEPTION_LIST_URL } from '../event_filters/constants';
import { HOST_ISOLATION_EXCEPTIONS_LIST_DEFINITION } from './constants';
Expand Down Expand Up @@ -69,66 +66,6 @@ export async function getHostIsolationExceptionItems({
return entries;
}

export async function createHostIsolationExceptionItem(
http: HttpStart,
exception: CreateExceptionListItemSchema
): Promise<ExceptionListItemSchema> {
await ensureHostIsolationExceptionsListExists(http);
const entry = transformNewItemOutput(exception);
return http.post<ExceptionListItemSchema>(EXCEPTION_LIST_ITEM_URL, {
body: JSON.stringify(entry),
});
}

export async function deleteOneHostIsolationExceptionItem(http: HttpStart, id: string) {
await ensureHostIsolationExceptionsListExists(http);
return http.delete<ExceptionListItemSchema>(EXCEPTION_LIST_ITEM_URL, {
query: {
id,
namespace_type: 'agnostic',
},
});
}

export async function getOneHostIsolationExceptionItem(
http: HttpStart,
id: string
): Promise<UpdateExceptionListItemSchema> {
await ensureHostIsolationExceptionsListExists(http);
return http.get<ExceptionListItemSchema>(EXCEPTION_LIST_ITEM_URL, {
query: {
id,
namespace_type: 'agnostic',
},
});
}

export async function updateOneHostIsolationExceptionItem(
http: HttpStart,
exception: UpdateExceptionListItemSchema
): Promise<ExceptionListItemSchema> {
await ensureHostIsolationExceptionsListExists(http);
const entry = transformOutput(exception);

// Clean unnecessary fields for update action
const fieldsToRemove: Array<keyof ExceptionListItemSchema> = [
'created_at',
'created_by',
'created_at',
'created_by',
'list_id',
'tie_breaker_id',
'updated_at',
'updated_by',
];

fieldsToRemove.forEach((field) => {
delete entry[field as keyof UpdateExceptionListItemSchema];
});
return http.put<ExceptionListItemSchema>(EXCEPTION_LIST_ITEM_URL, {
body: JSON.stringify(entry),
});
}
export async function getHostIsolationExceptionSummary(
http: HttpStart,
filter?: string
Expand Down
Loading