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 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.
  • Loading branch information
Cheddam committed Jul 4, 2024
1 parent dbc0288 commit d88fe81
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 d88fe81

Please sign in to comment.