-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution][Endpoint] Isolate Action should only be available to Platinum+ licenses #102374
Conversation
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
There was a problem hiding this 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woop woop
...ity_solution/public/management/pages/endpoint_hosts/view/hooks/use_endpoint_action_items.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
jenkins, test this (restarting due to jenkins upgrade) |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
… to Platinum+ licenses (elastic#102374) * Isolate action should only be available for platinum license * Moved `useLicense` hook mock into `__mocks__`
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…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
Summary
isolated
Screen capture below done with License set to basic

Checklist