cakephp / authentication

Authentication plugin for CakePHP. Can also be used in PSR7 based applications.
MIT License
115 stars 100 forks source link

SessionAuthenticator `'identify' => true` config does not work #596

Closed asgraf closed 1 year ago

asgraf commented 1 year ago

What i did:

$authenticationService->setConfig('identityClass', User::class);
$authenticationService->loadAuthenticator('Authentication.Session', [
    'fields' => [IdentifierInterface::CREDENTIAL_USERNAME => 'email'],
    'identify' => true,
]);
$authenticationService->loadIdentifier('Authentication.Password', [
    'fields' => [
        IdentifierInterface::CREDENTIAL_USERNAME => 'email',
        IdentifierInterface::CREDENTIAL_PASSWORD => 'password',
    ],
    'passwordHasher' => 'Authentication.Default',
    'resolver' => [
        'className' => 'Authentication.Orm',
        'userModel' => UsersTable::class,
        'finder' => 'session',
    ],
]);

After logging in, i manually modified logged user database row and refreshed page and identity in session did not update.

After investigation i have determined that new and updated identity that was indeed fetched from database but this condition resolves to false and this new identity value is never written to session

ADmad commented 1 year ago

You need to use AuthenticationComponent::setIdentity() to update the identity in session after modifying its database record.

asgraf commented 1 year ago

So you are telling me that 'identify' => true does nothing by design?

ADmad commented 1 year ago

It doesn't do what you think it does. It uses the existing session identity to find matching record in the db. If you change the db record you have to explicitly update the session identity.

markstory commented 1 year ago

To expand on @ADmad 's point. The identify option is useful when you need a way to revoke authentication/session. For example deactivating a user's account for abusive behavior.

asgraf commented 1 year ago

I'm trying to read $identity->fraud boolean field to restrict number of payment options available to logged in user. Like i said in earlier post, since request to database is already done why this value is just discarded. I don't see sense of why i need to make second identical request to database myself to update session. Why not have at least an option to update session with this already fetched from database fresh value?

ADmad commented 1 year ago

Having the option to set the identity refetched from db to the session does make sense.

othercorey commented 1 year ago

Having the option to set the identity refetched from db to the session does make sense.

This is why I don't rely on the identity for anything but id and password.

markstory commented 1 year ago

I agree that updating the session value, or at the very least ensuring that the identity request attribute always reflects the identified user makes sense.

markstory commented 1 year ago

I took a look at the existing code, and it should already be using the newly fetched user as $this->request->getAttribute('identity') or from $authService->getIdentity(). Starting here in AuthenticationService.

https://github.com/cakephp/authentication/blob/52a330dc42832306ce756508b680dea818e56a37/src/AuthenticationService.php#L183-L188

We see the authenticators being used to authenticate the user. When using SessionAuthenticator with identify=true we end up here.

https://github.com/cakephp/authentication/blob/52a330dc42832306ce756508b680dea818e56a37/src/Authenticator/SessionAuthenticator.php#L63-L79

This includes the fresh user fetched from the database. Then in the middleware the service's identity is set to the request attribute:

https://github.com/cakephp/authentication/blob/52a330dc42832306ce756508b680dea818e56a37/src/Middleware/AuthenticationMiddleware.php#L105-L119

How are you accessing a stale user record?

ADmad commented 1 year ago

Seems he might be directly accessing the session instead of getting the identity through the request or the authn service.

markstory commented 1 year ago

Seems he might be directly accessing the session instead of getting the identity through the request or the authn service.

So don't do that is the solution.

asgraf commented 1 year ago

Seems he might be directly accessing the session instead of getting the identity through the request or the authn service.

In code I'm accessing identity only by using AutenticatorComponent, IdentityHelper or by $request->getAttribute('identity') directly (because internal implementation of SessionAuthenticator can change in future) But during development indeed i was looking at DebugKit Toolbar's Session tab to see if values are updating instead of dumping $request->getAttribute('identity') directly

asgraf commented 1 year ago

Maybe i will try to explain what im trying to achieve. Currently you can have identity refreshed every web request or never. Basically i was experimenting with refreshing identity every few minutes and/or before/after some important actions Here are my experiments:

$authenticationService->loadAuthenticator('Authentication.Session', [
    'fields' => [IdentifierInterface::CREDENTIAL_USERNAME => 'email'],
    'identify' => $request->is(['POST', 'PUT', 'PATCH', 'DELETE']),
]);
    //my custom IdentityRefreshComponent, method called in important actions
    public function refreshIdentity(): ResultInterface
    {
        $authenticationComponent = $this->getAuthenticationComponent();
        $identity = $authenticationComponent->getIdentity();
        $authenticator = $authenticationComponent->getAuthenticationService()->getAuthenticationProvider();
        if (
            $identity instanceof IdentityInterface &&
            $authenticator instanceof SessionAuthenticator
        ) {
            if (!($identity instanceof EntityInterface)) {
                throw new InternalErrorException('Identity does not implement EntityInterface');
            }
            $authenticator->setConfig('identify', true);

            $result = $authenticator->authenticate($this->getController()->getRequest());
            if ($result->isValid()) {
                $authenticationComponent->setIdentity($result->getData());
            }
            return $result;
        }

        return new Result(null, ResultInterface::FAILURE_OTHER);
    }
markstory commented 1 year ago

Basically i was experimenting with refreshing identity every few minutes and/or before/after some important actions Here are my experiments:

This sounds like behavior that would require a custom Authenticator that behaves like SessionAuthenticator but also handles periodic refreshes.

asgraf commented 1 year ago

Here are some more code snippets from my experiments Refreshing identity on save:

$eventManager = EventManager::instance();
\assert($eventManager instanceof EventManager);
$eventManager->on(
    'Model.afterSaveCommit',
    function (EventInterface $event, EntityInterface $entity, ArrayObject $options) use ($identity) {
        if (
            $entity instanceof IdentityInterface &&
            $entity::class === $identity::class &&
            $entity->getIdentifier() === $identity->getIdentifier()
        ) {
            $this->IdentityRefresh->refreshIdentity();//source code in my previous comment
        }
    }
);

Refreshing identity every few X minutes


$interval = MINUTE;//example
if ($identity->modified->addSeconds($interval)->isPast()) {
    $table = TableRegistry::getTableLocator()->get($identity->getSource());
    $identity->modified = FrozenTime::now();
    $table->saveOrFail($identity);//this will trigger identity refresh on save
}
github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 120 days with no activity. Remove the stale label or comment or this will be closed in 15 days