Skip to content

Commit bf094ef

Browse files
fabpotfabpot
committed
feature #23042 Consistent error handling in remember me services (lstrojny)
This PR was merged into the 3.4 branch. Discussion ---------- Consistent error handling in remember me services | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT RememberMeServices lacked consistent error handling so far making it impossible for implementors to e.g. maintain sufficiently detailed audit logs for remember me errors. Since remember me is a very sensitive area in any application, detailed logging is crucial. The change proposed allows `loginFail` to optionally take the exception object as a second parameter and uses said exception consistently internally by calling `loginFail` instead of `cancelCookie`. Commits ------- eda1888 Consistent error handling in remember me services
2 parents 1ed41b5 + eda1888 commit bf094ef

File tree

5 files changed

+30
-16
lines changed

5 files changed

+30
-16
lines changed

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ public function handle(GetResponseEvent $event)
100100
);
101101
}
102102

103-
$this->rememberMeServices->loginFail($request);
103+
$this->rememberMeServices->loginFail($request, $e);
104104

105105
if (!$this->catchExceptions) {
106106
throw $e;

‎src/Symfony/Component/Security/Http/RememberMe/AbstractRememberMeServices.php

+21-11
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public function getSecret()
106106
final public function autoLogin(Request $request)
107107
{
108108
if (null === $cookie = $request->cookies->get($this->options['name'])) {
109-
return;
109+
return null;
110110
}
111111

112112
if (null !== $this->logger) {
@@ -128,24 +128,32 @@ final public function autoLogin(Request $request)
128128

129129
return new RememberMeToken($user, $this->providerKey, $this->secret);
130130
} catch (CookieTheftException $e) {
131-
$this->cancelCookie($request);
131+
$this->loginFail($request, $e);
132132

133133
throw $e;
134134
} catch (UsernameNotFoundException $e) {
135135
if (null !== $this->logger) {
136-
$this->logger->info('User for remember-me cookie not found.');
136+
$this->logger->info('User for remember-me cookie not found.', array('exception' => $e));
137137
}
138+
139+
$this->loginFail($request, $e);
138140
} catch (UnsupportedUserException $e) {
139141
if (null !== $this->logger) {
140-
$this->logger->warning('User class for remember-me cookie not supported.');
142+
$this->logger->warning('User class for remember-me cookie not supported.', array('exception' => $e));
141143
}
144+
145+
$this->loginFail($request, $e);
142146
} catch (AuthenticationException $e) {
143147
if (null !== $this->logger) {
144148
$this->logger->debug('Remember-Me authentication failed.', array('exception' => $e));
145149
}
146-
}
147150

148-
$this->cancelCookie($request);
151+
$this->loginFail($request, $e);
152+
} catch (\Exception $e) {
153+
$this->loginFail($request, $e);
154+
155+
throw $e;
156+
}
149157
}
150158

151159
/**
@@ -164,12 +172,13 @@ public function logout(Request $request, Response $response, TokenInterface $tok
164172
* Implementation for RememberMeServicesInterface. Deletes the cookie when
165173
* an attempted authentication fails.
166174
*
167-
* @param Request $request
175+
* @param Request $request
176+
* @param \Exception|null $exception
168177
*/
169-
final public function loginFail(Request $request)
178+
final public function loginFail(Request $request, \Exception $exception = null)
170179
{
171180
$this->cancelCookie($request);
172-
$this->onLoginFail($request);
181+
$this->onLoginFail($request, $exception);
173182
}
174183

175184
/**
@@ -226,9 +235,10 @@ final public function loginSuccess(Request $request, Response $response, TokenIn
226235
abstract protected function processAutoLoginCookie(array $cookieParts, Request $request);
227236

228237
/**
229-
* @param Request $request
238+
* @param Request $request
239+
* @param \Exception|null $exception
230240
*/
231-
protected function onLoginFail(Request $request)
241+
protected function onLoginFail(Request $request, \Exception $exception = null)
232242
{
233243
}
234244

‎src/Symfony/Component/Security/Http/RememberMe/PersistentTokenBasedRememberMeServices.php

+1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
*/
3030
class PersistentTokenBasedRememberMeServices extends AbstractRememberMeServices
3131
{
32+
/** @var TokenProviderInterface */
3233
private $tokenProvider;
3334

3435
/**

‎src/Symfony/Component/Security/Http/RememberMe/RememberMeServicesInterface.php

+3-2
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,10 @@ public function autoLogin(Request $request);
6060
*
6161
* This method needs to take care of invalidating the cookie.
6262
*
63-
* @param Request $request
63+
* @param Request $request
64+
* @param \Exception|null $exception
6465
*/
65-
public function loginFail(Request $request);
66+
public function loginFail(Request $request, \Exception $exception = null);
6667

6768
/**
6869
* Called whenever an interactive authentication attempt is successful

‎src/Symfony/Component/Security/Http/Tests/Firewall/RememberMeListenerTest.php

+4-2
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ public function testOnCoreSecurityDoesNothingWhenNoCookieIsSet()
6666
public function testOnCoreSecurityIgnoresAuthenticationExceptionThrownByAuthenticationManagerImplementation()
6767
{
6868
list($listener, $tokenStorage, $service, $manager) = $this->getListener();
69+
$request = new Request();
70+
$exception = new AuthenticationException('Authentication failed.');
6971

7072
$tokenStorage
7173
->expects($this->once())
@@ -82,9 +84,9 @@ public function testOnCoreSecurityIgnoresAuthenticationExceptionThrownByAuthenti
8284
$service
8385
->expects($this->once())
8486
->method('loginFail')
87+
->with($request, $exception)
8588
;
8689

87-
$exception = new AuthenticationException('Authentication failed.');
8890
$manager
8991
->expects($this->once())
9092
->method('authenticate')
@@ -95,7 +97,7 @@ public function testOnCoreSecurityIgnoresAuthenticationExceptionThrownByAuthenti
9597
$event
9698
->expects($this->once())
9799
->method('getRequest')
98-
->will($this->returnValue(new Request()))
100+
->will($this->returnValue($request))
99101
;
100102

101103
$listener->handle($event);

0 commit comments

Comments
 (0)