From 724e813b8fe0367601b918276fbf376855967983 Mon Sep 17 00:00:00 2001 From: Garion Herman Date: Thu, 4 Jul 2024 08:15:05 +0900 Subject: [PATCH] API Make token regeneration optional during autologin session renewal Resolves #11281. Renewing the token/hash during an active session can trigger a logout in the event of request failures or simultaneous requests. This also marks the renew method as deprecated, to be removed entirely in 6.0. --- .../CookieAuthenticationHandler.php | 27 +++++++------ src/Security/RememberLoginHash.php | 28 +++++++++++-- tests/php/Security/RememberLoginHashTest.php | 39 +++++++++++++++++++ 3 files changed, 79 insertions(+), 15 deletions(-) diff --git a/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php b/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php index 1029e0a4052..8373ba77242 100644 --- a/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php +++ b/src/Security/MemberAuthenticator/CookieAuthenticationHandler.php @@ -10,6 +10,7 @@ use SilverStripe\Security\Member; use SilverStripe\Security\RememberLoginHash; use SilverStripe\Security\Security; +use SilverStripe\Dev\Deprecation; /** * Authenticate a member passed on a session cookie @@ -175,17 +176,21 @@ 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 - ); + Deprecation::withNoReplacement(fn() => $rememberLoginHash->renew()); + + // 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..8af22057353 100644 --- a/src/Security/RememberLoginHash.php +++ b/src/Security/RememberLoginHash.php @@ -6,6 +6,7 @@ use DateTime; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\FieldType\DBDatetime; +use SilverStripe\Dev\Deprecation; /** * Persists a token associated with a device for users who opted for the "Remember Me" @@ -80,7 +81,18 @@ 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. + * @deprecated 5.3.0 Will be removed without equivalent functionality + */ + private static bool $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 +202,22 @@ public static function generate(Member $member) /** * Generates a new hash for this member but keeps the device ID intact * + * @deprecated 5.3.0 Will be removed without equivalent functionality * @return RememberLoginHash */ public function renew() { - $hash = $this->getNewHash($this->Member()); - $this->Hash = $hash; - $this->extend('onAfterRenewToken'); + // Only regenerate token if configured to do so + Deprecation::notice('5.3.0', 'Will be removed without equivalent functionality'); + $replaceToken = RememberLoginHash::config()->get('replace_token_during_session_renewal'); + if ($replaceToken) { + $hash = $this->getNewHash($this->Member()); + $this->Hash = $hash; + } + + $this->extend('onAfterRenewToken', $replaceToken); $this->write(); + return $this; } diff --git a/tests/php/Security/RememberLoginHashTest.php b/tests/php/Security/RememberLoginHashTest.php index 492ded89bf0..b62b6819295 100644 --- a/tests/php/Security/RememberLoginHashTest.php +++ b/tests/php/Security/RememberLoginHashTest.php @@ -6,6 +6,7 @@ use SilverStripe\Security\Member; use SilverStripe\Security\RememberLoginHash; use SilverStripe\SessionManager\Models\LoginSession; +use SilverStripe\Dev\Deprecation; class RememberLoginHashTest extends SapphireTest { @@ -95,4 +96,42 @@ public function testGetSetLogoutAcrossDevices() RememberLoginHash::setLogoutAcrossDevices(false); $this->assertFalse(RememberLoginHash::getLogoutAcrossDevices()); } + + /** + * @dataProvider provideRenew + * @param bool $replaceToken + */ + public function testRenew($replaceToken) + { + // 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'); + + RememberLoginHash::config()->set('replace_token_during_session_renewal', $replaceToken); + + $hash = RememberLoginHash::generate($member); + $oldToken = $hash->getToken(); + $oldHash = $hash->Hash; + + Deprecation::withNoReplacement(fn() => $hash->renew()); + + if ($replaceToken) { + $this->assertNotEquals($oldToken, $hash->getToken()); + $this->assertNotEquals($oldHash, $hash->Hash); + } else { + $this->assertEmpty($hash->getToken()); + $this->assertEquals($oldHash, $hash->Hash); + } + } + + public function provideRenew(): array + { + return [ + [true], + [false], + ]; + } }