feature #23042 Consistent error handling in remember me services (lst…

…rojny)

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

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

@@ -100,7 +100,7 @@ public function handle(GetResponseEvent $event)
                 );
             }
 
-            $this->rememberMeServices->loginFail($request);
+            $this->rememberMeServices->loginFail($request, $e);
 
             if (!$this->catchExceptions) {
                 throw $e;

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

@@ -106,7 +106,7 @@ public function getSecret()
     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 @@ final public function autoLogin(Request $request)
 
             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 @@ public function logout(Request $request, Response $response, TokenInterface $tok
      * 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 @@ final public function loginSuccess(Request $request, Response $response, TokenIn
     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)
     {
     }
 

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

@@ -29,6 +29,7 @@
  */
 class PersistentTokenBasedRememberMeServices extends AbstractRememberMeServices
 {
+    /** @var TokenProviderInterface */
     private $tokenProvider;
 
     /**

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

@@ -60,9 +60,10 @@ public function autoLogin(Request $request);
      *
      * 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

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

@@ -66,6 +66,8 @@ public function testOnCoreSecurityDoesNothingWhenNoCookieIsSet()
     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 @@ public function testOnCoreSecurityIgnoresAuthenticationExceptionThrownByAuthenti
         $service
             ->expects($this->once())
             ->method('loginFail')
+            ->with($request, $exception)
         ;
 
-        $exception = new AuthenticationException('Authentication failed.');
         $manager
             ->expects($this->once())
             ->method('authenticate')
@@ -95,7 +97,7 @@ public function testOnCoreSecurityIgnoresAuthenticationExceptionThrownByAuthenti
         $event
             ->expects($this->once())
             ->method('getRequest')
-            ->will($this->returnValue(new Request()))
+            ->will($this->returnValue($request))
         ;
 
         $listener->handle($event);