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

Reworked caching for proxies #389

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Toflar
Copy link
Member

Toflar commented Mar 7, 2019

As discussed in #370 (comment)

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 7, 2019

@ausi

This comment has been minimized.

Copy link
Member

ausi commented Mar 7, 2019

How about something like that:

// Do not cache response if caching was not configured at all
if (($objPage->cache === false || $objPage->cache < 1) && ($objPage->clientCache === false || $objPage->clientCache < 1))
{
	return $this->makeResponsePrivate($response);
}

// Do not cache response if any cookies were received or sent
if (0 !== \count($response->headers->getCookies()) || 0 !== \count(System::getContainer()->get('request_stack')->getCurrentRequest()->headers->getCookies()))
{
	return $this->makeResponsePrivate($response);
}

// Now we have cacheable requests. We do vary on Cookie for those responses that have **NO** cookies set which will
// cause reverse proxies to never load a response from the cache if the request contains a cookie.
// We only do this if you did not configure the page to **EXPLICITLY** cache even though a cookie is present.
if (!$objPage->forceCache) {
	$response->setVary(['Cookie']);
}

@leofeyer leofeyer added the feature label Mar 7, 2019

@leofeyer leofeyer added this to the 4.8.0 milestone Mar 7, 2019

@Toflar Toflar force-pushed the Toflar:feature/improve-proxy-caching branch from 787c80d to fc4a421 Mar 8, 2019

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 8, 2019

@ausi please review again, I found other issues and reworked the comments. The varying is now also only applied in the shared cache section because it's irrelevant for private caches.

@aschempp
Copy link
Contributor

aschempp left a comment

I also wonder if we should not check for cookies, but rather check for the security user and/or if a session was started? This way we would only check for session cookies and ignore all others.

Show resolved Hide resolved core-bundle/src/Resources/contao/classes/FrontendTemplate.php Outdated
Show resolved Hide resolved core-bundle/src/Resources/contao/classes/FrontendTemplate.php Outdated
Show resolved Hide resolved core-bundle/src/Resources/contao/models/PageModel.php Outdated
Show resolved Hide resolved core-bundle/src/Resources/contao/models/PageModel.php Outdated
// TODO: Add support for proxies so they can vary on member context
if (FE_USER_LOGGED_IN === true || BE_USER_LOGGED_IN === true || $objPage->protected || $this->hasAuthenticatedBackendUser())
// Do not cache response if any cookies were received or sent and make sure the response is private.
if (0 !== \count($response->headers->getCookies()) || 0 !== \count(System::getContainer()->get('request_stack')->getCurrentRequest()->headers->getCookies()))

This comment has been minimized.

@aschempp

aschempp Mar 8, 2019

Contributor

as usualy, very unsure if getCurrentRequest is correct, as the cookies are only on the master request and (probably) not added to subrequests.

This comment has been minimized.

@Toflar

Toflar Mar 8, 2019

Author Member

As usually, it is correct :) The master request is very, very rarely the right one. Cookies can also be set on a subrequest and we have to have the very same handling there. Imagine you create an ESI fragment that uses Template->getResponse(). It should work just the same.

This comment has been minimized.

@aschempp

aschempp Mar 11, 2019

Contributor

I'm not sure I understand this correctly.

  1. if Symfony's HttpCache is used, the current request might be a subrequest for ESI, which means cookies are likely not there (because its not a clone of the master request). So using the master request would be better.
  2. if this is a Varnish or other reverse proxy, it's always the master request, so using the master request would be fine?

Toflar and others added some commits Mar 8, 2019

Use built-in count method
Co-Authored-By: Toflar <yanick.witschi@terminal42.ch>
@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 8, 2019

I also wonder if we should not check for cookies, but rather check for the security user and/or if a session was started? This way we would only check for session cookies and ignore all others.

Which would be wrong. That's why don't do the session cookie. You don't know what the developers use the cookies for. They might use them to store filter settings incorrectly in which case we also have to make sure the response is not stored :)

@Toflar Toflar force-pushed the Toflar:feature/improve-proxy-caching branch from 55043ff to 17e20f4 Mar 8, 2019

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 8, 2019

All comments addressed. Looks good to me now :)

@@ -858,6 +859,7 @@ public function loadDetails()
$this->groups = $this->protected ? StringUtil::deserialize($this->groups) : false;
$this->layout = $this->includeLayout ? $this->layout : false;
$this->cache = $this->includeCache ? $this->cache : false;
$this->forceCache = (bool) $this->forceCache ? $this->forceCache : false;

This comment has been minimized.

@ausi

ausi Mar 8, 2019

Member
Suggested change
$this->forceCache = (bool) $this->forceCache ? $this->forceCache : false;
$this->forceCache = (bool) $this->forceCache;

or:

Suggested change
$this->forceCache = (bool) $this->forceCache ? $this->forceCache : false;
$this->forceCache = (bool) ($this->forceCache ? $this->forceCache : false);
Fixed boolean casting
Co-Authored-By: Toflar <yanick.witschi@terminal42.ch>
@ausi

This comment has been minimized.

Copy link
Member

ausi commented Mar 8, 2019

Should we also check for authorization headers like $request->getUserInfo() or similar?

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 8, 2019

In fact, personally, I wouldn't check any request data at all. It's the task of the reverse proxy not to cache responses to private requests. E.g. Symfony's HttpCache does that by default: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L476

So our code is useless for the HttpCache case. But it's probably not for Varnish and Co.

@ausi

This comment has been minimized.

Copy link
Member

ausi commented Mar 8, 2019

if ($this->isPrivateRequest($request) && !$response->headers->hasCacheControlDirective('public')) {

If I read that correctly, private requests do not get cached only if Cache-Control is not set to public. But we do set it to public in Contao which means we should probably also check for authorization headers in addition to cookies?

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 8, 2019

Yeah we might want to do that. The HttpCache does strange things...

@ausi

ausi approved these changes Mar 8, 2019

Copy link
Member

ausi left a comment

Apart from the missing check for authorization headers this looks good to me. 👍

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 8, 2019

Added now

@ausi

ausi approved these changes Mar 8, 2019

@@ -386,6 +409,29 @@ private function setCacheHeaders(Response $response)
return $response;
}
private function isCurrentRequestPrivate()

This comment has been minimized.

@leofeyer

leofeyer Mar 8, 2019

Member

: bool

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Mar 8, 2019

I think "Force server cache for logged-in members" is not very clear to non-technical users. I think the title and description should be something like "Always load from cache" and "Always load this page from cache, even if a member is logged in. (Note that you can no longer personalize the page for logged in members in this case.)".

Should we also rename the database field to alwaysLoadFromCache?

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 8, 2019

Fine with me, I've addressed your comments. I've also adjusted the labels to Private cache and Shared cache so they are techincally correct. I left (client cache) and (server cache) in brackets as additional hint for those who don't know the technically correct terms.

{
$response->headers->addCacheControlDirective('no-cache');
$response->headers->addCacheControlDirective('no-store');
$response->headers->set('Cache-Control', 'no-cache, no-store');
return $response->setPrivate();

This comment has been minimized.

@aschempp

aschempp Mar 11, 2019

Contributor

I think the $objPage->clientCache should also be applied here. If I configure a page in the tree to be user-cacheable, it can still be customized, right?

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Mar 11, 2019

I also wonder if we should not check for cookies, but rather check for the security user and/or if a session was started? This way we would only check for session cookies and ignore all others.

Which would be wrong. That's why don't do the session cookie. You don't know what the developers use the cookies for. They might use them to store filter settings incorrectly in which case we also have to make sure the response is not stored :)

I agree we don't know what the devs do. But they should not use a custom cookie for personalization, that's what the session is for, isn't that the case? That's why Symfony makes responses private if they have a session, and not if they have any cookie?

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 11, 2019

That's why Symfony makes responses private if they have a session, and not if they have any cookie?

That's because they know this is the only thing in Symfony that might be problematic. We don't know so I would stick with the current solution.

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Mar 11, 2019

I remember we had this discussion when we overrided the Symfony listener in Contao 4.4 etc. Maybe we need to discuss the topic again 😞

@Toflar

This comment has been minimized.

Copy link
Member Author

Toflar commented Mar 11, 2019

@leofeyer this is up4discussion again.

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Mar 12, 2019

https://www.heise.de/mac-and-i/meldung/Apple-erklaert-Tracking-Cookies-den-Krieg-4330798.html

Interesting read in general. But as as summery, Google and other already use "First-party-cookies" (i.e. from your domain) to work around browser cookie limitations. Which means they all get sent to Contao, and would disable the reverse proxy.

@m-vo

This comment has been minimized.

Copy link
Contributor

m-vo commented Mar 12, 2019

Valid point! Maybe there should be some sort of whitelisting? - a tracking cookie would not interfere with what content to deliver (although it could theoretically be used for that), so why not let the devs decide?

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.