diff --git a/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php b/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php index 1029e0a4052..7586c6ede29 100644 --- a/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php +++ b/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php @@ -176,16 +176,20 @@ public function authenticateRequest(HTTPRequest $request) // Renew the token $rememberLoginHash->renew(); - $tokenExpiryDays = RememberLoginHash::config()->uninherited('token_expiry_days'); - Cookie::set( - $this->getTokenCookieName(), - $member->ID . ':' . $rememberLoginHash->getToken(), - $tokenExpiryDays, - null, - null, - false, - true - ); + + // Send the new token to the client if it was changed + if ($rememberLoginHash->getToken()) { + $tokenExpiryDays = RememberLoginHash::config()->uninherited('token_expiry_days'); + Cookie::set( + $this->getTokenCookieName(), + $member->ID . ':' . $rememberLoginHash->getToken(), + $tokenExpiryDays, + null, + null, + false, + true + ); + } // Audit logging hook $member->extend('memberAutoLoggedIn'); diff --git a/src/Security/RememberLoginHash.php b/src/Security/RememberLoginHash.php index 389afcebded..ad58158d061 100644 --- a/src/Security/RememberLoginHash.php +++ b/src/Security/RememberLoginHash.php @@ -80,7 +80,21 @@ class RememberLoginHash extends DataObject private static $force_single_token = false; /** - * The token used for the hash + * If true, the token will be replaced during session renewal. This can cause unexpected + * logouts if the new token does not reach the client (e.g. due to a network error). + * + * This can be disabled as of CMS 5.3, and renewal will be removed entirely in CMS 6. + * + * @config + * + * @var bool + */ + private static $replace_token_during_session_renewal = true; + + /** + * The token used for the hash. Only present during the lifetime of the request + * that generates it, as the hash representation is stored in the database and + * the token itself is sent to the client. */ private $token = null; @@ -190,14 +204,21 @@ public static function generate(Member $member) /** * Generates a new hash for this member but keeps the device ID intact * + * @deprecated 5.3.0 Token renewal will be removed in 6.0.0 * @return RememberLoginHash */ public function renew() { - $hash = $this->getNewHash($this->Member()); - $this->Hash = $hash; + // Only regenerate token if configured to do so + $replaceToken = RememberLoginHash::config()->get('replace_token_during_session_renewal'); + if ($replaceToken) { + $hash = $this->getNewHash($this->Member()); + $this->Hash = $hash; + } + $this->extend('onAfterRenewToken'); $this->write(); + return $this; } diff --git a/tests/php/Security/RememberLoginHashTest.php b/tests/php/Security/RememberLoginHashTest.php index 492ded89bf0..a7fd1b77283 100644 --- a/tests/php/Security/RememberLoginHashTest.php +++ b/tests/php/Security/RememberLoginHashTest.php @@ -95,4 +95,32 @@ public function testGetSetLogoutAcrossDevices() RememberLoginHash::setLogoutAcrossDevices(false); $this->assertFalse(RememberLoginHash::getLogoutAcrossDevices()); } + + public function testRenew() + { + // If session-manager module is installed it expects an active request during renewal + if (class_exists(LoginSession::class)) { + $this->markTestSkipped(); + } + + $member = $this->objFromFixture(Member::class, 'main'); + + // With token replacement enabled, hash and token should change + RememberLoginHash::config()->set('replace_token_during_session_renewal', true); + $hash = RememberLoginHash::generate($member); + $oldToken = $hash->getToken(); + $oldHash = $hash->Hash; + $hash->renew(); + $this->assertNotEquals($oldToken, $hash->getToken()); + $this->assertNotEquals($oldHash, $hash->Hash); + + // With token regeneration disabled, hash should remain the same and token should remain empty + RememberLoginHash::config()->set('replace_token_during_session_renewal', false); + $hash = RememberLoginHash::generate($member); + $hash->setToken(''); + $oldHash = $hash->Hash; + $hash->renew(); + $this->assertEmpty($hash->getToken()); + $this->assertEquals($oldHash, $hash->Hash); + } }