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] Isolate Action should only be available to Platinum+ licenses #102374

21 changes: 21 additions & 0 deletions x-pack/plugins/security_solution/common/license/mocks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* 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 { LicenseService } from './license';

export const createLicenseServiceMock = (): jest.Mocked<LicenseService> => {
return ({
start: jest.fn(),
stop: jest.fn(),
getLicenseInformation: jest.fn(),
getLicenseInformation$: jest.fn(),
isAtLeast: jest.fn(),
isGoldPlus: jest.fn().mockReturnValue(true),
isPlatinumPlus: jest.fn().mockReturnValue(true),
isEnterprise: jest.fn().mockReturnValue(true),
} as unknown) as jest.Mocked<LicenseService>;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* 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 { createLicenseServiceMock } from '../../../../common/license/mocks';

export const licenseService = createLicenseServiceMock();
Copy link
Member

Choose a reason for hiding this comment

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

LicenseService is a concrete class. It should not need to be mocked. Best practice is to init the real class, and inject any needed/mocked state as necessary into the instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

A simpler approach can be using a spy for useLicense and return the mocked value for the specific describe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by concrete class?
In order to do this better, we would need to mock the observables that come out of kibana and that this class takes in during .start(). We can track that as tech. debt if needed - but I'm not trying to test LicenseService class here (if that was the case - then yes, totally agree 😄 ).

I'm just trying to mock the instance created by the useLicense hook. The issue is that the hook instantiates and uses a LicenseService instance that is also exported. That exported instance is then used during the UI's Plugin.start() to give it the licensing observable. Because of this, its difficult to avoid the mocking of the hook. the implementation approach would have to be changed to using a React context provider - similar to how we have others context providers defined and then we could instantiate and use a custom LicenseService instance in bootstrapping the UI app.

Copy link
Member

Choose a reason for hiding this comment

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

ok, so part of it is how the frontend is set up here.

Since the frontend/plugin.tsx is starting the service with licenseService.start(plugins.licensing.license$); where licenseService is the imported instance. You don't have great control to supplant with your own instance, or passing in an observable you control (replacing plugins.licensing.license$).

It's still not good to mock value classes (sorry this is what I meant, not concrete), specifically when they are not under test. But the singleton you don't control makes it hard.

Unless you can think of a sane way to replace what's in plugins.licensing.license$. That's really all the control you need to set the 'license state' for a given test.

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, another thing I don't like about this mocking of the singleton is that it creates state between tests - I just got a failure here in CI that I think was caused by that.

I'm going to create a refactor issue to revisit this.

export const useLicense = () => licenseService;
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ import React from 'react';
import { act } from '@testing-library/react';
import { endpointPageHttpMock } from '../../../mocks';
import { fireEvent } from '@testing-library/dom';
import { licenseService } from '../../../../../../common/hooks/use_license';

jest.mock('../../../../../../common/lib/kibana');
jest.mock('../../../../../../common/hooks/use_license');

describe('When using the Endpoint Details Actions Menu', () => {
let render: () => Promise<ReturnType<AppContextTestRender['render']>>;
Expand Down Expand Up @@ -112,4 +114,21 @@ describe('When using the Endpoint Details Actions Menu', () => {
expect(coreStart.application.navigateToApp).toHaveBeenCalled();
});
});

describe('and license is NOT PlatinumPlus', () => {
(licenseService as jest.Mocked<typeof licenseService>).isPlatinumPlus.mockReturnValue(false);

it('should not show the `isoalte` action', async () => {
setEndpointMetadataResponse();
await render();
expect(renderResult.queryByTestId('isolateLink')).toBeNull();
});

it('should still show `unisolate` action for endpoints that are currently isolated', async () => {
setEndpointMetadataResponse(true);
await render();
expect(renderResult.queryByTestId('isolateLink')).toBeNull();
expect(renderResult.getByTestId('unIsolateLink')).not.toBeNull();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import { useEndpointSelector } from './hooks';
import { agentPolicies, uiQueryParams } from '../../store/selectors';
import { useKibana } from '../../../../../common/lib/kibana';
import { ContextMenuItemNavByRouterProps } from '../components/context_menu_item_nav_by_rotuer';
import { isEndpointHostIsolated } from '../../../../../common/utils/validators/is_endpoint_host_isolated';
import { isEndpointHostIsolated } from '../../../../../common/utils/validators';
import { useLicense } from '../../../../../common/hooks/use_license';

/**
* Returns a list (array) of actions for an individual endpoint
Expand All @@ -26,6 +27,7 @@ import { isEndpointHostIsolated } from '../../../../../common/utils/validators/i
export const useEndpointActionItems = (
endpointMetadata: MaybeImmutable<HostMetadata> | undefined
): ContextMenuItemNavByRouterProps[] => {
const isPlatinumPlus = useLicense().isPlatinumPlus();
const { formatUrl } = useFormatUrl(SecurityPageName.administration);
const fleetAgentPolicies = useEndpointSelector(agentPolicies);
const allCurrentUrlParams = useEndpointSelector(uiQueryParams);
Expand Down Expand Up @@ -58,9 +60,9 @@ export const useEndpointActionItems = (
selected_endpoint: endpointId,
});

return [
isIsolated
? {
const isolationActions = isIsolated // Un-isolate is always available to users regardless of license level
Copy link
Member

Choose a reason for hiding this comment

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

woop woop

paul-tavares marked this conversation as resolved.
Show resolved Hide resolved
? [
{
'data-test-subj': 'unIsolateLink',
icon: 'logoSecurity',
key: 'unIsolateHost',
Expand All @@ -75,8 +77,11 @@ export const useEndpointActionItems = (
defaultMessage="Unisolate host"
/>
),
}
: {
},
]
: isPlatinumPlus // For Platinum++ licenses, users also have ability to isolate
? [
{
'data-test-subj': 'isolateLink',
icon: 'logoSecurity',
key: 'isolateHost',
Expand All @@ -92,6 +97,11 @@ export const useEndpointActionItems = (
/>
),
},
]
: [];

return [
...isolationActions,
{
'data-test-subj': 'hostLink',
icon: 'logoSecurity',
Expand Down Expand Up @@ -183,5 +193,12 @@ export const useEndpointActionItems = (
}

return [];
}, [allCurrentUrlParams, endpointMetadata, fleetAgentPolicies, formatUrl, getUrlForApp]);
}, [
allCurrentUrlParams,
endpointMetadata,
fleetAgentPolicies,
formatUrl,
getUrlForApp,
isPlatinumPlus,
]);
};
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,7 @@ import { policyListApiPathHandlers } from '../store/test_mock_utils';
import { licenseService } from '../../../../common/hooks/use_license';

jest.mock('../../../../common/components/link_to');
jest.mock('../../../../common/hooks/use_license', () => {
const licenseServiceInstance = {
isPlatinumPlus: jest.fn(),
};
return {
licenseService: licenseServiceInstance,
useLicense: () => {
return licenseServiceInstance;
},
};
});
jest.mock('../../../../common/hooks/use_license');

describe('Policy Details', () => {
type FindReactWrapperResponse = ReturnType<ReturnType<typeof render>['find']>;
Expand Down