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

[RTM] Support for replaying headers #749

Merged
merged 14 commits into from
Jun 13, 2017
2 changes: 2 additions & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
"simplepie/simplepie": "^1.3",
"tecnickcom/tcpdf": "^6.0",
"true/punycode": "^2.0",
"terminal42/header-replay-bundle": "^1.0",
"php-http/guzzle6-adapter": "^1.1",
"twig/twig": "^1.26",
"webmozart/path-util": "^2.0",
"contao/image": "^0.3.1",
Expand Down
3 changes: 2 additions & 1 deletion src/ContaoManager/Plugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use Symfony\Bundle\TwigBundle\TwigBundle;
use Symfony\Component\Config\Loader\LoaderResolverInterface;
use Symfony\Component\HttpKernel\KernelInterface;
use Terminal42\HeaderReplay\HeaderReplayBundle;

/**
* Plugin for the Contao Manager.
Expand All @@ -43,7 +44,7 @@ class Plugin implements BundlePluginInterface, RoutingPluginInterface
public function getBundles(ParserInterface $parser)
{
return [
BundleConfig::create(KnpMenuBundle::class),
Copy link
Member

Choose a reason for hiding this comment

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

sure this is correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, this is a rebase-bug! Fixed in 9fdb93e.

BundleConfig::create(HeaderReplayBundle::class),
BundleConfig::create(KnpTimeBundle::class),
BundleConfig::create(ContaoCoreBundle::class)
->setReplace(['core'])
Expand Down
77 changes: 77 additions & 0 deletions src/EventListener/HeaderReplay/BackendSessionListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php

/*
* This file is part of Contao.
*
* Copyright (c) 2005-2017 Leo Feyer
*
* @license LGPL-3.0+
*/

namespace Contao\CoreBundle\EventListener\HeaderReplay;

use Symfony\Component\HttpFoundation\Request;
use Terminal42\HeaderReplay\Event\HeaderReplayEvent;
use Terminal42\HeaderReplay\EventListener\HeaderReplayListener;

/**
* Disables the reverse proxy based on the terminal42/header-replay-bundle.
*
* @author Yanick Witschi <https://github.com/toflar>
*/
class BackendSessionListener
{
/**
* @var bool
*/
private $disableIpCheck;

/**
* BackendSessionListener constructor.
*
* @param bool $disableIpCheck
*/
public function __construct($disableIpCheck)
{
$this->disableIpCheck = $disableIpCheck;
}

/**
* @param HeaderReplayEvent $event
*/
public function onReplay(HeaderReplayEvent $event)
{
$request = $event->getRequest();
Copy link
Member

Choose a reason for hiding this comment

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

Same applies here. The event listener should only be triggered on $request->attributes->get('_scope') === 'backend' (actually use the constant!)


if (null === $request->getSession() || !$this->hasAuthenticatedBackendUser($request)) {
return;
}

$headers = $event->getHeaders();
$headers->set(HeaderReplayListener::FORCE_NO_CACHE_HEADER_NAME, 'true');
}

/**
* Checks if there is an authenticated back end user.
*
* @param Request $request
*
* @return bool
*/
private function hasAuthenticatedBackendUser(Request $request)
{
if (!$request->cookies->has('BE_USER_AUTH')) {
return false;
}

$sessionHash = sha1(
sprintf(
'%s%sBE_USER_AUTH',
$request->getSession()->getId(),
$this->disableIpCheck ? '' : $request->getClientIp()
)
);

return $request->cookies->get('BE_USER_AUTH') === $sessionHash;
}
}
64 changes: 64 additions & 0 deletions src/EventListener/HeaderReplay/PageLayoutListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php

/*
* This file is part of Contao.
*
* Copyright (c) 2005-2017 Leo Feyer
*
* @license LGPL-3.0+
*/

namespace Contao\CoreBundle\EventListener\HeaderReplay;

use Contao\CoreBundle\Framework\ContaoFrameworkInterface;
use Contao\Environment;
use Terminal42\HeaderReplay\Event\HeaderReplayEvent;

/**
* Extracts the page layout for proper Vary handling based
* on the terminal42/header-replay-bundle.
*
* @author Yanick Witschi <https://github.com/toflar>
*/
class PageLayoutListener
{
/**
* @var ContaoFrameworkInterface
*/
private $framework;

/**
* PageLayoutListener constructor.
*
* @param ContaoFrameworkInterface $framework
*/
public function __construct(ContaoFrameworkInterface $framework)
{
$this->framework = $framework;
}

/**
* @param HeaderReplayEvent $event
*/
public function onReplay(HeaderReplayEvent $event)
{
$request = $event->getRequest();

$this->framework->initialize();
Copy link
Member

Choose a reason for hiding this comment

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

pretty sure we should check for a Contao scope before booting the framework on EVERY request?

Copy link
Member Author

Choose a reason for hiding this comment

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

On every request with a Cookie or Authorization header, yeah. We could do that? How do I do this now?

Copy link
Member

Choose a reason for hiding this comment

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

No I mean only for routes with _scope=frontend.


$mobile = $this->framework->getAdapter(Environment::class)->get('agent')->mobile;

if ($request->cookies->has('TL_VIEW')) {
switch ($request->cookies->get('TL_VIEW')) {
case 'mobile':
$mobile = true;
break;
case 'desktop':
$mobile = false;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We could avoid initializing the framework if there is a TL_VIEW cookie:

if ($request->cookies->has('TL_VIEW')) {
    $mobile = 'mobile' === $request->cookies->get('TL_VIEW');
} else {
    $this->framework->initialize();
    $mobile = $this->framework->getAdapter(Environment::class)->get('agent')->mobile;
}


$headers = $event->getHeaders();
$headers->set('Contao-Page-Layout', $mobile ? 'mobile' : 'desktop');
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to call this Contao-Mobile-Layout with true only value or the header not being present. Also, creating a $headers variable is fairly unnecessary here, but I guess you're aware of that. Just a matter of choice :D

Copy link
Member Author

Choose a reason for hiding this comment

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

I like my version better. This applies to both, the header name and the variable.

Copy link
Member

Choose a reason for hiding this comment

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

The idea was that the header would not need to be present for 90% of pages (if not in mobile mode). Also, the Vary would only be needed if the current page actually has a mobile layout.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could introduce new layout types using the same header with @Toflar's variant. Therefore I am also in favor of his approach.

}
}
14 changes: 14 additions & 0 deletions src/Resources/config/listener.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,20 @@ services:
tags:
- { name: kernel.event_listener, event: kernel.exception, method: onKernelException, priority: 96 }

contao.listener.header_replay.backend_session:
class: Contao\CoreBundle\EventListener\HeaderReplay\BackendSessionListener
arguments:
- "%contao.security.disable_ip_check%"
tags:
- { name: kernel.event_listener, event: t42.header_replay, method: onReplay }
Copy link
Member

Choose a reason for hiding this comment

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

as far as I remember Symfony events, the correct way to name the method would be according to the event name. So t42.header_replay would result in an onHeaderReplay method. Also, if you follow that convention, you do not need to name the method (it will be discovered automatically).

On a side note, I think the event should be called terminal42.header_replay to not introduce another "namespace".

Copy link
Member Author

Choose a reason for hiding this comment

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

This would look shit, which is why I decided not to do it. onTerminal42HeaderReplay() is not a nice method name at all.

Copy link
Member

Choose a reason for hiding this comment

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

it would be onHeaderReplay. The key is not used. Think of kernel.response => onResponse

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's onKernelResponse.


contao.listener.header_replay.page_layout:
class: Contao\CoreBundle\EventListener\HeaderReplay\PageLayoutListener
arguments:
- "@contao.framework"
tags:
- { name: kernel.event_listener, event: t42.header_replay, method: onReplay }

contao.listener.insecure_installation:
class: Contao\CoreBundle\EventListener\InsecureInstallationListener
tags:
Expand Down
10 changes: 7 additions & 3 deletions src/Resources/contao/classes/FrontendTemplate.php
Original file line number Diff line number Diff line change
Expand Up @@ -371,9 +371,13 @@ private function setCacheHeaders(Response $response)
return $response->setPrivate();
}

// Do not cache the response if a user is logged in or the page is protected or uses a mobile layout
// TODO: Add support for proxies so they can vary on member context and page layout
if (FE_USER_LOGGED_IN === true || BE_USER_LOGGED_IN === true || $objPage->isMobile || $objPage->protected || $this->hasAuthenticatedBackendUser())
// Vary on page layout
$response->setVary(['Contao-Page-Layout'], false);
Copy link
Member

Choose a reason for hiding this comment

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

This is legacy code, so we must not use the short array notation.

$response->headers->set('Contao-Page-Layout', $objPage->isMobile ? 'mobile' : 'desktop');

// Do not cache the response if a user is logged in or the page is protected
// 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())
{
return $response->setPrivate();
}
Expand Down
103 changes: 103 additions & 0 deletions tests/EventListener/HeaderReplay/BackendSessionListenerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<?php

/*
* This file is part of Contao.
*
* Copyright (c) 2005-2017 Leo Feyer
*
* @license LGPL-3.0+
*/

namespace Contao\CoreBundle\Tests\EventListener\HeaderReplay;

use Contao\CoreBundle\EventListener\HeaderReplay\BackendSessionListener;
use Contao\CoreBundle\Tests\TestCase;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\ResponseHeaderBag;
use Symfony\Component\HttpFoundation\Session\Session;
use Terminal42\HeaderReplay\Event\HeaderReplayEvent;
use Terminal42\HeaderReplay\EventListener\HeaderReplayListener;

/**
* Tests the BackendSessionListener class.
*
* @author Yanick Witschi <https://github.com/toflar>
*/
class BackendSessionListenerTest extends TestCase
{
/**
* Tests the object instantiation.
*/
public function testInstantiation()
{
$listener = new BackendSessionListener(false);

$this->assertInstanceOf('Contao\CoreBundle\EventListener\HeaderReplay\BackendSessionListener', $listener);
}

public function testOnReplayWithNoSession()
{
$listener = new BackendSessionListener(false);

$request = new Request();
$headers = new ResponseHeaderBag();
$event = new HeaderReplayEvent($request, $headers);

$listener->onReplay($event);

$this->assertArrayNotHasKey(HeaderReplayListener::FORCE_NO_CACHE_HEADER_NAME, $event->getHeaders()->all());
}

public function testOnReplayWithNoAuthCookie()
{
$listener = new BackendSessionListener(false);

$session = new Session();
$request = new Request();
$request->setSession($session);
$headers = new ResponseHeaderBag();
$event = new HeaderReplayEvent($request, $headers);

$listener->onReplay($event);

$this->assertNotNull($request->getSession());
$this->assertArrayNotHasKey(HeaderReplayListener::FORCE_NO_CACHE_HEADER_NAME, $event->getHeaders()->all());
}

public function testOnReplayWithNoValidCookie()
{
$listener = new BackendSessionListener(false);

$session = new Session();
$request = new Request();
$request->cookies->set('BE_USER_AUTH', 'foobar');
$request->setSession($session);
$headers = new ResponseHeaderBag();
$event = new HeaderReplayEvent($request, $headers);

$listener->onReplay($event);

$this->assertNotNull($request->getSession());
$this->assertTrue($request->cookies->has('BE_USER_AUTH'));
$this->assertArrayNotHasKey(HeaderReplayListener::FORCE_NO_CACHE_HEADER_NAME, $event->getHeaders()->all());
}

public function testOnReplay()
{
$listener = new BackendSessionListener(false);

$session = new Session();
$session->setId('foobar-id');
$request = new Request();
$request->cookies->set('BE_USER_AUTH', 'f6d5c422c903288859fb5ccf03c8af8b0fb4b70a');
$request->setSession($session);
$headers = new ResponseHeaderBag();
$event = new HeaderReplayEvent($request, $headers);

$listener->onReplay($event);

$this->assertNotNull($request->getSession());
$this->assertTrue($request->cookies->has('BE_USER_AUTH'));
$this->assertArrayNotHasKey(HeaderReplayListener::FORCE_NO_CACHE_HEADER_NAME, $event->getHeaders()->all());
}
}
Loading