yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Login cookie destroyed after password change, following UPGRADE notes #19621

Open MarkoNV opened 2 years ago

MarkoNV commented 2 years ago

After implementing AuthKey check on every request, UPGRADE guide says:

Applications that change the underlying authKey of an authenticated identity, should now call yii\web\User::switchIdentity(), yii\web\User::login() or yii\web\User::logout() to recreate the active session with the new authKey.

First issue is documentation - yii\web\User::logout() will destroy session, not recreate it, so it should be removed from documentation; or updated to say:

Applications that change the underlying authKey of an authenticated identity, should now call yii\web\User::switchIdentity() or yii\web\User::login() to recreate the active session with the new authKey; or should call yii\web\User::logout() to destroy session data and log user out.

Main issue is destroying login cookie when calling yii\web\User::switchIdentity() or yii\web\User::login(). If you call ti without second parameter ($duration) cookie is destroyed. But yii\web\User doesn't have publicly available method to check if there is existing cookie nor to fetch duration from it.

What steps will reproduce the problem?

1.) Setup project to change authKey and call yii\web\User::switchIdentity($userModel) on password change 2.) Login with remember me option 3.) Close browser and reopen it (alternate: delete session cookie and refresh). Are you still logged in? 4.) Change password 5.) Close browser and reopen it (alternate: delete session cookie and refresh). Are you still logged in?

What is the expected result?

Step 3: YES Step 5: YES

What do you get instead?

Step 3: YES Step 5: NO

Additional info

This is detected in 2.0.46, but it's possible that it even predates 2.0.40 because logic is extended to check authKey at any request in 2.0.40 and instruction is added in UPGRADE guide to avoid logout at session level. But check was implemented earlier for cookie and there was no way of renewing cookie login authKey even before.

Proposed solution

Adding dedicated method which extracts current cookie and renews it if necessary is simplest and safest solution:

public function userAuthKeyUpdated($identity) {
    if(!($identity instanceof IdentityInterface)) {
        return;
    }
    /* Optional, see bellow */
    if($this->isGuest) {
        return;
    }
    if($this->identity->getId() !== $identity->getId()) {
        return;
    }
    /* End of optional */
    $value = Yii::$app->getRequest()->getCookies()->getValue($this->identityCookie['name']);
    if ($value !== null) {
        $data = json_decode($value, true);
        if (is_array($data) && count($data) == 3) {
            list($id, $authKey, $duration) = $data;
            if($identity->getId() == $id) {
                return $this->switchIdentity($identity, $duration);
            }
        }
    }
    $this->switchIdentity($identity, 0); 
}

About optional checks: If we don't want to use new method as another login alternative, we can prevent switching identity if it isn't same user or if user isn't logged in (e.g. changing password via password reset). If you think it should be part of application logic, not framework, feel free to skip it.

If requested, I could make pull request, but I don't regularly use github, so it would take some time.

Q A
Yii version 2.0.46
PHP version PHP 8.1
Operating system Win 7
samdark commented 2 years ago

Yes, it will be great to get a pull request. Thanks.

MarkoNV commented 2 years ago

I'm sorry, I stuck with message:

[Exception] Cannot apply patch Fix PHP 7 and 8 compatibility (https://yiisoft.github.io /phpunit-patches/phpunit_mock_objects.patch)!

Could be because my specific multiphp configuration. I could try on linux machine next week.

Do you have any suggestion about method name (if other name will be more in spirit od yii2)?

Should I keep additional checks?

samdark commented 2 years ago

Do you have any suggestion about method name (if other name will be more in spirit od yii2)?

No.

Should I keep additional checks?

Yes.