Merge pull request #11299 from Cheddam/pulls/5/session-token-renewal-…

…configuration API Make token regeneration optional during autologin session renewal
silverstripe · Jul 22, 2024 · d14ec28 · d14ec28
2 parents db665a8 + 724e813
commit d14ec28
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 15 deletions.
diff --git 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
@@ -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
@@ -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],
+        ];
+    }
 }