Identity data stays in session if findIdentity() returns null

[mikehaertl]

What steps will reproduce the problem?

1. Login a user
2. Make findIdentity() return null for that user (e.g. disable the user in the database)
3. Load any page that checks the login status (getIsGuest())

What is the expected result?

If findIdentity() returns null the user should be logged out properly and the identity information should be deleted from the session.

What do you get instead?

The user seems to be logged out (i.e. getIsGuest() returns true) but the identity information is still present in the session. The user is redirected to the login page on next request.

If now the user record is re-enabled in the DB the user suddenly is logged in again.

This is an unexpected user experience and can also lead to weird errors on the login page.

Additional info

This is related to #9718 but to me the issue here is more fundamental. It is unrelated to auth_key and the like.

I'm unsure about BC issues but wonder why this was implemented this way. I can't think of a situation where it would make sense to keep the idParam in session if the asssocitated identity is gone/no longer valid and getIsGuest() returns true.

The comment for IdentityInterface::findIdentity() also seems to suggest that this can be used to immediately un-authenticate (read: logout) a logged in user:

    /**
     * ...
     * @return IdentityInterface|null the identity object that matches the given ID.
     * Null should be returned if such an identity cannot be found
     * or the identity is not in an active state (disabled, deleted, etc.)
     */
    public static function findIdentity($id);

Q	A
Yii version	2.0.41
PHP version	7.4.15
Operating system	Debian

[mikehaertl]

My workaround is to use my custom User class and override renewAuthStatus() like this:

    /**
     * @see \yii\web\User
     */
    protected function renewAuthStatus()
    {
        parent::renewAuthStatus();
        if ($this->getIdentity() === null) {
            $this->switchIdentity(null);
        }
    }

I suggest to add this to the implementation of yii\web\User.

[bizley]

This sounds good, at least without digging more into the problem. @samdark ?

[bizley]

Hmm, I can see that renewAuthStatus is setting identity to null when this is returned by findIdentity, and since isGuest is checking for that all should be good, right? What am I missing?

[mikehaertl]

Right, setting identity to null is the one part. But like I said: the identity params are not removed from the session (e.g. you still see __id with the correct user id there). So the user is suddenly logged in again if findIdentity() returns the user record again.

I can't see, why it would make sense to have the id params kept in session if the user is actually seen as guest.

[bizley]

So the problem is more with that and enabling auto login, right? Is setting it to false fixing that use-case?

[mikehaertl]

It has nothing to do with auto login. It's really about proper cleanup if the identity ist not valid anymore. The implementation of getIsGuest() shows, that findIdentity() is the single source of truth, whether a user is authenticated or not.

If the return value of findIdentity() changes to null all authentication data should be completely removed:

1. Remove identity params from session
2. Delete auto login cookie, if enabled

Point 2) already happens, but I think this is rather by accident:

• After findIdentity() returns null, on the next request, it will try to login by cookie
• loginByCookie() will call getIdentityAndDurationFromCookie()
• There the cookie is deleted if findIdentity() returns null

Point 1) remains open.

[bizley]

Ok, I agree with what you are saying here. Would it work if the line I pointed previously was changed to $this->switchIdentity($identity);? There might be a wee overhead with the lines underneath it.

[samdark]

Yes, doing a cleanup is a good idea. Since you're into it, may I ask if you have time for a pull request?

[mikehaertl]

@bizley I think it must be done after the call to loginByCookie() because switchIdentity(null) would delete that cookie and thus break auto login. So probably the end of renewAuthStatus() is a good place to do the check.

@samdark Sure, I can provide a PR and we can discuss things further there.

[bizley]

Closed via c94d704