Skip to content
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

Allow to easily ask Symfony not to set a response to private automatically #26681

Merged
merged 1 commit into from Mar 30, 2018

Conversation

Projects
None yet
10 participants
@Toflar
Copy link
Contributor

commented Mar 27, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#9667

This PR is related to the many discussions going on about the latest (Symfony 3.4+) changes regarding the way Symfony handles the session. I think we're almost there now, Symfony 4.1 automatically turns responses into private responses once the session is started and it's all done in the AbstractSessionListener where the session is also saved.
In other issues/PRs (#25583, #25699, #24988) it was agreed that setting the response to private if the session is started is a good default for Symfony. It was also agreed that setting it to private does not always make sense because you can share a response across sessions, it just requires a more complex caching setup with shared user context etc.
So there must be an easy way to disable this behaviour. Right now it's very hard to do so because what you end up doing is basically decorating the session_listener which is very hard because you have to keep track on that over different Symfony versions as the base listener might get additional features etc.

The FOSCacheBundle is already having this problem, Contao has the same issue and there will be probably more. Basically everyone that wants to share a response cache across the session will have to decorate the default listener. That's just too hard, so I came up with this solution. The header is easy. Every project can add that easily. It does not require any extension, configuration or adjustment of any service. It's clean, transparent and has absolutely no impact on "default" Symfony setups.

Would be happy to have some feedback before 4.1 freeze.

@dbu

dbu approved these changes Mar 27, 2018

Copy link
Contributor

left a comment

that would be much more elegant than the hacks we currently have to do. any possibility to get this (or something similar) into 4.1?

@emodric

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

eZ Platform would also greatly benefit from this, since it also uses shared response across different sessions with the same user context.

@stof

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

If you share a response between sessions, you still have an issue with the cookie header containing one of the session ids. How do you handle it in this case ?

@emodric

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

@stof In eZ Platform case, and probably in FOS HTTP Cache bundle, Vary header does not contain Cookie header by default.

->setPrivate()
->setMaxAge(0)
->headers->addCacheControlDirective('must-revalidate');
if (!$response->headers->has('Symfony-NoPrivateIfSessionStarted')) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 27, 2018

Member

This would allow cache poisoning if an external request comes with this header, isn't it?

This comment has been minimized.

Copy link
@Toflar

Toflar Mar 27, 2018

Author Contributor

Why would the response contain such a header based on an external request?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 27, 2018

Member

OK no sorry, I read it as a "request". What about using a request parameter instead?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 27, 2018

Member

This check should also be moved upper in the method.
The name of the parameter should be more accurate, eg session.disable_auto_cache_control or better?

This comment has been minimized.

Copy link
@Toflar

Toflar Mar 27, 2018

Author Contributor

I don't think this is correct. The question in that piece of code is

Hello response. I will now make you private if the session is started because that's what's a sane default. However, if someone wanted you to really be public even though the session is started, I'll leave you alone.

The subject is the response, not the request. Normally caching systems would add cache tagging headers to the response, set the cache control headers etc. It's all done on the response, never the request.

This comment has been minimized.

Copy link
@Toflar

Toflar Mar 27, 2018

Author Contributor

This check should also be moved upper in the method.
The name of the parameter should be more accurate, eg session.disable_auto_cache_control or better?

Why upper? Where exactly do you mean? And the parameter can be changed yes but as it's a header I think it should be more in the header naming style, no?

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

@stof There's a lot more to consider, you are right. This is why Symfony defaults to setting the response private and it's what I meant by "requires a more complex caching setup". All this PR is about is that it's very hard to disable the current behaviour of Symfony and I don't think this is a good thing.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

But any new feature on Symfony would not be applicable to 3rd party bundles, unless they plan to support Symfony 4.1 only.
Since decorating this listener or adding a one next to it is all that is required, is it really worth it?

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

I'm sorry but I don't think I understand what you're saying, @nicolas-grekas.

@dbu

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

To illustrate the FOSHttpCache (and afaik ez publish) use case:

  1. Caching proxy does a request to get a cache context token. Usually, this is md5 of all groups the current user is part of.
  2. Caching proxy sends a request with a header Cache-Context-Hash using the hash it got, and also the original session cookie / auth header
  3. Symfony handles the request, renders a response, and decides that the response does not depend on the actual user in question, but only on the roles of that user, and is thus cacheable with a Vary: Cache-Context-Hash

Its ok that symfony by default ignores what the application is telling about caching when the session has been used to generate the response. But for special cases like this, we need a way to disable this behaviour. Doing it with a response header seems the best solution because then it can be per response, but without needing a complicated configuration for the symfony listener as to which responses to handle or not.

For symfony 4.0, we simply decorate the SessionListener and only call the inner listener it when there is no Vary: Cache-Context-Hash header present. Symfony 4.1 breaks this solution, because the SaveSessionListener is now integrated into the SessionListener. Plus the SessionListener is now more complicated to improve lazyness. We started a pull request in FOSHttpCache to port most of that lazyness logic into our decorator, but it feels really complicated and clumsy, and might break again at any time because the coupling with Symfony itself is too high.

@dbu

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

But any new feature on Symfony would not be applicable to 3rd party bundles, unless they plan to support Symfony 4.1 only.

We only register our decorator for symfony 3.4 or newer because before, the caching headers are not overwritten. If we can get such a solution into Symfony 4.1, we can limit the decorator to just 3.4 - 4.0

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

@Toflar @dbu thanks for the details, OK on my side then :)

This check should also be moved upper in the method.

I mean as early as possible in the method, eg not sure we need to check for the session service at all.

Symfony-NoPrivateIfSessionStarted

this name is not accurate: max-age and must-revalidate also are removed.
What about Symfony-Session-NoAutoCacheControl?

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

What about Symfony-Session-NoAutoCacheControl?

Naming things, right? 😄 Sounds perfect to me, changed.

@nicolas-grekas
Copy link
Member

left a comment

(never mind the comment about the place of the check, I missed the logic in the patched "if")

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 27, 2018

@dbu
Copy link
Contributor

left a comment

i was trying to think of an even better name but did not find any. this seems good enough. but i would make that into a constant, its also better in code using this as the relation would become explicit.

the class doc of the listener should be updated. it lacks mention of anything that is done with the response. i would say something like:

Checks if the session has been started and overrides the Cache-Control header to avoid all caching in that case. If you have a scenario where caching responses with session information in them makes sense, you can disable this behaviour by setting the header SessionListener::SESSION_NO_AUTO_CACHE_CONTROL on the response.

This listener also saves the session if it still open.

->setPrivate()
->setMaxAge(0)
->headers->addCacheControlDirective('must-revalidate');
if (!$response->headers->has('Symfony-Session-NoAutoCacheControl')) {

This comment has been minimized.

Copy link
@dbu

dbu Mar 28, 2018

Contributor

imho this should be a public constant. then the constant can also be documented a little bit to explain what this is about.

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

I agree but at the same time, I don't want to change a PR that has been approved already. Waiting for mergers to comment on that. BTW: Failing tests are unrelated.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 28, 2018

The const looks good to me.

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

Done then, thanks for the feedback!

*
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
* @author Tobias Schultze <http://tobion.de>
*/
abstract class AbstractSessionListener implements EventSubscriberInterface
{
const SESSION_NO_AUTO_CACHE_CONTROL = 'Symfony-Session-NoAutoCacheControl';

This comment has been minimized.

Copy link
@nicolas-grekas

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 28, 2018

Member

and remove SESSION_?
AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER?

* Sets the session in the request.
* Sets the session onto the request on the "kernel.request" event and saves
* it on the "kernel.response" event.
* In addition, if the session has been started it overrides the Cache-Control

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 28, 2018

Member

blank line need before

@Toflar Toflar force-pushed the Toflar:allow-disabling-auto-private-response branch from 6540ef3 to d85ba56 Mar 28, 2018

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

Done.

@fabpot

fabpot approved these changes Mar 30, 2018

@fabpot fabpot force-pushed the Toflar:allow-disabling-auto-private-response branch from d85ba56 to 0f36710 Mar 30, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 30, 2018

Thank you @Toflar.

@fabpot fabpot merged commit 0f36710 into symfony:master Mar 30, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 30, 2018

feature #26681 Allow to easily ask Symfony not to set a response to p…
…rivate automatically (Toflar)

This PR was squashed before being merged into the 4.1-dev branch (closes #26681).

Discussion
----------

Allow to easily ask Symfony not to set a response to private automatically

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

This PR is related to the many discussions going on about the latest (Symfony 3.4+) changes regarding the way Symfony handles the session. I think we're almost there now, Symfony 4.1 automatically turns responses into `private` responses once the session is started and it's all done in the `AbstractSessionListener` where the session is also saved.
In other issues/PRs (#25583, #25699, #24988) it was agreed that setting the response to `private` if the session is started is a good default for Symfony. It was also agreed that setting it to `private` does not always make sense because you **can share a response** across sessions, it just requires a more complex caching setup with shared user context etc.
So there must be an easy way to disable this behaviour. Right now it's very hard to do so because what you end up doing is basically decorating the `session_listener` which is very hard because you have to keep track on that over different Symfony versions as the base listener might get additional features etc.

The [FOSCacheBundle](FriendsOfSymfony/FOSHttpCacheBundle#438) is already having this problem, [Contao](contao/core-bundle#1388) has the same issue and there will be probably more. Basically everyone that wants to share a response cache across the session will have to decorate the default listener. That's just too hard, so I came up with this solution. The header is easy. Every project can add that easily. It does not require any extension, configuration or adjustment of any service. It's clean, transparent and has absolutely no impact on "default" Symfony setups.

Would be happy to have some feedback before 4.1 freeze.

Commits
-------

0f36710 Allow to easily ask Symfony not to set a response to private automatically

@Toflar Toflar deleted the Toflar:allow-disabling-auto-private-response branch Apr 20, 2018

@dbu dbu referenced this pull request Apr 21, 2018

Merged

Symfony 4 compatibility #420

6 of 6 tasks complete

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

@Toflar would you mind sending a doc PR to tell about this please?

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

Done a while ago 😄 symfony/symfony-docs#9667

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

Oups, sorry, I added the link in the description.

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

Ah sorry, yes @javiereguiluz pinged me before the 4.1 release in a separate ticket so I forgot to update this one. Cheers!

@guillaumedebeauvoir

This comment has been minimized.

Copy link

commented Jul 2, 2018

@nicolas-grekas maybe i have missed something, but have you planed to make the same changes in 3.4 version, meaning adding the Symfony-Session-NoAutoCacheControl header to make cache behaviour same as before! If not i need to urgently rollback to previous version! I proposed a PR #27814 ! hope that will do it

@yellow1912

This comment has been minimized.

Copy link

commented Oct 17, 2018

@nicolas-grekas Is this available in the latest 3.4.x, if not is there any plan to make it available?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

That's a new feature of 4.1, and 3.4 is in bug-fixes-only mode.

@yellow1912

This comment has been minimized.

Copy link

commented Oct 17, 2018

Thank you, I will look into upgrading to 4.1, seems like tons of work. In the mean time, would there be an easy way to migrate this to the 3.4 version? I'm thinking of overriding the session_listener for Symfony and use the new code inside. Would that be a good approach? In such case should I run a compiler pass to remove the core session_listener definition then re-define it with my pass to point to my custom class?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

I don't remember where exactly but yes, a listener can do it also.
Note that you should first understand why this behavior is correct.
Then you can opt-out from it :)

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Yes, you can decorate the core listener. Can you tell me what exactly you're trying to do? I'm like 99% sure I have a better alternative for you where you don't have to do this at all or at least not on your own 😄

@yellow1912

This comment has been minimized.

Copy link

commented Oct 17, 2018

@Toflar thank you for your response. My situation is that even with the session already started, I would like to be able to set the cache headers manually. NO_AUTO_CACHE_CONTROL_HEADER is exactly what I needed but I cannot upgrade to 4.1 at this moment (I'm on 3.4 and upgrading requires too much work that we cannot arrange the time at the moment)

What I ended up doing was:

  1. Added a compiler pass
class SessionListenerPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container)
    {
        if ($container->hasDefinition('session_listener')) {
            $container->getDefinition('session_listener')->setClass(SessionListener::class);
        }
    }
}
  1. Added my custom SessionListener with the onKernelResponse code take directly from 4.1 version

It seems to do the trick for now until I upgrade to 4.x version and remove this hack.

@Toflar

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Yeah basically that's what we did for FOSCacheBundle, which would maybe also cover your use-case, which is why I asked what you're trying to do :)

@phillipsnick

This comment has been minimized.

Copy link

commented Jan 16, 2019

Thanks @yellow1912 for the compiler pass idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.