Skip to content

Commit

Permalink
API Make token regeneration optional during autologin session renewal
Browse files Browse the repository at this point in the history
Resolves silverstripe#11281. Renewing the token/hash during an active session
can cause session failures in the event of request failures or
simultaneous requests.

This also marks the renew method as deprecated, to be removed
entirely in 6.0.
  • Loading branch information
Cheddam committed Jul 4, 2024
1 parent dbc0288 commit 1529918
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 13 deletions.
24 changes: 14 additions & 10 deletions src/Security/MemberAuthenticator/CookieAuthenticationHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
27 changes: 24 additions & 3 deletions src/Security/RememberLoginHash.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}

Expand Down
28 changes: 28 additions & 0 deletions tests/php/Security/RememberLoginHashTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

0 comments on commit 1529918

Please sign in to comment.