Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions DependencyInjection/FOSHttpCacheExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,9 @@ private function loadUserContext(ContainerBuilder $container, XmlFileLoader $loa
->replaceArgument(3, $config['user_hash_header'])
->replaceArgument(4, $config['hash_cache_ttl']);

$container->getDefinition($this->getAlias().'.user_context.anonymous_request_matcher')
->replaceArgument(0, $config['user_identifier_headers']);

if ($config['logout_handler']['enabled']) {
$container->getDefinition($this->getAlias().'.user_context.logout_handler')
->replaceArgument(1, $config['user_identifier_headers'])
Expand Down
30 changes: 28 additions & 2 deletions EventListener/UserContextSubscriber.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use FOS\HttpCache\UserContext\HashGenerator;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestMatcherInterface;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
Expand Down Expand Up @@ -61,12 +62,22 @@ class UserContextSubscriber implements EventSubscriberInterface
*/
private $ttl;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

and please explain here that not booting the symfony kernel is the reason why we use this matcher rather than just looking at symfony and see if there is a non-anonymous user.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, its not only performance. if we would boot the kernel, we would treat the expired session as anonymous which is exactly what #274 wanted to prevent. maybe explain the thing about anonymous fake hashes in the phpdoc here so we still know why this is done like this.

Copy link
Author

Choose a reason for hiding this comment

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

Added a comment.

* Used to exclude anonymous requests (no authentication nor session) from user hash sanity check.
* It prevents issues when the hash generator that is used returns a customized value for anonymous users,
* that differs from the documented, hardcoded one.
*
* @var RequestMatcherInterface
*/
private $anonymousRequestMatcher;

public function __construct(
RequestMatcherInterface $requestMatcher,
HashGenerator $hashGenerator,
array $userIdentifierHeaders = array('Cookie', 'Authorization'),
$hashHeader = 'X-User-Context-Hash',
$ttl = 0
$ttl = 0,
RequestMatcherInterface $anonymousRequestMatcher = null
) {
if (!count($userIdentifierHeaders)) {
throw new \InvalidArgumentException('The user context must vary on some request headers');
Expand All @@ -76,6 +87,7 @@ public function __construct(
$this->userIdentifierHeaders = $userIdentifierHeaders;
$this->hashHeader = $hashHeader;
$this->ttl = $ttl;
$this->anonymousRequestMatcher = $anonymousRequestMatcher;
}

/**
Expand All @@ -93,7 +105,7 @@ public function onKernelRequest(GetResponseEvent $event)
}

if (!$this->requestMatcher->matches($event->getRequest())) {
if ($event->getRequest()->headers->has($this->hashHeader)) {
if ($event->getRequest()->headers->has($this->hashHeader) && !$this->isAnonymous($event->getRequest())) {
$this->hash = $this->hashGenerator->generateHash();
}

Expand All @@ -120,6 +132,20 @@ public function onKernelRequest(GetResponseEvent $event)
$event->setResponse($response);
}

/**
* Tests if $request is an anonymous request or not.
*
* For backward compatibility reasons, true will be returned if no anonymous request matcher was provided.
*
* @param Request $request
*
* @return bool
*/
private function isAnonymous(Request $request)
{
return $this->anonymousRequestMatcher ? $this->anonymousRequestMatcher->matches($request) : false;
}

/**
* Add the context hash header to the headers to vary on if the header was
* present in the request.
Expand Down
5 changes: 5 additions & 0 deletions Resources/config/user_context.xml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
<argument />
<argument />
<argument />
<argument type="service" id="fos_http_cache.user_context.anonymous_request_matcher" />
<tag name="kernel.event_subscriber" />
</service>

Expand All @@ -39,5 +40,9 @@
<argument />
<argument />
</service>

<service id="fos_http_cache.user_context.anonymous_request_matcher" class="FOS\HttpCacheBundle\UserContext\AnonymousRequestMatcher">
<argument type="collection" />
</service>
</services>
</container>
33 changes: 33 additions & 0 deletions Tests/Unit/EventListener/UserContextSubscriberTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,39 @@ public function testFullRequestHashOk()
$this->assertContains('X-Hash', $event->getResponse()->headers->get('Vary'));
}

/**
* If the request is an anonymous one, no hash should be generated/validated.
*/
public function testFullAnonymousRequestHashNotGenerated()
{
$request = new Request();
$request->setMethod('GET');
$request->headers->set('X-Hash', 'anonymous-hash');

$requestMatcher = $this->getRequestMatcher($request, false);
$hashGenerator = \Mockery::mock('\FOS\HttpCache\UserContext\HashGenerator');
$hashGenerator->shouldReceive('generateHash')->never();

$anonymousRequestMatcher = \Mockery::mock('\Symfony\Component\HttpFoundation\RequestMatcherInterface');
$anonymousRequestMatcher->shouldReceive('matches')->andReturn(true);

// onKernelRequest
$userContextSubscriber = new UserContextSubscriber($requestMatcher, $hashGenerator, array('X-SessionId'), 'X-Hash', 0, $anonymousRequestMatcher);
$event = $this->getKernelRequestEvent($request);

$userContextSubscriber->onKernelRequest($event);

$response = $event->getResponse();

$this->assertNull($response);

// onKernelResponse
$event = $this->getKernelResponseEvent($request);
$userContextSubscriber->onKernelResponse($event);

$this->assertContains('X-Hash', $event->getResponse()->headers->get('Vary'));
}

/**
* If there is no hash in the requests but session changed, prevent setting bad cache.
*/
Expand Down
69 changes: 69 additions & 0 deletions Tests/Unit/UserContext/AnonymousRequestMatcherTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?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\Tests\Unit\UserContext;

use FOS\HttpCacheBundle\UserContext\AnonymousRequestMatcher;
use PHPUnit_Framework_TestCase;
use Symfony\Component\HttpFoundation\Request;

class AnonymousRequestMatcherTest extends PHPUnit_Framework_TestCase
{
public function testMatchAnonymousRequest()
{
$request = new Request();

$requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);

$this->assertTrue($requestMatcher->matches($request));
}

public function testNoMatchIfCookie()
{
$request = new Request();
$request->headers->set('Cookie', 'PHPSESSID7e476fc9f29f69d2ad6f11dbcd663b42=25f6d9c5a843e3c948cd26902385a527');
$request->cookies->set('PHPSESSID7e476fc9f29f69d2ad6f11dbcd663b42', '25f6d9c5a843e3c948cd26902385a527');

$requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);

$this->assertFalse($requestMatcher->matches($request));
}

public function testNoMatchIfEmptyCookieHeader()
{
$request = new Request();
$request->headers->set('Cookie', '');

$requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);

$this->assertTrue($requestMatcher->matches($request));
}

public function testNoMatchIfAuthenticationHeader()
{
$request = new Request();
$request->headers->set('Authorization', 'foo: bar');

$requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);

$this->assertFalse($requestMatcher->matches($request));
}

public function testMatchEmptyCookieHeaderHeader()
{
$request = new Request();
$request->headers->set('Cookie', '');

$requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);

$this->assertTrue($requestMatcher->matches($request));
}
}
47 changes: 47 additions & 0 deletions UserContext/AnonymousRequestMatcher.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?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\UserContext;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestMatcherInterface;

/**
* Matches anonymous requests using a list of identification headers.
*/
class AnonymousRequestMatcher implements RequestMatcherInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a class level comment to explain that this matcher matches all requests that are anonymous.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add tests for this matcher?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I will.

{
private $userIdentifierHeaders;

/**
* @param array $userIdentifierHeaders List of request headers that authenticate a non-anonymous request
*/
public function __construct(array $userIdentifierHeaders)
{
$this->userIdentifierHeaders = $userIdentifierHeaders;
}

public function matches(Request $request)
{
foreach ($this->userIdentifierHeaders as $header) {
if ($request->headers->has($header)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, one more question: what does symfony request do with the cookie header? is that removed from the header bag? what happens if we have a request with only an empty cookie header? won't we then get in here and see the cookie header, making the previous if on the cookies count kind of pointless? should we do

if ($request->headers->has($header)) {
    if (strtolower($header) === 'cookie' && 0 === $request->cookies->count()) {
        // ignore empty cookie header
        continue;
    }

    return false;
}

Copy link
Author

Choose a reason for hiding this comment

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

Done, with test, @dbu.

Copy link
Contributor

Choose a reason for hiding this comment

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

the first if with cookie header and nonzero number of cookies is now redundant and should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the if, adjusted the existing test, and added a new case.

if (strtolower($header) === 'cookie' && 0 === $request->cookies->count()) {
// ignore empty cookie header
continue;
}

return false;
}
}

return true;
}
}
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"mockery/mockery": "0.9.*",
"monolog/monolog": "*",
"sensio/framework-extra-bundle": "^2.3||^3.0",
"symfony/symfony": "^2.3||^3.0",
"symfony/symfony": "^2.3.4||^3.0",
"symfony/phpunit-bridge": "^2.7||^3.0",
"symfony/expression-language": "^2.4||^3.0",
"symfony/monolog-bundle": "^2.3||^3.0",
Expand Down