Skip to content

Commit 0570380

Browse files
nicolas-grekaswouterj
authored andcommitted
[Security] Migrate the session on login only when the user changes
1 parent 77e01ce commit 0570380

11 files changed

+101
-125
lines changed

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
}

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;

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

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
}

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
}

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
}

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
}

Tests/EventListener/PasswordMigratingListenerTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Symfony\Component\HttpFoundation\Request;
1616
use Symfony\Component\PasswordHasher\Hasher\PasswordHasherFactoryInterface;
1717
use Symfony\Component\PasswordHasher\PasswordHasherInterface;
18+
use Symfony\Component\Security\Core\Authentication\Token\NullToken;
1819
use Symfony\Component\Security\Core\User\InMemoryUser;
1920
use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface;
2021
use Symfony\Component\Security\Core\User\PasswordUpgraderInterface;
@@ -28,7 +29,6 @@
2829
use Symfony\Component\Security\Http\Event\LoginSuccessEvent;
2930
use Symfony\Component\Security\Http\EventListener\PasswordMigratingListener;
3031
use Symfony\Component\Security\Http\Tests\Fixtures\DummyAuthenticator;
31-
use Symfony\Component\Security\Http\Tests\Fixtures\DummyToken;
3232

3333
class PasswordMigratingListenerTest extends TestCase
3434
{
@@ -140,7 +140,7 @@ private static function createPasswordUpgrader()
140140

141141
private static function createEvent(PassportInterface $passport)
142142
{
143-
return new LoginSuccessEvent(new DummyAuthenticator(), $passport, new DummyToken(), new Request(), null, 'main');
143+
return new LoginSuccessEvent(new DummyAuthenticator(), $passport, new NullToken(), new Request(), null, 'main');
144144
}
145145
}
146146

Tests/EventListener/SessionStrategyListenerTest.php

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\HttpFoundation\Request;
1616
use Symfony\Component\HttpFoundation\Session\SessionInterface;
17-
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
17+
use Symfony\Component\Security\Core\Authentication\Token\NullToken;
1818
use Symfony\Component\Security\Core\User\InMemoryUser;
1919
use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface;
2020
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
@@ -35,7 +35,7 @@ protected function setUp(): void
3535
$this->sessionAuthenticationStrategy = $this->createMock(SessionAuthenticationStrategyInterface::class);
3636
$this->listener = new SessionStrategyListener($this->sessionAuthenticationStrategy);
3737
$this->request = new Request();
38-
$this->token = $this->createMock(TokenInterface::class);
38+
$this->token = $this->createMock(NullToken::class);
3939
}
4040

4141
public function testRequestWithSession()
@@ -62,6 +62,25 @@ public function testStatelessFirewalls()
6262
$listener->onSuccessfulLogin($this->createEvent('api_firewall'));
6363
}
6464

65+
public function testRequestWithSamePreviousUser()
66+
{
67+
$this->configurePreviousSession();
68+
$this->sessionAuthenticationStrategy->expects($this->never())->method('onAuthentication');
69+
70+
$token = $this->createMock(NullToken::class);
71+
$token->expects($this->once())
72+
->method('getUserIdentifier')
73+
->willReturn('test');
74+
$previousToken = $this->createMock(NullToken::class);
75+
$previousToken->expects($this->once())
76+
->method('getUserIdentifier')
77+
->willReturn('test');
78+
79+
$event = new LoginSuccessEvent($this->createMock(AuthenticatorInterface::class), new SelfValidatingPassport(new UserBadge('test', function () {})), $token, $this->request, null, 'main_firewall', $previousToken);
80+
81+
$this->listener->onSuccessfulLogin($event);
82+
}
83+
6584
private function createEvent($firewallName)
6685
{
6786
return new LoginSuccessEvent($this->createMock(AuthenticatorInterface::class), new SelfValidatingPassport(new UserBadge('test', function ($username) { return new InMemoryUser($username, null); })), $this->token, $this->request, null, $firewallName);

Tests/Fixtures/DummySupportsAuthenticator.php

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,6 @@
1212
namespace Symfony\Component\Security\Http\Tests\Fixtures;
1313

1414
use Symfony\Component\HttpFoundation\Request;
15-
use Symfony\Component\HttpFoundation\Response;
16-
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
17-
use Symfony\Component\Security\Core\Exception\AuthenticationException;
18-
use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface;
19-
use Symfony\Component\Security\Http\Authenticator\Passport\Passport;
20-
use Symfony\Component\Security\Http\Authenticator\Passport\PassportInterface;
2115

2216
class DummySupportsAuthenticator extends DummyAuthenticator
2317
{

0 commit comments

Comments
 (0)