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

Fix common license checker issues #31339

Merged
merged 8 commits into from Feb 26, 2019

Conversation

Projects
None yet
5 participants
@jen-huang
Copy link
Contributor

commented Feb 16, 2019

#30299 moved index management's license checker into common code directory. However, plugin name and valid license modes were left in the shared copy. This PR updates the shared license checker to take plugin ID and valid license modes as arguments. It also fixes the singleton nature of licensePreRoutingFactory (previously, every route license check would be against the plugin that registers their routes first.

@cjcenizal, we discussed reverting the shared nature license checker due to lack of consistency between each plugin's implementation, but I found that license management didn't have a license checker implementation at all. Since the shared license checker is only used by index management and license management at the moment, I think we can keep it shared. We will have to be mindful of any license checker contract differences when we refactor other plugins to use this in the future.

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

Pinging @elastic/es-ui

ID: 'index_management'
ID: 'index_management',
VALID_LICENSE_MODES: [
'trial',

This comment has been minimized.

Copy link
@jen-huang

jen-huang Feb 16, 2019

Author Contributor

This PR can be tested by:

  1. Running with a trial license and editing this file to remove trial license
  2. View index management page, a toast error message about license should appear.
  3. View license management page, there should be no errors

The inverse can also work as a test 😄 (remove trial from license_management and check the two apps)

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

@cjcenizal
Copy link
Contributor

left a comment

Thanks for doing this Jen! I haven't finished reviewing this, but I just wanted to leave a thought before I end the day...

Since the shared license checker is only used by index management and license management at the moment, I think we can keep it shared.

Before we offer an abstraction for the rest of the team to use, I think we should vet it thoroughly against all of the use cases we can think of and have a high degree of confidence that it will be stable and meet everyone's needs. There is at least one case in which this isn't true... CCR returns a differently-shaped object than this implementation does (https://github.com/elastic/kibana/blob/master/x-pack/plugins/cross_cluster_replication/server/lib/check_license/check_license.js#L52). Additionally, I don't think the original checkLicense implementation which we're borrowing from was fully thought-out -- what are showLinks and enableLinks supposed to mean? Why require the consumer to pass in all valid licenses instead of just the highest-level license at which the feature is available? I think we can fix these issues pretty quickly... but until we do, I'd be uncomfortable sharing an abstraction with the rest of the team that I'm not confident in.

// If, for some reason, we cannot get the license information
// from Elasticsearch, assume worst case and disable
if (!xpackLicenseInfo || !xpackLicenseInfo.isAvailable()) {
return {
isAvailable: false,
showLinks: true,
enableLinks: false,
message: `You cannot use ${pluginName} because license information is not available at this time.`
message: `You cannot use ${pluginId} because license information is not available at this time.`

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Feb 16, 2019

Contributor

I think we want to keep this as pluginName (and update the consumer to provide the plugin name, not the ID), to keep as a human-readable message: "You cannot use Index Management..." vs "You cannot use index_management..."

@sebelga
Copy link
Contributor

left a comment

This is a great initiative. Although I must say that I agree with @cjcenizal that it might be worthy to think through the different scenarios to improve the abstraction.

To me, to license configuration and validation is a mistery and it seems strange that the plugin is declaring the license it needs. I would expect a centralized license checker as the single source of truth.

IMO the flow should be:

  • "Hey I am a new plugin with id 'index_management' (our index.js entry) please insert me into the DOM"
  • A license checker layer checks the plugin id against a table of pluginId : license required and if there is an error responds
  • "Sorry, you don't have the correct license, you can't enter the DOM". (a shared license error component is displayed, with option to buy/upgrade the license).

This check would occur each time the user wants to navigate to the plugin (some sort of client router middleware).

Currently, having this screen is not the best UX, as the user is in the app but can't use it.

screen shot 2019-02-18 at 11 14 50

It seems more a work of the platform team than us, but if we don't agree on that, we could create that license layer just for our apps.

return null;
})(server, pluginId);
};
};

This comment has been minimized.

Copy link
@sebelga

sebelga Feb 18, 2019

Contributor

I don't think we need to call with the arguments as they are in the closure.

This is easier to follow for me

export const licensePreRoutingFactory = (server, pluginId) => {
  const licensePreRouting = () => {
    // License checking and enable/disable logic
    const xpackMainPlugin = server.plugins.xpack_main;
    const licenseCheckResults = xpackMainPlugin.info.feature(pluginId).getLicenseCheckResults();
    if (!licenseCheckResults.isAvailable) {
      const error = new Error(licenseCheckResults.message);
      const statusCode = 403;
      throw wrapCustomError(error, statusCode);
    }

    return null;
  }

  return () => once(licensePreRouting)();
};

This comment has been minimized.

Copy link
@bmcconaghy

bmcconaghy Feb 19, 2019

Contributor

I think using once here is an issue as the license info can change while Kibana is still running. There's some logic to reload the xpackInfo when this happens, and using once this change would not be observed.

This comment has been minimized.

Copy link
@sebelga

sebelga Feb 19, 2019

Contributor

Ah I see @bmcconaghy. This was just a refactor but apparently, it helped to spot this! 😊

This comment has been minimized.

Copy link
@bmcconaghy

bmcconaghy Feb 19, 2019

Contributor

The original did not use once AFAICT.

This comment has been minimized.

Copy link
@sebelga

sebelga Feb 20, 2019

Contributor

Sorry, misunderstood your comment. Indeed the original did not return the licensePreRouting wrapped in once. I was just saying that by refactoring Jen's PR (where she added the once around licensePreRouting) it helped to see the potential bug.

jen-huang added some commits Feb 22, 2019

PR feedback adjustments
* Use minimum license string instead of array of valid license
* Pass plugin name instead of ID for UX
* Return license status instead of `showLinks` and other flags
* Adjust factory closure for readability
@jen-huang

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Hi everyone, thanks for your reviews and feedback on this PR. Following our team discussion and PR feedback, I've made the following changes:

  • Use minimum license string instead of array of valid license. The list of all licenses are stored as an array constant and the license checker uses .splice against it to check validity.
  • Pass plugin name instead of ID for UX based on @cjcenizal's feedback.
  • Return license status instead of showLinks and other flags. The list of statuses are also stored as constants. This makes the shape returned by license checker more flexible, since plugins can use status downstream to determine any UI adjustments. For example, Watcher's conditionals could be changed to licenseService.status === LICENSE_STATUS.INVALID or licenseService.status === LICENSE_STATUS.INACTIVE.
  • Adjust factory closure for readability based on @sebelga's feedback.

What does everyone think of these changes? As always, I welcome additional feedback 😄

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 22, 2019

@sebelga

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

LGTM!

@bmcconaghy
Copy link
Contributor

left a comment

LGTM

@cjcenizal
Copy link
Contributor

left a comment

This is looking really good! I think the changes you've made to the license info response are awesome. Way more informative and self-explanatory. And providing the required minimum license to the license checker makes the code much more readable.

I had a question about the isAvailable property we're still returning and the use of the stats constants on the client. Once those are resolved I will give a 👍.

I also had some suggestions which aren't blockers but maybe you'll find them useful. When developing tools like this I like to also create a minimum example of consumption (like Rollups telemetry in the user actions PR), just so I can put it through its paces and make sure the design works for some basic use cases. Have you considered doing something similar here, by maybe adding a callout to Index Mgmt or License Mgmt to block the UI if the license is expired/invalid? It might give you some ideas on how to improve the ergonomics of the license info payload, or inform decisions on where the constants file should be located. It will also act as a guide for other teams that wish to use these new patterns.

Also, definitely not a blocker, but the license checker tests could be refactored to use Jest, which is something I've seen done with our other mocha tests.

@@ -5,5 +5,7 @@
*/

export const PLUGIN = {
ID: 'index_management'
ID: 'index_management',
NAME: 'Index Management',

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Feb 25, 2019

Contributor

Should the plugin NAME be internationalized for Index and License Mgmt?

'platinum',
'trial',
];
export const LICENSE_STATUS = {

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Feb 25, 2019

Contributor

If we expect these to be consumed on the client, should they be made available in a common directory, instead of server?

it('should set enableLinks to false', () => {
expect(checkLicense(mockLicenseInfo).enableLinks).to.be(false);
it('should set status to unavailable', () => {
expect(checkLicense(pluginName, minimumLicenseRequired, mockLicenseInfo).status).to.be('UNAVAILABLE');

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Feb 25, 2019

Contributor

Since we have the constants file, can we consume that instead of hardcoding these strings?

const { license } = xpackLicenseInfo;
const isLicenseModeValid = license.isOneOf([...RANKED_LICENSE_TYPES].splice(RANKED_LICENSE_TYPES.indexOf(minimumLicenseRequired)));
const isLicenseActive = license.isActive();
const licenseType = license.getType();

// License is not valid
if (!isLicenseModeValid) {
return {
isAvailable: false,

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Feb 25, 2019

Contributor

What does isAvailable mean? Do we need it if we have the UNAVAILABLE status?

export const LICENSE_STATUS = {
UNAVAILABLE: 'UNAVAILABLE',
INVALID: 'INVALID',
INACTIVE: 'INACTIVE',

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Feb 25, 2019

Contributor

What do you think of naming this to EXPIRED instead? An "expired" license is something I've heard of, whereas with "inactive" I always have to think about it for a second to figure out that it means the license is expired.

jen-huang added some commits Feb 25, 2019

Additional PR feedback:
* Move license status constants to `/common`
* Internationalize plugin names that are passed to check license, and its error messages
* Remove `isAvailable` flag, change pre-routing logic to use `LICENSE_STATUS.VALID` instead
* Change constant `INACTIVE` to `EXPIRED`
* Convert check license test from mocha to jest
@jen-huang

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@cjcenizal Thanks for the re-review! I agree with all of your feedback, and have implemented the following changes:

  • Move license status constants to /common
  • Internationalize plugin names that are passed to check license, and its error messages (note: plugin name i18n keys are taken from existing keys used to register the apps into management section)
  • Remove isAvailable flag, change pre-routing logic to use LICENSE_STATUS.VALID instead
  • Change constant INACTIVE to EXPIRED
  • Convert check license test from mocha to jest

The current plan is to use the new snapshot and restore plugin as the guinea pig for UX consumption of this new license checker contract. I hesitate to include changes to Index Management and License Management in this PR, since the goal is primarily bug fixing. Once this PR goes in, we can revisit those apps and continue to iterate on license checker if there are any gaps.

@cjcenizal
Copy link
Contributor

left a comment

Code LGTM! Had a couple minor suggestions that eluded me during the last review.

NAME: i18n.translate('xpack.idxMgmt.appTitle', {
defaultMessage: 'Index Management'
}),
MINIMUM_LICENSE_REQUIRED: 'basic',

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Feb 25, 2019

Contributor

Maybe LICENSE.BASIC should be a constant exported by server/lib/constants?

@@ -29,7 +29,7 @@ export function indexManagement(kibana) {
init: function (server) {
const router = createRouter(server, PLUGIN.ID, '/api/index_management/');
server.expose('addIndexManagementDataEnricher', addIndexManagementDataEnricher);
registerLicenseChecker(server, PLUGIN.ID);
registerLicenseChecker(server, PLUGIN.ID, PLUGIN.NAME, PLUGIN.MINIMUM_LICENSE_REQUIRED);

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Feb 25, 2019

Contributor

Is it necessary to even store NAME and MINIMUM_LICENSE_REQUIRED in the constants file? I just ask because if not then we can get rid of some indirection by turning those into local variables.

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2019

@jen-huang jen-huang merged commit 50c7da3 into elastic:master Feb 26, 2019

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

@jen-huang jen-huang deleted the jen-huang:fix/shared-license-checker branch Feb 26, 2019

jen-huang added a commit to jen-huang/kibana that referenced this pull request Feb 26, 2019

Fix common license checker issues (elastic#31339)
* Fix hardcoded plugin name and license modes in common license checker

* Fix licensePreRoutingFactory scope issues

* PR feedback adjustments

* Use minimum license string instead of array of valid license
* Pass plugin name instead of ID for UX
* Return license status instead of `showLinks` and other flags
* Adjust factory closure for readability

* Fix test

* Additional PR feedback:

* Move license status constants to `/common`
* Internationalize plugin names that are passed to check license, and its error messages
* Remove `isAvailable` flag, change pre-routing logic to use `LICENSE_STATUS.VALID` instead
* Change constant `INACTIVE` to `EXPIRED`
* Convert check license test from mocha to jest

* Fix test

jen-huang added a commit that referenced this pull request Feb 26, 2019

Fix common license checker issues (#31339) (#32047)
* Fix hardcoded plugin name and license modes in common license checker

* Fix licensePreRoutingFactory scope issues

* PR feedback adjustments

* Use minimum license string instead of array of valid license
* Pass plugin name instead of ID for UX
* Return license status instead of `showLinks` and other flags
* Adjust factory closure for readability

* Fix test

* Additional PR feedback:

* Move license status constants to `/common`
* Internationalize plugin names that are passed to check license, and its error messages
* Remove `isAvailable` flag, change pre-routing logic to use `LICENSE_STATUS.VALID` instead
* Change constant `INACTIVE` to `EXPIRED`
* Convert check license test from mocha to jest

* Fix test

alakahakai added a commit to alakahakai/kibana that referenced this pull request Apr 2, 2019

Fix common license checker issues (elastic#31339)
* Fix hardcoded plugin name and license modes in common license checker

* Fix licensePreRoutingFactory scope issues

* PR feedback adjustments

* Use minimum license string instead of array of valid license
* Pass plugin name instead of ID for UX
* Return license status instead of `showLinks` and other flags
* Adjust factory closure for readability

* Fix test

* Additional PR feedback:

* Move license status constants to `/common`
* Internationalize plugin names that are passed to check license, and its error messages
* Remove `isAvailable` flag, change pre-routing logic to use `LICENSE_STATUS.VALID` instead
* Change constant `INACTIVE` to `EXPIRED`
* Convert check license test from mocha to jest

* Fix test

@cjcenizal cjcenizal added the non-issue label May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.