Commit 450cc989 authored by alexpott's avatar alexpott

Issue #2322809 by chx, dawehner, tim.plunkett: Tighten routing security by...

Issue #2322809 by chx, dawehner, tim.plunkett: Tighten routing security by access checking in matchRequest.
parent bf28ded5
......@@ -431,12 +431,15 @@ services:
link_generator:
class: Drupal\Core\Utility\LinkGenerator
arguments: ['@url_generator', '@module_handler']
router:
class: Drupal\Core\Routing\AccessAwareRouter
arguments: ['@router.no_access_checks', '@access_manager', '@current_user']
router.dynamic:
class: Symfony\Cmf\Component\Routing\DynamicRouter
arguments: ['@router.request_context', '@router.matcher', '@url_generator']
tags:
- { name: service_collector, tag: route_enhancer, call: addRouteEnhancer }
router:
router.no_access_checks:
class: Symfony\Cmf\Component\Routing\ChainRouter
calls:
- [setContext, ['@router.request_context']]
......@@ -490,7 +493,7 @@ services:
arguments: ['@config.factory']
path.validator:
class: Drupal\Core\Path\PathValidator
arguments: ['@router', '@router.route_provider', '@request_stack', '@access_manager', '@current_user']
arguments: ['@router', '@router.route_provider', '@request_stack']
# The argument to the hashing service defined in services.yml, to the
# constructor of PhpassHashedPassword is the log2 number of iterations for
......@@ -633,13 +636,6 @@ services:
arguments: ['@router.route_provider', '@url_generator', '@paramconverter_manager', '@access_arguments_resolver', '@request_stack']
calls:
- [setContainer, ['@service_container']]
access_subscriber:
class: Drupal\Core\EventSubscriber\AccessSubscriber
arguments: ['@access_manager', '@current_user']
calls:
- [setCurrentUser, ['@?current_user']]
tags:
- { name: event_subscriber }
access_route_subscriber:
class: Drupal\Core\EventSubscriber\AccessRouteSubscriber
tags:
......
<?php
/**
* @file
* Contains Drupal\Core\EventSubscriber\AccessSubscriber.
*/
namespace Drupal\Core\EventSubscriber;
use Drupal\Core\Access\AccessManagerInterface;
use Drupal\Core\Session\AccountInterface;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\HttpKernel\HttpKernelInterface;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
/**
* Access subscriber for controller requests.
*/
class AccessSubscriber implements EventSubscriberInterface {
/**
* The current user.
*
* @var \Drupal\Core\Session\AccountInterface
*/
protected $currentUser;
/**
* The access manager.
*
* @var \Drupal\Core\Access\AccessManagerInterface
*/
protected $accessManager;
/**
* Constructs a new AccessSubscriber.
*
* @param \Drupal\Core\Access\AccessManagerInterface $access_manager
* The access check manager that will be responsible for applying
* AccessCheckers against routes.
* @param \Drupal\Core\Session\AccountInterface $current_user
* The current user.
*/
public function __construct(AccessManagerInterface $access_manager, AccountInterface $current_user) {
$this->accessManager = $access_manager;
$this->currentUser = $current_user;
}
/**
* Verifies that the current user can access the requested path.
*
* @param \Symfony\Component\HttpKernel\Event\GetResponseEvent $event
* The Event to process.
*
* @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
* Thrown when the access got denied.
*/
public function onKernelRequestAccessCheck(GetResponseEvent $event) {
$request = $event->getRequest();
// The controller is being handled by the HTTP kernel, so add an attribute
// to tell us this is the controller request.
$request->attributes->set('_controller_request', TRUE);
if (!$request->attributes->has(RouteObjectInterface::ROUTE_OBJECT)) {
// If no Route is available it is likely a static resource and access is
// handled elsewhere.
return;
}
// Wrap this in a try/catch to ensure the '_controller_request' attribute
// can always be removed.
try {
$access = $this->accessManager->check($request->attributes->get(RouteObjectInterface::ROUTE_OBJECT), $request, $this->currentUser);
}
catch (\Exception $e) {
$request->attributes->remove('_controller_request');
throw $e;
}
$request->attributes->remove('_controller_request');
if (!$access) {
throw new AccessDeniedHttpException();
}
}
/**
* Sets the current user.
*
* @param \Drupal\Core\Session\AccountInterface|null $current_user
* The current user service.
*/
public function setCurrentUser(AccountInterface $current_user = NULL) {
$this->currentUser = $current_user;
}
/**
* Registers the methods in this class that should be listeners.
*
* @return array
* An array of event listener definitions.
*/
static function getSubscribedEvents() {
$events[KernelEvents::REQUEST][] = array('onKernelRequestAccessCheck', 30);
return $events;
}
}
......@@ -8,12 +8,11 @@
namespace Drupal\Core\Path;
use Drupal\Component\Utility\UrlHelper;
use Drupal\Core\Access\AccessManagerInterface;
use Drupal\Core\ParamConverter\ParamNotConvertedException;
use Drupal\Core\Routing\RequestHelper;
use Drupal\Core\Routing\RouteProviderInterface;
use Drupal\Core\Session\AccountInterface;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
/**
......@@ -42,20 +41,6 @@ class PathValidator implements PathValidatorInterface {
*/
protected $requestStack;
/**
* The access manager.
*
* @var \Drupal\Core\Access\AccessManagerInterface
*/
protected $accessManager;
/**
* The user account.
*
* @var \Drupal\Core\Session\AccountInterface
*/
protected $account;
/**
* Creates a new PathValidator.
*
......@@ -65,17 +50,11 @@ class PathValidator implements PathValidatorInterface {
* The route provider.
* @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
* The request stack.
* @param \Drupal\Core\Access\AccessManagerInterface $access_manager
* The access manager.
* @param \Drupal\Core\Session\AccountInterface $account
* The user account.
*/
public function __construct(RequestMatcherInterface $request_matcher, RouteProviderInterface $route_provider, RequestStack $request_stack, AccessManagerInterface $access_manager, AccountInterface $account) {
public function __construct(RequestMatcherInterface $request_matcher, RouteProviderInterface $route_provider, RequestStack $request_stack) {
$this->requestMatcher = $request_matcher;
$this->routeProvider = $route_provider;
$this->requestStack = $request_stack;
$this->accessManager = $access_manager;
$this->account = $account;
}
/**
......@@ -93,25 +72,23 @@ public function isValid($path) {
return FALSE;
}
// We can not use $this->requestMatcher->match() because we need to set
// the _menu_admin attribute to indicate a menu administrator is running
// the menu access check.
$request = RequestHelper::duplicate($this->requestStack->getCurrentRequest(), '/' . $path);
$request->attributes->set('_system_path', $path);
// We indicate that a menu administrator is running the menu access check.
$request->attributes->set('_menu_admin', TRUE);
// Attempt to match this path to provide a fully built request to the
// access checker.
try {
$request->attributes->add($this->requestMatcher->matchRequest($request));
$this->requestMatcher->matchRequest($request);
}
catch (ParamNotConvertedException $e) {
return FALSE;
}
// Consult the access manager.
$routes = $collection->all();
$route = reset($routes);
return $this->accessManager->check($route, $request, $this->account);
catch (AccessDeniedHttpException $e) {
return FALSE;
}
return TRUE;
}
}
<?php
/**
* @file
* Contains \Drupal\Core\Routing\Router.
*/
namespace Drupal\Core\Routing;
use Drupal\Core\Access\AccessManagerInterface;
use Drupal\Core\Session\AccountInterface;
use Symfony\Cmf\Component\Routing\ChainRouter;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\Routing\RequestContext;
/**
* A router class for Drupal with access check and upcasting.
*/
class AccessAwareRouter implements AccessAwareRouterInterface {
/**
* The chain router doing the actual routing.
*
* @var \Symfony\Cmf\Component\Routing\ChainRouter
*/
protected $chainRouter;
/**
* The access manager.
*
* @var \Drupal\Core\Access\AccessManagerInterface
*/
protected $accessManager;
/**
* The account to use in access checks.
*
* @var \Drupal\Core\Session\AccountInterface;
*/
protected $account;
/**
* Constructs a router for Drupal with access check and upcasting.
*
* @param \Symfony\Cmf\Component\Routing\ChainRouter $chain_router
* The chain router doing the actual routing.
* @param \Drupal\Core\Access\AccessManagerInterface $access_manager
* The access manager.
* @param \Drupal\Core\Session\AccountInterface $account
* The account to use in access checks.
*/
public function __construct(ChainRouter $chain_router, AccessManagerInterface $access_manager, AccountInterface $account) {
$this->chainRouter = $chain_router;
$this->accessManager = $access_manager;
$this->account = $account;
}
/**
* {@inheritdoc}
*/
public function __call($name, $arguments) {
// Ensure to call every other function to the chained router.
// @todo Sadly does the ChainRouter not implement an interface in CMF.
return call_user_func_array([$this->chainRouter, $name], $arguments);
}
/**
* {@inheritdoc}
*/
public function setContext(RequestContext $context) {
$this->chainRouter->setContext($context);
}
/**
* {@inheritdoc}
*/
public function getContext() {
return $this->chainRouter->getContext();
}
/**
* {@inheritdoc}
*
* @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
* Thrown when access checking failed.
*/
public function matchRequest(Request $request) {
$parameters = $this->chainRouter->matchRequest($request);
$request->attributes->add($parameters);
$this->checkAccess($request);
// We can not return $parameters because the access check can change the
// request attributes.
return $request->attributes->all();
}
/**
* Apply access check service to the route and parameters in the request.
*
* @param \Symfony\Component\HttpFoundation\Request $request
* The request to access check.
*/
protected function checkAccess(Request $request) {
// The controller is being handled by the HTTP kernel, so add an attribute
// to tell us this is the controller request.
$request->attributes->set('_controller_request', TRUE);
$e = FALSE;
try {
if (!$this->accessManager->check($request->attributes->get(RouteObjectInterface::ROUTE_OBJECT), $request, $this->account)) {
$e = new AccessDeniedHttpException();
}
}
catch (\Exception $e) {
}
// @todo Once PHP 5.5 is a requirement refactor this using finally.
$request->attributes->remove('_controller_request');
if ($e) {
throw $e;
}
}
/**
* {@inheritdoc}
*/
public function getRouteCollection() {
return $this->chainRouter->getRouteCollection();
}
/**
* {@inheritdoc}
*/
public function generate($name, $parameters = array(), $referenceType = self::ABSOLUTE_PATH) {
return $this->chainRouter->generate($name, $parameters, $referenceType);
}
/**
* {@inheritdoc}
*
* @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
* Thrown when access checking failed.
*/
public function match($pathinfo) {
return $this->matchRequest(Request::create($pathinfo));
}
}
<?php
/**
* @file
* Contains
*/
namespace Drupal\Core\Routing;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
use Symfony\Component\Routing\RouterInterface;
/**
* Interface for a router class for Drupal with access check and upcasting.
*/
interface AccessAwareRouterInterface extends RouterInterface, RequestMatcherInterface {
/**
* {@inheritdoc}
*
* @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
* Thrown when access checking failed.
*/
public function matchRequest(Request $request);
/**
* {@inheritdoc}
*
* @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
* Thrown when $access_check is enabled and access checking failed.
*/
public function match($pathinfo);
}
......@@ -103,7 +103,8 @@ public function __construct($route_name, $route_parameters = array(), $options =
* A path (e.g. 'node/1', 'http://drupal.org').
*
* @return static
* An Url object.
* An Url object. Warning: the object is created even if the current user
* can not access the path.
*
* @throws \Drupal\Core\Routing\MatchingRouteNotFoundException
* Thrown when the path cannot be matched.
......@@ -123,7 +124,9 @@ public static function createFromPath($path) {
else {
// Look up the route name and parameters used for the given path.
try {
$result = \Drupal::service('router')->match('/' . $path);
// We use the router without access checks because URL objects might be
// created and stored for different users.
$result = \Drupal::service('router.no_access_checks')->match('/' . $path);
}
catch (ResourceNotFoundException $e) {
throw new MatchingRouteNotFoundException(sprintf('No matching route could be found for the path "%s"', $path), 0, $e);
......@@ -141,14 +144,18 @@ public static function createFromPath($path) {
* A request object.
*
* @return static
* A Url object.
* A Url object. Warning: the object is created even if the current user
* would get an access denied running the same request via the normal page
* flow.
*
* @throws \Drupal\Core\Routing\MatchingRouteNotFoundException
* Thrown when the request cannot be matched.
*/
public static function createFromRequest(Request $request) {
try {
$result = \Drupal::service('router')->matchRequest($request);
// We use the router without access checks because URL objects might be
// created and stored for different users.
$result = \Drupal::service('router.no_access_checks')->matchRequest($request);
}
catch (ResourceNotFoundException $e) {
throw new MatchingRouteNotFoundException(sprintf('No matching route could be found for the request: %s', $request), 0, $e);
......
......@@ -12,13 +12,10 @@
use Drupal\Core\Controller\ControllerBase;
use Drupal\Core\Language\Language;
use Drupal\Core\Language\LanguageManagerInterface;
use Drupal\Core\ParamConverter\ParamNotConvertedException;
use Drupal\Core\PathProcessor\InboundPathProcessorInterface;
use Drupal\Core\Session\AccountInterface;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
/**
......@@ -167,15 +164,7 @@ public function itemPage(Request $request, $plugin_id) {
// Check access for the path/route for editing, so we can decide to
// include a link to edit or not.
$route_request = $this->getRequestForPath($request, $mapper->getBasePath());
$edit_access = FALSE;
if (!empty($route_request)) {
$route_name = $route_request->attributes->get(RouteObjectInterface::ROUTE_NAME);
// Note that the parameters don't really matter here since we're
// passing in the request which already has the upcast attributes.
$parameters = array();
$edit_access = $this->accessManager->checkNamedRoute($route_name, $parameters, $this->account, $route_request);
}
$edit_access = $this->accessManager->checkNamedRoute($mapper->getBaseRouteName(), $request->attributes->get('_raw_variables')->all(), $this->account);
// Build list of operations.
$operations = array();
......@@ -228,35 +217,4 @@ public function itemPage(Request $request, $plugin_id) {
return $page;
}
/**
* Matches a path in the router.
*
* @param \Symfony\Component\HttpFoundation\Request $request
* Page request object.
* @param string $path
* Path to look up.
*
* @return \Symfony\Component\HttpFoundation\Request|null
* A populated request object or NULL if the patch could not be matched.
*/
protected function getRequestForPath(Request $request, $path) {
// @todo Use RequestHelper::duplicate once https://drupal.org/node/2090293
// is fixed.
$route_request = Request::create($request->getBaseUrl() . '/' . $path);
// Find the system path by resolving aliases, language prefix, etc.
$processed = $this->pathProcessor->processInbound($path, $route_request);
$route_request->attributes->set('_system_path', $processed);
// Attempt to match this path to provide a fully built request.
try {
$route_request->attributes->add($this->router->matchRequest($route_request));
return $route_request;
}
catch (ParamNotConvertedException $e) {
return NULL;
}
catch (ResourceNotFoundException $e) {
return NULL;
}
}
}
......@@ -704,9 +704,6 @@ protected function drupalLogin(AccountInterface $account) {
if ($pass) {
$this->loggedInUser = $account;
$this->container->get('current_user')->setAccount($account);
// @todo Temporary workaround for not being able to use synchronized
// services in non dumped container.
$this->container->get('access_subscriber')->setCurrentUser($account);
}
}
......
......@@ -20,6 +20,7 @@
use Drupal\Component\Utility\Unicode;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\Routing\Matcher\RequestMatcherInterface;
use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Routing\Exception\MethodNotAllowedException;
......@@ -209,6 +210,9 @@ protected function getRequestForPath($path, array $exclude) {
catch (MethodNotAllowedException $e) {
return NULL;
}
catch (AccessDeniedHttpException $e) {
return NULL;
}
}
}
......@@ -56,7 +56,7 @@ public function test8() {
public function test9($uid) {
$text = 'Route not matched.';
try {
$match = \Drupal::service('router')->match('/user/' . $uid);
$match = \Drupal::service('router.no_access_checks')->match('/user/' . $uid);
if (isset($match['user']) && $match['user'] instanceof UserInterface) {
$text = sprintf('User route "%s" was matched.', $match[RouteObjectInterface::ROUTE_NAME]);
}
......
......@@ -14,6 +14,7 @@
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
use Symfony\Component\Routing\Matcher\UrlMatcherInterface;
......@@ -110,7 +111,7 @@ public function getLangcode(Request $request = NULL) {
* @return bool
* TRUE if the path is administrative, FALSE otherwise.
*/
public function isAdminPath(Request $request) {
protected function isAdminPath(Request $request) {
$result = FALSE;
if ($request && $this->adminContext) {
// If called from an event subscriber, the request may not have the route
......@@ -127,6 +128,9 @@ public function isAdminPath(Request $request) {
catch (ResourceNotFoundException $e) {
return FALSE;
}
catch (AccessDeniedHttpException $e) {
return FALSE;
}
$route_object = $attributes[RouteObjectInterface::ROUTE_OBJECT];
}
$result = $this->adminContext->isAdminRoute($route_object);
......
<?php
/**
* @file
* Contains \Drupal\Core\EventSubscriber\AccessSubscriberTest.
*/
namespace Drupal\Tests\Core\EventSubscriber;
use Drupal\Core\EventSubscriber\AccessSubscriber;
use Drupal\Core\Session\AccountInterface;
use Drupal\Tests\UnitTestCase;
use Symfony\Cmf\Component\Routing\RouteObjectInterface;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
use Symfony\Component\HttpFoundation\ParameterBag;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Route;
/**
* @coversDefaultClass \Drupal\Core\EventSubscriber\AccessSubscriber
* @group EventSubscriber
*/
class AccessSubscriberTest extends UnitTestCase {
/**
* @var \Symfony\Component\HttpKernel\Event\GetResponseEvent|PHPUnit_Framework_MockObject_MockObject
*/
protected $event;
/**
* @var \Symfony\Component\HttpFoundation\Request|PHPUnit_Framework_MockObject_MockObject
*/
protected $request;
/**
* @var \Symfony\Component\HttpFoundation\ParameterBag|PHPUnit_Framework_MockObject_MockObject
*/
protected $parameterBag;
/**
* @var \Symfony\Component\Routing\Route|PHPUnit_Framework_MockObject_MockObject
*/
protected $route;
/**
* @var \Drupal\Core\Access\AccessManagerInterface|\PHPUnit_Framework_MockObject_MockObject
*/
protected $accessManager;
/**
* @var Drupal\Core\Session\AccountInterface|PHPUnit_Framework_MockObject_MockObject
*/
protected $currentUser;
/**
* {@inheritdoc}
*/
protected function setUp() {
$this->event = $this->getMockBuilder('Symfony\Component\HttpKernel\Event\GetResponseEvent')
->disableOriginalConstructor()
->getMock();
$this->request = $this->getMockBuilder('Symfony\Component\HttpFoundation\Request')
->disableOriginalConstructor()
->getMock();
$this->parameterBag = $this->getMockBuilder('Symfony\Component\HttpFoundation\ParameterBag')
->disableOriginalConstructor()
->getMock();
$this->route = $this->getMockBuilder('Symfony\Component\Routing\Route')
->disableOriginalConstructor()
->getMock();
$this->request->attributes = $this->parameterBag;
$this->event->expects($this->any())
->method('getRequest')
->will($this->returnValue($this->request));
$this->accessManager = $this->getMock('Drupal\Core\Access\AccessManagerInterface');
$this->currentUser = $this->getMockBuilder('Drupal\Core\Session\AccountInterface')
->disableOriginalConstructor()
->getMock();
}
/**
* Tests access denied throws an exception.
*
* @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
*/
public function testAccessSubscriberThrowsAccessDeniedException() {
$this->parameterBag->expects($this->any())
->method('has')
->with(RouteObjectInterface::ROUTE_OBJECT)
->will($this->returnValue(TRUE));
$this->parameterBag->expects($this->any())
->method('get')
->with(RouteObjectInterface::ROUTE_OBJECT)
->will($this->returnValue($this->route));
$this->accessManager->expects($this->any())
->method('check')
->with($this->anything())
->will($this->returnValue(FALSE));
$subscriber = new AccessSubscriber($this->accessManager, $this->currentUser);
$subscriber->onKernelRequestAccessCheck($this->event);
}
/**
* Tests that the AccessSubscriber only acts on requests with route object.