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

lolautruche · 2014-10-20T13:09:21Z

Ref #26
Relates to ezsystems/ezpublish-kernel#1025

Finally the PR 😄
This adds support for Symfony RP for user context hash feature.

For this to work, you need to:

Make your AppCache class extend FOS\HttpCacheBundle\HttpCache instead of Symfony\Bundle\FrameworkBundle\HttpCache\HttpCache.
Ensure USER_HASH_ACCEPT_HEADER, USER_HASH_HEADER and USER_HASH_METHOD constants match your configuration (cannot rely on semantic config here as we're way too early for that).
Ensure SESSION_NAME_PREFIX matches your environment.

In any case, it is possible to redefine those constants to fit your configuration.

TODO

Implement user context hash support for Symfony RP
Clear documentation on how to set it up
Make user context hash response vary on sessionId(s) only as Cookie may contain undesired info (e.g. analytics)
Try to do some tests

Depends on: * FriendsOfSymfony/FOSHttpCacheBundle#152 * FriendsOfSymfony/FOSHttpCache#128

lolautruche · 2014-10-20T13:24:57Z

Travis fail seems to be only linked to HHVM. Didn't touch impacted part.

stof · 2014-10-20T13:30:58Z

yeah, HHVM is currently broken on Travis. It does not allow parsing XML files using an XSD

stof · 2014-10-20T13:36:06Z

HttpCache.php

+        return strpos($name, $this->getSessionNamePrefix()) === 0;
+    }
+
+    protected function getSessionNamePrefix()


is this method needed ? Other places are relying on overwriting the constant itself

Well, I actually left it here for convenience because of the code we have in eZ Publish. However I reckon that this I not very consistent...

Depends on: * FriendsOfSymfony/FOSHttpCacheBundle#152 * FriendsOfSymfony/FOSHttpCache#128

lolautruche · 2014-10-21T19:02:58Z

OK, code has been updated and I added a test for new HttpCache class.
Now I need to have a look at the failure with Symfony 2.3 and write some documentation.

lolautruche · 2014-10-22T10:03:58Z

Tests with Symfony 2.3 fixed, documentation updated, PR rebased.
Ready for review ! 😄

Ping @dbu @ddeboer @stof @andrerom

andrerom · 2014-10-22T17:53:55Z

+1

Just meta comment to add from my side; @dbu as this and also some earlier code is based on code from eZ Publish, would it be ok to add eZ Systems AS to Resources/meta/LICENSE ? (maybe I'm wrong, but seems to be the convention with both liip and driebit mentioned atm)

stof · 2014-10-22T18:34:36Z

@andrerom The license mentions them because they are the original authors of the bundle (FOSHttpCache has been created by merging 2 bundles together). We don't update the license holder each time a new contributor appears.
the place where it could appear is with an @author annotation on the class being contributed (alongside @lolautruche's name)

this looks good to me, but I will wait for @dbu to review as well

andrerom · 2014-10-22T23:05:44Z

Ok, clear.

dbu · 2014-10-23T07:27:41Z

HttpCache.php

+     */
+    protected function isInternalRequest(Request $request)
+    {
+        return $request->attributes->get('internalRequest', false) === true;


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

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

dbu · 2014-10-23T07:55:18Z

thanks a lot! very cool, and seems sane to me. i commented on a bunch of details. i really want to be paranoid and in doubt trade default performance for default security in case people misconfigured things.

for the doc:

i think we should have the HttpCache specific doc in its own chapter - i hope the kernel will provide at least purge/refresh at some point, even if ban is not very realistic. lets add a new file to the Features section named "Symfony2 HttpCache" that explains that instead of varnish or nginx you can also use this kernel. the doc would explain to extend our kernel and for the rest of the topic link to the relevant symfony core doc.

then we should

add a link to that page in prerequisites on top at http://foshttpcachebundle.readthedocs.org/en/latest/features/user-context.html
add a ..note:: that when you use the HttpCache and change any of the values you need to look at that HttpCache doc (link)

dbu · 2014-10-23T08:02:40Z

HttpCache.php

+    protected function generateForwardRequest(Request $request)
+    {
+        $forwardReq = Request::create(static::USER_HASH_URI, static::USER_HASH_METHOD, array(), $request->cookies->all(), array(), $request->server->all());
+        $forwardReq->attributes->set('internalRequest', true);


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?

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?

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.

So, can I leave it as is ?

yeah i think its better than my proposal.

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

ddeboer · 2014-10-28T21:12:20Z

HttpCache.php

+    {
+        $sessionIds = array();
+        foreach ($originalRequest->cookies as $name => $value)
+        {


Syntax is not according to PSR.

ddeboer · 2014-10-29T07:46:40Z

@lolautruche lolautruche Can you fix the last few CS issues?

Otherwise, I think we can merge this.

dbu · 2014-10-29T07:48:48Z

@lolautruche can you please add a line to the CHANGELOG.md file? i would say add a title 1.1.x-dev and then something like * **2014-10-29** Adding support for the user context lookup with the Symfony built-in reverse proxy HttpCache

lolautruche · 2014-10-29T10:00:18Z

CS fixed and last request from @dbu taken into account (cookie string for user hash request).

One request from me: Once merged, would it be possible to tag 1.1.0? We're indeed about to tag eZ 5.4 beta and we'd need to rely on a stable tag if possible :-). Thanks

dbu · 2014-10-29T14:00:12Z

HttpCache.php

+                $forwardReq->cookies->set($name, $value);
+            }
+        }
+        $forwardReq->headers->set('Cookie', http_build_query($sessionIds, '', '; '));


thanks, i think this is more robust now.

ddeboer · 2014-10-29T21:34:03Z

One thing I’m noticing now: @lolautruche, you created protected methods in HttpCache, while we generally prefer private ones in order to improve backwards compatibility. Do you have a reason for using protected here?

lolautruche · 2014-10-29T21:37:17Z

@ddeboer in one word: extensibility 😉
This class is abstract and will be extended and might need to be tweaked for some reason.

lolautruche · 2014-10-29T21:40:16Z

@ddeboer However, isInternalRequest() and getUserHash() might better be private...

ddeboer · 2014-10-29T21:45:22Z

I think so, yes. But perhaps the other methods (except handle()), too. Having private methods will allow us to change implementation details without breaking BC. While I agree that the class might need to be tweaked, users of the bundle can always open issues or PRs if they need some extra functionality.

@dbu What do you think?

Ref FriendsOfSymfony#26 Added tests for Symfony RP

lolautruche · 2014-10-29T21:57:14Z

Changed. But I kept cleanupForwardRequest() protected though.

dbu · 2014-10-30T15:18:03Z

HttpCache.php

+     *
+     * @return bool
+     */
+    private function isSessionName($name)


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

Implemented Symfony reverse proxy support for user context hash.

lolautruche · 2014-10-30T15:29:09Z

Thanks for merging !
But you didn't leave me time to make some private methods to protected :-)

I guess I'll open a new PR

dbu · 2014-10-30T15:29:56Z

i don't want to block you any longer, so i merged and tagged 1.1.0 !

i created #155 to track the question of basic auth.

lolautruche · 2014-10-30T15:31:37Z

Thank you so much @dbu @ddeboer 😃

Depends on: * FriendsOfSymfony/FOSHttpCacheBundle#152 * FriendsOfSymfony/FOSHttpCache#128

ddeboer · 2014-10-31T09:39:42Z

Thank you, @lolautruche!

Depends on: * FriendsOfSymfony/FOSHttpCacheBundle#152 * FriendsOfSymfony/FOSHttpCache#128

lolautruche pushed a commit to ezsystems/ezpublish-kernel that referenced this pull request Oct 20, 2014

EZP-22400: Delegate user context hash generation to FOSHttpCacheBundle

ee9295a

Depends on: * FriendsOfSymfony/FOSHttpCacheBundle#152 * FriendsOfSymfony/FOSHttpCache#128

stof reviewed Oct 20, 2014
View reviewed changes

lolautruche pushed a commit to ezsystems/ezpublish-kernel that referenced this pull request Oct 21, 2014

EZP-22400: Delegate user context hash generation to FOSHttpCacheBundle

d70ca29

Depends on: * FriendsOfSymfony/FOSHttpCacheBundle#152 * FriendsOfSymfony/FOSHttpCache#128

lolautruche mentioned this pull request Oct 21, 2014

Fix EZP-22400: Use FOSHttpCacheBundle ezsystems/ezpublish-kernel#1025

Merged

3 tasks

lolautruche force-pushed the contextHashSymfonyReverseProxy branch from bdb1fb5 to de4ca66 Compare October 21, 2014 15:09

lolautruche force-pushed the contextHashSymfonyReverseProxy branch 3 times, most recently from 0a35388 to c1a083e Compare October 22, 2014 10:02

dbu reviewed Oct 23, 2014
View reviewed changes

ddeboer reviewed Oct 28, 2014
View reviewed changes

lolautruche force-pushed the contextHashSymfonyReverseProxy branch from 7f31ece to 4f3c874 Compare October 29, 2014 09:56

dbu reviewed Oct 29, 2014
View reviewed changes

dbu and others added 3 commits October 29, 2014 22:56

hhvm broke their xml support. ignore it until they can fix it

7152dc7

Implemented Symfony reverse proxy support for user context hash.

e4e59f4

Ref FriendsOfSymfony#26 Added tests for Symfony RP

Updated Changelog

106c4ed

lolautruche force-pushed the contextHashSymfonyReverseProxy branch from 4f3c874 to 106c4ed Compare October 29, 2014 21:56

dbu reviewed Oct 30, 2014
View reviewed changes

dbu added a commit that referenced this pull request Oct 30, 2014

Merge pull request #152 from lolautruche/contextHashSymfonyReverseProxy

d495ec8

Implemented Symfony reverse proxy support for user context hash.

dbu merged commit d495ec8 into FriendsOfSymfony:master Oct 30, 2014

lolautruche deleted the contextHashSymfonyReverseProxy branch October 30, 2014 15:28

dbu mentioned this pull request Oct 30, 2014

Symfony HttpCache should support basic auth too #155

Closed

lolautruche pushed a commit to ezsystems/ezpublish-spi that referenced this pull request Oct 30, 2014

EZP-22400: Delegate user context hash generation to FOSHttpCacheBundle

dfbb4e6

Depends on: * FriendsOfSymfony/FOSHttpCacheBundle#152 * FriendsOfSymfony/FOSHttpCache#128

lolautruche pushed a commit to ezsystems/ezpublish-spi that referenced this pull request Dec 12, 2014

EZP-22400: Delegate user context hash generation to FOSHttpCacheBundle

4082823

Depends on: * FriendsOfSymfony/FOSHttpCacheBundle#152 * FriendsOfSymfony/FOSHttpCache#128

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

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

Uh oh!

Conversation

lolautruche commented Oct 20, 2014

TODO

Uh oh!

lolautruche commented Oct 20, 2014

Uh oh!

stof commented Oct 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolautruche commented Oct 21, 2014

Uh oh!

lolautruche commented Oct 22, 2014

Uh oh!

andrerom commented Oct 22, 2014

Uh oh!

stof commented Oct 22, 2014

Uh oh!

andrerom commented Oct 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Oct 23, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeboer commented Oct 29, 2014

Uh oh!

dbu commented Oct 29, 2014

Uh oh!

lolautruche commented Oct 29, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddeboer commented Oct 29, 2014

Uh oh!

lolautruche commented Oct 29, 2014

Uh oh!

lolautruche commented Oct 29, 2014

Uh oh!

ddeboer commented Oct 29, 2014

Uh oh!

lolautruche commented Oct 29, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolautruche commented Oct 30, 2014

Uh oh!

dbu commented Oct 30, 2014

Uh oh!

lolautruche commented Oct 30, 2014

Uh oh!

ddeboer commented Oct 31, 2014

Uh oh!

Uh oh!