Skip to content

Commit 018ee58

Browse files
nicolas-grekasnicolas-grekas
committed
[HttpKernel] Fix session handling: decouple "save" from setting response "private"
1 parent f95ac4f commit 018ee58

File tree

7 files changed

+143
-16
lines changed

7 files changed

+143
-16
lines changed

‎src/Symfony/Component/HttpFoundation/Session/Session.php

+13-2
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ class Session implements SessionInterface, \IteratorAggregate, \Countable
2929
private $flashName;
3030
private $attributeName;
3131
private $data = array();
32+
private $wasStarted;
3233

3334
/**
3435
* @param SessionStorageInterface $storage A SessionStorageInterface instance
@@ -140,6 +141,16 @@ public function count()
140141
return count($this->getAttributeBag()->all());
141142
}
142143

144+
/**
145+
* @return bool
146+
*
147+
* @internal
148+
*/
149+
public function wasStarted()
150+
{
151+
return $this->wasStarted;
152+
}
153+
143154
/**
144155
* @return bool
145156
*
@@ -227,7 +238,7 @@ public function getMetadataBag()
227238
*/
228239
public function registerBag(SessionBagInterface $bag)
229240
{
230-
$this->storage->registerBag(new SessionBagProxy($bag, $this->data));
241+
$this->storage->registerBag(new SessionBagProxy($bag, $this->data, $this->wasStarted));
231242
}
232243

233244
/**
@@ -257,6 +268,6 @@ public function getFlashBag()
257268
*/
258269
private function getAttributeBag()
259270
{
260-
return $this->storage->getBag($this->attributeName)->getBag();
271+
return $this->getBag($this->attributeName);
261272
}
262273
}

‎src/Symfony/Component/HttpFoundation/Session/SessionBagProxy.php

+4-1
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,13 @@ final class SessionBagProxy implements SessionBagInterface
2020
{
2121
private $bag;
2222
private $data;
23+
private $wasStarted;
2324

24-
public function __construct(SessionBagInterface $bag, array &$data)
25+
public function __construct(SessionBagInterface $bag, array &$data, &$wasStarted)
2526
{
2627
$this->bag = $bag;
2728
$this->data = &$data;
29+
$this->wasStarted = &$wasStarted;
2830
}
2931

3032
/**
@@ -56,6 +58,7 @@ public function getName()
5658
*/
5759
public function initialize(array &$array)
5860
{
61+
$this->wasStarted = true;
5962
$this->data[$this->bag->getStorageKey()] = &$array;
6063

6164
$this->bag->initialize($array);

‎src/Symfony/Component/HttpKernel/EventListener/AbstractTestSessionListener.php

+4-4
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,10 @@ public function onKernelResponse(FilterResponseEvent $event)
6161
$session = $event->getRequest()->getSession();
6262
if ($session && $session->isStarted()) {
6363
$session->save();
64-
if (!$session instanceof Session || !\method_exists($session, 'isEmpty') || !$session->isEmpty()) {
65-
$params = session_get_cookie_params();
66-
$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']));
67-
}
64+
}
65+
if ($session && ($session instanceof Session && \method_exists($session, 'isEmpty') ? !$session->isEmpty() : $session->isStarted())) {
66+
$params = session_get_cookie_params();
67+
$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']));
6868
}
6969
}
7070

‎src/Symfony/Component/HttpKernel/EventListener/SaveSessionListener.php

-4
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,6 @@ public function onKernelResponse(FilterResponseEvent $event)
5353
$session = $event->getRequest()->getSession();
5454
if ($session && $session->isStarted()) {
5555
$session->save();
56-
$event->getResponse()
57-
->setPrivate()
58-
->setMaxAge(0)
59-
->headers->addCacheControlDirective('must-revalidate');
6056
}
6157
}
6258

‎src/Symfony/Component/HttpKernel/EventListener/SessionListener.php

+38
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
namespace Symfony\Component\HttpKernel\EventListener;
1313

1414
use Psr\Container\ContainerInterface;
15+
use Symfony\Component\HttpFoundation\Session\Session;
16+
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
17+
use Symfony\Component\HttpKernel\KernelEvents;
1518

1619
/**
1720
* Sets the session in the request.
@@ -29,6 +32,33 @@ public function __construct(ContainerInterface $container)
2932
$this->container = $container;
3033
}
3134

35+
public function onKernelResponse(FilterResponseEvent $event)
36+
{
37+
if (!$event->isMasterRequest()) {
38+
return;
39+
}
40+
41+
if (!$session = $event->getRequest()->getSession()) {
42+
return;
43+
}
44+
45+
$sessionWasStarted = $session->isStarted();
46+
if ($sessionWasStarted || !$session instanceof Session) {
47+
// no-op
48+
} elseif (\method_exists($session, 'wasStarted') && $session->wasStarted()) {
49+
$sessionWasStarted = true;
50+
} elseif (\method_exists($session, 'isEmpty') && !$session->isEmpty()) {
51+
$sessionWasStarted = true;
52+
}
53+
54+
if ($sessionWasStarted) {
55+
$event->getResponse()
56+
->setPrivate()
57+
->setMaxAge(0)
58+
->headers->addCacheControlDirective('must-revalidate');
59+
}
60+
}
61+
3262
protected function getSession()
3363
{
3464
if (!$this->container->has('session')) {
@@ -37,4 +67,12 @@ protected function getSession()
3767

3868
return $this->container->get('session');
3969
}
70+
71+
public static function getSubscribedEvents()
72+
{
73+
return parent::getSubscribedEvents() + array(
74+
// low priority to come after regular response listeners
75+
KernelEvents::RESPONSE => array(array('onKernelResponse', -1000)),
76+
);
77+
}
4078
}

‎src/Symfony/Component/HttpKernel/Tests/EventListener/SaveSessionListenerTest.php

+1-5
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public function testOnlyTriggeredOnMasterRequest()
3232
$listener->onKernelResponse($event);
3333
}
3434

35-
public function testSessionSavedAndResponsePrivate()
35+
public function testSessionSaved()
3636
{
3737
$listener = new SaveSessionListener();
3838
$kernel = $this->getMockBuilder(HttpKernelInterface::class)->disableOriginalConstructor()->getMock();
@@ -45,9 +45,5 @@ public function testSessionSavedAndResponsePrivate()
4545
$request->setSession($session);
4646
$response = new Response();
4747
$listener->onKernelResponse(new FilterResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST, $response));
48-
49-
$this->assertTrue($response->headers->hasCacheControlDirective('private'));
50-
$this->assertTrue($response->headers->hasCacheControlDirective('must-revalidate'));
51-
$this->assertSame('0', $response->headers->getCacheControlDirective('max-age'));
5248
}
5349
}

‎src/Symfony/Component/HttpKernel/Tests/EventListener/SessionListenerTest.php

+83
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\HttpKernel\Tests\EventListener;
13+
14+
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\DependencyInjection\Container;
16+
use Symfony\Component\HttpFoundation\Request;
17+
use Symfony\Component\HttpFoundation\Response;
18+
use Symfony\Component\HttpFoundation\Session\Session;
19+
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
20+
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
21+
use Symfony\Component\HttpKernel\EventListener\AbstractSessionListener;
22+
use Symfony\Component\HttpKernel\EventListener\SessionListener;
23+
use Symfony\Component\HttpKernel\HttpKernelInterface;
24+
25+
class SessionListenerTest extends TestCase
26+
{
27+
public function testOnlyTriggeredOnMasterRequest()
28+
{
29+
$listener = $this->getMockForAbstractClass(AbstractSessionListener::class);
30+
$event = $this->getMockBuilder(GetResponseEvent::class)->disableOriginalConstructor()->getMock();
31+
$event->expects($this->once())->method('isMasterRequest')->willReturn(false);
32+
$event->expects($this->never())->method('getRequest');
33+
34+
// sub request
35+
$listener->onKernelRequest($event);
36+
}
37+
38+
public function testSessionIsSet()
39+
{
40+
$session = $this->getMockBuilder(Session::class)->disableOriginalConstructor()->getMock();
41+
42+
$container = new Container();
43+
$container->set('session', $session);
44+
45+
$request = new Request();
46+
$listener = new SessionListener($container);
47+
48+
$event = $this->getMockBuilder(GetResponseEvent::class)->disableOriginalConstructor()->getMock();
49+
$event->expects($this->once())->method('isMasterRequest')->willReturn(true);
50+
$event->expects($this->once())->method('getRequest')->willReturn($request);
51+
52+
$listener->onKernelRequest($event);
53+
54+
$this->assertTrue($request->hasSession());
55+
$this->assertSame($session, $request->getSession());
56+
}
57+
58+
public function testResponseIsPrivate()
59+
{
60+
$session = $this->getMockBuilder(Session::class)->disableOriginalConstructor()->getMock();
61+
$session->expects($this->once())->method('isStarted')->willReturn(false);
62+
if (method_exists($session, 'wasStarted')) {
63+
$session->expects($this->once())->method('wasStarted')->willReturn(true);
64+
} else {
65+
$session->expects($this->once())->method('isEmpty')->willReturn(false);
66+
}
67+
68+
$container = new Container();
69+
$container->set('session', $session);
70+
71+
$listener = new SessionListener($container);
72+
$kernel = $this->getMockBuilder(HttpKernelInterface::class)->disableOriginalConstructor()->getMock();
73+
74+
$request = new Request();
75+
$response = new Response();
76+
$listener->onKernelRequest(new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST));
77+
$listener->onKernelResponse(new FilterResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST, $response));
78+
79+
$this->assertTrue($response->headers->hasCacheControlDirective('private'));
80+
$this->assertTrue($response->headers->hasCacheControlDirective('must-revalidate'));
81+
$this->assertSame('0', $response->headers->getCacheControlDirective('max-age'));
82+
}
83+
}

0 commit comments

Comments
 (0)