Problem/Motivation

#2351777: Do not depend on event subscribers for security: Replace AccessRouteSubscriber with build-in checks would like to inject the router.builder service into access_manager but this leads to a circular dependency: access_manager -> route.provider -> router.bouilder

Proposed resolution

Separate the loading of checks out of AccessManager. Now we can inject this check provider service into both access manager and access subscriber or router.builder when they get merged into one. Circular dependency broken.

The separation is nice. For example, we could remove four setContainer calls from the access manager test and easily/mechanically convert the rest of the test by just moving the relevant calls from AccessManager to the checkProvider.

Remaining tasks

If we were passing $request all along then getChecksNeedRequest would be removable. Do we want to do that? The current way is simpler. Removing getChecksNeedRequest would be slightly nicer.

User interface changes

API changes

setChecks and addCheckService is removed from AccessManager. The only service holding them becomes private. This is a good thing as there is no reason to call them.

CommentFileSizeAuthor
#5 interdiff.txt1.47 KBchx
#5 2354657_5.patch32.29 KBchx
#4 interdiff.txt1.85 KBdawehner
#2 2354657_2.patch31.81 KBchx
checkprovider.patch30.09 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, checkprovider.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review
FileSize
2354657_2.patch31.81 KB
Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC - looks great to me

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt1.85 KB

@committer
Here is a doc improvement you might want to apply on top of it. Don't give me credit, its just annoying to get undocumented stuff in.
Its adds some documentation WHY things are designed in the way they are designed, which is really valuable information, if you ask me.

chx’s picture

chx CreditAttribution: chx commented
FileSize
2354657_5.patch32.29 KB
interdiff.txt1.47 KB

Thanks, rerolled with slightly changed doxygen.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Thank you!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented 
  1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -28,50 +23,7 @@
-class AccessManager implements ContainerAwareInterface, AccessManagerInterface {

    -- 

  2. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -28,50 +23,7 @@
+class AccessManager implements AccessManagerInterface {

    ++ 

  3. +++ b/core/lib/Drupal/Core/Access/CheckProvider.php
@@ -0,0 +1,170 @@
+class CheckProvider extends ContainerAware implements CheckProviderInterface {

    Even though we add it back, it's required for a very specific reason in a much smaller class:

    $check = $this->container->get($service_id);
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed c6f0238 on 8.0.x 
    Issue #2354657 by chx, dawehner: Separate access manager from loading...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.