Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[license] Allow to limit some packages to a specific license plan #4651

Merged
merged 23 commits into from
May 6, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Apr 25, 2022

  • Handle KEYVERSION=2 in verifyLicense
  • Handle both perpetual and subscription sales models
  • Do not accept pro licenses for premium features
  • Change the doc demonstration license to be a v2 license with the premium scope*
  • Check salesModel and scope value validity both on license encoding and decoding to avoid typos

Close #3925

https://deploy-preview-4651--material-ui-x.netlify.app/x/advanced-components/#license-key-installation

@flaviendelangle flaviendelangle requested a review from DanailH April 25, 2022 16:58
@flaviendelangle flaviendelangle self-assigned this Apr 25, 2022
@@ -27,7 +28,11 @@ export function useLicenseVerifier(
return sharedLicenseStatuses[packageName]!.status;
}

const licenseStatus = verifyLicense(releaseInfo, licenseKey);
const acceptedScopes: LicenseScope[] = packageName.includes('premium')
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic would have to become smarter if we release other plans

Copy link
Member

Choose a reason for hiding this comment

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

So to be sure that I understand it - the goal here is to if you have premium you also get pro right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes
I first did a scale const scopes = ['community', 'pro', 'premium'] and then compare the index of the current plan with the one of the license
But it would not work with more granular scopes if we have some in the future
Right now it's a very basic check so both options work fine.

});
describe('key version: 1', () => {
const license =
'0f94d8b65161817ca5d7f7af8ac2f042T1JERVI6TVVJLVN0b3J5Ym9vayxFWFBJUlk9MTY1NDg1ODc1MzU1MCxLRVlWRVJTSU9OPTE=';
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have the v1 generation anymore but it's worth testing it
This key is the doc key

@flaviendelangle flaviendelangle added package: x-license Specific to @mui/x-license. plan: Premium Impact at least one Premium user labels Apr 25, 2022
@flaviendelangle flaviendelangle added this to the Launch Premium Plan milestone Apr 25, 2022
@mui-bot
Copy link

mui-bot commented Apr 25, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 262.1 468.3 375.6 357.78 72.723
Sort 100k rows ms 447.3 865 651.4 661.42 133.248
Select 100k rows ms 111 195.3 161.5 156.74 34.419
Deselect 100k rows ms 105.7 208 164.7 162.44 39.975

Generated by 🚫 dangerJS against 336bd21

@@ -0,0 +1 @@
export type LicenseScope = 'pro' | 'premium';
Copy link
Member

Choose a reason for hiding this comment

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

To be honest I was thinking to rework this to an enum because we are also using the strings themselves and it would be nice to avoid typos. It will also help if we have more licenses in the future. What do you think?

Copy link
Member Author

@flaviendelangle flaviendelangle Apr 27, 2022

Choose a reason for hiding this comment

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

Not sure

Here we type the value, so we are sure that TS will complain if we put scope = "pri", just like for the enum.
And it will be obvious that a change to the type is breaking.

But if we use an enum, the typing is on the key of the enum, not its values.

So if by mistake we switch from

enum LicenseScope {
  pro = 'pro',
  premium = 'premium'
}

To

enum LicenseScope {
  pro, // defaults to 0
  premium, // defaults to 1
}

Then all our code will still work, but we will have done a breaking change for the licenses generated.


With that being said, I think both are viable as long as we follow the rule that an enum whose value can be stringified in any way must have its keys equal to its values.
In my previous company we had a ton of them on the plan generator specification and it never caused any issue.

@flaviendelangle flaviendelangle requested a review from DanailH April 27, 2022 15:49
@mbrookes
Copy link
Member

We need to be able to distinguish between Pro perpetual and Pro subscription, and display the appropriate console message/watermark when the license is expiring soon/expired. (Existing perpetual customers will be able to renew their perpetual license.)

@flaviendelangle
Copy link
Member Author

We need to be able to distinguish between Pro perpetual and Pro subscription, and display the appropriate console message/watermark when the license is expiring soon/expired. (Existing perpetual customers will be able to renew their perpetual license.)

@DanailH that's information we currently don't have I think

@DanailH
Copy link
Member

DanailH commented Apr 28, 2022

Nope, we don't have that info in the license. When I was updating it I didn't know what this was a requirement.

@flaviendelangle
Copy link
Member Author

Me neither @mbrookes, here is the current v2 format:

ORDER=${orderNumber},EXPIRY=${expiryTimestamp},KEYVERSION=2,SCOPE=${scope}`

We would need to add a TIMEOUT_STRATEGY: 'perpetual' | 'annual' (if you have a shorter key name that would be great @DanailH)
If there are other information we need to add ?

Do we switch the TIMEOUT_STRATEGY to annual for all new licenses after the next release ?

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented Apr 28, 2022

Do we switch the TIMEOUT_STRATEGY to annual for all new licenses after the next release ?

No, I thought that'd be the case, and then we could simply use the expiryDate. But renewals from perpetual plans will remain perpetual as well. So we'll need an extra attribute for those cases.

@flaviendelangle
Copy link
Member Author

But renewals from perpetual plans will remain perpetual as well. So we'll need an extra attribute for those cases.

Yes, otherwise we could have said "if license creation date > XXX then if's perpetual".

@DanailH
Copy link
Member

DanailH commented Apr 28, 2022

How about DURATION='perpetual' | 'annual' instead of TIMEOUT_STRATEGY?

@mbrookes
Copy link
Member

How about DURATION='perpetual' | 'annual' instead of TIMEOUT_STRATEGY?

PLAN = 'perpetual' | 'subscription'

What's SCOPE=${scope} ?

@flaviendelangle
Copy link
Member Author

PLAN = 'perpetual' | 'subscription'

We use the term "plan" to differentiate "pro plan" and "premium plan" so I would not use it to describe the duration during which the license is valid.

What's SCOPE=${scope} ?

SCOPE currently contains the plan
I think @DanailH and @oliviertassinari went for scope instead of plan in the license to be more abstract and be able to add scopes that are not plans (maybe an additional license for one specific very high value data grid plugin).

@mbrookes
Copy link
Member

mbrookes commented Apr 28, 2022

TERM = 'perpetual' | 'subscription' ?

@mbrookes
Copy link
Member

The alternative is that we continue to use the v1 key format for perpetual licenses.

@flaviendelangle
Copy link
Member Author

The alternative is that we continue to use the v1 key format for perpetual licenses.

We dropped the algo so we would need to re-add it
But that would work yes

@DanailH @oliviertassinari what's your opinion here ?

@DanailH
Copy link
Member

DanailH commented Apr 28, 2022

I think it would be better to just use the new version. I guess even if we use v1 for perpetual licenses we would still have the problem of not knowing the SCOPE unless the Premium plan can't be perpetual (initially it was PLAN but as we can have addons in the future we thought that SCOPE is more appropriate).

@DanailH
Copy link
Member

DanailH commented May 2, 2022

Ok, breaking is probably not the correct term but rather if we do it like this then you should also update the Pro product on the store. With the other approach, one only needs to handle the Premium product in the store.

@flaviendelangle
Copy link
Member Author

The new pro licenses will be perpetual ? I thought they were going to be annual just like the premium licenses.

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented May 2, 2022

What about
PERPETUITY = 'production' | 'development' ?
After all, licenses will still be perpetual, but in production only.
And we'll need to "advertise" that (Not to say that this is an argument per se).

We may also consider, in the future, removing the console warning from test environments (and leaving the watermark). We have complaints on zendesk about the annoying volume of log messages related to the license.
In this case, we could later expand the implementation with:
PERPETUITY = 'production' | 'development' | 'test'

We might not need to expand it though.*

@joserodolfofreitas
Copy link
Member

The new pro licenses will be perpetual ? I thought they were going to be annual just like the premium licenses.

New Pro licenses generated for a previous customer will still be perpetual in development. But for new customers, it will be perpetual in production only.

@flaviendelangle
Copy link
Member Author

New Pro licenses generated for a previous customer will still be perpetual in development. But for new customers, it will be perpetual in production only.

Can we call the "annual in dev but perpetual in prod" just "annual" for simplicity ?

@flaviendelangle
Copy link
Member Author

PERPETUITY = 'production' | 'development'
PERPETUITY = 'production' | 'development' | 'test'

Not a huge fan tbh
If we set `PERPETUITY=test", it's not obvious if it means "test" + "development" or just "test".
I would prefer to keep the abstraction and let the code choose on which env to do the check rather than trying to describe the env on the license itself.

And if we decide to remove the warning on test, I guess it should be retroactive, not just for the new licenses. Otherwise we would basically be saying to the user complaining "ok you are right, you will get the fix in one year".

@mbrookes
Copy link
Member

mbrookes commented May 4, 2022

PERPETUITY = 'production' | 'development'
PERPETUITY = 'production' | 'development' | 'test'`

Not a huge fan tbh

Me neither, sorry, just for the name alone.

Can we call the "annual in dev but perpetual in prod" just "annual" for simplicity ?

That would be "subscription in dev, perpetual in prod", and its proposed to just call it "subscription", so sounds like that's agreed.

@joserodolfofreitas
Copy link
Member

PERPETUITY = 'production' | 'development'
PERPETUITY = 'production' | 'development' | 'test'`

Not a huge fan tbh

Me neither, sorry, just for the name alone.

Ok, np. But I think "term" is not really exact either. I think it's going to be confused with the "License Term" we'll use to define how many years the license will be valid.

License Term means the period of time during which Licensee is authorized to use Program(s) in accordance with the applicable license grant.

@flaviendelangle
Copy link
Member Author

VALIDITY_DURATION ? VD in the license

@joserodolfofreitas
Copy link
Member

joserodolfofreitas commented May 4, 2022

Validity and duration kinda mix two concepts, but I like the idea of using very explicit keys.

But I'm pondering, and model seems indeed the best name that we could use for this situation.

If you think model is not clear, what about:
SALES_MODEL: 'perpetual' | 'subscription'
TERM: number (int) ?

p.s.: TERM doesn't need to be accounted for on the key generation as it will receive a correct expiry date based on the license term picked in the UI

@flaviendelangle
Copy link
Member Author

SALES_MODEL is fine for me

@mbrookes
Copy link
Member

mbrookes commented May 4, 2022

@flaviendelangle

model is not very explicit to me

It's "licensing model". I used "model" for short as we were looking for short words.

https://www.10duke.com/software-licensing-models/
https://cpl.thalesgroup.com/software-monetization/software-license-models
https://www.google.com/search?hl=en&q=licensing%20model

Now that we're using initials instead, it isn't a problem.

SALES_MODEL is fine for me

A sales model is something different.

@flaviendelangle
Copy link
Member Author

I will name it licensing model / lm, has been approved by the team during the product meeting 👍

@flaviendelangle flaviendelangle merged commit 1748bd3 into mui:master May 6, 2022
@flaviendelangle flaviendelangle deleted the license-plan-check branch May 6, 2022 08:44
throw new Error('MUI: Invalid sales model');
}

return `O=${details.orderNumber},E=${details.expiryDate.getTime()},S=${
Copy link
Member

@oliviertassinari oliviertassinari May 15, 2022

Choose a reason for hiding this comment

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

Without this change, a v2 license key is not usable by < v5.11.0 of the software. It's not backward compatible. It surfaced in https://groups.google.com/a/mui.com/g/x/c/Nkq2fQbGkn0/. Time will tell if this causes trouble with more users, I suspect it will. If the intent is to shorten the string key, I doubt it's worth the breaking change. If a breaking change needs to happen, then I think bundling with #4892 could be great.

How about?

  1. We revert the breaking change:
Suggested change
return `O=${details.orderNumber},E=${details.expiryDate.getTime()},S=${
return `O=${details.orderNumber},EXPIRY=${details.expiryDate.getTime()},S=${
  1. We add support for both E and EXPIRY as license key prefix
  2. We add support for [x-license] Use a simpler checksum for the license key #4892
  3. We release and wait 6 months.
  4. We replace EXPIRY -> E, md5 -> simpler checksum.

Copy link
Member

Choose a reason for hiding this comment

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

@joserodolfofreitas we are one week in, how are the upgrades support cases going? I haven't seen new ones, so maybe it's fine.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
package: x-license Specific to @mui/x-license. plan: Premium Impact at least one Premium user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[license] Update license key manager to accommodate Premium plan.
6 participants