From cad6b5d5b100cf794f6b17f33c229132d343c9e3 Mon Sep 17 00:00:00 2001 From: Bertrand Dunogier Date: Thu, 3 Mar 2016 18:11:11 +0100 Subject: [PATCH 1/5] Do not generate hash for anonymous requests --- DependencyInjection/FOSHttpCacheExtension.php | 3 ++ EventListener/UserContextSubscriber.php | 30 +++++++++++- Resources/config/user_context.xml | 5 ++ .../UserContextSubscriberTest.php | 33 +++++++++++++ .../AnonymousRequestMatcherTest.php | 48 +++++++++++++++++++ UserContext/AnonymousRequestMatcher.php | 45 +++++++++++++++++ 6 files changed, 162 insertions(+), 2 deletions(-) create mode 100644 Tests/Unit/UserContext/AnonymousRequestMatcherTest.php create mode 100644 UserContext/AnonymousRequestMatcher.php diff --git a/DependencyInjection/FOSHttpCacheExtension.php b/DependencyInjection/FOSHttpCacheExtension.php index 20fabc42..1a49b545 100644 --- a/DependencyInjection/FOSHttpCacheExtension.php +++ b/DependencyInjection/FOSHttpCacheExtension.php @@ -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']) diff --git a/EventListener/UserContextSubscriber.php b/EventListener/UserContextSubscriber.php index 4ea6ac88..ebb31a7a 100644 --- a/EventListener/UserContextSubscriber.php +++ b/EventListener/UserContextSubscriber.php @@ -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; @@ -61,12 +62,22 @@ class UserContextSubscriber implements EventSubscriberInterface */ private $ttl; + /** + * 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'); @@ -76,6 +87,7 @@ public function __construct( $this->userIdentifierHeaders = $userIdentifierHeaders; $this->hashHeader = $hashHeader; $this->ttl = $ttl; + $this->anonymousRequestMatcher = $anonymousRequestMatcher; } /** @@ -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(); } @@ -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. diff --git a/Resources/config/user_context.xml b/Resources/config/user_context.xml index 243132c2..773e4bd4 100644 --- a/Resources/config/user_context.xml +++ b/Resources/config/user_context.xml @@ -27,6 +27,7 @@ + @@ -39,5 +40,9 @@ + + + + diff --git a/Tests/Unit/EventListener/UserContextSubscriberTest.php b/Tests/Unit/EventListener/UserContextSubscriberTest.php index 3758698b..2627b208 100644 --- a/Tests/Unit/EventListener/UserContextSubscriberTest.php +++ b/Tests/Unit/EventListener/UserContextSubscriberTest.php @@ -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. */ diff --git a/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php b/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php new file mode 100644 index 00000000..7ac3656e --- /dev/null +++ b/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php @@ -0,0 +1,48 @@ + + * + * 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->cookies->set('PHPSESSID7e476fc9f29f69d2ad6f11dbcd663b42', '25f6d9c5a843e3c948cd26902385a527'); + + $requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']); + + $this->assertFalse($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)); + } +} diff --git a/UserContext/AnonymousRequestMatcher.php b/UserContext/AnonymousRequestMatcher.php new file mode 100644 index 00000000..e2acde11 --- /dev/null +++ b/UserContext/AnonymousRequestMatcher.php @@ -0,0 +1,45 @@ + + * + * 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 +{ + 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 (strtolower($header) === 'cookie' && $request->cookies->count()) { + return false; + } + if ($request->headers->has($header)) { + return false; + } + } + + return true; + } +} From 64921607a3d85ccf5925fa0694633ef33295c4aa Mon Sep 17 00:00:00 2001 From: Bertrand Dunogier Date: Tue, 5 Jul 2016 13:59:07 +0200 Subject: [PATCH 2/5] Require symfony/symfony:2.3.4 Fixes the "Unexpected character in input" notice that happens on Travis, when running tests using the phar version. --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 98ff0ee4..bcc9cdb0 100644 --- a/composer.json +++ b/composer.json @@ -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", From fe38fb509e99e8f9e79b639d87aa3e1e49eda510 Mon Sep 17 00:00:00 2001 From: Bertrand Dunogier Date: Thu, 25 Aug 2016 09:40:43 +0200 Subject: [PATCH 3/5] Ignored empty cookie header in anonymous request matcher --- Tests/Unit/UserContext/AnonymousRequestMatcherTest.php | 10 ++++++++++ UserContext/AnonymousRequestMatcher.php | 5 +++++ 2 files changed, 15 insertions(+) diff --git a/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php b/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php index 7ac3656e..4cb52874 100644 --- a/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php +++ b/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php @@ -45,4 +45,14 @@ public function testNoMatchIfAuthenticationHeader() $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)); + } } diff --git a/UserContext/AnonymousRequestMatcher.php b/UserContext/AnonymousRequestMatcher.php index e2acde11..1c12b872 100644 --- a/UserContext/AnonymousRequestMatcher.php +++ b/UserContext/AnonymousRequestMatcher.php @@ -36,6 +36,11 @@ public function matches(Request $request) return false; } if ($request->headers->has($header)) { + if (strtolower($header) === 'cookie' && 0 === $request->cookies->count()) { + // ignore empty cookie header + continue; + } + return false; } } From d19e9fa443beffe6e80d06b7f3d891ceb4e99555 Mon Sep 17 00:00:00 2001 From: Bertrand Dunogier Date: Mon, 29 Aug 2016 10:31:26 +0200 Subject: [PATCH 4/5] fixup! Ignored empty cookie header in anonymous request matcher --- Tests/Unit/UserContext/AnonymousRequestMatcherTest.php | 1 + UserContext/AnonymousRequestMatcher.php | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php b/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php index 4cb52874..e9b329aa 100644 --- a/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php +++ b/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php @@ -29,6 +29,7 @@ public function testMatchAnonymousRequest() public function testNoMatchIfCookie() { $request = new Request(); + $request->headers->set('Cookie', ''); $request->cookies->set('PHPSESSID7e476fc9f29f69d2ad6f11dbcd663b42', '25f6d9c5a843e3c948cd26902385a527'); $requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']); diff --git a/UserContext/AnonymousRequestMatcher.php b/UserContext/AnonymousRequestMatcher.php index 1c12b872..4d251b17 100644 --- a/UserContext/AnonymousRequestMatcher.php +++ b/UserContext/AnonymousRequestMatcher.php @@ -32,9 +32,6 @@ public function __construct(array $userIdentifierHeaders) public function matches(Request $request) { foreach ($this->userIdentifierHeaders as $header) { - if (strtolower($header) === 'cookie' && $request->cookies->count()) { - return false; - } if ($request->headers->has($header)) { if (strtolower($header) === 'cookie' && 0 === $request->cookies->count()) { // ignore empty cookie header From 0990981f87b0edd55a957273334f298d108aac76 Mon Sep 17 00:00:00 2001 From: Bertrand Dunogier Date: Mon, 29 Aug 2016 10:33:57 +0200 Subject: [PATCH 5/5] fixup! Ignored empty cookie header in anonymous request matcher --- .../Unit/UserContext/AnonymousRequestMatcherTest.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php b/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php index e9b329aa..f4de981e 100644 --- a/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php +++ b/Tests/Unit/UserContext/AnonymousRequestMatcherTest.php @@ -29,7 +29,7 @@ public function testMatchAnonymousRequest() public function testNoMatchIfCookie() { $request = new Request(); - $request->headers->set('Cookie', ''); + $request->headers->set('Cookie', 'PHPSESSID7e476fc9f29f69d2ad6f11dbcd663b42=25f6d9c5a843e3c948cd26902385a527'); $request->cookies->set('PHPSESSID7e476fc9f29f69d2ad6f11dbcd663b42', '25f6d9c5a843e3c948cd26902385a527'); $requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']); @@ -37,6 +37,16 @@ public function testNoMatchIfCookie() $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();