josegonzalez / cakephp-version

CakePHP3: plugin that facilitates versioned database entities
MIT License
51 stars 22 forks source link

feature suggestion: store the user who made the change in projects that use authentication #6

Closed chris48s closed 8 years ago

chris48s commented 8 years ago

Hi, I recently used this plugin in a project I was working on. We needed database version history, but there was an additional requirement - we needed to record which user made the change. I was able to solve this for my situation by adding a 'user' field to the 'version' table and extending your plugin roughly as shown in this gist: https://gist.github.com/chris48s/0614201e36fe493a2bc9

You will note that I've referenced $_SESSION directly here, which is not ideal. This is because:

  1. The AuthComponent::user() method has been removed in CakePHP3. There is further discussion of this here: https://github.com/cakephp/cakephp/issues/3929
  2. The session object can not be used in a Model Behavior ( http://book.cakephp.org/3.0/en/development/sessions.html#accessing-the-session-object )
  3. We also can not access SessionHelper in a Model Behavior and it is now deprecated ( http://book.cakephp.org/3.0/en/views/helpers/session.html )

so I think there is no abstraction for session that can be used in a Model Behavior.

Nevertheless, I think this is probably a feature that other users would find useful - I can see that recording which user made a change would be a common requirement in this situation - so I've had a think about how to make this a bit more generic so it could be contributed back to the core plugin. In my case, the user info was stored in $_SESSION['Auth']['User']['name'] but if I rewrite the getUserDetails() function to something like:

private function getUserDetails()
{
    if (isset($this->_config['user'])) {
        $keys = $this->_config['user'];
    } else {
        return null;
    }

    $session = $_SESSION;
    foreach ($keys as $key) {
        if (isset($session[$key]) && !empty($session[$key])) {
            $session = $session[$key];
        } else {
            return null;
        }
    }
    $user = $session;

    return $user;
}

then the user can add the behaviour using a call like:

$this->addBehavior('MyVersion', [ 'user' => ['Auth', 'User', 'name'] ]);
$this->addBehavior('MyVersion', [ 'user' => ['Auth', 'User', 'id'] ]);
$this->addBehavior('MyVersion', [ 'user' => ['Auth', 'User', 'LDAP', 'fullName'] ]);

..and so on, depending on the implementation of the user's auth class. Simultaneously we can still call $this->addBehavior('MyVersion'); to avoid breaking backwards compatibility and accommodate projects where authentication is not used.

This would cover the situation where the user's Auth implementation is using the session storage engine, but does not address the case where the memory storage engine is used. So, I guess my questions are:

  1. If I were to put a bit of time into generalising this feature and submit a pull request (including update to documentation, tests, ensuring backwards-compatibility, etc), would you be interested in adding this feature to the plugin?
  2. Can anyone suggest a more elegant way of accessing the user info?
  3. Can anyone think of a way that it would also be possible to also support the memory storage engine?
  4. If not, would it be acceptable for this feature to only support use of the session storage engine?

Cheers

josegonzalez commented 8 years ago

We can probably put at event here that lets you modify the data as necessary - maybe put the data in an ArrayObject first? - and then you can just configure an event handler to deal with adding extra data.

Re: accessing user info, you could implement blame in your app and then have access to the auth data in the model. That can definitely be separate, and doesn't need to depend upon the storage engine I don't think.

chris48s commented 8 years ago

That's a good idea and it also makes the feature much more flexible as the user could handle the event to attach other custom metadata and it does not change the behaviour of the plugin. I've submitted a little PR that implements your suggestion - hopefully this is ok. Perhaps you can see about including this in a future version? If there is further work you would like to see on this, let me know.

I think based on your suggestion I can now come up with a neater abstraction to access the user data in my project.

This seems to have given you a nice idea for a blog post too :)

josegonzalez commented 8 years ago

Closing as there is a PR open. Thanks for including tests as well!