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

Validate premium feature license correctly when cancelled #13064

Merged
merged 9 commits into from Jun 21, 2018

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Jun 14, 2018

So far we did not consider a cancelled subscription as "missing license", only when no license existed at all. Also we now deactivated a plugin when the license is missing. We check the license once per hour. I would cache it for longer but the problem is, if someone renews the license then it would take eg 12 hours to activate the plugin. Also I wanted to initially use the eager cache which would be slightly faster, however the eager cache is being cached for 12 hours...

Tried to write a test for this but it's quite hard.

@tsteur tsteur added the Needs Review PRs that need a code review label Jun 14, 2018
@tsteur tsteur added this to the 3.6.0 milestone Jun 14, 2018
$info = $newPlugin->getInformation();
$hasInternet = Config::getInstance()->General['enable_internet_features'];

if (!empty($info['price']['base']) && $hasInternet) {
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 check this only for premium features, when the feature is activated, and only when the instance has the marketplace (we wouldn't be able to get the Marketplace class otherwise etc) and internet features enabled.

@diosmosis
Copy link
Member

Found an issue while trying to test: if you have a plugin available that isn't installed or activated, and requires special configuration before it can be activated (like having a central database config defined), then the check will result in trying to install this plugin & failing.

Seems to be because Plugins::getPluginInfo() results in loading every plugin, then FrontController calls Plugin\Manager::installLoadedPlugins() resulting in trying to install all of those plugins, even if they aren't supposed to be loaded.

@tsteur
Copy link
Member Author

tsteur commented Jun 19, 2018

Cheers, I'll try to have a look into this some day. Might get quite tricky to workaround it.

@diosmosis
Copy link
Member

Was thinking a possible solution would be to use a scheduled task to check plugin status & then in Plugin\Manager just check the result.

@tsteur
Copy link
Member Author

tsteur commented Jun 19, 2018

Was good you found the issue. I now implemented a new method that only checks the license so we also avoid checking lots of other not needed things like whether a plugin is activated, bogus, requirements, ... The problem was that it tried to execute a plugin update check under circumstances which then executes the loading of all plugins.

@diosmosis
Copy link
Member

My suggestion was just that, a suggestion, I wouldn't take it personally if there was a better one ;)

@tsteur
Copy link
Member Author

tsteur commented Jun 19, 2018

fyi l also disabled the check for developers as it can get slow without having a cache.

@tsteur
Copy link
Member Author

tsteur commented Jun 20, 2018

fyi: the reason I didn't use the task in the first place is that it can get tricky with caching it in tracker mode. eg the tracker would need to set a DB option but this means we would need to cache it in general tracker cache. This in turn means it gets all a bit complicated and am not sure if we would initialize that cache too early before all plugins are loaded etc. In tracker mode with dev enabled or so I'm worried it could even end in an endless loop...

A workaround be to set an option in the task and then cache it within plugin\manager through lazy cache as it is right now but then we could as well keep it simple and maybe not do it through a task?

I will have a think again...

@diosmosis
Copy link
Member

Will tag as Pull Request WIP (feel free to change it back now or whenever it should be reviewed again)

@diosmosis diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Jun 20, 2018
@tsteur
Copy link
Member Author

tsteur commented Jun 21, 2018

@diosmosis I've tried to implement it in a task but it's not really working. The problem is that in the task I would need to get a list of all activated (or loaded) plugins. However, I cannot really get the loaded/activated plugin because it would be not loaded in Plugin\Manager when there is no license.

I would basically need to add a flag to the plugin manager to "ignore" the license check and everything starts getting even more complicated.

I've made a few more minor changes though that make the code a bit more readable and faster.

// should currently load between 10 and 27 plugins
$this->assertLessThan(27, count($this->manager->getLoadedPlugins()));
// should currently load between 10 and 28 plugins
$this->assertLessThan(28, count($this->manager->getLoadedPlugins()));
Copy link
Member Author

Choose a reason for hiding this comment

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

Took me a while to figure out why this increased but that's because Marketplace is now a tracker plugin

@tsteur
Copy link
Member Author

tsteur commented Jun 21, 2018

@diosmosis increased the cache TTL and fixed tests. Setting it to "needs review" again.

@tsteur tsteur added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jun 21, 2018
@diosmosis diosmosis merged commit 90cb7d6 into 3.x-dev Jun 21, 2018
@diosmosis diosmosis deleted the missinglicense branch June 21, 2018 23:47
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…#13064)

* Check license correctly and disable if needed

* make sure instance has internet features

* make sure to verify it in tracker mode

* because the text changed we need to change the translation key

* prevent update check from being executed

* simplify and improve performance

* set caching time to 6 hours and invalidate cache when viewing subscriptions

* fix tests

* update ui screenshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants