Use RouteMatch in access-checks and remove RequestHelper::duplicate()

This is just an idea how to determine whether to call the ones with request or not.

1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -186,7 +186,10 @@ public function checkNamedRoute($route_name, array $parameters, AccountInterface
      public function checkRequest(Request $request, AccountInterface $account) {
   +    // @todo: Temporary workaround: Ensure that the request is available as an
   +    // upcasted route argument in the AccessArgumentsResolver.
        $route_match = RouteMatch::createFromRequest($request);
   +    $route_match->getParameters()->set('request', $request);
        return $this->check($route_match, $account, $request);
   

   Mh, in case we do that, I would suggest to add a dedicated method on the route match

2. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
   @@ -48,9 +48,9 @@ function __construct(CsrfTokenGenerator $csrf_token) {
   -  public function access(Route $route, Request $request) {
   +  public function access(Route $route, Request $request = NULL) {
        // If this is the controller request, check CSRF access as normal.
   -    if ($request->attributes->get('_controller_request')) {
   +    if ($request) {
          // @todo Remove dependency on the internal _system_path attribute:
          //   https://www.drupal.org/node/2293501.
          return $this->csrfToken->validate($request->query->get('token'), $request->attributes->get('_system_path')) ? static::ALLOW : static::KILL;
   

   Mh, I would like to avoid that the access checkers itself have to be aware of that.

3. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
   @@ -102,21 +102,8 @@ public function matchRequest(Request $request) {
      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->checkRequest($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;
   +    if (!$this->accessManager->checkRequest($request, $this->account)) {
   +      throw new AccessDeniedHttpException();
        }
   

   The main reason why we have that complex code here is that you maybe have a not-existing route in the request, are you sure this will fly?

Regarding #6.2, this is in fact the important decision. The patch just makes current behavior more explicit. This does not mean that we should keep it that way though. Given that we already are using optional arguments in many access-checkers, it feels natural to also make the request optional.

Not sure whether you had a look at #3 but this skipped those access checkers, in case they require the request for the non-existance of the request.
This is at least the best DX from the perspective of the module developer.

Regarding #6.3 git log -S _controller_request turns up two issues: #1798296: Integrate CSRF link token directly into routing system and #2322809: Tighten routing security by access checking in matchRequest. Therefore I'm pretty sure that the _controller_request request attribute has been introduce specifically in order to be able to figure out whether the csrf access check is executed against the current request (attribute present) or against a fake-request (attribute absent). The try...catch block is only there such that it is possible to remove the request-attribute in any case after the access-check did run.

You are absolute right, so we kinda have solved that implicit now, by distinct request aware and not request aware access checkers.

+++ b/core/lib/Drupal/Core/Access/AccessArgumentsResolver.php
@@ -49,6 +49,11 @@ protected function getArgument(\ReflectionParameter $parameter, RouteMatchInterf
+    // @todo Remove this once AccessManagerInterface::checkNamedRoute() is fixed
+    //   to not leak _raw_variables from the request being duplicated.
+    // @see https://drupal.org/node/2265939

Well #2265939: [sechole] AccessManager::checkNamedRoute() leaks attributes from current request went in. Do we have an @todo for a new issue? Or do you intend to fix it in this issue?

Didn't review the whole patch yet, but in general really, really like this.

I really like how this turned out, got a lot of understanding already from our discussion.

1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -114,32 +104,29 @@ class AccessManager implements ContainerAwareInterface, AccessManagerInterface {
   +    if ($needs_incoming_request) {
   +      $this->checkNeedsRequest[$service_id] = $service_id;
   +    }
   

   I'd still like some interface to ensure that.

2. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -220,19 +202,40 @@ public function checkNamedRoute($route_name, array $parameters = array(), Accoun
   +  public function checkRequest(Request $request, AccountInterface $account = NULL) {
   ...
   +  public function check(RouteMatchInterface $route_match, AccountInterface $account = NULL, Request $request = NULL) {
   

   Do we need all those three methods to be public? I though having just checkedNamedRoute and checkRequest would be enough

3. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
   @@ -49,25 +49,9 @@ function __construct(CsrfTokenGenerator $csrf_token) {
   -    // If this is the controller request, check CSRF access as normal.
   -    if ($request->attributes->get('_controller_request')) {
   -      // @todo Remove dependency on the internal _system_path attribute:
   -      //   https://www.drupal.org/node/2293501.
   -      return $this->csrfToken->validate($request->query->get('token'), $request->attributes->get('_system_path')) ? static::ALLOW : static::KILL;
   -    }
   -
   -    // Otherwise, this could be another requested access check that we don't
   -    // want to check CSRF tokens on.
   -    $conjunction = $route->getOption('_access_mode') ?: AccessManagerInterface::ACCESS_MODE_ANY;
   -    // Return ALLOW if all access checks are needed.
   -    if ($conjunction == AccessManagerInterface::ACCESS_MODE_ALL) {
   -      return static::ALLOW;
   -    }
   -    // Return DENY otherwise, as another access checker should grant access
   -    // for the route.
   -    else {
   -      return static::DENY;
   -    }
   +    // @todo Remove dependency on the internal _system_path attribute:
   +    //   https://www.drupal.org/node/2293501.
   +    return $this->csrfToken->validate($request->query->get('token'), $request->attributes->get('_system_path')) ? static::ALLOW : static::KILL;
   

   <3

4. +++ b/core/modules/config_translation/src/Access/ConfigTranslationFormAccess.php
   @@ -33,7 +32,7 @@ public function access(Route $route, Request $request, AccountInterface $account
   -        $target_language->id != $this->sourceLanguage->id;
   +        (empty($this->sourceLanguage) || ($target_language->id != $this->sourceLanguage->id));
   

   What is the reason to check for the existence of the source language?

5. +++ b/core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
   index 0000000..c786308
   --- /dev/null
   
   --- /dev/null
   +++ b/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
   
   +++ b/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
   +++ b/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
   @@ -0,0 +1,221 @@
   
   @@ -0,0 +1,221 @@
   +<?php
   +
   
   deleted file mode 100644
   index 2a46556..0000000
   
   index 2a46556..0000000
   --- a/core/tests/Drupal/Tests/Core/Access/AccessArgumentsResolverTest.php
   
   --- a/core/tests/Drupal/Tests/Core/Access/AccessArgumentsResolverTest.php
   +++ /dev/null
   
   +++ /dev/null
   +++ /dev/null
   @@ -1,242 +0,0 @@
   
   @@ -1,242 +0,0 @@
   -<?php
   -
   -/**
   

   I wonder whether you have configured git to show moves in the patch files, see https://www.drupal.org/documentation/git/configure

6. +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
   @@ -690,13 +627,33 @@ protected static function convertAccessCheckInterfaceToString($constant) {
   +   * Add defalut expectations to the access arguments resolver factory.
   +   */
   +  protected function setupAccessArgumentsResolverFactory($constraint = NULL) {
   +    if (!isset($constraint)) {
   

   It is great to have a helper method!

We could make check() a protected method. But that would mean we'd have to rewrite all the test-coverage. I'm okay with doing that, but in order to keep risk to a minimum and reviewability at a sane level, let's defer that to a follow-up.

Agreed.

We only want to deny access when a language is locked but we want to allow access when it is missing (yes, really).

He, well, is there a reason this has to be changed here?

Great work!

diff --git a/core/lib/Drupal/Core/Access/AccessArgumentsResolverFactoryInterface.php b/core/lib/Drupal/Core/Access/AccessArgumentsResolverFactoryInterface.php
index 1fdbb80..b4c3219 100644
--- a/core/lib/Drupal/Core/Access/AccessArgumentsResolverFactoryInterface.php
+++ b/core/lib/Drupal/Core/Access/AccessArgumentsResolverFactoryInterface.php
@@ -26,7 +26,7 @@
    * @param \Symfony\Component\HttpFoundation\Request $request
    *   Optional, the request object.
    *
-   * @return \Drupal\Component\ArgumentsResolverInterface
+   * @return \Drupal\Component\Utility\ArgumentsResolverInterface
    *   The parametrized arguments resolver instance.
    */
   public function getArgumentsResolver(RouteMatchInterface $route_match, AccountInterface $account, Request $request = NULL);
diff --git a/core/lib/Drupal/Core/Access/AccessManagerInterface.php b/core/lib/Drupal/Core/Access/AccessManagerInterface.php
index 4312951..00ac6fb 100644
--- a/core/lib/Drupal/Core/Access/AccessManagerInterface.php
+++ b/core/lib/Drupal/Core/Access/AccessManagerInterface.php
@@ -8,7 +8,6 @@
 namespace Drupal\Core\Access;
 
 use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\Routing\Route;
 use Symfony\Component\Routing\RouteCollection;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\Core\Routing\RouteMatchInterface;
@@ -55,10 +54,10 @@
   public function checkNamedRoute($route_name, array $parameters = array(), AccountInterface $account = NULL);
 
   /**
-   * Execute access checks against the incomming request.
+   * Execute access checks against the incoming request.
    *
    * @param Request $request
-   *   The incomming request.
+   *   The incoming request.
    * @param \Drupal\Core\Session\AccountInterface $account
    *   (optional) Run access checks for this account. Defaults to the current
    *   user.
@@ -100,7 +99,7 @@ public function addCheckService($service_id, $service_method, array $applies_che
    *   user.
    * @param \Symfony\Component\HttpFoundation\Request $request
    *   Optional, a request. Only supply this parameter when checking the
-   *   incomming request, do not specify when checking routes on output.
+   *   incoming request, do not specify when checking routes on output.
    *
    * @return bool
    *   Returns TRUE if the user has access to the route, otherwise FALSE.
diff --git a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
index 307cfc9..3ca0c1e 100644
--- a/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -10,7 +10,6 @@
 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;
diff --git a/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php b/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
index c786308..660ff2a 100644
--- a/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
+++ b/core/tests/Drupal/Tests/Component/Utility/ArgumentsResolverTest.php
@@ -84,7 +84,7 @@ public function testGetArgumentObject() {
   /**
    * Tests getArgument() with a wildcard object for a parameter with a custom name.
    */
-  public function testGetWilcardArgument() {
+  public function testGetWildcardArgument() {
     $callable = function(\stdClass $custom_name) {};
 
     $object = new \stdClass();
diff --git a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
index 18f32db..f2f39b1 100644
--- a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
@@ -635,7 +635,7 @@ protected function setupAccessChecker() {
   }
 
   /**
-   * Add defalut expectations to the access arguments resolver factory.
+   * Add default expectations to the access arguments resolver factory.
    */
   protected function setupAccessArgumentsResolverFactory($constraint = NULL) {
     if (!isset($constraint)) {
diff --git a/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php
index 1bebde8..3ec5d21 100644
--- a/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/CsrfAccessCheckTest.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\Tests\Core\Access;
 
-use Drupal\Core\Access\AccessManagerInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Route;
 use Drupal\Core\Access\CsrfAccessCheck;

Above are some changes to make... I was thinking about doing them on commit but I think we need a list of all CRs that need an update eg https://www.drupal.org/node/1851490 so that once committed someone can fix them. I also think we need to ensure (if necessary) documentation on d.o is updated once this gets in.

Yeah I think it it totally fine to limit custom access checkers for just route match based ones. One big reason is that this callback
is considered as a lazy excuse anyway, so it is okay to not have the full freedom.

I really like the amount of cleanups provided by this patch though, well, this will probably not land before #2287071: Add cacheability metadata to access checks so that you need to rework quite a lot here.

Amazing simplification and clean-up!

This will indeed unfortunately conflict with #2287071, but fortunately they touch the same files for very different reasons, so the conflict should be doable to resolve.

I just committed [##2287071] so this will need a rebase. Adding "avoid commit conflicts" since I would have put this in before any other patch that wasn't 250kb..

Wow, that was fast!

I created a diff locally between the patches in #57 and #61, and all changes looked sane from a first read-through.

Awesome!

@znerol
Still curious about your git config, see above.

Nice work on the quick reroll and back to rtbc. Let's update those change records.

Committed d617be8 and pushed to 8.0.x. Thanks!

The config translation changes here are wrong. Surely config_translation could be refactored to be less dependant on the request object and instead utilize a RouteMatch or whatever is fancy these days. As it stands, though, ConfigMapperInterface::populateFromRequest() must be run in the access checker. Not running it will lead to very hard to catch bugs in multilingual scenarios. In the same light the empty() check is not correct. If there is no source language then access should not be granted.

Since we didn't write any tests for this complicated behavior it's our own fault that this regressed and thus I also do not suggest rolling this back. I will open a follow-up where I will try to write a test to prove my point.

Opened #2340533: Regression: ConfigTranslation(Form|Overview)Access grant access if the source language of entity mappers is locked and should not.

Oh, you are in fact right in the second point - translation should only be granted if the source language exists, except if it is english, then it should be always granted.

Config translation accounts for that fallback, see ConfigMapperInterface::getLanguageWithFallback().

I disagree. The access check does not depend on the populated mapper.

This is not a matter of opinion. It does depend on the populated mapper as described in the follow-up. Sorry to be so blunt, but you are wrong here.

Another thing, this broke Page Manager's way of doing access checks.

Page manager currently uses _entity_create in combination with a hardcoded id that is not coming from the route but specified as a default.

That is in $request->attributes, but does not end up in $routeMatch->getParameters() (nor getRawParameters()).

The result is that the access check is now failing as it can't find the entity anymore.

Any suggestions on how to fix this? Would be sad if we'd have to go with custom access checks, because we also use _entity_view, so if that at some point stops working too, page_manager will be sad.

I think we need to fix RouteMatch. I needed to prefix _page to prevent conflicts, see #2284157: Use _page instead of plain 'page' in route defaults.