Skip to content

Conversation

@fdel-car
Copy link
Contributor

@fdel-car fdel-car commented Dec 11, 2022

What does it do?

It introduces an online verification of the license, which consists of the following:

  • Pretty early during the startup - whenever the strapi.EE is read, the local license is verified to ensure that Strapi generated it, then the project is optimistically considered licensed.
    This is needed as the license state is used to determine which admin panel version to build.
  • Later, if applicable, during "bootstrap" the license is updated online to get the latest data. The data is stored in the database as a potential fallback and a way to avoid too many unnecessary requests for horizontally scaled projects.
    Also, the content of the license is checked to enable the right features and verify the expiration date. At this step, the project can switch back to the community edition.

@alexandrebodin introduced a wrapper and a middleware to ensure that EE features could not be consumed on an unlicensed project even if the EE admin panel was used.

Why is it needed?

The offline system that we are currently using has its limitations.
For instance, a user has to restart the app after updating the project's license whenever the feature he/she has access to changes since the content of the license would be outdated.

How to test it?

You can make sure that you're project is licensed when using a valid license and that only one outbound call should be made, even when starting your project across multiple processes.

It should be working on every supported database, SQLite doesn't support the FOR UPDATE syntax, so when multiple processes are started simultaneously, multiple requests will be sent.
It's not a big deal, as SQLite should be used in production rarely.

What's missing?

  • Deploy the changes required on the license registry
  • Make sure the locking mechanism leveraged in the database for the license works as expected. I noticed something with SQLite.
  • Schedule a cron job that will be responsible for periodically updating the license, as well as verifying its content to update the project if needed.

This PR was co-authored with @alexandrebodin.

@fdel-car fdel-car added the pr: feature This PR adds a new feature label Dec 11, 2022
@fdel-car fdel-car self-assigned this Dec 11, 2022
@codecov
Copy link

codecov bot commented Dec 11, 2022

Codecov Report

Base: 61.68% // Head: 61.60% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (a0b344e) compared to base (de2e6f2).
Patch coverage: 28.81% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15147      +/-   ##
==========================================
- Coverage   61.68%   61.60%   -0.08%     
==========================================
  Files        1365     1369       +4     
  Lines       33729    33808      +79     
  Branches     6472     6501      +29     
==========================================
+ Hits        20807    20829      +22     
- Misses      11107    11149      +42     
- Partials     1815     1830      +15     
Flag Coverage Δ
back 51.98% <28.97%> (-0.18%) ⬇️
front 66.11% <0.00%> (ø)
unit_back 51.98% <28.97%> (-0.18%) ⬇️
unit_front 66.11% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core/admin/admin/src/index.js 0.00% <0.00%> (ø)
packages/core/admin/ee/server/services/passport.js 100.00% <ø> (ø)
packages/core/admin/server/controllers/admin.js 36.78% <0.00%> (ø)
packages/core/strapi/lib/services/metrics/index.js 89.28% <0.00%> (+26.54%) ⬆️
packages/core/strapi/ee/index.js 21.68% <21.68%> (ø)
packages/core/strapi/ee/license.js 23.52% <23.52%> (ø)
packages/core/strapi/lib/utils/cron.js 29.16% <29.16%> (ø)
...ages/core/admin/ee/server/services/passport/sso.js 87.50% <75.00%> (-12.50%) ⬇️
...ckages/core/admin/ee/server/services/audit-logs.js 79.31% <100.00%> (+0.73%) ⬆️
packages/core/strapi/ee/ee-store.js 100.00% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fdel-car fdel-car marked this pull request as ready for review December 12, 2022 19:55
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

Reading the code I see a big issue: we can't switch from EE to CE at runtime and that's what this PR is assuming.
We need to stop the start of the app at the 1st online check if the license is expired the user will have to clear their env or licnese file to start in CE.

After normal startup when the app is running we need to do something as the app can't just switch to CE. What was specified for this?

@fdel-car fdel-car added the source: core:strapi Source is core/strapi package label Jan 16, 2023
@fdel-car fdel-car force-pushed the chore/online-license-skeleton branch from d0004ab to 37145fd Compare January 16, 2023 11:10
@fdel-car
Copy link
Contributor Author

I would like to point out some potential discrepancies.

As of today, the permissions are registered only during startup. This includes permissions related to EE features.
As such, whenever the project switches to CE or EE, some permissions could still be listed or missing as pointed out by this comment.

It's something we can tackle later, or we would need to rework the way the permissions are registered and cleared now if it's blocking us from merging this PR.

@fdel-car fdel-car requested a review from Bassel17 January 20, 2023 17:35
@fdel-car fdel-car force-pushed the chore/online-license-skeleton branch from a476c6d to ecc4551 Compare January 23, 2023 10:39
Bassel17
Bassel17 previously approved these changes Jan 23, 2023
Copy link
Member

@Bassel17 Bassel17 left a comment

Choose a reason for hiding this comment

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

LGTM! nicely done

@fdel-car fdel-car force-pushed the chore/online-license-skeleton branch from ecc4551 to 8a6a81d Compare January 25, 2023 11:59
@Convly Convly self-requested a review January 25, 2023 13:48
@remidej
Copy link
Contributor

remidej commented Jan 25, 2023

FYI the diff currently includes the entire audit logs feature, because #15536 is not merged yet

@fdel-car fdel-car force-pushed the chore/online-license-skeleton branch from f4056ae to c8441a7 Compare January 25, 2023 14:55
Convly
Convly previously approved these changes Jan 25, 2023
@Convly Convly added this to the 4.6.0 milestone Jan 25, 2023
@fdel-car fdel-car force-pushed the chore/online-license-skeleton branch from b690a9b to a4c0d24 Compare January 25, 2023 18:49
@fdel-car fdel-car force-pushed the chore/online-license-skeleton branch from a4c0d24 to a0b344e Compare January 25, 2023 18:52
Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

I haven't reviewed all the diff, but the changes related to audit logs look good to me ✅

@Convly Convly merged commit b23bba9 into main Jan 25, 2023
@Convly Convly deleted the chore/online-license-skeleton branch January 25, 2023 19:37
@strapi-bot
Copy link

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/multiple-update-queries-are-triggered-for-strapi-core-store-settings-on-application-start/29655/8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: feature This PR adds a new feature source: core:strapi Source is core/strapi package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants