getgrav / grav-plugin-login

Grav Login Plugin
http://getgrav.org
MIT License
44 stars 54 forks source link

User Accounts cache #252

Closed garytube closed 4 years ago

garytube commented 4 years ago

Grav v1.6.11 - Admin v1.9.7

The Admin User account does not reflect the the permissions set in [USER].yaml file. Seams to be cache related. Is this indented? It would be greate if I can update User Accounts without manually clearing the cache.

image

rhukster commented 4 years ago

The current user is loaded into the session, so any changes made will not show up until they log out and back in. This is probably something that could be improved in the future.

Bennycopter commented 4 years ago

Here's a hack I wrote to get around this. It's not going to work if you're using flex users. Also, I'm still trying to understand Grav, so apologies in advance if this raises some eyebrows.

In the login plugin, open login.php, and find the code block at line 113, which starts like this:

// Define current user service.
$this->grav['user'] = function (Grav $c) {
    $session = $c['session'];

and then after the $session = $c['session']; line, add this:

$username = isset($session->user) ? $session->user["username"] : "";
if ($username !== "") {

    // Find the user file
    $filename = 'account://' . $username . YAML_EXT;
    $path = $c["locator"]->findResource($filename);
    $file = \Grav\Common\File\CompiledYamlFile::instance($path);

    if ($file->exists()) {

        // (Re)Load the user file from disk/cache, not from session
        $file->load();

        // Update the session user details from the user file
        $user = $c["accounts"]->load($username);
        $session->user->update($user->toArray());
    }
    else {

        // The user file no longer exists, so let's log out
        unset($session->user);
    }
}
mahagr commented 4 years ago

This won't work either if the user is coming from external service such as LDAP (where users are not stored locally). We probably need an event to handle this so that every plugin integration can handle this in their own way.

Bennycopter commented 4 years ago

I would love to see this feature be implemented, perhaps via a cache_current_user: false config option.

hughbris commented 4 years ago

Thanks for your workaround @Bennycopter. To minimise technical debt, at first I tried to adapt your code into the only part of my application where this was causing a problem. However, I seemed unable to access the session user object.

On discord, @mahagr suggested logging the user out and then in again. Doing this manually certainly resets the data cache. This seemed a relatively harmless option for this one application event, but I couldn't easily figure out how to access a Login object as instantiated in the login plugin. Probably just being a dumbass.

I had a fork of the login plugin anyway, so making your modification, the next best option, was somewhat sustainable. I'll unfortunately need to rebase my fork frequently with a security related plugin like this. Also I can't use another storage backend, as mentioned. Those are the downsides.

Thanks again, just thought you'd appreciate an update and some validation for your effort :)

hughbris commented 4 years ago

I would love to see this feature be implemented, perhaps via a cache_current_user: false config option.

As well as a global option/flag, it would be sweet to allow Grav coders to toggle this behavior per request, so something like a flag parameter on User->load() that overrides the global setting would achieve that. (Default to current behaviour to avoid breaking things, of course.)

mahagr commented 4 years ago

Well that you cannot do. As the session is shared between requests, it's always stored to the session in a single way and there's nothing we can do to change it.

mahagr commented 4 years ago

Should now be fixed for the next login version, though you need to enable the Sync User in Session setting in order to make it to work. It also only works in Grav 1.7 because of some limitations in the older implementation.