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
-------
eda1888f71
Consistent error handling in remember me services
This commit is contained in:
commit
bf094efa9c
@ -100,7 +100,7 @@ class RememberMeListener implements ListenerInterface
|
||||
);
|
||||
}
|
||||
|
||||
$this->rememberMeServices->loginFail($request);
|
||||
$this->rememberMeServices->loginFail($request, $e);
|
||||
|
||||
if (!$this->catchExceptions) {
|
||||
throw $e;
|
||||
|
@ -106,7 +106,7 @@ abstract class AbstractRememberMeServices implements RememberMeServicesInterface
|
||||
final public function autoLogin(Request $request)
|
||||
{
|
||||
if (null === $cookie = $request->cookies->get($this->options['name'])) {
|
||||
return;
|
||||
return null;
|
||||
}
|
||||
|
||||
if (null !== $this->logger) {
|
||||
@ -128,24 +128,32 @@ abstract class AbstractRememberMeServices implements RememberMeServicesInterface
|
||||
|
||||
return new RememberMeToken($user, $this->providerKey, $this->secret);
|
||||
} catch (CookieTheftException $e) {
|
||||
$this->cancelCookie($request);
|
||||
$this->loginFail($request, $e);
|
||||
|
||||
throw $e;
|
||||
} catch (UsernameNotFoundException $e) {
|
||||
if (null !== $this->logger) {
|
||||
$this->logger->info('User for remember-me cookie not found.');
|
||||
$this->logger->info('User for remember-me cookie not found.', array('exception' => $e));
|
||||
}
|
||||
|
||||
$this->loginFail($request, $e);
|
||||
} catch (UnsupportedUserException $e) {
|
||||
if (null !== $this->logger) {
|
||||
$this->logger->warning('User class for remember-me cookie not supported.');
|
||||
$this->logger->warning('User class for remember-me cookie not supported.', array('exception' => $e));
|
||||
}
|
||||
|
||||
$this->loginFail($request, $e);
|
||||
} catch (AuthenticationException $e) {
|
||||
if (null !== $this->logger) {
|
||||
$this->logger->debug('Remember-Me authentication failed.', array('exception' => $e));
|
||||
}
|
||||
}
|
||||
|
||||
$this->cancelCookie($request);
|
||||
$this->loginFail($request, $e);
|
||||
} catch (\Exception $e) {
|
||||
$this->loginFail($request, $e);
|
||||
|
||||
throw $e;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
@ -164,12 +172,13 @@ abstract class AbstractRememberMeServices implements RememberMeServicesInterface
|
||||
* Implementation for RememberMeServicesInterface. Deletes the cookie when
|
||||
* an attempted authentication fails.
|
||||
*
|
||||
* @param Request $request
|
||||
* @param Request $request
|
||||
* @param \Exception|null $exception
|
||||
*/
|
||||
final public function loginFail(Request $request)
|
||||
final public function loginFail(Request $request, \Exception $exception = null)
|
||||
{
|
||||
$this->cancelCookie($request);
|
||||
$this->onLoginFail($request);
|
||||
$this->onLoginFail($request, $exception);
|
||||
}
|
||||
|
||||
/**
|
||||
@ -226,9 +235,10 @@ abstract class AbstractRememberMeServices implements RememberMeServicesInterface
|
||||
abstract protected function processAutoLoginCookie(array $cookieParts, Request $request);
|
||||
|
||||
/**
|
||||
* @param Request $request
|
||||
* @param Request $request
|
||||
* @param \Exception|null $exception
|
||||
*/
|
||||
protected function onLoginFail(Request $request)
|
||||
protected function onLoginFail(Request $request, \Exception $exception = null)
|
||||
{
|
||||
}
|
||||
|
||||
|
@ -29,6 +29,7 @@ use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
|
||||
*/
|
||||
class PersistentTokenBasedRememberMeServices extends AbstractRememberMeServices
|
||||
{
|
||||
/** @var TokenProviderInterface */
|
||||
private $tokenProvider;
|
||||
|
||||
/**
|
||||
|
@ -60,9 +60,10 @@ interface RememberMeServicesInterface
|
||||
*
|
||||
* This method needs to take care of invalidating the cookie.
|
||||
*
|
||||
* @param Request $request
|
||||
* @param Request $request
|
||||
* @param \Exception|null $exception
|
||||
*/
|
||||
public function loginFail(Request $request);
|
||||
public function loginFail(Request $request, \Exception $exception = null);
|
||||
|
||||
/**
|
||||
* Called whenever an interactive authentication attempt is successful
|
||||
|
@ -66,6 +66,8 @@ class RememberMeListenerTest extends TestCase
|
||||
public function testOnCoreSecurityIgnoresAuthenticationExceptionThrownByAuthenticationManagerImplementation()
|
||||
{
|
||||
list($listener, $tokenStorage, $service, $manager) = $this->getListener();
|
||||
$request = new Request();
|
||||
$exception = new AuthenticationException('Authentication failed.');
|
||||
|
||||
$tokenStorage
|
||||
->expects($this->once())
|
||||
@ -82,9 +84,9 @@ class RememberMeListenerTest extends TestCase
|
||||
$service
|
||||
->expects($this->once())
|
||||
->method('loginFail')
|
||||
->with($request, $exception)
|
||||
;
|
||||
|
||||
$exception = new AuthenticationException('Authentication failed.');
|
||||
$manager
|
||||
->expects($this->once())
|
||||
->method('authenticate')
|
||||
@ -95,7 +97,7 @@ class RememberMeListenerTest extends TestCase
|
||||
$event
|
||||
->expects($this->once())
|
||||
->method('getRequest')
|
||||
->will($this->returnValue(new Request()))
|
||||
->will($this->returnValue($request))
|
||||
;
|
||||
|
||||
$listener->handle($event);
|
||||
|
Reference in New Issue
Block a user