Add authentication support to user context subscriber

[sprain]

This is a draft for #147, feedback is welcome! Tests will be added once the approach is accepted (I am new to all this caching stuff).

---

everything below is obsolete

I added the functionality to determine whether a request is anonymous based on session, basic authentication or oAuth2.

In my Symfony2 app, to only use oAuth2, I would overwrite getDefaultSubscribers in my own cache class like this:

class AppCache extends EventDispatchingHttpCache
{
    protected function getDefaultSubscribers()
    {
        $options = $this->getOptions();
        $subscribers = array();
        $defaultSubscribersOption = isset($options['fos_default_subscribers']) ? $options['fos_default_subscribers'] : self::SUBSCRIBER_ALL;
        if ($defaultSubscribersOption & self::SUBSCRIBER_USER_CONTEXT) {
            $subscribers[] = new UserContextSubscriber(array('authorization' => array('oauth2')));
        }

        return $subscribers;
    }
}

[ddeboer]

Does it make sense to separate the different authentication mechanisms (OAuth2, HTTP) or should we just check generically on whether an Authorization header is present? The problem I see with differentiating between mechanisms is that we can keep adding them: now e.g. OAuth1 (people still use that) is missing.

[dbu]

i would vote for a simpler approach in the sense of what varnish is doing: just check if a Authorization header exists and if so consider the request not anonymous. no configuration, no options. this is the expected behaviour to me, and if i need to tweak that i would extend the subscriber class to change the behaviour

[dbu]

@sprain and thanks a lot for your contribution! i appreciate that.

[sprain]

Simplifying sounds good ;) Will look at it.

[sprain]

Ok, I have simplified this. Any feedbacks?

[dbu]

lets do if ($request->server->has('HTTP_AUTHORIZATION') ... rather than loop through all the headers.

looking at the ServerBag::getHeaders method, i have the impression that the safe thing to do would be to check for any of the headers AUTHORIZATION, HTTP_AUTHORIZATION or PHP_AUTH_USER and consider the request not anonymous if any of those headers is present.

[dbu]

can you please add some tests like https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/tests/Unit/SymfonyCache/UserContextSubscriberTest.php#L120 but with AUTHORIZATION headers instead of session cookie? the anonymous case is already covered, but we should have a test where we do have one of these headers and end up being not anonymous.

[sprain]

Updated, please have a look.

[dbu]

what is this doing? and why not just pass true literally?

[sprain]

To be honest: I have no idea. I just tried to adjust the very complicated existing test from the session approach.

[ddeboer]

Argument $catch can be removed.

[dbu]

coming good, thanks!
@ddeboer can you also have a look at the test and think whether this is exactly what should happen? and do you agree with removing the complete cookie header if there is no single session cookie?

[dbu]

CS: please take the last ) on the same line as the {, so that we have ) {

[ddeboer]

@dbu Where is the cookie header removed completely?

[ddeboer]

You don't need a mock here: just construct a Response object directly with the context header.

[ddeboer]

Added some comments on the test; otherwise it’s valid.

@sprain Can you update the test and fix the coding standard violation mentioned by @dbu so we can merge this?

[dbu]

@ddeboer unless i misread https://github.com/sprain/FOSHttpCache/blob/master/src/SymfonyCache/UserContextSubscriber.php#L226 severely, there never is a cookie header on the hash lookup request, unless we manually set it in that method where i commented (and then removed my comments because i thought it less confusing.)

[sprain]

@ddeboer Updated.

[dbu]

@sprain can you please add a cookie header in the test (with a cookie that is not a session cookie), and assert that there is no cookie header in the hash lookup? if the basic auth header is copied along, i fear the cookie header might actually be copied as well...

[sprain]

@dbu @ddeboer Done!

[ddeboer]

assertCount is shorter

[dbu]

+1 for assertCount - its also more expressive when failing

[sprain]

@ddeboer done

[ddeboer]

@dbu Looks good to me now. Let’s merge?

[dbu]

looks good! so the cookie is not propagated. good we have a test that proves it.

now the last thing that is missing is the doc. @sprain can you please update https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/doc/symfony-cache-configuration.rst#user-context accordingly? right now its rather implicit that there is only session with cookies support - we should make it explicit that it can be both basic auth or a session cookie. think you manage that one? otherwise i can have a go at that.

[sprain]

@dbu Updated the docs. Please have a look at contents and format. I am not skilled with rst as I much more often use markdown.

[dbu]

s/php/PHP/

[dbu]

looks good to me. the spellchecker complains about php - not sure if PHP will be recognized - otherwise we just add PHP to the list of extra words it should know about.

[sprain]

@dbu Updated to PHP.

[dbu]

i did a tiny cleanup on the unit test in 437e05c (after convincing myself that this really is all correct)

thanks a lot @sprain !