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

Conversation

paul-tavares
Copy link
Contributor

Summary

  • Isolate action should not be available for use in the Endpoint List and Endpoint Details flyout unless the license is Platinum+
  • Un-isolate action will remain available if the Endpoint host is currently isolated

Screen capture below done with License set to basic
olm-1246-isolation-status-on-endpoint-list-details

Checklist

@paul-tavares paul-tavares added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 16, 2021
@paul-tavares paul-tavares self-assigned this Jun 16, 2021
@paul-tavares paul-tavares requested a review from a team as a code owner June 16, 2021 16:06
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@pzl pzl left a comment

Choose a reason for hiding this comment

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

looks okay, but see if you can remove the license service mock.

on the server side, it hasn't required any substantial workaround to do, but if there's a problem doing the same front-end, then oh well


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.

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

Copy link
Contributor

@dasansol92 dasansol92 left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@spalger
Copy link
Contributor

spalger commented Jun 16, 2021

jenkins, test this

(restarting due to jenkins upgrade)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 6.9MB 6.9MB -122.0B

History

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

cc @paul-tavares

@paul-tavares paul-tavares merged commit 2507d37 into elastic:master Jun 16, 2021
@paul-tavares paul-tavares deleted the task/olm-1304-isolate-action-is-platinum-only branch June 16, 2021 22:45
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 16, 2021
… to Platinum+ licenses (elastic#102374)

* Isolate action should only be available for platinum license
* Moved `useLicense` hook mock into `__mocks__`
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 17, 2021
… to Platinum+ licenses (#102374) (#102434)

* Isolate action should only be available for platinum license
* Moved `useLicense` hook mock into `__mocks__`

Co-authored-by: Paul Tavares <56442535+paul-tavares@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 17, 2021
…egrations-to-global-search

* 'master' of github.com:elastic/kibana: (46 commits)
  [Lens] Add some more documentation for dynamic coloring (elastic#101369)
  hide not searchable results when no term (elastic#102401)
  [Lens] Fix Formula functional test with multiple suggestions (elastic#102378)
  Fix trusted apps modified by field displayed as a date field (elastic#102377)
  [Lens] Docs for time shift (elastic#102048)
  update readme of logs-metrics-ui (elastic#101968)
  Refactor observability plugin breadcrumbs (elastic#102290)
  [Index Patterns] Move rollup config to index pattern management v2 (elastic#102285)
  [Security Solution][Endpoint] Isolate Action should only be available to Platinum+ licenses (elastic#102374)
  [build] Updates Ironbank templates (elastic#102407)
  Update security best practices document (elastic#100814)
  [Enterprise Search] Set up initial KibanaPageTemplate  (elastic#102170)
  [Reporting/Docs] Add section to troubleshooting guide to explain the StatusCodeError logs (elastic#102278)
  [DOCS] Updating Elastic Security Overview topic  (elastic#101922)
  [Uptime] refactor Synthetics Integration package UI (elastic#102080)
  [Task Manager] Log at different levels based on the state (elastic#101751)
  [APM] Fixing time comparison types (elastic#101423)
  [RAC] Update alert documents in lifecycle rule type helper (elastic#101598)
  [ML] Functional tests - fix and re-activate alerting flyout test (elastic#102368)
  [Reporting] remove unused reference to path.data config (elastic#102267)
  ...

# Conflicts:
#	x-pack/plugins/fleet/kibana.json
majagrubic pushed a commit to majagrubic/kibana that referenced this pull request Jun 18, 2021
… to Platinum+ licenses (elastic#102374)

* Isolate action should only be available for platinum license
* Moved `useLicense` hook mock into `__mocks__`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants