Skip to content

[HttpKernel] Make AbstractTestSessionListener compatible with CookieClearingLogoutHandler #27659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 1, 2018
Merged

[HttpKernel] Make AbstractTestSessionListener compatible with CookieClearingLogoutHandler #27659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Symfony/Component/HttpKernel/EventListener/AbstractTestSessionListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ public function onKernelResponse(FilterResponseEvent $event)

if ($session instanceof Session ? !$session->isEmpty() || (null !== $this->sessionId && $session->getId() !== $this->sessionId) : $wasStarted) {
$params = session_get_cookie_params();

foreach ($event->getResponse()->headers->getCookies() as $cookie) {
if ($session->getName() === $cookie->getName() && $params['path'] === $cookie->getPath() && $params['domain'] == $cookie->getDomain()) {
return;
}
}

$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']));
$this->sessionId = $session->getId();
}
Expand Down
34 changes: 32 additions & 2 deletions src/Symfony/Component/HttpKernel/Tests/EventListener/TestSessionListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,36 @@ public function testEmptySessionWithNewSessionIdDoesSendCookie()
$this->assertNotEmpty($response->headers->getCookies());
}

/**
* @dataProvider anotherCookieProvider
*/
public function testSessionWithNewSessionIdAndNewCookieDoesNotSendAnotherCookie($existing, array $expected)
{
$this->sessionHasBeenStarted();
$this->sessionIsEmpty();
$this->fixSessionId('456');

$kernel = $this->getMockBuilder('Symfony\Component\HttpKernel\HttpKernelInterface')->getMock();
$request = Request::create('/', 'GET', array(), array('MOCKSESSID' => '123'));
$event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST);
$this->listener->onKernelRequest($event);

$response = new Response('', 200, array('Set-Cookie' => $existing));

$response = $this->filterResponse(new Request(), HttpKernelInterface::MASTER_REQUEST, $response);

$this->assertSame($expected, $response->headers->get('Set-Cookie', null, false));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really clear to me what this PR for. You state that it is to make it compatible with the CookieClearingLogoutHandler, but then I would expect a test that shows that no cookies are sent. However, you've added no tests that show this.

Furthermore, I doubt if this fix is the correct way to do it. It feels like it's battling symptoms instead of the root problem, but I can't quite lay my finger on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still expect the original cookie to be sent (ie the one set by CookieClearingLogoutHandler).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the CookieClearingLogoutHandler doesn't set cookies, it removes cookies. That's the whole point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To 'remove' a cookie you have to set a new expired one, hence:

symfony/src/Symfony/Component/HttpFoundation/ResponseHeaderBag.php

Lines 248 to 251 in 6064cfe

public function clearCookie($name, $path = '/', $domain = null, $secure = false, $httpOnly = true)
{
$this->setCookie(new Cookie($name, null, 1, $path, $domain, $secure, $httpOnly));
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aaah, check, I get it now.

In that case, LGTM

}

public function anotherCookieProvider()
{
return array(
'same' => array('MOCKSESSID=789; path=/', array('MOCKSESSID=789; path=/')),
'different domain' => array('MOCKSESSID=789; path=/; domain=example.com', array('MOCKSESSID=789; path=/; domain=example.com', 'MOCKSESSID=456; path=/')),
'different path' => array('MOCKSESSID=789; path=/foo', array('MOCKSESSID=789; path=/foo', 'MOCKSESSID=456; path=/')),
);
}

public function testUnstartedSessionIsNotSave()
{
$this->sessionHasNotBeenStarted();
Expand All @@ -123,10 +153,10 @@ public function testDoesNotImplementServiceSubscriberInterface()
$this->assertFalse(is_subclass_of(TestSessionListener::class, ServiceSubscriberInterface::class, 'Implementing ServiceSubscriberInterface would create a dep on the DI component, which eg Silex cannot afford'));
}

private function filterResponse(Request $request, $type = HttpKernelInterface::MASTER_REQUEST)
private function filterResponse(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, Response $response = null)
{
$request->setSession($this->session);
$response = new Response();
$response = $response ?: new Response();
$kernel = $this->getMockBuilder('Symfony\Component\HttpKernel\HttpKernelInterface')->getMock();
$event = new FilterResponseEvent($kernel, $request, $type, $response);

Expand Down