Unverified Commit 41dfad38 authored by larowlan's avatar larowlan

Issue #2854543 by Wim Leers, polynya, dawehner, mpdonadio, gabesullice,...

Issue #2854543 by Wim Leers, polynya, dawehner, mpdonadio, gabesullice, larowlan, klausi: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't
parent c2585a18
......@@ -4,6 +4,7 @@
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
/**
......@@ -17,7 +18,14 @@ class RequestFormatRouteFilter implements FilterInterface {
public function filter(RouteCollection $collection, Request $request) {
// Determine the request format.
$default_format = static::getDefaultFormat($collection);
$format = $request->getRequestFormat($default_format);
// If the request does not specify a format then use the default.
if (is_null($request->getRequestFormat(NULL))) {
$format = $default_format;
$request->setRequestFormat($default_format);
}
else {
$format = $request->getRequestFormat($default_format);
}
$routes_with_requirement = [];
$routes_without_requirement = [];
......@@ -60,7 +68,9 @@ public function filter(RouteCollection $collection, Request $request) {
*
* By default, use 'html' as the default format. But when there's only a
* single route match, and that route specifies a '_format' requirement
* listing a single format, then use that as the default format.
* listing a single format, then use that as the default format. Also, if
* there are multiple routes which all require the same single format then
* use it.
*
* @param \Symfony\Component\Routing\RouteCollection $collection
* The route collection to filter.
......@@ -69,15 +79,20 @@ public function filter(RouteCollection $collection, Request $request) {
* The default format.
*/
protected static function getDefaultFormat(RouteCollection $collection) {
$default_format = 'html';
if ($collection->count() === 1) {
$only_route = $collection->getIterator()->current();
$required_format = $only_route->getRequirement('_format');
if (strpos($required_format, '|') === FALSE) {
$default_format = $required_format;
}
}
return $default_format;
// Get the set of formats across all routes in the collection.
$all_formats = array_reduce($collection->all(), function (array $carry, Route $route) {
// Routes without a '_format' requirement are assumed to require HTML.
$route_formats = !$route->hasRequirement('_format')
? ['html']
: explode('|', $route->getRequirement('_format'));
return array_merge($carry,$route_formats);
}, []);
$formats = array_unique(array_filter($all_formats));
// The default format is 'html' unless ALL routes require the same format.
return count($formats) === 1
? reset($formats)
: 'html';
}
}
......@@ -46,7 +46,9 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
}
// Determine the request format using the negotiator.
$request->setRequestFormat($this->getContentType($request));
if ($requested_format = $this->getContentType($request)) {
$request->setRequestFormat($requested_format);
}
return $this->app->handle($request, $type, $catch);
}
......@@ -88,8 +90,8 @@ protected function getContentType(Request $request) {
return $request->query->get('_format');
}
// Do HTML last so that it always wins.
return 'html';
// No format was specified in the request.
return NULL;
}
}
......@@ -12,3 +12,7 @@ services:
public: false
tags:
- { name: page_cache_request_policy }
rest_test.encoder.foobar:
class: Drupal\serialization\Encoder\JsonEncoder
tags:
- { name: encoder, format: foobar }
......@@ -156,12 +156,22 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
/**
* Provides an entity resource.
*
* @param bool $single_format
* Provisions a single-format entity REST resource. Defaults to FALSE.
*/
protected function provisionEntityResource() {
protected function provisionEntityResource($single_format = FALSE) {
if ($existing = $this->resourceConfigStorage->load(static::$resourceConfigId)) {
$existing->delete();
}
$format = $single_format
? [static::$format]
: [static::$format, 'foobar'];
// It's possible to not have any authentication providers enabled, when
// testing public (anonymous) usage of a REST resource.
$auth = isset(static::$auth) ? [static::$auth] : [];
$this->provisionResource([static::$format], $auth);
$this->provisionResource($format, $auth);
}
/**
......@@ -434,20 +444,6 @@ public function testGet() {
}
$this->provisionEntityResource();
// Simulate the developer again forgetting the ?_format query string.
$url->setOption('query', []);
// DX: 406 when ?_format is missing, except when requesting a canonical HTML
// route.
$response = $this->request('GET', $url, $request_options);
if ($has_canonical_url && (!static::$auth || static::$auth === 'cookie')) {
$this->assertSame(403, $response->getStatusCode());
}
else {
$this->assert406Response($response);
}
$url->setOption('query', ['_format' => static::$format]);
// DX: forgetting authentication: authentication provider-specific error
// response.
......@@ -472,10 +468,44 @@ public function testGet() {
unset($request_options[RequestOptions::HEADERS]['REST-test-auth-global']);
$request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions('GET'));
// DX: 403 when unauthorized.
$response = $this->request('GET', $url, $request_options);
// First: single format. Drupal will automatically pick the only format.
$this->provisionEntityResource(TRUE);
$expected_403_cacheability = $this->getExpectedUnauthorizedAccessCacheability();
$this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS');
// DX: 403 because unauthorized single-format route, ?_format is omittable.
$url->setOption('query', []);
$response = $this->request('GET', $url, $request_options);
if ($has_canonical_url) {
$this->assertSame(403, $response->getStatusCode());
$this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
}
else {
$this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS');
}
$this->assertSame(static::$auth ? [] : ['MISS'], $response->getHeader('X-Drupal-Cache'));
// DX: 403 because unauthorized.
$url->setOption('query', ['_format' => static::$format]);
$response = $this->request('GET', $url, $request_options);
$this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', $has_canonical_url ? 'MISS' : 'HIT');
// Then, what we'll use for the remainder of the test: multiple formats.
$this->provisionEntityResource();
// DX: 406 because despite unauthorized, ?_format is not omittable.
$url->setOption('query', []);
$response = $this->request('GET', $url, $request_options);
if ($has_canonical_url) {
$this->assertSame(403, $response->getStatusCode());
$this->assertSame(['HIT'], $response->getHeader('X-Drupal-Dynamic-Cache'));
}
else {
$this->assertSame(406, $response->getStatusCode());
$this->assertSame(['UNCACHEABLE'], $response->getHeader('X-Drupal-Dynamic-Cache'));
}
$this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
$this->assertSame(static::$auth ? [] : ['MISS'], $response->getHeader('X-Drupal-Cache'));
// DX: 403 because unauthorized.
$url->setOption('query', ['_format' => static::$format]);
$response = $this->request('GET', $url, $request_options);
$this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'HIT');
$this->assertArrayNotHasKey('Link', $response->getHeaders());
$this->setUpAuthorization('GET');
......
name: 'Default format test'
type: module
description: 'Support module for testing default route format.'
package: Testing
version: VERSION
core: 8.x
default_format_test.machine:
path: '/default_format_test/machine'
defaults:
# Same controller + method!
_controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
requirements:
_access: 'TRUE'
_format: 'json'
default_format_test.human:
path: '/default_format_test/human'
defaults:
# Same controller + method!
_controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
requirements:
_access: 'TRUE'
_format: 'html'
# Route definition identical to default_format_test.machine, only different name.
# @see \Drupal\FunctionalTests\Routing\DefaultFormatTest::testMultiple
default_format_test.machine.alias:
path: '/default_format_test/machine'
defaults:
# Same controller + method!
_controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
requirements:
_access: 'TRUE'
_format: 'json'
<?php
namespace Drupal\default_format_test;
use Drupal\Core\Cache\CacheableResponse;
use Symfony\Component\HttpFoundation\Request;
class DefaultFormatTestController {
public function content(Request $request) {
$format = $request->getRequestFormat();
return new CacheableResponse('format:' . $format, 200, ['Content-Type' => $request->getMimeType($format)]);
}
}
<?php
namespace Drupal\FunctionalTests\Routing;
use Drupal\Tests\BrowserTestBase;
/**
* @group routing
*/
class DefaultFormatTest extends BrowserTestBase {
/**
* {@inheritdoc}
*/
public static $modules = ['system', 'default_format_test'];
public function testFoo() {
$this->drupalGet('/default_format_test/human');
$this->assertSame('format:html', $this->getSession()->getPage()->getContent());
$this->assertSame('MISS', $this->drupalGetHeader('X-Drupal-Cache'));
$this->drupalGet('/default_format_test/machine');
$this->assertSame('format:json', $this->getSession()->getPage()->getContent());
$this->assertSame('MISS', $this->drupalGetHeader('X-Drupal-Cache'));
}
public function testMultipleRoutesWithSameSingleFormat() {
$this->drupalGet('/default_format_test/machine');
$this->assertSame('format:json', $this->getSession()->getPage()->getContent());
}
}
......@@ -68,10 +68,10 @@ public function testFormatViaQueryParameter() {
*
* @covers ::getContentType
*/
public function testUnknowContentTypeReturnsHtmlByDefault() {
public function testUnknowContentTypeReturnsNull() {
$request = new Request();
$this->assertSame('html', $this->contentNegotiation->getContentType($request));
$this->assertNull($this->contentNegotiation->getContentType($request));
}
/**
......@@ -83,7 +83,7 @@ public function testUnknowContentTypeButAjaxRequest() {
$request = new Request();
$request->headers->set('X-Requested-With', 'XMLHttpRequest');
$this->assertSame('html', $this->contentNegotiation->getContentType($request));
$this->assertNull($this->contentNegotiation->getContentType($request));
}
/**
......@@ -98,7 +98,7 @@ public function testHandle() {
$request->setFormat()->shouldNotBeCalled();
// Request format will be set with default format.
$request->setRequestFormat('html')->shouldBeCalled();
$request->setRequestFormat()->shouldNotBeCalled();
// Some getContentType calls we don't really care about but have to mock.
$request_data = $this->prophesize(ParameterBag::class);
......@@ -127,7 +127,7 @@ public function testSetFormat() {
$request->setFormat('david', 'geeky/david')->shouldBeCalled();
// Some calls we don't care about.
$request->setRequestFormat('html')->shouldBeCalled();
$request->setRequestFormat()->shouldNotBeCalled();
$request_data = $this->prophesize(ParameterBag::class);
$request_data->get('ajax_iframe_upload', FALSE)->shouldBeCalled();
$request_mock = $request->reveal();
......
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