Issue #2858482 by Wim Leers, dawehner, benjy, larowlan, tedbow, borisson_:...

Issue #2858482 by Wim Leers, dawehner, benjy, larowlan, tedbow, borisson_: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent

core/core.services.yml

@@ -936,11 +936,22 @@ services:
   method_filter:
     class: Drupal\Core\Routing\MethodFilter
     tags:
-      - { name: route_filter, priority: 1 }
+      # The HTTP method route filter must run very early: it removes any routes
+      # whose requirements do not allow the HTTP method of the current request.
+      # Throws a 405 if no routes match the current request's HTTP method.
+      # (If it runs before content_type_header_matcher, it can ensure that only
+      # receives routes which can have a Content-Type request header.)
+      - { name: route_filter, priority: 10 }
   content_type_header_matcher:
     class: Drupal\Core\Routing\ContentTypeHeaderMatcher
     tags:
-      - { name: route_filter }
+      # The Content-Type request header route filter must run early: it removes
+      # any routes whose requirements do not allow the Content-Type request
+      # header of the current request.
+      # Throws a 415 if no routes match the Content-Type request header of the
+      # current request, or if it has no Content-Type request header.
+      # Note it does nothing for GET requests.
+      - { name: route_filter, priority: 5 }
   paramconverter_manager:
     class: Drupal\Core\ParamConverter\ParamConverterManager
     tags:

core/lib/Drupal/Core/Routing/BcRoute.php

+<?php
+
+namespace Drupal\Core\Routing;
+
+use Symfony\Component\Routing\Route;
+
+/**
+ * A backwards compatibility route.
+ *
+ * When a route is deprecated for another one, and backwards compatibility is
+ * provided, then it's best practice to:
+ * - not duplicate all route definition metadata, to instead have an "as empty
+ *   as possible" route
+ * - have an accompanying outbound route processor, that overwrites this empty
+ *   route definition with the redirected route's definition.
+ *
+ * @see \Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC
+ */
+class BcRoute extends Route {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function __construct() {
+    parent::__construct('');
+    $this->setOption('bc_route', TRUE);
+  }
+
+}

core/modules/dblog/tests/src/Functional/DbLogResourceTest.php

@@ -62,7 +62,7 @@ public function testWatchdog() {
       ->fetchField();
 
     $this->initAuthentication();
-    $url = Url::fromRoute('rest.dblog.GET.' . static::$format, ['id' => $id, '_format' => static::$format]);
+    $url = Url::fromRoute('rest.dblog.GET', ['id' => $id, '_format' => static::$format]);
     $request_options = $this->getAuthenticationRequestOptions('GET');
 
     $response = $this->request('GET', $url, $request_options);

core/modules/rest/rest.services.yml

@@ -43,3 +43,8 @@ services:
     arguments: ['@entity_type.manager']
     tags:
       - { name: path_processor_inbound }
+  rest.route_processor_get_bc:
+    class: \Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC
+    arguments: ['%serializer.formats%', '@router.route_provider']
+    tags:
+      - { name: route_processor_outbound }

core/modules/rest/src/Plugin/ResourceBase.php

@@ -4,6 +4,7 @@
 
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Drupal\Core\Plugin\PluginBase;
+use Drupal\Core\Routing\BcRoute;
 use Psr\Log\LoggerInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\Routing\Route;
@@ -114,29 +115,24 @@ public function routes() {
 
     $methods = $this->availableMethods();
     foreach ($methods as $method) {
-      $route = $this->getBaseRoute($canonical_path, $method);
-
-      switch ($method) {
-        case 'POST':
-          $route->setPath($create_path);
-          $collection->add("$route_name.$method", $route);
-          break;
-
-        case 'GET':
-        case 'HEAD':
-          // Restrict GET and HEAD requests to the media type specified in the
-          // HTTP Accept headers.
-          foreach ($this->serializerFormats as $format_name) {
-            // Expose one route per available format.
-            $format_route = clone $route;
-            $format_route->addRequirements(['_format' => $format_name]);
-            $collection->add("$route_name.$method.$format_name", $format_route);
-          }
-          break;
-
-        default:
-          $collection->add("$route_name.$method", $route);
-          break;
+      $path = $method === 'POST'
+        ? $create_path
+        : $canonical_path;
+      $route = $this->getBaseRoute($path, $method);
+
+      // Note that '_format' and '_content_type_format' route requirements are
+      // added in ResourceRoutes::getRoutesForResourceConfig().
+      $collection->add("$route_name.$method", $route);
+
+      // BC: the REST module originally created per-format GET routes, instead
+      // of a single route. To minimize the surface of this BC layer, this uses
+      // route definitions that are as empty as possible, plus an outbound route
+      // processor.
+      // @see \Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC
+      if ($method === 'GET' || $method === 'HEAD') {
+        foreach ($this->serializerFormats as $format_name) {
+          $collection->add("$route_name.$method.$format_name", (new BcRoute())->setRequirement('_format', $format_name));
+        }
       }
     }
 

core/modules/rest/src/RouteProcessor/RestResourceGetRouteProcessorBC.php

+<?php
+
+namespace Drupal\rest\RouteProcessor;
+
+use Drupal\Core\Render\BubbleableMetadata;
+use Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface;
+use Drupal\Core\Routing\RouteProviderInterface;
+use Symfony\Component\Routing\Route;
+
+/**
+ * Processes the BC REST routes, to ensure old route names continue to work.
+ */
+class RestResourceGetRouteProcessorBC implements OutboundRouteProcessorInterface {
+
+  /**
+   * The available serialization formats.
+   *
+   * @var string[]
+   */
+  protected $serializerFormats = [];
+
+  /**
+   * The route provider.
+   *
+   * @var \Drupal\Core\Routing\RouteProviderInterface
+   */
+  protected $routeProvider;
+
+  /**
+   * Constructs a RestResourceGetRouteProcessorBC object.
+   *
+   * @param string[] $serializer_formats
+   *   The available serialization formats.
+   * @param \Drupal\Core\Routing\RouteProviderInterface $route_provider
+   *   The route provider.
+   */
+  public function __construct(array $serializer_formats, RouteProviderInterface $route_provider) {
+    $this->serializerFormats = $serializer_formats;
+    $this->routeProvider = $route_provider;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function processOutbound($route_name, Route $route, array &$parameters, BubbleableMetadata $bubbleable_metadata = NULL) {
+    $route_name_parts = explode('.', $route_name);
+    // BC: the REST module originally created per-format GET routes, instead
+    // of a single route. To minimize the surface of this BC layer, this uses
+    // route definitions that are as empty as possible, plus an outbound route
+    // processor.
+    // @see \Drupal\rest\Plugin\ResourceBase::routes()
+    if ($route_name_parts[0] === 'rest' && $route_name_parts[count($route_name_parts) - 2] === 'GET' && in_array($route_name_parts[count($route_name_parts) - 1], $this->serializerFormats, TRUE)) {
+      array_pop($route_name_parts);
+      $redirected_route_name = implode('.', $route_name_parts);
+      @trigger_error(sprintf("The '%s' route is deprecated since version 8.5.x and will be removed in 9.0.0. Use the '%s' route instead.", $route_name, $redirected_route_name), E_USER_DEPRECATED);
+      static::overwriteRoute($route, $this->routeProvider->getRouteByName($redirected_route_name));
+    }
+  }
+
+  /**
+   * Overwrites one route's metadata with the other's.
+   *
+   * @param \Symfony\Component\Routing\Route $target_route
+   *   The route whose metadata to overwrite.
+   * @param \Symfony\Component\Routing\Route $source_route
+   *   The route whose metadata to read from.
+   *
+   * @see \Symfony\Component\Routing\Route
+   */
+  protected static function overwriteRoute(Route $target_route, Route $source_route) {
+    $target_route->setPath($source_route->getPath());
+    $target_route->setDefaults($source_route->getDefaults());
+    $target_route->setRequirements($source_route->getRequirements());
+    $target_route->setOptions($source_route->getOptions());
+    $target_route->setHost($source_route->getHost());
+    $target_route->setSchemes($source_route->getSchemes());
+    $target_route->setMethods($source_route->getMethods());
+  }
+
+}

core/modules/rest/src/Routing/ResourceRoutes.php

@@ -92,8 +92,11 @@ protected function getRoutesForResourceConfig(RestResourceConfigInterface $rest_
       /** @var \Symfony\Component\Routing\Route $route */
       // @todo: Are multiple methods possible here?
       $methods = $route->getMethods();
-      // Only expose routes where the method is enabled in the configuration.
-      if ($methods && ($method = $methods[0]) && $supported_formats = $rest_resource_config->getFormats($method)) {
+      // Only expose routes
+      // - that have an explicit method and allow >=1 format for that method
+      // - that exist for BC
+      // @see \Drupal\rest\RouteProcessor\RestResourceGetRouteProcessorBC
+      if (($methods && ($method = $methods[0]) && $supported_formats = $rest_resource_config->getFormats($method)) || $route->hasOption('bc_route')) {
         $route->setRequirement('_csrf_request_header_token', 'TRUE');
 
         // Check that authentication providers are defined.
@@ -108,20 +111,24 @@ protected function getRoutesForResourceConfig(RestResourceConfigInterface $rest_
           continue;
         }
 
-        // If the route has a format requirement, then verify that the
-        // resource has it.
-        $format_requirement = $route->getRequirement('_format');
-        if ($format_requirement && !in_array($format_requirement, $rest_resource_config->getFormats($method))) {
-          continue;
+        // Remove BC routes for unsupported formats.
+        if ($route->getOption('bc_route') === TRUE) {
+          $format_requirement = $route->getRequirement('_format');
+          if ($format_requirement && !in_array($format_requirement, $rest_resource_config->getFormats($method))) {
+            continue;
+          }
         }
 
         // The configuration has been validated, so we update the route to:
+        // - set the allowed response body content types/formats for methods
+        //   that may send response bodies
         // - set the allowed request body content types/formats for methods that
         //   allow request bodies to be sent
         // - set the allowed authentication providers
+        if (in_array($method, ['GET', 'HEAD', 'POST', 'PUT', 'PATCH'], TRUE)) {
+          $route->addRequirements(['_format' => implode('|', $rest_resource_config->getFormats($method))]);
+        }
         if (in_array($method, ['POST', 'PATCH', 'PUT'], TRUE)) {
-          // Restrict the incoming HTTP Content-type header to the allowed
-          // formats.
           $route->addRequirements(['_content_type_format' => implode('|', $rest_resource_config->getFormats($method))]);
         }
         $route->setOption('_auth', $rest_resource_config->getAuthenticationProviders($method));

core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php

@@ -21,6 +21,7 @@
 use Drupal\Tests\rest\Functional\ResourceTestBase;
 use GuzzleHttp\RequestOptions;
 use Psr\Http\Message\ResponseInterface;
+use Symfony\Component\Routing\Exception\RouteNotFoundException;
 
 /**
  * Even though there is the generic EntityResource, it's necessary for every
@@ -693,15 +694,27 @@ public function testGet() {
     $this->assert406Response($response);
     $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
 
-    $url = Url::fromRoute('rest.entity.' . static::$entityTypeId . '.GET.' . static::$format);
+    $url = Url::fromRoute('rest.entity.' . static::$entityTypeId . '.GET');
     $url->setRouteParameter(static::$entityTypeId, 987654321);
     $url->setOption('query', ['_format' => static::$format]);
 
     // DX: 404 when GETting non-existing entity.
     $response = $this->request('GET', $url, $request_options);
     $path = str_replace('987654321', '{' . static::$entityTypeId . '}', $url->setAbsolute()->setOptions(['base_url' => '', 'query' => []])->toString());
-    $message = 'The "' . static::$entityTypeId . '" parameter was not converted for the path "' . $path . '" (route name: "rest.entity.' . static::$entityTypeId . '.GET.' . static::$format . '")';
+    $message = 'The "' . static::$entityTypeId . '" parameter was not converted for the path "' . $path . '" (route name: "rest.entity.' . static::$entityTypeId . '.GET")';
     $this->assertResourceErrorResponse(404, $message, $response);
+
+    // BC: Format-specific GET routes are deprecated. They are available on both
+    // new and old sites, but trigger deprecation notices.
+    $bc_route = Url::fromRoute('rest.entity.' . static::$entityTypeId . '.GET.' . static::$format, $url->getRouteParameters(), $url->getOptions());
+    $bc_route->setUrlGenerator($this->container->get('url_generator'));
+    $this->assertSame($url->toString(TRUE)->getGeneratedUrl(), $bc_route->toString(TRUE)->getGeneratedUrl());
+    // Verify no format-specific GET BC routes are created for other formats.
+    $other_format = static::$format === 'json' ? 'xml' : 'json';
+    $bc_route_other_format = Url::fromRoute('rest.entity.' . static::$entityTypeId . '.GET.' . $other_format, $url->getRouteParameters(), $url->getOptions());
+    $bc_route_other_format->setUrlGenerator($this->container->get('url_generator'));
+    $this->setExpectedException(RouteNotFoundException::class);
+    $bc_route_other_format->toString(TRUE);
   }
 
   /**
@@ -968,6 +981,7 @@ public function testPost() {
     if ($this->entity->getEntityType()->hasLinkTemplate('create')) {
       $this->entityStorage->load(static::$secondCreatedEntityId)->delete();
       $old_url = Url::fromUri('base:entity/' . static::$entityTypeId);
+      $old_url->setOption('query', ['_format' => static::$format]);
       $response = $this->request('POST', $old_url, $request_options);
       $this->assertResourceResponse(201, FALSE, $response);
     }

core/modules/user/src/Tests/RestRegisterUserTest.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\user\Tests;
 
+use Drupal\Core\Url;
 use Drupal\rest\Tests\RESTTestBase;
 use Drupal\user\Entity\Role;
 use Drupal\user\RoleInterface;
@@ -166,7 +167,7 @@ protected function registerUser($name, $include_password = TRUE) {
    */
   protected function registerRequest($name, $include_password = TRUE) {
     $serialized = $this->createSerializedUser($name, $include_password);
-    $this->httpRequest('/user/register', 'POST', $serialized, 'application/hal+json');
+    $this->httpRequest(Url::fromRoute('rest.user_registration.POST', ['_format' => 'hal_json']), 'POST', $serialized, 'application/hal+json');
   }
 
 }