\Drupal\Core\Access\AccessManager::check loops over all the _access_checks for a given route. It has the following logic:

      $result = AccessResult::allowed();
      foreach ($checks as $service_id) {
        $result = $result->andIf($this->performCheck($service_id, $arguments_resolver));
      }

There are two bits that seem odd in here to me:

  • 1) The use of andIf means that each check can only allow/forbit, as any return of neutral essentially equates to forbid in Kleene's weak three-valued logic. This seems a bit unexpected from a developers POV, the natural assumption would be that is I return a neutral, it means I have nothing to say and so it should essentially ignore my input (essentially the behaviour of orIf).
  • 2) It starts with the assumption of allowing access. This seems like a strange assumption and is possibly the reason why point 1 has been done as is? Wouldn't it be better to start with a neutral, so things are explicitly allowed rather than explicitly denied?

    (This is not a security risk, as this only applies if an access check is present at which point access checks will either explicitly allow or explicitly/implicitly deny. It would need to change if 1 was implemented.)

CommentFileSizeAuthor
#9 2861074-allow_access_check_to_return_neutral.patch913 bytesandrewbelcher
#8 2861074-assumes-allowed-2.patch1.56 KBspheresh
#5 2861074-assumes-allowed.patch582 bytesspheresh
#3 2861074-2.patch1.22 KBdawehner

Comments

andrewbelcher created an issue. See original summary.

andrewbelcher’s picture

andrewbelcher commented

Adding to the above, it means that access checks can't use AccessResult::forbiddenIf, as that returns a neutral if the condition is false...

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2861074-2.patch1.22 KB

Here is a test to just see whether its really the case that we return access.

andrewbelcher’s picture

andrewbelcher commented
Issue summary: View changes

Sorry - my explanation probably wasn't clear enough. If there are access checks, we assume access and then every check has to either allow or forbid. I think if we expect all access checks to allow/forbid (with neutral being the same as forbid), then defaulting to allow is definitely the correct approach (and the only one).

It's more that I wonder if if would be better to start at neutral and use orIf to allow access checks to allow/forbid/neutral (with neutral being nothing to say on the matter), in which case starting at allowed would be a problem.

I've added a note to the summary to make that clearer.

spheresh’s picture

spheresh commented
StatusFileSize
new 2861074-assumes-allowed.patch582 bytes

Status: Needs review » Needs work

The last submitted patch, 5: 2861074-assumes-allowed.patch, failed testing.

andrewbelcher’s picture

andrewbelcher commented

@spheresh, I don't think that is the right approach (and if is working correctly on the logic that is being implemented), it's \Drupal\Core\Access\AccessManager::check that is potentially an issue.

spheresh’s picture

spheresh commented
StatusFileSize
new 2861074-assumes-allowed-2.patch1.56 KB

Patch #5 breaks down a logic of Drupal\user\Access\LoginStatusCheck class at least :(

We can see that AccessManager::check will approve by default:
$result = AccessResult::allowed();

to make sure that the method does not work properly enough, a simple test

$result = AccessResult::allowed();
$result->andif(AccessResult::neutral());
$result->isAllowed();

or

$result = AccessResult::allowed();
$result->andif(AccessResult::allowedIf($some1==some2));
$result->isAllowed();

In my test I explicitly indicate that I want to allow

If I would like to disallow I should add a correct condition like:

$result->andif(AccessResult::forbiddenIf($some1!=some2));

P.S. Sure we have some uncertainty in the case given below:

$result = AccessResult::neutral();
$result->andif(AccessResult::neutral());
$result->isAllowed();

but I think that this uncertainty magic issue we should discuss in another topic
In our case, I'm sure that "allowed" and "neutral" should be equal to the "allowed"

andrewbelcher’s picture

andrewbelcher commented
Status: Needs work » Needs review
StatusFileSize
new 2861074-allow_access_check_to_return_neutral.patch913 bytes

@spheresh, Drupal's AccessResult is based on Kleene's weak three-valued logic, so the behaviour below is already correct (see the truth table in the documentation):

$result = AccessResult::allowed();
$result->andif(AccessResult::neutral());
$result->isAllowed(); // FALSE
$result->isNeutral(); // TRUE
$result->isForbidden(); // FALSE

The problem in this issue is that \Drupal\Core\Access\AccessManager::check treats neutral as forbidden due to using andIf, where as what I'm proposing is that it starts with neutral and uses orIf.

Attached is a patch that does what I am proposing.

spheresh’s picture

spheresh commented

It works fine!

I've made some very fast tests.
It seems now it works as intended.

Thanks.

andrewbelcher’s picture

andrewbelcher commented
Issue tags: +Needs tests

Need to add some test coverage to demonstrate neutral responses are treated correctly by AccessManager.

Status: Needs review » Needs work

The last submitted patch, 9: 2861074-allow_access_check_to_return_neutral.patch, failed testing.

andrewbelcher’s picture

andrewbelcher commented

Looks like there is something somewhere which returns a 404 (not found) rather than a 403 (denied) (e.g. this fail) if the access result is neutral, but I can't see what is causing that...

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kriboogh’s picture

kriboogh commented

Any progress on this issue? Been running against this problem with a custom module we are building... neutral should indeed have no influence on other decisions (hence the name... neutral).

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

See #2991698: Why does Route access checking differ from Entity access checking? especially #3, #7, #9

andrewbelcher’s picture

andrewbelcher commented
Status: Needs work » Closed (duplicate)
Related issues: +#2991698: Why does Route access checking differ from Entity access checking?

Closing as a duplicate of #2991698: Why does Route access checking differ from Entity access checking? as though this issue is older, that has some more detailed documentation of what is happening and what possible solutions could be made.