Skip to content

Commit 7be1d8d

Browse files
wouterjwouterj
committed
bug symfony#49526 [Security] Migrate the session on login only when the user changes (nicolas-grekas)
This PR was merged into the 5.4 branch. Discussion ---------- [Security] Migrate the session on login only when the user changes | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix symfony#49194 | License | MIT | Doc PR | - As described in the linked issue, the recent security fix breaks submitting forms when stateless authenticators are mixed with stateful CSRF storage. This PR fixes the issue by ensuring that the session is *not* migrated when the user selected by the stateless authenticator is the same as the one retrieved from the session. In order to do so, I had add to the previous token to `LoginSuccessEvent`. /cc `@wouterj` `@weaverryan` `@stof` can you please have a look? This should be part of the next release if possible. Commits ------- 238f25c [Security] Migrate the session on login only when the user changes
2 parents 51b859f + 238f25c commit 7be1d8d

14 files changed

+125
-131
lines changed

‎src/Symfony/Bundle/SecurityBundle/Resources/config/guard.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
abstract_arg('Authenticators'),
5050
service('logger')->nullOnInvalid(),
5151
param('security.authentication.hide_user_not_found'),
52+
service('security.token_storage'),
5253
])
5354
->tag('monolog.logger', ['channel' => 'security'])
5455
->deprecate('symfony/security-bundle', '5.3', 'The "%service_id%" service is deprecated, use the new authenticator system instead.')

‎src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\HttpFoundation\Response;
1717
use Symfony\Component\HttpKernel\Event\RequestEvent;
1818
use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface;
19+
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
1920
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
2021
use Symfony\Component\Security\Core\Exception\AccountStatusException;
2122
use Symfony\Component\Security\Core\Exception\AuthenticationException;
@@ -49,12 +50,13 @@ class GuardAuthenticationListener extends AbstractListener
4950
private $logger;
5051
private $rememberMeServices;
5152
private $hideUserNotFoundExceptions;
53+
private $tokenStorage;
5254

5355
/**
5456
* @param string $providerKey The provider (i.e. firewall) key
5557
* @param iterable<array-key, AuthenticatorInterface> $guardAuthenticators The authenticators, with keys that match what's passed to GuardAuthenticationProvider
5658
*/
57-
public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, string $providerKey, iterable $guardAuthenticators, LoggerInterface $logger = null, bool $hideUserNotFoundExceptions = true)
59+
public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, string $providerKey, iterable $guardAuthenticators, LoggerInterface $logger = null, bool $hideUserNotFoundExceptions = true, TokenStorageInterface $tokenStorage = null)
5860
{
5961
if (empty($providerKey)) {
6062
throw new \InvalidArgumentException('$providerKey must not be empty.');
@@ -66,6 +68,7 @@ public function __construct(GuardAuthenticatorHandler $guardHandler, Authenticat
6668
$this->guardAuthenticators = $guardAuthenticators;
6769
$this->logger = $logger;
6870
$this->hideUserNotFoundExceptions = $hideUserNotFoundExceptions;
71+
$this->tokenStorage = $tokenStorage;
6972
}
7073

7174
/**
@@ -135,6 +138,7 @@ public function authenticate(RequestEvent $event)
135138
private function executeGuardAuthenticator(string $uniqueGuardKey, AuthenticatorInterface $guardAuthenticator, RequestEvent $event)
136139
{
137140
$request = $event->getRequest();
141+
$previousToken = $this->tokenStorage ? $this->tokenStorage->getToken() : null;
138142
try {
139143
if (null !== $this->logger) {
140144
$this->logger->debug('Calling getCredentials() on guard authenticator.', ['firewall_key' => $this->providerKey, 'authenticator' => \get_class($guardAuthenticator)]);
@@ -162,7 +166,11 @@ private function executeGuardAuthenticator(string $uniqueGuardKey, Authenticator
162166
}
163167

164168
// sets the token on the token storage, etc
165-
$this->guardHandler->authenticateWithToken($token, $request, $this->providerKey);
169+
if ($this->tokenStorage) {
170+
$this->guardHandler->authenticateWithToken($token, $request, $this->providerKey, $previousToken);
171+
} else {
172+
$this->guardHandler->authenticateWithToken($token, $request, $this->providerKey);
173+
}
166174
} catch (AuthenticationException $e) {
167175
// oh no! Authentication failed!
168176

‎src/Symfony/Component/Security/Guard/GuardAuthenticatorHandler.php

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,9 @@ public function __construct(TokenStorageInterface $tokenStorage, EventDispatcher
5656
/**
5757
* Authenticates the given token in the system.
5858
*/
59-
public function authenticateWithToken(TokenInterface $token, Request $request, string $providerKey = null)
59+
public function authenticateWithToken(TokenInterface $token, Request $request, string $providerKey = null, TokenInterface $previousToken = null)
6060
{
61-
$this->migrateSession($request, $token, $providerKey);
61+
$this->migrateSession($request, $token, $providerKey, 3 < \func_num_args() ? $previousToken : $this->tokenStorage->getToken());
6262
$this->tokenStorage->setToken($token);
6363

6464
if (null !== $this->dispatcher) {
@@ -91,7 +91,7 @@ public function authenticateUserAndHandleSuccess(UserInterface $user, Request $r
9191
// create an authenticated token for the User
9292
$token = $authenticator->createAuthenticatedToken($user, $providerKey);
9393
// authenticate this in the system
94-
$this->authenticateWithToken($token, $request, $providerKey);
94+
$this->authenticateWithToken($token, $request, $providerKey, $this->tokenStorage->getToken());
9595

9696
// return the success metric
9797
return $this->handleAuthenticationSuccess($token, $request, $authenticator, $providerKey);
@@ -122,12 +122,21 @@ public function setSessionAuthenticationStrategy(SessionAuthenticationStrategyIn
122122
$this->sessionStrategy = $sessionStrategy;
123123
}
124124

125-
private function migrateSession(Request $request, TokenInterface $token, ?string $providerKey)
125+
private function migrateSession(Request $request, TokenInterface $token, ?string $providerKey, ?TokenInterface $previousToken)
126126
{
127127
if (\in_array($providerKey, $this->statelessProviderKeys, true) || !$this->sessionStrategy || !$request->hasSession() || !$request->hasPreviousSession()) {
128128
return;
129129
}
130130

131+
if ($previousToken) {
132+
$user = method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername();
133+
$previousUser = method_exists($previousToken, 'getUserIdentifier') ? $previousToken->getUserIdentifier() : $previousToken->getUsername();
134+
135+
if ('' !== ($user ?? '') && $user === $previousUser) {
136+
return;
137+
}
138+
}
139+
131140
$this->sessionStrategy->onAuthentication($request, $token);
132141
}
133142
}

‎src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ public function authenticateUser(UserInterface $user, AuthenticatorInterface $au
8484
$token = $this->eventDispatcher->dispatch(new AuthenticationTokenCreatedEvent($token, $passport))->getAuthenticatedToken();
8585

8686
// authenticate this in the system
87-
return $this->handleAuthenticationSuccess($token, $passport, $request, $authenticator);
87+
return $this->handleAuthenticationSuccess($token, $passport, $request, $authenticator, $this->tokenStorage->getToken());
8888
}
8989

9090
public function supports(Request $request): ?bool
@@ -174,6 +174,7 @@ private function executeAuthenticators(array $authenticators, Request $request):
174174
private function executeAuthenticator(AuthenticatorInterface $authenticator, Request $request): ?Response
175175
{
176176
$passport = null;
177+
$previousToken = $this->tokenStorage->getToken();
177178

178179
try {
179180
// get the passport from the Authenticator
@@ -224,7 +225,7 @@ private function executeAuthenticator(AuthenticatorInterface $authenticator, Req
224225
}
225226

226227
// success! (sets the token on the token storage, etc)
227-
$response = $this->handleAuthenticationSuccess($authenticatedToken, $passport, $request, $authenticator);
228+
$response = $this->handleAuthenticationSuccess($authenticatedToken, $passport, $request, $authenticator, $previousToken);
228229
if ($response instanceof Response) {
229230
return $response;
230231
}
@@ -236,7 +237,7 @@ private function executeAuthenticator(AuthenticatorInterface $authenticator, Req
236237
return null;
237238
}
238239

239-
private function handleAuthenticationSuccess(TokenInterface $authenticatedToken, PassportInterface $passport, Request $request, AuthenticatorInterface $authenticator): ?Response
240+
private function handleAuthenticationSuccess(TokenInterface $authenticatedToken, PassportInterface $passport, Request $request, AuthenticatorInterface $authenticator, ?TokenInterface $previousToken): ?Response
240241
{
241242
// @deprecated since Symfony 5.3
242243
$user = $authenticatedToken->getUser();
@@ -252,7 +253,7 @@ private function handleAuthenticationSuccess(TokenInterface $authenticatedToken,
252253
$this->eventDispatcher->dispatch($loginEvent, SecurityEvents::INTERACTIVE_LOGIN);
253254
}
254255

255-
$this->eventDispatcher->dispatch($loginSuccessEvent = new LoginSuccessEvent($authenticator, $passport, $authenticatedToken, $request, $response, $this->firewallName));
256+
$this->eventDispatcher->dispatch($loginSuccessEvent = new LoginSuccessEvent($authenticator, $passport, $authenticatedToken, $request, $response, $this->firewallName, $previousToken));
256257

257258
return $loginSuccessEvent->getResponse();
258259
}

‎src/Symfony/Component/Security/Http/Event/LoginSuccessEvent.php

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,15 @@ class LoginSuccessEvent extends Event
3737
private $authenticator;
3838
private $passport;
3939
private $authenticatedToken;
40+
private $previousToken;
4041
private $request;
4142
private $response;
4243
private $firewallName;
4344

4445
/**
4546
* @param Passport $passport
4647
*/
47-
public function __construct(AuthenticatorInterface $authenticator, PassportInterface $passport, TokenInterface $authenticatedToken, Request $request, ?Response $response, string $firewallName)
48+
public function __construct(AuthenticatorInterface $authenticator, PassportInterface $passport, TokenInterface $authenticatedToken, Request $request, ?Response $response, string $firewallName, TokenInterface $previousToken = null)
4849
{
4950
if (!$passport instanceof Passport) {
5051
trigger_deprecation('symfony/security-http', '5.4', 'Not passing an instance of "%s" as "$passport" argument of "%s()" is deprecated, "%s" given.', Passport::class, __METHOD__, get_debug_type($passport));
@@ -53,6 +54,7 @@ public function __construct(AuthenticatorInterface $authenticator, PassportInter
5354
$this->authenticator = $authenticator;
5455
$this->passport = $passport;
5556
$this->authenticatedToken = $authenticatedToken;
57+
$this->previousToken = $previousToken;
5658
$this->request = $request;
5759
$this->response = $response;
5860
$this->firewallName = $firewallName;
@@ -83,6 +85,11 @@ public function getAuthenticatedToken(): TokenInterface
8385
return $this->authenticatedToken;
8486
}
8587

88+
public function getPreviousToken(): ?TokenInterface
89+
{
90+
return $this->previousToken;
91+
}
92+
8693
public function getRequest(): Request
8794
{
8895
return $this->request;

‎src/Symfony/Component/Security/Http/EventListener/SessionStrategyListener.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,16 @@ public function onSuccessfulLogin(LoginSuccessEvent $event): void
4343
return;
4444
}
4545

46+
if ($previousToken = $event->getPreviousToken()) {
47+
// @deprecated since Symfony 5.3, change to $token->getUserIdentifier() in 6.0
48+
$user = method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername();
49+
$previousUser = method_exists($previousToken, 'getUserIdentifier') ? $previousToken->getUserIdentifier() : $previousToken->getUsername();
50+
51+
if ('' !== ($user ?? '') && $user === $previousUser) {
52+
return;
53+
}
54+
}
55+
4656
$this->sessionAuthenticationStrategy->onAuthentication($request, $token);
4757
}
4858

‎src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,12 +133,14 @@ public function authenticate(RequestEvent $event)
133133
throw new SessionUnavailableException('Your session has timed out, or you have disabled cookies.');
134134
}
135135

136+
$previousToken = $this->tokenStorage->getToken();
137+
136138
if (null === $returnValue = $this->attemptAuthentication($request)) {
137139
return;
138140
}
139141

140142
if ($returnValue instanceof TokenInterface) {
141-
$this->sessionStrategy->onAuthentication($request, $returnValue);
143+
$this->migrateSession($request, $returnValue, $previousToken);
142144

143145
$response = $this->onSuccess($request, $returnValue);
144146
} elseif ($returnValue instanceof Response) {
@@ -226,4 +228,18 @@ private function onSuccess(Request $request, TokenInterface $token): Response
226228

227229
return $response;
228230
}
231+
232+
private function migrateSession(Request $request, TokenInterface $token, ?TokenInterface $previousToken)
233+
{
234+
if ($previousToken) {
235+
$user = method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername();
236+
$previousUser = method_exists($previousToken, 'getUserIdentifier') ? $previousToken->getUserIdentifier() : $previousToken->getUsername();
237+
238+
if ('' !== ($user ?? '') && $user === $previousUser) {
239+
return;
240+
}
241+
}
242+
243+
$this->sessionStrategy->onAuthentication($request, $token);
244+
}
229245
}

‎src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,13 +98,14 @@ public function authenticate(RequestEvent $event)
9898
}
9999

100100
try {
101+
$previousToken = $token;
101102
$token = $this->authenticationManager->authenticate(new PreAuthenticatedToken($user, $credentials, $this->providerKey));
102103

103104
if (null !== $this->logger) {
104105
$this->logger->info('Pre-authentication successful.', ['token' => (string) $token]);
105106
}
106107

107-
$this->migrateSession($request, $token);
108+
$this->migrateSession($request, $token, $previousToken);
108109

109110
$this->tokenStorage->setToken($token);
110111

@@ -149,12 +150,21 @@ private function clearToken(AuthenticationException $exception)
149150
*/
150151
abstract protected function getPreAuthenticatedData(Request $request);
151152

152-
private function migrateSession(Request $request, TokenInterface $token)
153+
private function migrateSession(Request $request, TokenInterface $token, ?TokenInterface $previousToken)
153154
{
154155
if (!$this->sessionStrategy || !$request->hasSession() || !$request->hasPreviousSession()) {
155156
return;
156157
}
157158

159+
if ($previousToken) {
160+
$user = method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername();
161+
$previousUser = method_exists($previousToken, 'getUserIdentifier') ? $previousToken->getUserIdentifier() : $previousToken->getUsername();
162+
163+
if ('' !== ($user ?? '') && $user === $previousUser) {
164+
return;
165+
}
166+
}
167+
158168
$this->sessionStrategy->onAuthentication($request, $token);
159169
}
160170
}

‎src/Symfony/Component/Security/Http/Firewall/BasicAuthenticationListener.php

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,10 @@ public function authenticate(RequestEvent $event)
8888
}
8989

9090
try {
91+
$previousToken = $token;
9192
$token = $this->authenticationManager->authenticate(new UsernamePasswordToken($username, $request->headers->get('PHP_AUTH_PW'), $this->providerKey));
9293

93-
$this->migrateSession($request, $token);
94+
$this->migrateSession($request, $token, $previousToken);
9495

9596
$this->tokenStorage->setToken($token);
9697
} catch (AuthenticationException $e) {
@@ -121,12 +122,21 @@ public function setSessionAuthenticationStrategy(SessionAuthenticationStrategyIn
121122
$this->sessionStrategy = $sessionStrategy;
122123
}
123124

124-
private function migrateSession(Request $request, TokenInterface $token)
125+
private function migrateSession(Request $request, TokenInterface $token, ?TokenInterface $previousToken)
125126
{
126127
if (!$this->sessionStrategy || !$request->hasSession() || !$request->hasPreviousSession()) {
127128
return;
128129
}
129130

131+
if ($previousToken) {
132+
$user = method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername();
133+
$previousUser = method_exists($previousToken, 'getUserIdentifier') ? $previousToken->getUserIdentifier() : $previousToken->getUsername();
134+
135+
if ('' !== ($user ?? '') && $user === $previousUser) {
136+
return;
137+
}
138+
}
139+
130140
$this->sessionStrategy->onAuthentication($request, $token);
131141
}
132142
}

‎src/Symfony/Component/Security/Http/Firewall/UsernamePasswordJsonAuthenticationListener.php

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ public function authenticate(RequestEvent $event)
101101
{
102102
$request = $event->getRequest();
103103
$data = json_decode($request->getContent());
104+
$previousToken = $this->tokenStorage->getToken();
104105

105106
try {
106107
if (!$data instanceof \stdClass) {
@@ -134,7 +135,7 @@ public function authenticate(RequestEvent $event)
134135
$token = new UsernamePasswordToken($username, $password, $this->providerKey);
135136

136137
$authenticatedToken = $this->authenticationManager->authenticate($token);
137-
$response = $this->onSuccess($request, $authenticatedToken);
138+
$response = $this->onSuccess($request, $authenticatedToken, $previousToken);
138139
} catch (AuthenticationException $e) {
139140
$response = $this->onFailure($request, $e);
140141
} catch (BadRequestHttpException $e) {
@@ -150,14 +151,14 @@ public function authenticate(RequestEvent $event)
150151
$event->setResponse($response);
151152
}
152153

153-
private function onSuccess(Request $request, TokenInterface $token): ?Response
154+
private function onSuccess(Request $request, TokenInterface $token, ?TokenInterface $previousToken): ?Response
154155
{
155156
if (null !== $this->logger) {
156157
// @deprecated since Symfony 5.3, change to $token->getUserIdentifier() in 6.0
157158
$this->logger->info('User has been authenticated successfully.', ['username' => method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername()]);
158159
}
159160

160-
$this->migrateSession($request, $token);
161+
$this->migrateSession($request, $token, $previousToken);
161162

162163
$this->tokenStorage->setToken($token);
163164

@@ -224,12 +225,21 @@ public function setTranslator(TranslatorInterface $translator)
224225
$this->translator = $translator;
225226
}
226227

227-
private function migrateSession(Request $request, TokenInterface $token)
228+
private function migrateSession(Request $request, TokenInterface $token, ?TokenInterface $previousToken)
228229
{
229230
if (!$this->sessionStrategy || !$request->hasSession() || !$request->hasPreviousSession()) {
230231
return;
231232
}
232233

234+
if ($previousToken) {
235+
$user = method_exists($token, 'getUserIdentifier') ? $token->getUserIdentifier() : $token->getUsername();
236+
$previousUser = method_exists($previousToken, 'getUserIdentifier') ? $previousToken->getUserIdentifier() : $previousToken->getUsername();
237+
238+
if ('' !== ($user ?? '') && $user === $previousUser) {
239+
return;
240+
}
241+
}
242+
233243
$this->sessionStrategy->onAuthentication($request, $token);
234244
}
235245
}

0 commit comments

Comments
 (0)