Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

X-HTTP-Method-Override listener #81

Closed
wants to merge 14 commits into from
7 changes: 7 additions & 0 deletions config/module.config.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use ZF\ContentNegotiation\AcceptListener;
use ZF\ContentNegotiation\ContentNegotiationOptions;
use ZF\ContentNegotiation\ContentTypeFilterListener;
use ZF\ContentNegotiation\HttpMethodOverrideListener;
use ZF\ContentNegotiation\ContentTypeListener;
use ZF\ContentNegotiation\ControllerPlugin;
use ZF\ContentNegotiation\Factory;
Expand Down Expand Up @@ -45,6 +46,7 @@
AcceptFilterListener::class => Factory\AcceptFilterListenerFactory::class,
ContentTypeFilterListener::class => Factory\ContentTypeFilterListenerFactory::class,
ContentNegotiationOptions::class => Factory\ContentNegotiationOptionsFactory::class,
HttpMethodOverrideListener::class => Factory\HttpMethodOverrideListenerFactory::class,
],
],

Expand Down Expand Up @@ -76,6 +78,11 @@
// Array of controller service name => allowed content type pairs.
// The allowed content type may be a string, or an array of strings.
'content_type_whitelist' => [],

// Enable x-http method override feature
// When set to 'true' the http method in the request will be overridden
// by the method inside the 'X-HTTP-Method-Override' header (if present)
'x_http_method_override_enabled' => false,
],

'controller_plugins' => [
Expand Down
42 changes: 42 additions & 0 deletions src/ContentNegotiationOptions.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,16 @@ class ContentNegotiationOptions extends AbstractOptions
*/
protected $contentTypeWhitelist = [];

/**
* @var boolean
*/
protected $xHttpMethodOverrideEnabled = false;

/**
* @var array
*/
protected $httpOverrideMethods = [];

/**
* {@inheritDoc}
*
Expand Down Expand Up @@ -128,4 +138,36 @@ public function getContentTypeWhitelist()
{
return $this->contentTypeWhitelist;
}

/**
* @param boolean $xHttpMethodOverrideEnabled
*/
public function setXHttpMethodOverrideEnabled($xHttpMethodOverrideEnabled)
{
$this->xHttpMethodOverrideEnabled = $xHttpMethodOverrideEnabled;
}

/**
* @return boolean
*/
public function getXHttpMethodOverrideEnabled()
{
return $this->xHttpMethodOverrideEnabled;
}

/**
* @param array $httpOverrideMethods
*/
public function setHttpOverrideMethods(array $httpOverrideMethods)
{
$this->httpOverrideMethods = $httpOverrideMethods;
}

/**
* @return array
*/
public function getHttpOverrideMethods()
{
return $this->httpOverrideMethods;
}
}
27 changes: 27 additions & 0 deletions src/Factory/HttpMethodOverrideListenerFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php
/**
* @license http://opensource.org/licenses/BSD-3-Clause BSD-3-Clause
* @copyright Copyright (c) 2014-2016 Zend Technologies USA Inc. (http://www.zend.com)
Copy link
Member

Choose a reason for hiding this comment

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

Please use only 2016 year in new files.

*/

namespace ZF\ContentNegotiation\Factory;

use Interop\Container\ContainerInterface;
use ZF\ContentNegotiation\ContentNegotiationOptions;
use ZF\ContentNegotiation\HttpMethodOverrideListener;

class HttpMethodOverrideListenerFactory
{
/**
* @param ContainerInterface $container
* @return HttpMethodOverrideListener
*/
public function __invoke(ContainerInterface $container)
{
$options = $container->get(ContentNegotiationOptions::class);
$httpOverrideMethods = $options->getHttpOverrideMethods();
$listener = new HttpMethodOverrideListener($httpOverrideMethods);

return $listener;
}
}
85 changes: 85 additions & 0 deletions src/HttpMethodOverrideListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
<?php
/**
* @license http://opensource.org/licenses/BSD-3-Clause BSD-3-Clause
* @copyright Copyright (c) 2016 Zend Technologies USA Inc. (http://www.zend.com)
*/

namespace ZF\ContentNegotiation;

use Zend\EventManager\AbstractListenerAggregate;
use Zend\EventManager\EventManagerInterface;
use Zend\Mvc\MvcEvent;
use ZF\ApiProblem\ApiProblem;
use ZF\ApiProblem\ApiProblemResponse;
use Zend\Http\Request as HttpRequest;

class HttpMethodOverrideListener extends AbstractListenerAggregate
{
/**
* @var array
*/
protected $httpMethodOverride = [];

/**
* HttpMethodOverrideListener constructor.
*
* @param array $httpMethodOverride
*/
public function __construct(array $httpMethodOverride)
{
$this->httpMethodOverride = $httpMethodOverride;
}

/**
* Priority is set very high (should be executed before all other listeners that rely on the request method value).
* TODO: Check priority value, maybe value should be even higher??
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 an appropriate value. The OPTIONS handling (i.e., HTTP method negotiation) happens at -100. Currently, we already have versioning (for Accept headers) happening at -40 (route-based version checking happens at -41). The main thing is that it happens before the OPTIONS handling, so this works fine.

*
* @param EventManagerInterface $events
Copy link
Member

Choose a reason for hiding this comment

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

one space after @param please

* @param int $priority
*/
public function attach(EventManagerInterface $events, $priority = 1)
{
$this->listeners[] = $events->attach(MvcEvent::EVENT_ROUTE, [$this, 'onRoute'], -40);
}

/**
* Checks for X-HTTP-Method-Override header and sets header inside request object.
*
* @param MvcEvent $event
* @return void|ApiProblemResponse
*/
public function onRoute(MvcEvent $event)
{
$request = $event->getRequest();

if (! $request instanceof HttpRequest) {
return;
}

if (! $request->getHeaders()->has('X-HTTP-Method-Override')) {
return;
}

$method = $request->getMethod();

if (! array_key_exists($method, $this->httpMethodOverride)) {
return new ApiProblemResponse(new ApiProblem(
400,
sprintf('Overriding %s method with X-HTTP-Method-Override header is not allowed', $method)
));
}

$header = $request->getHeader('X-HTTP-Method-Override');
$overrideMethod = $header->getFieldValue();
$allowedMethods = $this->httpMethodOverride[$method];

if (! in_array($overrideMethod, $allowedMethods)) {
return new ApiProblemResponse(new ApiProblem(
400,
sprintf('Illegal override method %s in X-HTTP-Method-Override header', $overrideMethod)
));
}

$request->setMethod($overrideMethod);
}
}
5 changes: 5 additions & 0 deletions src/Module.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ public function onBootstrap(MvcEvent $e)
$services->get(AcceptFilterListener::class)->attach($eventManager);
$services->get(ContentTypeFilterListener::class)->attach($eventManager);

$contentNegotiationOptions = $services->get(ContentNegotiationOptions::class);
if ($contentNegotiationOptions->getXHttpMethodOverrideEnabled()) {
$services->get(HttpMethodOverrideListener::class)->attach($eventManager);
Copy link
Member

Choose a reason for hiding this comment

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

I very much like this conditional attachment.

}

$sharedEventManager = $eventManager->getSharedManager();
$sharedEventManager->attach(
DispatchableInterface::class,
Expand Down
2 changes: 2 additions & 0 deletions test/ContentNegotiationOptionsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ public function dashSeparatedOptions()
return [
'accept-whitelist' => ['accept-whitelist', 'accept_whitelist'],
'content-type-whitelist' => ['content-type-whitelist', 'content_type_whitelist'],
'x-http-method-override-enabled' => ['x-http-method-override-enabled', 'x_http_method_override_enabled'],
'http-override-methods' => ['http-override-methods', 'http_override_methods'],
];
}

Expand Down
31 changes: 31 additions & 0 deletions test/Factory/HttpMethodOverrideListenerFactoryTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php
/**
* @license http://opensource.org/licenses/BSD-3-Clause BSD-3-Clause
* @copyright Copyright (c) 2014 Zend Technologies USA Inc. (http://www.zend.com)
Copy link
Member

Choose a reason for hiding this comment

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

2016 year please

*/

namespace ZFTest\ContentNegotiation\Factory;

use PHPUnit_Framework_TestCase as TestCase;
use Zend\ServiceManager\ServiceManager;
use ZF\ContentNegotiation\ContentNegotiationOptions;
use ZF\ContentNegotiation\Factory\HttpMethodOverrideListenerFactory;
use ZF\ContentNegotiation\HttpMethodOverrideListener;

class HttpMethodOverrideListenerFactoryTest extends TestCase
{
public function testCreateServiceShouldReturnContentTypeFilterListenerInstance()
{
$serviceManager = new ServiceManager();
$serviceManager->setService(
ContentNegotiationOptions::class,
new ContentNegotiationOptions()
);
Copy link
Member

Choose a reason for hiding this comment

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

You can use ->prophesize(...) to create service manager mock (please check how it is done in other tests)


$factory = new HttpMethodOverrideListenerFactory();

$service = $factory($serviceManager, 'HttpMethodOverrideListener');
Copy link
Member

Choose a reason for hiding this comment

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

Please use here as second parameter HttpMethodOverrideListener::class (I know, it is not used)


$this->assertInstanceOf(HttpMethodOverrideListener::class, $service);
}
}
127 changes: 127 additions & 0 deletions test/HttpMethodOverrideListenerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
<?php
/**
* @license http://opensource.org/licenses/BSD-3-Clause BSD-3-Clause
* @copyright Copyright (c) 2016 Zend Technologies USA Inc. (http://www.zend.com)
*/

namespace ZFTest\ContentNegotiation;

use PHPUnit_Framework_TestCase as TestCase;
use Zend\Http\Request as HttpRequest;
use Zend\Mvc\MvcEvent;
use ZF\ApiProblem\ApiProblemResponse;
use ZF\ContentNegotiation\HttpMethodOverrideListener;

class HttpMethodOverrideListenerTest extends TestCase
{
use RouteMatchFactoryTrait;

/**
* @var HttpMethodOverrideListener
*/
protected $listener;

/**
* @var array
*/
protected $httpMethodOverride = [
HttpRequest::METHOD_GET => [
HttpRequest::METHOD_HEAD,
HttpRequest::METHOD_POST,
HttpRequest::METHOD_PUT,
HttpRequest::METHOD_DELETE,
HttpRequest::METHOD_PATCH,
],
HttpRequest::METHOD_POST => [
]
Copy link
Member

Choose a reason for hiding this comment

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

Trailing comma please.

Copy link
Member

Choose a reason for hiding this comment

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

please add trailing comma

];

/**
* Set up test
*/
public function setUp()
{
$this->listener = new HttpMethodOverrideListener($this->httpMethodOverride);
}

/**
* @return array
*/
public function httpMethods()
{
return [
'head' => [HttpRequest::METHOD_HEAD],
'post' => [HttpRequest::METHOD_POST],
'put' => [HttpRequest::METHOD_PUT],
'delete' => [HttpRequest::METHOD_DELETE],
'patch' => [HttpRequest::METHOD_PATCH],
];
}

/**
* @dataProvider httpMethods
*/
public function testHttpMethodOverrideListener($method)
{
$listener = $this->listener;

$request = new HttpRequest();
$request->setMethod('GET');
$request->getHeaders()->addHeaderLine('X-HTTP-Method-Override', $method);

$event = new MvcEvent();
$event->setRequest($request);
$event->setRouteMatch($this->createRouteMatch([]));

$result = $listener->onRoute($event);
$this->assertEquals($method, $request->getMethod());
}

Copy link
Member

Choose a reason for hiding this comment

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

Please remove empty line above.

/**
* @dataProvider httpMethods
*/
Copy link
Member

Choose a reason for hiding this comment

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

What does it for?

public function testHttpMethodOverrideListenerReturnsProblemResponseForMethodNotInConfig($method)
{
$listener = $this->listener;

$request = new HttpRequest();
$request->setMethod('PATCH');
$request->getHeaders()->addHeaderLine('X-HTTP-Method-Override', $method);

$event = new MvcEvent();
$event->setRequest($request);

$result = $listener->onRoute($event);
$this->assertInstanceOf(ApiProblemResponse::class, $result);
$problem = $result->getApiProblem();
$this->assertEquals(400, $problem->status);
$this->assertContains(
'Overriding PATCH method with X-HTTP-Method-Override header is not allowed',
$problem->detail
);
}

/**
* @dataProvider httpMethods
*/
public function testHttpMethodOverrideListenerReturnsProblemResponseForIllegalOverrideValue($method)
{
$listener = $this->listener;

$request = new HttpRequest();
$request->setMethod('POST');
$request->getHeaders()->addHeaderLine('X-HTTP-Method-Override', $method);

$event = new MvcEvent();
$event->setRequest($request);

$result = $listener->onRoute($event);
$this->assertInstanceOf(ApiProblemResponse::class, $result);
$problem = $result->getApiProblem();
$this->assertEquals(400, $problem->status);
$this->assertContains(
sprintf('Illegal override method %s in X-HTTP-Method-Override header', $method),
$problem->detail
);
}
}