summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAlex Pott2016-07-29 15:11:36 (GMT)
committerAlex Pott2016-07-29 15:11:36 (GMT)
commit3de79990b339537404813f053c23a8b5a795e801 (patch)
tree7d817bd22b5362de8dc7588ae8b73d362888d315
parent776613979d5312c28baf1aceb1cac0ddc5f16cfb (diff)
Issue #2681911 by garphy, Wim Leers, gabesullice: REST requests without X-CSRF-Token header: unhelpful response significantly hinders DX, should receive a 401 response
-rw-r--r--core/lib/Drupal/Core/Access/AccessResult.php17
-rw-r--r--core/lib/Drupal/Core/Access/AccessResultForbidden.php35
-rw-r--r--core/lib/Drupal/Core/Access/AccessResultReasonInterface.php36
-rw-r--r--core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php2
-rw-r--r--core/lib/Drupal/Core/Routing/AccessAwareRouter.php3
-rw-r--r--core/modules/rest/src/Tests/CreateTest.php4
-rw-r--r--core/modules/rest/src/Tests/DeleteTest.php3
-rw-r--r--core/modules/rest/src/Tests/RESTTestBase.php18
-rw-r--r--core/modules/rest/src/Tests/UpdateTest.php5
-rw-r--r--core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php45
-rw-r--r--core/tests/Drupal/Tests/Core/Access/AccessResultTest.php18
-rw-r--r--core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php17
12 files changed, 193 insertions, 10 deletions
diff --git a/core/lib/Drupal/Core/Access/AccessResult.php b/core/lib/Drupal/Core/Access/AccessResult.php
index 8148d4c..f67b9f5 100644
--- a/core/lib/Drupal/Core/Access/AccessResult.php
+++ b/core/lib/Drupal/Core/Access/AccessResult.php
@@ -51,11 +51,16 @@ abstract class AccessResult implements AccessResultInterface, RefinableCacheable
/**
* Creates an AccessResultInterface object with isForbidden() === TRUE.
*
+ * @param string|null $reason
+ * (optional) The reason why access is forbidden. Intended for developers,
+ * hence not translatable.
+ *
* @return \Drupal\Core\Access\AccessResult
* isForbidden() will be TRUE.
*/
- public static function forbidden() {
- return new AccessResultForbidden();
+ public static function forbidden($reason = NULL) {
+ assert('is_string($reason) || is_null($reason)');
+ return new AccessResultForbidden($reason);
}
/**
@@ -334,8 +339,16 @@ abstract class AccessResult implements AccessResultInterface, RefinableCacheable
if ($this->isForbidden() || $other->isForbidden()) {
$result = static::forbidden();
if (!$this->isForbidden()) {
+ if ($other instanceof AccessResultReasonInterface) {
+ $result->setReason($other->getReason());
+ }
$merge_other = TRUE;
}
+ else {
+ if ($this instanceof AccessResultReasonInterface) {
+ $result->setReason($this->getReason());
+ }
+ }
}
elseif ($this->isAllowed() && $other->isAllowed()) {
$result = static::allowed();
diff --git a/core/lib/Drupal/Core/Access/AccessResultForbidden.php b/core/lib/Drupal/Core/Access/AccessResultForbidden.php
index 4dc0120..2dc9137 100644
--- a/core/lib/Drupal/Core/Access/AccessResultForbidden.php
+++ b/core/lib/Drupal/Core/Access/AccessResultForbidden.php
@@ -5,7 +5,25 @@ namespace Drupal\Core\Access;
/**
* Value object indicating a forbidden access result, with cacheability metadata.
*/
-class AccessResultForbidden extends AccessResult {
+class AccessResultForbidden extends AccessResult implements AccessResultReasonInterface {
+
+ /**
+ * The reason why access is forbidden. For use in error messages.
+ *
+ * @var string|null
+ */
+ protected $reason;
+
+ /**
+ * Constructs a new AccessResultForbidden instance.
+ *
+ * @param null|string $reason
+ * (optional) a message to provide details about this access result
+ */
+ public function __construct($reason = NULL) {
+ $this->reason = $reason;
+ }
+
/**
* {@inheritdoc}
@@ -14,4 +32,19 @@ class AccessResultForbidden extends AccessResult {
return TRUE;
}
+ /**
+ * {@inheritdoc}
+ */
+ public function getReason() {
+ return $this->reason;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setReason($reason) {
+ $this->reason = $reason;
+ return $this;
+ }
+
}
diff --git a/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
new file mode 100644
index 0000000..468d998
--- /dev/null
+++ b/core/lib/Drupal/Core/Access/AccessResultReasonInterface.php
@@ -0,0 +1,36 @@
+<?php
+
+namespace Drupal\Core\Access;
+
+/**
+ * Interface for access result value objects with stored reason for developers.
+ *
+ * For example, a developer can specify the reason for forbidden access:
+ * @code
+ * new AccessResultForbidden('You are not authorized to hack core');
+ * @endcode
+ *
+ * @see \Drupal\Core\Access\AccessResultInterface
+ */
+interface AccessResultReasonInterface extends AccessResultInterface {
+
+ /**
+ * Gets the reason for this access result.
+ *
+ * @return string|NULL
+ * The reason of this access result or NULL if no reason is provided.
+ */
+ public function getReason();
+
+ /**
+ * Sets the reason for this access result.
+ *
+ * @param $reason string|NULL
+ * The reason of this access result or NULL if no reason is provided.
+ *
+ * @return \Drupal\Core\Access\AccessResultInterface
+ * The access result instance.
+ */
+ public function setReason($reason);
+
+}
diff --git a/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
index 470ea9f..53f326d 100644
--- a/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
+++ b/core/lib/Drupal/Core/Access/CsrfRequestHeaderAccessCheck.php
@@ -102,7 +102,7 @@ class CsrfRequestHeaderAccessCheck implements AccessCheckInterface {
// Kept here for sessions active during update.
if (!$this->csrfToken->validate($csrf_token, self::TOKEN_KEY)
&& !$this->csrfToken->validate($csrf_token, 'rest')) {
- return AccessResult::forbidden()->setCacheMaxAge(0);
+ return AccessResult::forbidden()->setReason('X-CSRF-Token request header is missing')->setCacheMaxAge(0);
}
}
// Let other access checkers decide if the request is legit.
diff --git a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
index 80e16eb..9346b0a 100644
--- a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -3,6 +3,7 @@
namespace Drupal\Core\Routing;
use Drupal\Core\Access\AccessManagerInterface;
+use Drupal\Core\Access\AccessResultReasonInterface;
use Drupal\Core\Session\AccountInterface;
use Symfony\Cmf\Component\Routing\ChainRouter;
use Symfony\Component\HttpFoundation\Request;
@@ -105,7 +106,7 @@ class AccessAwareRouter implements AccessAwareRouterInterface {
$request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $access_result);
}
if (!$access_result->isAllowed()) {
- throw new AccessDeniedHttpException();
+ throw new AccessDeniedHttpException($access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL);
}
}
diff --git a/core/modules/rest/src/Tests/CreateTest.php b/core/modules/rest/src/Tests/CreateTest.php
index 28c8af6..d71b9e4 100644
--- a/core/modules/rest/src/Tests/CreateTest.php
+++ b/core/modules/rest/src/Tests/CreateTest.php
@@ -356,6 +356,10 @@ class CreateTest extends RESTTestBase {
public function assertCreateEntityOverRestApi($entity_type, $serialized = NULL) {
// Note: this will fail with PHP 5.6 when always_populate_raw_post_data is
// set to something other than -1. See https://www.drupal.org/node/2456025.
+ // Try first without the CSRF token, which should fail.
+ $this->httpRequest('entity/' . $entity_type, 'POST', $serialized, $this->defaultMimeType, TRUE);
+ $this->assertResponse(403, 'X-CSRF-Token request header is missing');
+ // Then try with the CSRF token.
$response = $this->httpRequest('entity/' . $entity_type, 'POST', $serialized, $this->defaultMimeType);
$this->assertResponse(201);
diff --git a/core/modules/rest/src/Tests/DeleteTest.php b/core/modules/rest/src/Tests/DeleteTest.php
index 4cc8016..7de3fb4 100644
--- a/core/modules/rest/src/Tests/DeleteTest.php
+++ b/core/modules/rest/src/Tests/DeleteTest.php
@@ -38,6 +38,9 @@ class DeleteTest extends RESTTestBase {
// Create an entity programmatically.
$entity = $this->entityCreate($entity_type);
$entity->save();
+ // Try first to delete over REST API without the CSRF token.
+ $this->httpRequest($entity->urlInfo(), 'DELETE', NULL, NULL, TRUE);
+ $this->assertResponse(403, 'X-CSRF-Token request header is missing');
// Delete it over the REST API.
$response = $this->httpRequest($entity->urlInfo(), 'DELETE');
// Clear the static cache with entity_load(), otherwise we won't see the
diff --git a/core/modules/rest/src/Tests/RESTTestBase.php b/core/modules/rest/src/Tests/RESTTestBase.php
index 4e660c3..a3f6a5f 100644
--- a/core/modules/rest/src/Tests/RESTTestBase.php
+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -85,11 +85,13 @@ abstract class RESTTestBase extends WebTestBase {
* The body for POST and PUT.
* @param string $mime_type
* The MIME type of the transmitted content.
+ * @param bool $forget_xcsrf_token
+ * If TRUE, the CSRF token won't be included in request.
*
* @return string
* The content returned from the request.
*/
- protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL) {
+ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL, $forget_xcsrf_token = FALSE) {
if (!isset($mime_type)) {
$mime_type = $this->defaultMimeType;
}
@@ -130,9 +132,11 @@ abstract class RESTTestBase extends WebTestBase {
CURLOPT_POSTFIELDS => $body,
CURLOPT_URL => $url,
CURLOPT_NOBODY => FALSE,
- CURLOPT_HTTPHEADER => array(
+ CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array(
'Content-Type: ' . $mime_type,
'X-CSRF-Token: ' . $token,
+ ) : array(
+ 'Content-Type: ' . $mime_type,
),
);
break;
@@ -144,9 +148,11 @@ abstract class RESTTestBase extends WebTestBase {
CURLOPT_POSTFIELDS => $body,
CURLOPT_URL => $url,
CURLOPT_NOBODY => FALSE,
- CURLOPT_HTTPHEADER => array(
+ CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array(
'Content-Type: ' . $mime_type,
'X-CSRF-Token: ' . $token,
+ ) : array(
+ 'Content-Type: ' . $mime_type,
),
);
break;
@@ -158,9 +164,11 @@ abstract class RESTTestBase extends WebTestBase {
CURLOPT_POSTFIELDS => $body,
CURLOPT_URL => $url,
CURLOPT_NOBODY => FALSE,
- CURLOPT_HTTPHEADER => array(
+ CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array(
'Content-Type: ' . $mime_type,
'X-CSRF-Token: ' . $token,
+ ) : array(
+ 'Content-Type: ' . $mime_type,
),
);
break;
@@ -171,7 +179,7 @@ abstract class RESTTestBase extends WebTestBase {
CURLOPT_CUSTOMREQUEST => 'DELETE',
CURLOPT_URL => $url,
CURLOPT_NOBODY => FALSE,
- CURLOPT_HTTPHEADER => array('X-CSRF-Token: ' . $token),
+ CURLOPT_HTTPHEADER => !$forget_xcsrf_token ? array('X-CSRF-Token: ' . $token) : array(),
);
break;
}
diff --git a/core/modules/rest/src/Tests/UpdateTest.php b/core/modules/rest/src/Tests/UpdateTest.php
index b8f1b54..dcab1ff 100644
--- a/core/modules/rest/src/Tests/UpdateTest.php
+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -374,6 +374,11 @@ class UpdateTest extends RESTTestBase {
}
$serialized = $serializer->serialize($normalized, $format, $context);
+ // Try first without CSRF token which should fail.
+ $this->httpRequest($url, 'PATCH', $serialized, $mime_type, TRUE);
+ $this->assertResponse(403, 'X-CSRF-Token request header is missing');
+
+ // Then try with CSRF token.
$this->httpRequest($url, 'PATCH', $serialized, $mime_type);
$this->assertResponse(200);
}
diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php
new file mode 100644
index 0000000..3409f37
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Access/AccessResultForbiddenTest.php
@@ -0,0 +1,45 @@
+<?php
+
+namespace Drupal\Tests\Core\Access;
+
+use Drupal\Core\Access\AccessResultForbidden;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Access\AccessResultForbidden
+ * @group Access
+ */
+class AccessResultForbiddenTest extends UnitTestCase {
+
+ /**
+ * Tests the construction of an AccessResultForbidden object.
+ *
+ * @covers ::__construct
+ * @covers ::getReason
+ */
+ public function testConstruction() {
+
+ $a = new AccessResultForbidden();
+ $this->assertEquals(NULL, $a->getReason());
+
+ $reason = $this->getRandomGenerator()->string();
+ $b = new AccessResultForbidden($reason);
+ $this->assertEquals($reason, $b->getReason());
+ }
+
+ /**
+ * Test setReason()
+ *
+ * @covers ::setReason
+ */
+ public function testSetReason() {
+ $a = new AccessResultForbidden();
+
+ $reason = $this->getRandomGenerator()->string();
+ $return = $a->setReason($reason);
+
+ $this->assertSame($reason, $a->getReason());
+ $this->assertSame($a, $return);
+ }
+
+}
diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
index 287c344..ca262c7 100644
--- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
@@ -10,6 +10,7 @@ namespace Drupal\Tests\Core\Access;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Access\AccessResultInterface;
use Drupal\Core\Access\AccessResultNeutral;
+use Drupal\Core\Access\AccessResultReasonInterface;
use Drupal\Core\Cache\Cache;
use Drupal\Core\Cache\CacheableDependencyInterface;
use Drupal\Core\DependencyInjection\ContainerBuilder;
@@ -113,6 +114,23 @@ class AccessResultTest extends UnitTestCase {
}
/**
+ * @covers ::forbidden
+ */
+ public function testAccessForbiddenReason() {
+ $verify = function (AccessResult $access, $reason) {
+ $this->assertInstanceOf(AccessResultReasonInterface::class, $access);
+ $this->assertSame($reason, $access->getReason());
+ };
+
+ $b = AccessResult::forbidden();
+ $verify($b, NULL);
+
+ $reason = $this->getRandomGenerator()->string();
+ $b = AccessResult::forbidden($reason);
+ $verify($b, $reason);
+ }
+
+ /**
* @covers ::allowedIf
* @covers ::isAllowed
* @covers ::isForbidden
diff --git a/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
index df3b580..0b7bcc6 100644
--- a/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
+++ b/core/tests/Drupal/Tests/Core/Routing/AccessAwareRouterTest.php
@@ -8,6 +8,7 @@ use Drupal\Core\Routing\AccessAwareRouterInterface;
use Drupal\Tests\UnitTestCase;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\Routing\Route;
/**
@@ -106,6 +107,22 @@ class AccessAwareRouterTest extends UnitTestCase {
}
/**
+ * Tests the matchRequest() function for access denied with reason message.
+ */
+ public function testCheckAccessResultWithReason() {
+ $this->setupRouter();
+ $request = new Request();
+ $reason = $this->getRandomGenerator()->string();
+ $access_result = AccessResult::forbidden($reason);
+ $this->accessManager->expects($this->once())
+ ->method('checkRequest')
+ ->with($request)
+ ->willReturn($access_result);
+ $this->setExpectedException(AccessDeniedHttpException::class, $reason);
+ $this->router->matchRequest($request);
+ }
+
+ /**
* Ensure that methods are passed to the wrapped router.
*
* @covers ::__call