always do a hash lookup, even for anonymous requests

Author: dbu

CHANGELOG.md

@@ -36,6 +36,8 @@ See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpC
 * Moved Varnish 4 and 5 configuration files from `resources/config/varnish-4/`
   to `resources/config/varnish/`.
 * Changed default Varnish version to 5.
+* Removed special case for anonymous users in user context behaviour. Varnish
+  now does a hash lookup for anonymous users as well.
 
 ### NGINX
 
@@ -49,8 +51,8 @@ See also the [GitHub releases page](https://github.com/FriendsOfSymfony/FOSHttpC
   options array for customization.
 * Provide a trait for the event dispatching kernel, instead of a base class.
   The trait offers both the addSubscriber and the addListener methods.
-* By default, no hash header is sent for anonymous users anymore, to sync
-  behaviour with Varnish behaviour.
+* The user context by default does not use a hardcoded hash for anonymous users
+  but does a hash lookup. You can still configure a hardcoded hash.
 
 ### Testing
 

doc/varnish-configuration.rst

@@ -203,11 +203,23 @@ User Context
 Feature: :doc:`user context hashing <user-context>`
 
 The ``fos_user_context.vcl`` needs the ``user_context_hash_url`` subroutine
-that sets a URL to the request lookup URL. The default URL is
+that sets the URL to do the hash lookup. The default URL is
 ``/_fos_user_context_hash`` and you can simply include
 ``resources/config/varnish-[version]/fos_user_context_url.vcl`` in your
-configuration to provide this. If you need a different URL, include a custom
-file implementing the ``user_context_hash_url`` subroutine.
+configuration to provide this. If you need a different URL, write your own
+``user_context_hash_url`` subroutine instead.
+
+.. tip::
+
+    The provided VCL to fetch the user hash restarts GET/HEAD requests. It
+    would be more efficient to do the hash lookup request with curl, using the
+    `curl Varnish plugin`_. If you can enable curl support, the recommended way
+    is to implement your own VCL to do a curl request for the hash lookup
+    instead of using the VCL provided here.
+
+    Also note that restarting a GET request leads to Varnish discarding the
+    body of the request. If you have some special case where you have GET
+    requests with a body, use curl.
 
 To enable this feature, add the following to ``your_varnish.vcl``:
 
@@ -265,13 +277,6 @@ To enable this feature, add the following to ``your_varnish.vcl``:
 Your backend application needs to respond to the ``application/vnd.fos.user-context-hash``
 request with :ref:`a proper user hash <return context hash>`.
 
-.. note::
-
-    We do not use ``X-Original-Url`` here, as the header will be sent to the
-    backend and the header has semantical meaning for some applications, which
-    would lead to problems. For example, the Microsoft IIS rewriting module
-    uses it, and consequently Symfony also looks into it to support IIS.
-
 .. tip::
 
     The provided VCL assumes that you want the context hash to be cached, so we
@@ -391,5 +396,6 @@ To enable this feature, add the following to ``your_varnish.vcl``:
 .. _banning for Varnish 3: https://www.varnish-software.com/book/3/Cache_invalidation.html#banning
 .. _ban lurker: https://www.varnish-software.com/blog/ban-lurker
 .. _explained in the Varnish documentation: https://www.varnish-cache.org/trac/wiki/VCLExampleRemovingSomeCookies#RemovingallBUTsomecookies
-.. _`builtin VCL`: https://www.varnish-cache.org/trac/browser/bin/varnishd/builtin.vcl?rev=4.0
-.. _`default VCL`: https://www.varnish-cache.org/trac/browser/bin/varnishd/default.vcl?rev=3.0
+.. _curl Varnish plugin: https://github.com/varnish/libvmod-curl
+.. _`builtin VCL`: https://github.com/varnishcache/varnish-cache/blob/5.0/bin/varnishd/builtin.vcl
+.. _`default VCL`: https://github.com/varnishcache/varnish-cache/blob/3.0/bin/varnishd/default.vcl

resources/config/varnish-3/fos_user_context.vcl

@@ -19,10 +19,9 @@ sub fos_user_context_recv {
     }
 
     # Lookup the context hash if there are credentials on the request
-    # Only do this for cacheable requests. Returning a hash lookup discards the request body.
+    # Note that the hash lookup discards the request body.
     # https://www.varnish-cache.org/trac/ticket/652
     if (req.restarts == 0
-        && (req.http.cookie || req.http.authorization)
         && (req.request == "GET" || req.request == "HEAD")
     ) {
         # Backup accept header, if set
@@ -31,9 +30,15 @@ sub fos_user_context_recv {
         }
         set req.http.accept = "application/vnd.fos.user-context-hash";
 
-        # Backup original URL
+        # Backup original URL.
+        #
+        # We do not use X-Original-Url here, as the header will be sent to the
+        # backend and X-Original-Url has semantical meaning for some applications.
+        # For example, the Microsoft IIS rewriting module uses it, and thus
+        # frameworks like Symfony also have to handle that header to integrate with IIS.
+
         set req.http.X-Fos-Original-Url = req.url;
-        
+
         call user_context_hash_url;
 
         # Force the lookup, the backend must tell not to cache or vary on all

resources/config/varnish/fos_user_context.vcl

@@ -19,10 +19,9 @@ sub fos_user_context_recv {
     }
 
     # Lookup the context hash if there are credentials on the request
-    # Only do this for cacheable requests. Returning a hash lookup discards the request body.
+    # Note that the hash lookup discards the request body.
     # https://www.varnish-cache.org/trac/ticket/652
     if (req.restarts == 0
-        && (req.http.cookie || req.http.authorization)
         && (req.method == "GET" || req.method == "HEAD")
     ) {
         # Backup accept header, if set
@@ -31,9 +30,15 @@ sub fos_user_context_recv {
         }
         set req.http.accept = "application/vnd.fos.user-context-hash";
 
-        # Backup original URL
+        # Backup original URL.
+        #
+        # We do not use X-Original-Url here, as the header will be sent to the
+        # backend and X-Original-Url has semantical meaning for some applications.
+        # For example, the Microsoft IIS rewriting module uses it, and thus
+        # frameworks like Symfony also have to handle that header to integrate with IIS.
+
         set req.http.X-Fos-Original-Url = req.url;
-        
+
         call user_context_hash_url;
 
         # Force the lookup, the backend must tell not to cache or vary on all

src/SymfonyCache/UserContextListener.php

@@ -43,7 +43,7 @@ class UserContextListener implements EventSubscriberInterface
     /**
      * When creating this listener, you can configure a number of options.
      *
-     * - anonymous_hash:          Hash used for anonymous user.
+     * - anonymous_hash:          Hash used for anonymous user. Hash lookup skipped for anonymous if this is set.
      * - user_hash_accept_header: Accept header value to be used to request the user hash to the
      *                            backend application. Must match the setup of the backend application.
      * - user_hash_header:        Name of the header the user context hash will be stored into. Must
@@ -162,7 +162,7 @@ private function getUserHash(HttpKernelInterface $kernel, Request $request)
             return $this->userHash;
         }
 
-        if ($this->isAnonymous($request)) {
+        if ($this->options['anonymous_hash'] && $this->isAnonymous($request)) {
             return $this->userHash = $this->options['anonymous_hash'];
         }
 

tests/Functional/Symfony/EventDispatchingHttpCacheTest.php

@@ -50,7 +50,11 @@ public function testEventListeners()
         $kernel->addSubscriber(new DebugListener());
         $kernel->addSubscriber(new PurgeListener());
         $kernel->addSubscriber(new RefreshListener());
-        $kernel->addSubscriber(new UserContextListener());
+        $kernel->addSubscriber(new UserContextListener([
+            // avoid having to provide mocking for the hash lookup
+            // we already test anonymous hash lookup in the UserContextListener unit test
+            'anonymous_hash' => 'abcdef',
+        ]));
 
         $response = $kernel->handle($request);
         $this->assertSame($expectedResponse, $response);

tests/Unit/SymfonyCache/UserContextListenerTest.php

@@ -98,21 +98,60 @@ public function testPassingUserHashNotAllowed($arg, $options)
     public function testUserHashAnonymous($arg, $options)
     {
         $userContextListener = new UserContextListener($arg);
-
         $request = new Request();
 
-        $event = new CacheEvent($this->kernel, $request);
-
-        $userContextListener->preHandle($event);
-        $response = $event->getResponse();
-
-        $this->assertNull($response);
         if ($options['anonymous_hash']) {
+            $event = new CacheEvent($this->kernel, $request);
+            $userContextListener->preHandle($event);
+
             $this->assertTrue($request->headers->has($options['user_hash_header']));
             $this->assertSame($options['anonymous_hash'], $request->headers->get($options['user_hash_header']));
         } else {
-            $this->assertFalse($request->headers->has($options['user_hash_header']));
+            $hashRequest = Request::create($options['user_hash_uri'], $options['user_hash_method'], [], [], [], $request->server->all());
+            $hashRequest->attributes->set('internalRequest', true);
+            $hashRequest->headers->set('Accept', $options['user_hash_accept_header']);
+            // Ensure request properties have been filled up.
+            $hashRequest->getPathInfo();
+            $hashRequest->getMethod();
+
+            $expectedContextHash = 'my_generated_hash';
+            // Just avoid the response to modify the request object, otherwise it's impossible to test objects equality.
+            /** @var \Symfony\Component\HttpFoundation\Response|\PHPUnit_Framework_MockObject_MockObject $hashResponse */
+            $hashResponse = $this->getMockBuilder('\Symfony\Component\HttpFoundation\Response')
+                ->setMethods(['prepare'])
+                ->getMock();
+            $hashResponse->headers->set($options['user_hash_header'], $expectedContextHash);
+
+            $that = $this;
+            $kernel = $this->kernel
+                ->shouldReceive('handle')
+                ->once()
+                ->with(
+                    \Mockery::on(
+                        function (Request $request) use ($that, $hashRequest) {
+                            // we need to call some methods to get the internal fields initialized
+                            $request->getMethod();
+                            $request->getPathInfo();
+                            $that->assertEquals($hashRequest, $request);
+                            $that->assertCount(0, $request->cookies->all());
+
+                            return true;
+                        }
+                    )
+                )
+                ->andReturn($hashResponse)
+                ->getMock();
+
+            $event = new CacheEvent($kernel, $request);
+            $userContextListener->preHandle($event);
+
+            $this->assertTrue($request->headers->has($options['user_hash_header']));
+            $this->assertSame($expectedContextHash, $request->headers->get($options['user_hash_header']));
         }
+
+        $response = $event->getResponse();
+
+        $this->assertNull($response);
     }
 
     /**