getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.27k stars 167 forks source link

User model is not loaded for already cached users #2902

Closed PaulRaschke closed 3 years ago

PaulRaschke commented 3 years ago

Describe the bug If somehow the caching of existing kirby users or the currently logged in user is triggered, e.g. when calling kirby()->user() in a plugin, all definitions of userModels in all plugins running afterwards have no effect.

For example, this plugin code won't work:

<?php

use Kirby\Cms\App as Kirby;
use Kirby\Cms\User;
use Kirby\Cms\Users;

class PluginRoleUser extends User {};

kirby()->user();

Kirby::plugin('my/rolePlugin', [
    'blueprints' => [
        'users/pluginRole' => [
            'name' => 'pluginRole'
        ]
    ],
    'userModels' => [
        'pluginRole' => 'PluginRoleUser'
    ],
]);

Calling kirby()->user() e.g. in a template returns an instance of Kirby\Cms\User instead of PluginRoleUser.

As far as I have correctly traced the cause of the bug, these should be the mainly problematic code parts:

To Reproduce Steps to reproduce the behavior:

  1. Login with an existing user
  2. Call kirby()->user() in code
  3. Register an user model for the user-specific role in code that will be executed afterwards
  4. Get the currently logged in user by calling kirby()->user() which will still be an instance of Kirby\Cms\User

Expected behavior The user model should be updated correctly after it has been defined by a plugin. In this case the user should be an instance of PluginRoleUser.

Workaround A quick workaround to fix this issue is to manually reset the cached values with these two lines of code:

kirby()->users()->data(Users::load(kirby()->root('accounts'), ['kirby' => kirby()])->data());
if(($user = kirby()->user()) !== null) kirby()->auth()->setUser(kirby()->user($user->id()));

These two plugin files will work correctly:

<?php

use Kirby\Cms\App as Kirby;
use Kirby\Cms\User;
use Kirby\Cms\Users;

class PluginRoleUser extends User {};

kirby()->user();

Kirby::plugin('my/rolePlugin', [
    'blueprints' => [
        'users/pluginRole' => [
            'name' => 'pluginRole'
        ]
    ],
    'userModels' => [
        'pluginRole' => 'PluginRoleUser'
    ],
    'hooks' => [
        'system.loadPlugins:after' => function() {
            kirby()->users()->data(Users::load(kirby()->root('accounts'), ['kirby' => kirby()])->data());
            if(($user = kirby()->user()) !== null) kirby()->auth()->setUser(kirby()->user($user->id()));
        }
    ]
]);

or

<?php

use Kirby\Cms\App as Kirby;
use Kirby\Cms\User;
use Kirby\Cms\Users;

class PluginRoleUser extends User {};

kirby()->extend([
    'blueprints' => [
        'users/pluginRole' => [
            'name' => 'pluginRole'
        ]
    ],
    'userModels' => [
        'pluginRole' => 'PluginRoleUser'
    ],
]);

kirby()->users()->data(Users::load(kirby()->root('accounts'), ['kirby' => kirby()])->data());
if(($user = kirby()->user()) !== null) kirby()->auth()->setUser(kirby()->user($user->id()));

Kirby Version 3.4.4

Additional context Probably connected to https://github.com/getkirby/kirby/issues/1892#issuecomment-516403797

distantnative commented 3 years ago

Tried to wrap my head around solutions, but checking on each user call if the model applied. is still the correct doesn't seem feasible.

What maybe could work (without haven't tried it in code), is that when registering new user models from plugins, we check if a user object is already cached and if that user object should be built on the model we are registering at the moment instead. And if so make the switch.

lukasbestle commented 3 years ago

@PaulRaschke What is your use-case for getting the current user in your plugin's index.php outside of the system.loadPlugins:after hook? Do the objects you register from your plugin depend on the current user?

PaulRaschke commented 3 years ago

I have developed a proof of concept for a potential plugin that would allow more flexible permissions (see my forum post). Since this was some time ago, I don't remember everything in detail. If I recall correctly, I had another plugin that patched the panel blueprint definitions to enable user role specific blueprints. This plugin accessed the currently logged in user outside of the system.loadPlugins:after hook, as the blueprints had to be generated before the permissions plugin accessed them to create page models. I hope I was able to describe the rather complex problem chain in a comprehensible way... With the workaround everything functioned correctly.

lukasbestle commented 3 years ago

@PaulRaschke You can still register blueprints from the system.loadPlugins:after hook with $kirby->extendBlueprints(). The same applies to all other plugin extensions.

I'm adding the "unsolvable" label to this issue because it's very hard to implement access to Kirby objects from plugin index.php files correctly. This is currently unsupported, so if we can find a different solution for your use-case like the solution I described above, that would be much more solid in every way.

PaulRaschke commented 3 years ago

I don't think that would work. I used the Kirby::plugin() function to register the blueprints and assume it yields the same result as the undocumented $kirby->extendBlueprints() function, correct? The central problem was a race condition between the two plugins when initializing them in the system.loadPlugins:after hook, since the permissions plugin depended on the registered blueprints. I haven't found an alternative sophisticated implementation that guarantees that all blueprints were registered correctly before the initialization of the permissions plugin. I assume the best solution would probably be to not rely on the blueprints (and glob(kirby()->root('templates') . '/*.php')) for auto-generating page models.

That being said, my primary intention behind opening this issue was to inform you of what I consider to be buggy code and to provide a workaround for anyone facing a similar problem who stumbles across this issue. Because it's quite difficult to debug.

If additional information is needed, I am happy to provide it :)

lukasbestle commented 3 years ago

I used the Kirby::plugin() function to register the blueprints and assume it yields the same result as the undocumented $kirby->extendBlueprints() function, correct?

Yep, the $kirby->extend*() methods are called internally after a plugin was registered.

You can find a code example for the $kirby->extend() method in the reference. I realized the extendBlueprints() method is protected and internal, better use the public extend() method as in the example code.

The central problem was a race condition between the two plugins

Which are the two plugins? If they are not public, could you maybe created a reduced test case (two simple plugins that do what you need and allow to reproduce the issue)?

I assume the best solution would probably be to not rely on the blueprints (and glob(kirby()->root('templates') . '/*.php')) for auto-generating page models.

Do you mean in Kirby or in your plugin(s)?

PaulRaschke commented 3 years ago

Which are the two plugins? If they are not public, could you maybe created a reduced test case (two simple plugins that do what you need and allow to reproduce the issue)?

I don't think more concrete code examples could illustrate the problem any clearer than the provided code snippet from my original post. Therefore, I will try to describe the implementations of the plugins and the reasons for certain architectural decisions in a (hopefully) comprehensible way.

Originally, I wanted both plugins to finish registering before other plugins would execute their routines in the system.loadPlugins:after hook, because I considered it possible that other plugins might rely on user permissions or blueprints, and therefore on one or both plugins. By registering them outside the hook and thus beforehand, possible race conditions could be prevented.

The role-blueprints plugin determined the role ID of the logged in user (and therefore had to call kirby()->user()) and chose an appropriate set of page blueprints from all available blueprints. So, for example, if a user logged in with the role editor, the blueprint default.editor.yml was selected instead of default.yml.

The advanced-permissions plugin should enable the definition of more granular (user) permissions. Internally it worked by automatically registering user models for all existing user roles so that they extend the plugin's AdvancedPermissionsUser class (in my initial post I called it PluginRoleUser). The AdvancedPermissionsUser class overrides methods of the Kirby\Cms\User class, injecting a custom permissions system. Page models were registered in a similar way. If I remember correctly, the idea was to enable more granular permissions by allowing the usage of kirby queries when defining permissions in user and/or page blueprints in addition to the existing binary true or false options (see the other reason for relying on the page blueprints below). Therefore, it relied on the role-blueprints plugin, which forced me to move the initialization of the advanced-permissions plugin inside the system.loadPlugins:after hook (which went against my original intention to initialize it before the hook) and use the kirby()->extend() method to avoid any potential race condition between the two plugins. But with the role-blueprints plugin reliably initializing before the advanced-permissions plugin and calling kirby()->user() in this process, which triggers the caching of the user model, it was (without the workaround used to flush the cache) impossible to set the user model to the AdvancedPermissionsUser class.

Additional Info: My suggested workaround doesn't seem to work with newer kirby versions. It didn't work with v3.5.6, but correctly flushed the cached user model with v3.4.4. I didn't test it with the version in between though. The problem remained with the newest version.

Do you mean in Kirby or in your plugin(s)?

I was referring to the permissions plugin. Sry for not making this clear. I used glob(kirby()->root('templates') . '/*.php') (in addition to kirby()->blueprints('pages')) in the advanced-permissions plugin to determine the available page templates and set the page models for all of them to the page model defined by the plugin in order to enable the more advanced permissions handling.

lukasbestle commented 3 years ago

Thanks for the additional insights.

If I understand you correctly, accessing kirby()->user() directly from the plugins' index.php is only necessary to prevent race conditions during plugin registration. Because all other features for themselves should be doable from the system.loadPlugins:after hook.

A possible workaround could be a helper plugin like zzz-initialize with a system.loadPlugins:after hook. As Kirby loads plugins in alphabetical order, this hook should always be called last and can then call functions in other plugins in the correct order to stitch the plugins together.

Apart from that, I don't think there is a good solution for use-cases like yours at the moment. To be honest I still think the issue is quite unsolvable, sorry. :(

PaulRaschke commented 3 years ago

Thank you for your time and the suggestion for a workaround utilizing the system.loadPlugins:after hook. If I ever stumble across this issue again in the future, I will definitely give it a try!

I completely understand why you consider a solution in the kirby core not really feasible at the moment, and therefore classify the issue as unsolvable. Just a small suggestion (please bear in mind that I have very limited knowledge of the inner workings of kirby): Maybe a more sophisticated solution would be to have multiple system.* hooks so that plugins have more flexibility, based on the lifecycle stages of kirby.

lukasbestle commented 3 years ago

Do you mean additional hooks that fire at different times during Kirby's initialization? We thought about this before, but it's hard to find good points that are generally useful. Triggering a hook any earlier is not really possible (otherwise it won't fire in plugins as they have to be registered first). And pretty much directly after the system.loadPlugins:after hook, Kirby's router is executed. So hooks at different times would either not be triggered correctly or they would be triggered after Kirby is already fully loaded. This leaves just very specific hooks that would be triggered during rendering etc.

If you see a use case for a new hook that isn't yet covered, feel free to open a new issue at any time. Until then I'm closing this issue as explained in the previous posts.