Fix ContextInvalidationLogoutHandler (#394)

Replace SessionLogoutHandler service instead of a logout listener

The logout listener is only called after the session has been invalidated and thus invalidates the wrong session

Author: wesnick

CHANGELOG.md

@@ -24,6 +24,13 @@ Changelog
 
 * User context is more reliable not cache when the hash mismatches. (E.g. after
   login/logout.)
+  
+* The `ContextInvalidationLogoutHandler` has been deprecated in favor of the
+  `ContextInvalidationSessionLogoutHandler`. The original handler was called
+  after the invalidation of the session, and thus did not invalidate the session
+  it should have but a newly created one. You should remove the deprecated service
+  `fos_http_cache.user_context.logout_handler` from the logout.handlers section
+  of your firewall configuration.
 
 2.1.0
 -----

Resources/doc/reference/configuration/user-context.rst

@@ -147,18 +147,11 @@ For the handler to work:
 * Symfony’s default behavior of regenerating the session id when users log in
   and out must be enabled (``invalidate_session``).
 
-Add the handler to your firewall configuration:
-
-.. code-block:: yaml
-
-    # app/config/security.yml
-    security:
-        firewalls:
-            secured_area:
-                logout:
-                    invalidate_session: true
-                    handlers:
-                        - fos_http_cache.user_context.logout_handler
+.. note::
+    The logout handler is active on all firewalls.  If your application has multiple firewalls
+    with different user context, you need to create your own custom invalidation handler. Be
+    aware that Symfony's `LogoutSuccessHandler` places the `SessionLogoutHandler` before any
+    configured logout handlers.
 
 enabled
 """""""

src/DependencyInjection/FOSHttpCacheExtension.php

@@ -292,11 +292,15 @@ private function loadUserContext(ContainerBuilder $container, XmlFileLoader $loa
             ->replaceArgument(0, $config['user_identifier_headers']);
 
         if ($config['logout_handler']['enabled']) {
-            $container->getDefinition($this->getAlias().'.user_context.logout_handler')
+            $container->getDefinition($this->getAlias().'.user_context_invalidator')
                 ->replaceArgument(1, $config['user_identifier_headers'])
                 ->replaceArgument(2, $config['match']['accept']);
+
+            $container->setAlias('security.logout.handler.session', $this->getAlias().'.user_context.session_logout_handler');
         } else {
             $container->removeDefinition($this->getAlias().'.user_context.logout_handler');
+            $container->removeDefinition($this->getAlias().'.user_context.session_logout_handler');
+            $container->removeDefinition($this->getAlias().'.user_context_invalidator');
         }
 
         if ($config['role_provider']) {

src/Resources/config/user_context.xml

@@ -25,12 +25,21 @@
             <argument type="service" id="security.token_storage" on-invalid="ignore" />
         </service>
 
-        <service id="fos_http_cache.user_context.logout_handler" class="FOS\HttpCacheBundle\Security\Http\Logout\ContextInvalidationLogoutHandler">
+        <service id="fos_http_cache.user_context_invalidator" class="FOS\HttpCacheBundle\UserContextInvalidator">
             <argument type="service" id="fos_http_cache.default_proxy_client" />
             <argument />
             <argument />
         </service>
 
+        <service id="fos_http_cache.user_context.logout_handler" class="FOS\HttpCacheBundle\Security\Http\Logout\ContextInvalidationLogoutHandler" public="false">
+            <argument type="service" id="fos_http_cache.user_context_invalidator" />
+            <deprecated>The "%service_id%" service is deprecated since 2.2 and will be removed in 3.0.</deprecated>
+        </service>
+
+        <service id="fos_http_cache.user_context.session_logout_handler" class="FOS\HttpCacheBundle\Security\Http\Logout\ContextInvalidationSessionLogoutHandler" public="false">
+            <argument type="service" id="fos_http_cache.user_context_invalidator" />
+        </service>
+
         <service id="fos_http_cache.user_context.anonymous_request_matcher" class="FOS\HttpCacheBundle\UserContext\AnonymousRequestMatcher">
             <argument type="collection" />
         </service>

src/Security/Http/Logout/ContextInvalidationLogoutHandler.php

@@ -11,58 +11,33 @@
 
 namespace FOS\HttpCacheBundle\Security\Http\Logout;
 
-use FOS\HttpCache\ProxyClient\Invalidation\BanCapable;
+use FOS\HttpCacheBundle\UserContextInvalidator;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Http\Logout\LogoutHandlerInterface;
 
-class ContextInvalidationLogoutHandler implements LogoutHandlerInterface
+/**
+ * @deprecated use ContextInvalidationSessionLogoutHandler in this same namespace as a replacement
+ *
+ * This handler is deprecated because it never did what it was supposed to do. The session is already invalidated by the SessionLogoutHandler
+ * which is always the first logout handler executed
+ */
+final class ContextInvalidationLogoutHandler implements LogoutHandlerInterface
 {
-    /**
-     * Service used to ban hash request.
-     *
-     * @var BanCapable
-     */
-    private $banner;
+    private $invalidator;
 
-    /**
-     * Accept header.
-     *
-     * @var string
-     */
-    private $acceptHeader;
-
-    /**
-     * User identifier headers.
-     *
-     * @var string[]
-     */
-    private $userIdentifierHeaders;
-
-    public function __construct(BanCapable $banner, $userIdentifierHeaders, $acceptHeader)
+    public function __construct(UserContextInvalidator $invalidator)
     {
-        $this->banner = $banner;
-        $this->acceptHeader = $acceptHeader;
-        $this->userIdentifierHeaders = $userIdentifierHeaders;
+        $this->invalidator = $invalidator;
     }
 
     /**
-     * Invalidate the user context hash.
-     *
-     * @param Request        $request
-     * @param Response       $response
-     * @param TokenInterface $token
+     * {@inheritdoc}
      */
     public function logout(Request $request, Response $response, TokenInterface $token)
     {
-        $sessionId = $request->getSession()->getId();
-
-        foreach ($this->userIdentifierHeaders as $header) {
-            $this->banner->ban([
-                'accept' => $this->acceptHeader,
-                $header => sprintf('.*%s.*', $sessionId),
-            ]);
-        }
+        @trigger_error('Using the ContextInvalidationLogoutHandler is deprecated', E_USER_DEPRECATED);
+        $this->invalidator->invalidateContext($request->getSession()->getId());
     }
 }

src/Security/Http/Logout/ContextInvalidationSessionLogoutHandler.php

@@ -0,0 +1,34 @@
+<?php
+
+/*
+ * This file is part of the FOSHttpCacheBundle package.
+ *
+ * (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\HttpCacheBundle\Security\Http\Logout;
+
+use FOS\HttpCacheBundle\UserContextInvalidator;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Http\Logout\SessionLogoutHandler;
+
+final class ContextInvalidationSessionLogoutHandler extends SessionLogoutHandler
+{
+    private $invalidator;
+
+    public function __construct(UserContextInvalidator $invalidator)
+    {
+        $this->invalidator = $invalidator;
+    }
+
+    public function logout(Request $request, Response $response, TokenInterface $token)
+    {
+        $this->invalidator->invalidateContext($request->getSession()->getId());
+        parent::logout($request, $response, $token);
+    }
+}

src/UserContextInvalidator.php

@@ -0,0 +1,60 @@
+<?php
+
+/*
+ * This file is part of the FOSHttpCacheBundle package.
+ *
+ * (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\HttpCacheBundle;
+
+use FOS\HttpCache\ProxyClient\Invalidation\BanCapable;
+
+class UserContextInvalidator
+{
+    /**
+     * Service used to ban hash request.
+     *
+     * @var BanCapable
+     */
+    private $banner;
+
+    /**
+     * Accept header.
+     *
+     * @var string
+     */
+    private $acceptHeader;
+
+    /**
+     * User identifier headers.
+     *
+     * @var string[]
+     */
+    private $userIdentifierHeaders;
+
+    public function __construct(BanCapable $banner, $userIdentifierHeaders, $acceptHeader)
+    {
+        $this->banner = $banner;
+        $this->acceptHeader = $acceptHeader;
+        $this->userIdentifierHeaders = $userIdentifierHeaders;
+    }
+
+    /**
+     * Invalidate the user context hash.
+     *
+     * @param string $sessionId
+     */
+    public function invalidateContext($sessionId)
+    {
+        foreach ($this->userIdentifierHeaders as $header) {
+            $this->banner->ban([
+                'accept' => $this->acceptHeader,
+                $header => sprintf('.*%s.*', $sessionId),
+            ]);
+        }
+    }
+}

tests/Functional/Fixtures/app/config/config.yml

@@ -74,5 +74,3 @@ security:
       anonymous:
       logout:
         path:     /secured_area/logout
-        handlers:
-          - fos_http_cache.user_context.logout_handler

tests/Functional/Security/Http/Logout/ContextInvalidationLogoutHandlerTest.php

@@ -20,12 +20,6 @@ class ContextInvalidationLogoutHandlerTest extends WebTestCase
 {
     public function testLogout()
     {
-        $this->markTestSkipped(<<<'EOF'
-Session is invalidated in LogoutListener before Proxy Client can invalidate cache.
-@see https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/pull/390#issuecomment-333545374
-EOF
-        );
-
         $client = static::createClient();
         $session = $client->getContainer()->get('session');