[HttpKernel] Fix session handling: decouple "save" from setting respo…

…nse "private"
symfony · Jan 10, 2018 · f8727b8 · f8727b8
1 parent f95ac4f
commit f8727b8
Show file tree

Hide file tree

Showing 8 changed files with 130 additions and 19 deletions.
diff --git a/src/Symfony/Component/HttpFoundation/Session/Session.php b/src/Symfony/Component/HttpFoundation/Session/Session.php
@@ -29,6 +29,7 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
     private $flashName;
     private $attributeName;
     private $data = array();
+    private $hasBeenStarted;
 
     /**
      * @param SessionStorageInterface $storage    A SessionStorageInterface instance
@@ -140,6 +141,16 @@ public function count()
         return count($this->getAttributeBag()->all());
     }
 
+    /**
+     * @return bool
+     *
+     * @internal
+     */
+    public function hasBeenStarted()
+    {
+        return $this->hasBeenStarted;
+    }
+
     /**
      * @return bool
      *
@@ -227,7 +238,7 @@ public function getMetadataBag()
      */
     public function registerBag(SessionBagInterface $bag)
     {
-        $this->storage->registerBag(new SessionBagProxy($bag, $this->data));
+        $this->storage->registerBag(new SessionBagProxy($bag, $this->data, $this->hasBeenStarted));
     }
 
     /**
@@ -257,6 +268,6 @@ public function getFlashBag()
      */
     private function getAttributeBag()
     {
-        return $this->storage->getBag($this->attributeName)->getBag();
+        return $this->getBag($this->attributeName);
     }
 }
diff --git a/src/Symfony/Component/HttpFoundation/Session/SessionBagProxy.php b/src/Symfony/Component/HttpFoundation/Session/SessionBagProxy.php
@@ -20,11 +20,13 @@ final class SessionBagProxy implements SessionBagInterface
 {
     private $bag;
     private $data;
+    private $hasBeenStarted;
 
-    public function __construct(SessionBagInterface $bag, array &$data)
+    public function __construct(SessionBagInterface $bag, array &$data, &$hasBeenStarted)
     {
         $this->bag = $bag;
         $this->data = &$data;
+        $this->hasBeenStarted = &$hasBeenStarted;
     }
 
     /**
@@ -56,6 +58,7 @@ public function getName()
      */
     public function initialize(array &$array)
     {
+        $this->hasBeenStarted = true;
         $this->data[$this->bag->getStorageKey()] = &$array;
 
         $this->bag->initialize($array);

diff --git a/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php b/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php
@@ -11,7 +11,9 @@
 
 namespace Symfony\Component\HttpKernel\EventListener;
 
+use Symfony\Component\HttpFoundation\Session\Session;
 use Symfony\Component\HttpFoundation\Session\SessionInterface;
+use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
 use Symfony\Component\HttpKernel\Event\GetResponseEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -38,10 +40,30 @@ public function onKernelRequest(GetResponseEvent $event)
         $request->setSession($session);
     }
 
+    public function onKernelResponse(FilterResponseEvent $event)
+    {
+        if (!$event->isMasterRequest()) {
+            return;
+        }
+
+        if (!$session = $event->getRequest()->getSession()) {
+            return;
+        }
+
+        if ($session->isStarted() || ($session instanceof Session && $session->hasBeenStarted())) {
+            $event->getResponse()
+                ->setPrivate()
+                ->setMaxAge(0)
+                ->headers->addCacheControlDirective('must-revalidate');
+        }
+    }
+
     public static function getSubscribedEvents()
     {
         return array(
             KernelEvents::REQUEST => array('onKernelRequest', 128),
+            // low priority to come after regular response listeners, same as SaveSessionListener
+            KernelEvents::RESPONSE => array('onKernelResponse', -1000),
         );
     }
 

diff --git a/src/Symfony/Component/HttpKernel/EventListener/AbstractTestSessionListener.php b/src/Symfony/Component/HttpKernel/EventListener/AbstractTestSessionListener.php
@@ -58,13 +58,17 @@ public function onKernelResponse(FilterResponseEvent $event)
             return;
         }
 
-        $session = $event->getRequest()->getSession();
-        if ($session && $session->isStarted()) {
+        if (!$session = $event->getRequest()->getSession()) {
+            return;
+        }
+
+        if ($wasStarted = $session->isStarted()) {
             $session->save();
-            if (!$session instanceof Session || !\method_exists($session, 'isEmpty') || !$session->isEmpty()) {
-                $params = session_get_cookie_params();
-                $event->getResponse()->headers->setCookie(new Cookie($session->getName(), $session->getId(), 0 === $params['lifetime'] ? 0 : time() + $params['lifetime'], $params['path'], $params['domain'], $params['secure'], $params['httponly']));
-            }
+        }
+
+        if ($session instanceof Session ? !$session->isEmpty() : $wasStarted) {
+            $params = session_get_cookie_params();
+            $event->getResponse()->headers->setCookie(new Cookie($session->getName(), $session->getId(), 0 === $params['lifetime'] ? 0 : time() + $params['lifetime'], $params['path'], $params['domain'], $params['secure'], $params['httponly']));
         }
     }
 

diff --git a/src/Symfony/Component/HttpKernel/EventListener/SaveSessionListener.php b/src/Symfony/Component/HttpKernel/EventListener/SaveSessionListener.php
@@ -53,10 +53,6 @@ public function onKernelResponse(FilterResponseEvent $event)
         $session = $event->getRequest()->getSession();
         if ($session && $session->isStarted()) {
             $session->save();
-            $event->getResponse()
-                ->setPrivate()
-                ->setMaxAge(0)
-                ->headers->addCacheControlDirective('must-revalidate');
         }
     }
 

diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/SaveSessionListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/SaveSessionListenerTest.php
@@ -32,7 +32,7 @@ public function testOnlyTriggeredOnMasterRequest()
         $listener->onKernelResponse($event);
     }
 
-    public function testSessionSavedAndResponsePrivate()
+    public function testSessionSaved()
     {
         $listener = new SaveSessionListener();
         $kernel = $this->getMockBuilder(HttpKernelInterface::class)->disableOriginalConstructor()->getMock();
@@ -45,9 +45,5 @@ public function testSessionSavedAndResponsePrivate()
         $request->setSession($session);
         $response = new Response();
         $listener->onKernelResponse(new FilterResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST, $response));
-
-        $this->assertTrue($response->headers->hasCacheControlDirective('private'));
-        $this->assertTrue($response->headers->hasCacheControlDirective('must-revalidate'));
-        $this->assertSame('0', $response->headers->getCacheControlDirective('max-age'));
     }
 }
diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/SessionListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/SessionListenerTest.php
@@ -0,0 +1,79 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\Tests\EventListener;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\DependencyInjection\Container;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpFoundation\Session\Session;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
+use Symfony\Component\HttpKernel\EventListener\AbstractSessionListener;
+use Symfony\Component\HttpKernel\EventListener\SessionListener;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+
+class SessionListenerTest extends TestCase
+{
+    public function testOnlyTriggeredOnMasterRequest()
+    {
+        $listener = $this->getMockForAbstractClass(AbstractSessionListener::class);
+        $event = $this->getMockBuilder(GetResponseEvent::class)->disableOriginalConstructor()->getMock();
+        $event->expects($this->once())->method('isMasterRequest')->willReturn(false);
+        $event->expects($this->never())->method('getRequest');
+
+        // sub request
+        $listener->onKernelRequest($event);
+    }
+
+    public function testSessionIsSet()
+    {
+        $session = $this->getMockBuilder(Session::class)->disableOriginalConstructor()->getMock();
+
+        $container = new Container();
+        $container->set('session', $session);
+
+        $request = new Request();
+        $listener = new SessionListener($container);
+
+        $event = $this->getMockBuilder(GetResponseEvent::class)->disableOriginalConstructor()->getMock();
+        $event->expects($this->once())->method('isMasterRequest')->willReturn(true);
+        $event->expects($this->once())->method('getRequest')->willReturn($request);
+
+        $listener->onKernelRequest($event);
+
+        $this->assertTrue($request->hasSession());
+        $this->assertSame($session, $request->getSession());
+    }
+
+    public function testResponseIsPrivate()
+    {
+        $session = $this->getMockBuilder(Session::class)->disableOriginalConstructor()->getMock();
+        $session->expects($this->once())->method('isStarted')->willReturn(false);
+        $session->expects($this->once())->method('hasBeenStarted')->willReturn(true);
+
+        $container = new Container();
+        $container->set('session', $session);
+
+        $listener = new SessionListener($container);
+        $kernel = $this->getMockBuilder(HttpKernelInterface::class)->disableOriginalConstructor()->getMock();
+
+        $request = new Request();
+        $response = new Response();
+        $listener->onKernelRequest(new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST));
+        $listener->onKernelResponse(new FilterResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST, $response));
+
+        $this->assertTrue($response->headers->hasCacheControlDirective('private'));
+        $this->assertTrue($response->headers->hasCacheControlDirective('must-revalidate'));
+        $this->assertSame('0', $response->headers->getCacheControlDirective('max-age'));
+    }
+}
diff --git a/src/Symfony/Component/HttpKernel/composer.json b/src/Symfony/Component/HttpKernel/composer.json
@@ -18,7 +18,7 @@
     "require": {
         "php": "^5.5.9|>=7.0.8",
         "symfony/event-dispatcher": "~2.8|~3.0|~4.0",
-        "symfony/http-foundation": "^3.3.11|~4.0",
+        "symfony/http-foundation": "^3.4.4|^4.0.4",
         "symfony/debug": "~2.8|~3.0|~4.0",
         "psr/log": "~1.0"
     },