From d88fe81d27db8f692a1f7cd4134c1b97ce65684b Mon Sep 17 00:00:00 2001
From: Garion Herman <cheddam@me.com>
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           | 24 +++++++++-------
 src/Security/RememberLoginHash.php            | 27 ++++++++++++++++--
 tests/php/Security/RememberLoginHashTest.php  | 28 +++++++++++++++++++
 3 files changed, 66 insertions(+), 13 deletions(-)

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);
+    }
 }