Skip to content

Implemented Symfony reverse proxy support for user context hash. #152

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.

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
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
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ branches:
- /^\d+\.\d+$/

matrix:
allow_failures:
- php: hhvm
include:
- php: 5.5
env: SYMFONY_VERSION='2.3.*'
Expand Down
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
Changelog
=========

1.1.0
-----

* **2014-10-14** Allow cache headers overwrite.
* **2014-10-29** Added support for the user context lookup with Symfony built-in
reverse proxy, aka `HttpCache`.

1.0.0
-----

Expand Down
1 change: 1 addition & 0 deletions EventListener/UserContextSubscriber.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ public function onKernelRequest(GetResponseEvent $event)
if ($this->ttl > 0) {
$response->setClientTtl($this->ttl);
$response->setVary($this->userIdentifierHeaders);
$response->setPublic();
Copy link
Contributor

Choose a reason for hiding this comment

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

is this to have the context lookup response being cached?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. Without it, HttpCache won't cache it... Varnish doesn't care by default, though we added a specific rule in VCL for eZ.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, then this is a very important fix indeed. symfony docs explicitly recommend respecting private and varnish 4 respects it by default...

seems we should have a test that the hash lookup is cached (not in this PR), we should see a failure on varnish 4 for that test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(not in this PR)

Do you mean it's not present and it should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

no, that is not needed, see https://www.varnish-cache.org/trac/browser/bin/varnishd/builtin.vcl?rev=5c5634dbd9211c4b784268166709a22177d643de#L159 - i would remove it to avoid confusion.

about my comment regarding a missing test for the context being cached by the caches, i created #154

} else {
$response->setClientTtl(0);
$response->headers->addCacheControlDirective('no-cache');
Expand Down
188 changes: 188 additions & 0 deletions HttpCache.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
<?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;

use Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache as BaseHttpCache;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\HttpKernelInterface;

/**
* Base class for enhanced Symfony reverse proxy.
*
* @author Jérôme Vieilledent <[email protected]> (courtesy of eZ Systems AS)
*
* {@inheritdoc}
*/
abstract class HttpCache extends BaseHttpCache
{
/**
* Hash for anonymous user.
*/
const ANONYMOUS_HASH = '38015b703d82206ebc01d17a39c727e5';
Copy link
Member

Choose a reason for hiding this comment

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

could this hash conflict with a hash for an authenticated user ? How is it built ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it couldn't. This one is computed with md5 while user context hashes are generated with sha256.
And this actually means something 😉


/**
* Accept header value to be used to request the user hash to the backend application.
* It must match the one defined in FOSHttpCacheBundle's configuration.
*/
const USER_HASH_ACCEPT_HEADER = 'application/vnd.fos.user-context-hash';
Copy link
Contributor

Choose a reason for hiding this comment

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

what about making the constants private properties instead that are injected through the constructor? then a user could overwrite them much easier when extending the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this would complexify things IMHO. Constants are very easy to customize in that case thanks to late static binding. And in the case of several apps in a project, you would duplicate this class as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, was not aware of this. i thought constant would mean its
unchangeable :-)
for the doc, could you please show an example in the chapter we add, of
how customization would look? i assume other people also are not aware
of this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll do that :-)

Copy link
Member

Choose a reason for hiding this comment

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

Symfony already defines a concept of options in the HttpCache, and the end-user can overwrite a getOptions method to pass their own values for options. Relying on this would make more sense than overwriting constants IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could avoid changing constructor signature using $options with dedicated keys (e.g. fos_user_hash_header), but I still find the syntax cumbersome for the end developer...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking about it and trying a few things, I couldn't find a clean approach to get a dedicated $fosOptions apart from doing the following:

abstract class HttpCache extends BaseHttpCache
{
    public function __contruct(HttpKernelInterface $kernel, $cacheDir = null)
    {
        $this->fosOptions = $this->getFosOptions() + array(
            'user_has_header' => 'X-User-Context-Hash',
            'user_hash_uri' => '/_fos_user_context_hash',
            // ...
        );
        parent::__construct($kernel, $cacheDir);
    }

    /**
     * One might override this method
     */
    protected function getFosOptions()
    {
        return array();
    }
}

But I don't find this pretty 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, the signature of the HttpCache constructor has $options. why not have this and then keep a copy of $options for us?

public function __construct(HttpKernelInterface $kernel, StoreInterface $store, Esi $esi = null, array $options = array())
{
$options = array_merge(array(
            'user_has_header' => 'X-User-Context-Hash',
            'user_hash_uri' => '/_fos_user_context_hash',
            // ...
), $options);
$this->fosOptions = $options;
parent::__construct(...);

i see the thing about missing getOption on the HttpCache, otherwise we would not need a $fosOptions property. we can then follow the same logic and have a private $fosOptions method. and, if you want, a getFosOption() method but i think it should be ok to set all options in constructor and not later. they should not be dynamic i guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, the signature of the HttpCache constructor has $options. why not have this and then keep a copy of $options for us?

Because it's ugly ? 😄
More seriously, I really don't like the idea of keeping unrelated information. Sounds more like a hack where the constant solution keep it clean.

but i think it should be ok to set all options in constructor and not later. they should not be dynamic i guess.

This is precisely why I use constants 😉.
IMHO this would be complex for no gain in the developer perspective. In base HttpCache you have getOptions() that you can override... That's why I'm proposing this alternative, even if I don't like it much.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, lets leave as is.


/**
* Name of the header the user context hash will be stored into.
* It must match the one defined in FOSHttpCacheBundle's configuration.
*/
const USER_HASH_HEADER = 'X-User-Context-Hash';

/**
* URI used with the forwarded request for user context hash generation.
*/
const USER_HASH_URI = '/_fos_user_context_hash';

/**
* HTTP Method used with the forwarded request for user context hash generation.
*/
const USER_HASH_METHOD = 'GET';

/**
* Prefix for session names.
* Must match your session configuration.
*/
const SESSION_NAME_PREFIX = 'PHPSESSID';

/**
* Generated user hash.
*
* @var string
*/
private $userHash;

public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, $catch = true)
{
if (!$this->isInternalRequest($request)) {
// Prevent tampering attacks on the hash mechanism
if ($request->headers->get('accept') === static::USER_HASH_ACCEPT_HEADER
|| $request->headers->get(static::USER_HASH_HEADER) !== null) {
return new Response('', 400);
}

if ($request->isMethodSafe()) {
$request->headers->set(static::USER_HASH_HEADER, $this->getUserHash($request));
}
}

return parent::handle($request, $type, $catch);
}

/**
* Checks if passed request object is to be considered internal (e.g. for user hash lookup).
*
* @param Request $request
*
* @return bool
*/
private function isInternalRequest(Request $request)
{
return $request->attributes->get('internalRequest', false) === true;
Copy link
Contributor

Choose a reason for hiding this comment

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

is there nothing more explicit for this? if there isn't, this solution seems ok to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I could think about, there is no req.restart like in Varnish 😉

}

/**
* Returns the user context hash for $request.
*
* @param Request $request
*
* @return string
*/
private function getUserHash(Request $request)
{
if (isset($this->userHash)) {
return $this->userHash;
}

if ($this->isAnonymous($request)) {
return $this->userHash = static::ANONYMOUS_HASH;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a performance shortcut with a hardcoded hash for anonymous, right? as the hash lookup is cached, the price seems not that high to me, but still on many cms sites 99% will be anonymous, so it might still be worth 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.

Yes it's cached, but having it hardcoded is much much faster for complete anonymous users.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ddboer we might want to propose the same idea in the varnish config for
user context, as an optional optimization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

i created FriendsOfSymfony/FOSHttpCache#132 for this. @lolautruche did you try how relevant this optimization is when using varnish? for the HttpCache php class i totally see its relevant, but do you have any numbers how many milliseconds / load this saves on varnish once the anonymous hash request is cached?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I never benchmarked this. It's saving a whole request restart anyway so it definitely worths it IMHO, even if it's saving a few milliseconds.


// Forward the request to generate the user hash
$forwardReq = $this->generateForwardRequest($request);
$resp = $this->handle($forwardReq);
Copy link
Contributor

Choose a reason for hiding this comment

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

this request will be cached by the HttpCache kernel class, right? otherwise it would be very expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it will. Tested 1.000 times 😃

// Store the user hash in memory for sub-requests (processed in the same thread).
$this->userHash = $resp->headers->get(static::USER_HASH_HEADER);

return $this->userHash;
}

/**
* Checks if current request is considered anonymous.
*
* @param Request $request
*
* @return bool
*/
private function isAnonymous(Request $request)
{
foreach ($request->cookies as $name => $value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

makes me wonder: what about other authentication methods? basic authentication with the "Authorization" header? or custom authentication methods.

i think this method needs to be protected too (sorry @ddeboer, i agree with private as much as we can, but seems here we can't).

then for basic auth, you would instead check if that header is not present, and also overwrite the cleanup method to unconditionally remove all cookies.

if ($this->isSessionName($name)) {
return false;
}
}

return true;
}

/**
* Checks if passed string can be considered as a session name, such as would be used in cookies.
*
* @param string $name
*
* @return bool
*/
private function isSessionName($name)
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, i think we should keep this protected. i could have a different logic or support a legacy cookie or something like that.

{
return strpos($name, static::SESSION_NAME_PREFIX) === 0;
}

/**
* Generates the request object that will be forwarded to get the user context hash.
*
* @param Request $request
*
* @return Request
*/
private function generateForwardRequest(Request $request)
{
$forwardReq = Request::create(static::USER_HASH_URI, static::USER_HASH_METHOD, array(), array(), array(), $request->server->all());
$forwardReq->attributes->set('internalRequest', true);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this also done by symfony in other cases of internal requests? or is this your way of transporting the information that this is a valid request for the hash through the call to handle()? if its the later, can't we do this simpler, e.g. have a dedicated method that calls parent::handle at the end and have our handle() unconditionally refuse hash requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not used anywhere in Symfony, this is just a convenient flag used in handle() method only. I don't understand your proposal. Example?

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line, but in getUserHash do $this->handleInternalRequest which just calls parent::handleInternalRequest.

okay, i see the problem, if the user extends handle() it could be surprising. might not be the best idea then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, can I leave it as is ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah i think its better than my proposal.

(just trying to challenge your solutions to see if it could be simpler)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:octocat:

$forwardReq->headers->set('Accept', static::USER_HASH_ACCEPT_HEADER);
$this->cleanupForwardRequest($forwardReq, $request);

return $forwardReq;
}

/**
* Cleans up request to forward for user hash generation.
* Cleans cookie header to only get proper sessionIds in it. This is to make the hash request cacheable.
*
* @param Request $forwardReq
* @param Request $originalRequest
*/
protected function cleanupForwardRequest(Request $forwardReq, Request $originalRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that was the whole point why we moved that into its own method, so that it can be tweaked.

{
$sessionIds = array();
foreach ($originalRequest->cookies as $name => $value) {
if ( $this->isSessionName($name)) {
$sessionIds[$name] = $value;
$forwardReq->cookies->set($name, $value);
}
}
$forwardReq->headers->set('Cookie', http_build_query($sessionIds, '', '; '));
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, i think this is more robust now.

}
}
1 change: 1 addition & 0 deletions Resources/doc/features.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ corresponding reference section.
features/user-context
features/helpers
features/testing
features/symfony-http-cache
82 changes: 82 additions & 0 deletions Resources/doc/features/symfony-http-cache.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
Symfony HttpCache
=================

Symfony comes with a built-in reverse proxy written in PHP, known as
``HttpCache``. It can be useful when one hosts a Symfony application on shared
hosting for instance
(see [HttpCache documentation](http://symfony.com/doc/current/book/http_cache.html#symfony-reverse-proxy).

If you use Symfony ``HttpCache``, you'll need to make your ``AppCache`` class
extend ``FOS\HttpCacheBundle\HttpCache`` instead of
``Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache``.

.. warning::

Symfony HttpCache support is currently limited to following features:

* User context

Class constants
---------------

``FOS\HttpCacheBundle\HttpCache`` defines constants that can easily be overriden
in your ``AppCache`` class:

.. code-block:: php

use FOS\HttpCacheBundle\HttpCache;

class AppCache extends HttpCache
{
/**
* Overriding default value for SESSION_NAME_PREFIX
* to use eZSESSID instead.
*/
const SESSION_NAME_PREFIX = 'eZSESSID';
}

User context
~~~~~~~~~~~~

.. note::

For detailed information on user context, please read the
`user context documentation page </features/user-context>`

* ``SESSION_NAME_PREFIX``: Prefix for session names. Must match your session
configuration.
Needed for caching correctly generated user context hash for each user session.

**default**: ``PHPSESSID``

.. warning::

If you have a customized session name, it is **very important** that this
constant matches it.
Session IDs are indeed used as keys to cache the generated use context hash.

Wrong session name will lead to unexpected results such as having the same
user context hash for every users,
or not having it cached at all (painful for performance.

* ``USER_HASH_ACCEPT_HEADER``: Accept header value to be used to request the
user hash to the backend application.
It must match the one defined in FOSHttpCacheBundle's configuration (see below).

**default**: ``application/vnd.fos.user-context-hash``

* ``USER_HASH_HEADER``: Name of the header the user context hash will be stored
into.
It must match the one defined in FOSHttpCacheBundle's configuration (see below).

**default**: ``X-User-Context-Hash``

* ``USER_HASH_URI``: URI used with the forwarded request for user context hash
generation.

**default**: ``/_fos_user_context_hash``

* ``USER_HASH_METHOD``: HTTP Method used with the forwarded request for user
context hash generation.

**default**: ``GET``
10 changes: 9 additions & 1 deletion Resources/doc/reference/configuration/user-context.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,17 @@ Configuration
Caching Proxy Configuration
~~~~~~~~~~~~~~~~~~~~~~~~~~~

First you need to set up your caching proxy as explained in the
Varnish
"""""""

Set up Varnish caching proxy as explained in the
:ref:`user context documentation <foshttpcache:user-context>`.

Symfony reverse proxy
"""""""""""""""""""""

Set up Symfony reverse proxy as explained in the :doc:`Symfony HttpCache dedicated documentation page </features/symfony-http-cache>`.

Copy link
Contributor

Choose a reason for hiding this comment

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

one blank line too much.

Context Hash Route
~~~~~~~~~~~~~~~~~~

Expand Down
2 changes: 1 addition & 1 deletion Tests/Unit/EventListener/UserContextSubscriberTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public function testOnKernelRequestCached()
$this->assertInstanceOf('\Symfony\Component\HttpFoundation\Response', $response);
$this->assertEquals('hash', $response->headers->get('X-Hash'));
$this->assertEquals('X-SessionId', $response->headers->get('Vary'));
$this->assertEquals('max-age=30, private', $response->headers->get('Cache-Control'));
$this->assertEquals('max-age=30, public', $response->headers->get('Cache-Control'));
}

public function testOnKernelRequestNotMatched()
Expand Down
Loading