Skip to content

Commit edc9a67

Browse files
fabpotfabpot
committed
feature #41175 [Security] [RememberMe] Add support for parallel requests doing remember-me re-authentication (Seldaek)
This PR was squashed before being merged into the 5.3-dev branch. Discussion ---------- [Security] [RememberMe] Add support for parallel requests doing remember-me re-authentication | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | yes | New feature? | yes ish <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #40971, Fix #28314, Fix #18384 | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> This is a possible implementation to gather feedback mostly.. `TokenVerifierInterface` naming is kinda bad perhaps.. But my goal would be to merge it in TokenProviderInterface for 6.0 so it's not so important. Not sure if/how to best indicate this in terms of deprecation notices. Anyway wondering if this would be an acceptable implementation (ideally in an application I would probably override the new methods from DoctrineTokenProvider to something like this which is less of a hack and does expiration properly: ```php public function verifyToken(PersistentTokenInterface $token, string $tokenValue) { if (hash_equals($token->getTokenValue(), $tokenValue)) { return true; } if (!$this->cache->hasItem('rememberme-' . $token->getSeries())) { return false; } /** `@var` CacheItem $item */ $item = $this->cache->getItem('rememberme-' . $token->getSeries()); $oldToken = $item->get(); return hash_equals($oldToken, $tokenValue); } public function updateExistingToken(PersistentTokenInterface $token, string $tokenValue, \DateTimeInterface $lastUsed): void { $this->updateToken($token->getSeries(), $tokenValue, $lastUsed); /** `@var` CacheItem $item */ $item = $this->cache->getItem('rememberme-'.$token->getSeries()); $item->set($token->getTokenValue()); $item->expiresAfter(60); $this->cache->save($item); } ``` If you think it'd be fine to require optionally the cache inside DoctrineTokenProvider to enable this feature instead of the hackish way I did it, that'd be ok for me too. The current `DoctrineTokenProvider` implementation of `TokenVerifierInterface` relies on the lucky fact that series are generated using `base64_encode(random_bytes(64))` which always ends in the `==` padding of base64, so that allowed me to store an alternative token value temporarily by replacing `==` with `_`. Alternative implementation options: 1. Inject cache in `DoctrineTokenProvider` and do a proper implementation (as shown above) that way 2. Do not implement at all in `DoctrineTokenProvider` and let users who care implement this themselves. 3. Implement as a new `token_verifier` option that could be configured on the `firewall->remember_me` key so you can pass an implementation if needed, and possibly ship a default one using cache that could be autoconfigured 4. Add events that allow modifying the token to be verified, and allow receiving the newly updated token incl series, instead of TokenVerifierInterface, but then we need to inject a dispatcher in RememberMeAuthenticator. `@chalasr` `@wouterj` sorry for the long description but in the hope of getting this included in 5.3.0, if you can provide guidance I will happily work on this further tomorrow to try and wrap it up ASAP. Commits ------- 1992337d87 [Security] [RememberMe] Add support for parallel requests doing remember-me re-authentication
2 parents 7e1aba8 + 95f22dd commit edc9a67

File tree

3 files changed

+112
-1
lines changed

3 files changed

+112
-1
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ CHANGELOG
88
* Deprecate `DoctrineTestHelper` and `TestRepositoryFactory`
99
* [BC BREAK] Remove `UuidV*Generator` classes
1010
* Add `UuidGenerator`
11+
* Add support for the new security-core `TokenVerifierInterface` in `DoctrineTokenProvider`, fixing parallel requests handling in remember-me
1112

1213
5.2.0
1314
-----

‎Security/RememberMe/DoctrineTokenProvider.php‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use Symfony\Component\Security\Core\Authentication\RememberMe\PersistentToken;
2020
use Symfony\Component\Security\Core\Authentication\RememberMe\PersistentTokenInterface;
2121
use Symfony\Component\Security\Core\Authentication\RememberMe\TokenProviderInterface;
22+
use Symfony\Component\Security\Core\Authentication\RememberMe\TokenVerifierInterface;
2223
use Symfony\Component\Security\Core\Exception\TokenNotFoundException;
2324

2425
/**
@@ -39,7 +40,7 @@
3940
* `username` varchar(200) NOT NULL
4041
* );
4142
*/
42-
class DoctrineTokenProvider implements TokenProviderInterface
43+
class DoctrineTokenProvider implements TokenProviderInterface, TokenVerifierInterface
4344
{
4445
private $conn;
4546

@@ -136,6 +137,65 @@ public function createNewToken(PersistentTokenInterface $token)
136137
}
137138
}
138139

140+
/**
141+
* {@inheritdoc}
142+
*/
143+
public function verifyToken(PersistentTokenInterface $token, string $tokenValue): bool
144+
{
145+
// Check if the token value matches the current persisted token
146+
if (hash_equals($token->getTokenValue(), $tokenValue)) {
147+
return true;
148+
}
149+
150+
// Generate an alternative series id here by changing the suffix == to _
151+
// this is needed to be able to store an older token value in the database
152+
// which has a PRIMARY(series), and it works as long as series ids are
153+
// generated using base64_encode(random_bytes(64)) which always outputs
154+
// a == suffix, but if it should not work for some reason we abort
155+
// for safety
156+
$tmpSeries = preg_replace('{=+$}', '_', $token->getSeries());
157+
if ($tmpSeries === $token->getSeries()) {
158+
return false;
159+
}
160+
161+
// Check if the previous token is present. If the given $tokenValue
162+
// matches the previous token (and it is outdated by at most 60seconds)
163+
// we also accept it as a valid value.
164+
try {
165+
$tmpToken = $this->loadTokenBySeries($tmpSeries);
166+
} catch (TokenNotFoundException $e) {
167+
return false;
168+
}
169+
170+
if ($tmpToken->getLastUsed()->getTimestamp() + 60 < time()) {
171+
return false;
172+
}
173+
174+
return hash_equals($tmpToken->getTokenValue(), $tokenValue);
175+
}
176+
177+
/**
178+
* {@inheritdoc}
179+
*/
180+
public function updateExistingToken(PersistentTokenInterface $token, string $tokenValue, \DateTimeInterface $lastUsed): void
181+
{
182+
if (!$token instanceof PersistentToken) {
183+
return;
184+
}
185+
186+
// Persist a copy of the previous token for authentication
187+
// in verifyToken should the old token still be sent by the browser
188+
// in a request concurrent to the one that did this token update
189+
$tmpSeries = preg_replace('{=+$}', '_', $token->getSeries());
190+
// if we cannot generate a unique series it is not worth trying further
191+
if ($tmpSeries === $token->getSeries()) {
192+
return;
193+
}
194+
195+
$this->deleteTokenBySeries($tmpSeries);
196+
$this->createNewToken(new PersistentToken($token->getClass(), $token->getUserIdentifier(), $tmpSeries, $token->getTokenValue(), $lastUsed));
197+
}
198+
139199
/**
140200
* Adds the Table to the Schema if "remember me" uses this Connection.
141201
*/

‎Tests/Security/RememberMe/DoctrineTokenProviderTest.php‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,56 @@ public function testDeleteToken()
5656
$provider->loadTokenBySeries('someSeries');
5757
}
5858

59+
public function testVerifyOutdatedTokenAfterParallelRequest()
60+
{
61+
$provider = $this->bootstrapProvider();
62+
$series = base64_encode(random_bytes(64));
63+
$oldValue = 'oldValue';
64+
$newValue = 'newValue';
65+
66+
// setup existing token
67+
$token = new PersistentToken('someClass', 'someUser', $series, $oldValue, new \DateTime('2013-01-26T18:23:51'));
68+
$provider->createNewToken($token);
69+
70+
// new request comes in requiring remember-me auth, which updates the token
71+
$provider->updateExistingToken($token, $newValue, new \DateTime('-5 seconds'));
72+
$provider->updateToken($series, $newValue, new \DateTime('-5 seconds'));
73+
74+
// parallel request comes in with the old remember-me cookie and session, which also requires reauth
75+
$token = $provider->loadTokenBySeries($series);
76+
$this->assertEquals($newValue, $token->getTokenValue());
77+
78+
// new token is valid
79+
$this->assertTrue($provider->verifyToken($token, $newValue));
80+
// old token is still valid
81+
$this->assertTrue($provider->verifyToken($token, $oldValue));
82+
}
83+
84+
public function testVerifyOutdatedTokenAfterParallelRequestFailsAfter60Seconds()
85+
{
86+
$provider = $this->bootstrapProvider();
87+
$series = base64_encode(random_bytes(64));
88+
$oldValue = 'oldValue';
89+
$newValue = 'newValue';
90+
91+
// setup existing token
92+
$token = new PersistentToken('someClass', 'someUser', $series, $oldValue, new \DateTime('2013-01-26T18:23:51'));
93+
$provider->createNewToken($token);
94+
95+
// new request comes in requiring remember-me auth, which updates the token
96+
$provider->updateExistingToken($token, $newValue, new \DateTime('-61 seconds'));
97+
$provider->updateToken($series, $newValue, new \DateTime('-5 seconds'));
98+
99+
// parallel request comes in with the old remember-me cookie and session, which also requires reauth
100+
$token = $provider->loadTokenBySeries($series);
101+
$this->assertEquals($newValue, $token->getTokenValue());
102+
103+
// new token is valid
104+
$this->assertTrue($provider->verifyToken($token, $newValue));
105+
// old token is not valid anymore after 60 seconds
106+
$this->assertFalse($provider->verifyToken($token, $oldValue));
107+
}
108+
59109
/**
60110
* @return DoctrineTokenProvider
61111
*/

0 commit comments

Comments
 (0)