summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAlex Pott2016-07-28 23:32:38 (GMT)
committerAlex Pott2016-07-28 23:32:38 (GMT)
commitaf80cb499a04d15b5905d19e66c8d5a8efd9b293 (patch)
tree0b7bfb2ea5d4e56ce368e7d2d03a6d0826d0eead
parent9b07bc544c49296bd4fb76b08d676665d9e2d0c1 (diff)
Issue #2659070 by Wim Leers, dawehner, tic2000, Crell: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response
-rw-r--r--core/core.services.yml4
-rw-r--r--core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php11
-rw-r--r--core/lib/Drupal/Core/Routing/MethodFilter.php57
-rw-r--r--core/modules/rest/src/Tests/RESTTestBase.php4
-rw-r--r--core/modules/rest/src/Tests/UpdateTest.php7
-rw-r--r--core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php13
-rw-r--r--core/tests/Drupal/Tests/Core/Routing/MethodFilterTest.php127
7 files changed, 223 insertions, 0 deletions
diff --git a/core/core.services.yml b/core/core.services.yml
index 398a81d..8182903 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -918,6 +918,10 @@ services:
class: Drupal\Core\Routing\RequestFormatRouteFilter
tags:
- { name: route_filter }
+ method_filter:
+ class: Drupal\Core\Routing\MethodFilter
+ tags:
+ - { name: route_filter, priority: 1 }
content_type_header_matcher:
class: Drupal\Core\Routing\ContentTypeHeaderMatcher
tags:
diff --git a/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
index 34eda1f..2853b98 100644
--- a/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
@@ -82,4 +82,15 @@ class ExceptionJsonSubscriber extends HttpExceptionSubscriberBase {
$event->setResponse($response);
}
+ /**
+ * Handles a 415 error for JSON.
+ *
+ * @param \Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent $event
+ * The event to process.
+ */
+ public function on415(GetResponseForExceptionEvent $event) {
+ $response = new JsonResponse(['message' => $event->getException()->getMessage()], Response::HTTP_UNSUPPORTED_MEDIA_TYPE);
+ $event->setResponse($response);
+ }
+
}
diff --git a/core/lib/Drupal/Core/Routing/MethodFilter.php b/core/lib/Drupal/Core/Routing/MethodFilter.php
new file mode 100644
index 0000000..e565a3e
--- /dev/null
+++ b/core/lib/Drupal/Core/Routing/MethodFilter.php
@@ -0,0 +1,57 @@
+<?php
+
+namespace Drupal\Core\Routing;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Route;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * Filters routes based on the HTTP method.
+ */
+class MethodFilter implements RouteFilterInterface {
+
+ /**
+ * {@inheritdoc}
+ */
+ public function filter(RouteCollection $collection, Request $request) {
+ $method = $request->getMethod();
+
+ $all_supported_methods = [];
+
+ foreach ($collection->all() as $name => $route) {
+ $supported_methods = $route->getMethods();
+
+ // A route not restricted to specific methods allows any method. If this
+ // is the case, we'll also have at least one route left in the collection,
+ // hence we don't need to calculate the set of all supported methods.
+ if (empty($supported_methods)) {
+ continue;
+ }
+
+ // If the GET method is allowed we also need to allow the HEAD method
+ // since HEAD is a GET method that doesn't return the body.
+ if (in_array('GET', $supported_methods, TRUE)) {
+ $supported_methods[] = 'HEAD';
+ }
+
+ if (!in_array($method, $supported_methods, TRUE)) {
+ $all_supported_methods = array_merge($supported_methods, $all_supported_methods);
+ $collection->remove($name);
+ }
+ }
+ if (count($collection)) {
+ return $collection;
+ }
+ throw new MethodNotAllowedException(array_unique($all_supported_methods));
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function applies(Route $route) {
+ return !empty($route->getMethods());
+ }
+
+}
diff --git a/core/modules/rest/src/Tests/RESTTestBase.php b/core/modules/rest/src/Tests/RESTTestBase.php
index f7cf3dd..4e660c3 100644
--- a/core/modules/rest/src/Tests/RESTTestBase.php
+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -176,6 +176,10 @@ abstract class RESTTestBase extends WebTestBase {
break;
}
+ if ($mime_type === 'none') {
+ unset($curl_options[CURLOPT_HTTPHEADER]['Content-Type']);
+ }
+
$this->responseBody = $this->curlExec($curl_options);
// Ensure that any changes to variables in the other thread are picked up.
diff --git a/core/modules/rest/src/Tests/UpdateTest.php b/core/modules/rest/src/Tests/UpdateTest.php
index d503386..b8f1b54 100644
--- a/core/modules/rest/src/Tests/UpdateTest.php
+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -8,6 +8,7 @@ use Drupal\Component\Serialization\Json;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\entity_test\Entity\EntityTest;
+use Symfony\Component\HttpFoundation\Response;
/**
* Tests the update of resources.
@@ -67,6 +68,12 @@ class UpdateTest extends RESTTestBase {
$patch_entity->set('uuid', NULL);
$serialized = $serializer->serialize($patch_entity, $this->defaultFormat, $context);
+ // Update the entity over the REST API but forget to specify a Content-Type
+ // header, this should throw the proper exception.
+ $this->httpRequest($entity->toUrl(), 'PATCH', $serialized, 'none');
+ $this->assertResponse(Response::HTTP_UNSUPPORTED_MEDIA_TYPE);
+ $this->assertRaw('No route found that matches &quot;Content-Type: none&quot;');
+
// Update the entity over the REST API.
$response = $this->httpRequest($entity->urlInfo(), 'PATCH', $serialized, $this->defaultMimeType);
$this->assertResponse(200);
diff --git a/core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php b/core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php
index ab9cd92..7d11b81 100644
--- a/core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Routing/ExceptionHandlingTest.php
@@ -29,6 +29,19 @@ class ExceptionHandlingTest extends KernelTestBase {
}
/**
+ * Tests on a route with a non-supported HTTP method.
+ */
+ public function test405() {
+ $request = Request::create('/router_test/test15', 'PATCH');
+
+ /** @var \Symfony\Component\HttpKernel\HttpKernelInterface $kernel */
+ $kernel = \Drupal::getContainer()->get('http_kernel');
+ $response = $kernel->handle($request);
+
+ $this->assertEqual(Response::HTTP_METHOD_NOT_ALLOWED, $response->getStatusCode());
+ }
+
+ /**
* Tests the exception handling for json and 403 status code.
*/
public function testJson403() {
diff --git a/core/tests/Drupal/Tests/Core/Routing/MethodFilterTest.php b/core/tests/Drupal/Tests/Core/Routing/MethodFilterTest.php
new file mode 100644
index 0000000..1b72b75
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Routing/MethodFilterTest.php
@@ -0,0 +1,127 @@
+<?php
+
+namespace Drupal\Tests\Core\Routing;
+
+use Drupal\Core\Routing\MethodFilter;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Route;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Routing\MethodFilter
+ * @group Routing
+ */
+class MethodFilterTest extends \PHPUnit_Framework_TestCase {
+
+ /**
+ * @covers ::applies
+ * @dataProvider providerApplies
+ */
+ public function testApplies(array $route_methods, $expected_applies) {
+ $route = new Route('/test', [], [], [], '', [], $route_methods);
+ $method_filter = new MethodFilter();
+
+ $this->assertSame($expected_applies, $method_filter->applies($route));
+ }
+
+ /**
+ * Data provider for testApplies().
+ *
+ * @return array
+ */
+ public function providerApplies() {
+ return [
+ 'only GET' => [['GET'], TRUE],
+ 'only PATCH' => [['PATCH'], TRUE],
+ 'only POST' => [['POST'], TRUE],
+ 'only DELETE' => [['DELETE'], TRUE],
+ 'only HEAD' => [['HEAD'], TRUE],
+ 'all' => [['GET', 'PATCH', 'POST', 'DELETE', 'HEAD'], TRUE],
+ 'none' => [[], FALSE],
+ ];
+ }
+
+ /**
+ * @covers ::filter
+ */
+ public function testWithAllowedMethod() {
+ $request = Request::create('/test', 'GET');
+ $collection = new RouteCollection();
+ $collection->add('test_route.get', new Route('/test', [], [], [], '', [], ['GET']));
+ $collection_before = clone $collection;
+
+ $method_filter = new MethodFilter();
+ $result_collection = $method_filter->filter($collection, $request);
+
+ $this->assertEquals($collection_before, $result_collection);
+ }
+
+ /**
+ * @covers ::filter
+ */
+ public function testWithAllowedMethodAndMultipleMatchingRoutes() {
+ $request = Request::create('/test', 'GET');
+ $collection = new RouteCollection();
+ $collection->add('test_route.get', new Route('/test', [], [], [], '', [], ['GET']));
+ $collection->add('test_route2.get', new Route('/test', [], [], [], '', [], ['GET']));
+ $collection->add('test_route3.get', new Route('/test', [], [], [], '', [], ['GET']));
+
+ $collection_before = clone $collection;
+
+ $method_filter = new MethodFilter();
+ $result_collection = $method_filter->filter($collection, $request);
+
+ $this->assertEquals($collection_before, $result_collection);
+ }
+
+ /**
+ * @covers ::filter
+ */
+ public function testMethodNotAllowedException() {
+ $request = Request::create('/test', 'PATCH');
+ $collection = new RouteCollection();
+ $collection->add('test_route.get', new Route('/test', [], [], [], '', [], ['GET']));
+
+ $this->setExpectedException(MethodNotAllowedException::class);
+
+ $method_filter = new MethodFilter();
+ $method_filter->filter($collection, $request);
+ }
+
+ /**
+ * @covers ::filter
+ */
+ public function testMethodNotAllowedExceptionWithMultipleRoutes() {
+ $request = Request::create('/test', 'PATCH');
+ $collection = new RouteCollection();
+ $collection->add('test_route.get', new Route('/test', [], [], [], '', [], ['GET']));
+ $collection->add('test_route2.get', new Route('/test', [], [], [], '', [], ['GET']));
+ $collection->add('test_route3.get', new Route('/test', [], [], [], '', [], ['GET']));
+
+ $this->setExpectedException(MethodNotAllowedException::class);
+
+ $method_filter = new MethodFilter();
+ $method_filter->filter($collection, $request);
+ }
+
+ /**
+ * @covers ::filter
+ */
+ public function testFilteredMethods() {
+ $request = Request::create('/test', 'PATCH');
+ $collection = new RouteCollection();
+ $collection->add('test_route.get', new Route('/test', [], [], [], '', [], ['GET']));
+ $collection->add('test_route2.get', new Route('/test', [], [], [], '', [], ['PATCH']));
+ $collection->add('test_route3.get', new Route('/test', [], [], [], '', [], ['POST']));
+
+ $expected_collection = new RouteCollection();
+ $expected_collection->add('test_route2.get', new Route('/test', [], [], [], '', [], ['PATCH']));
+
+ $method_filter = new MethodFilter();
+ $result_collection = $method_filter->filter($collection, $request);
+
+ $this->assertEquals($expected_collection, $result_collection);
+ }
+
+}