Unverified Commit b15791c0 authored by larowlan's avatar larowlan

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

(cherry picked from commit b4a02309)
parent 6c92a2a8
......@@ -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:
......
<?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);
}
}
......@@ -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);
......
......@@ -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 }
......@@ -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));
}
}
}
......
<?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());
}
}
......@@ -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));
......
......@@ -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);
}
......
......@@ -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');
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment