Make the new AccessResult API and implementation even better

I really like that we do have now a proper foundation and no hand-wavy implementation any longer.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -392,67 +323,43 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
       */
      public function orIf(AccessResultInterface $other) {
   -    // If this AccessResult already is forbidden, then that already is the
   -    // conclusion. We can completely disregard $other.
   -    if ($this->isForbidden()) {
   -      return $this;
   +    if ($this->isForbidden() || $other->isForbidden()) {
   +      $result = static::forbidden();
   +    }
   +    elseif ($this->isAllowed() || $other->isAllowed()) {
   +      $result = static::allowed();
        }
   -    // Otherwise, we make this AccessResult forbidden if the other is, or
   -    // allowed if the other is, and we merge in the cacheability metadata if the
   -    // other access result also has cacheability metadata.
        else {
   -      if ($other->isForbidden()) {
   -        $this->forbid();
   -      }
   -      else if ($other->isAllowed()) {
   -        $this->allow();
   -      }
   -      $this->mergeCacheabilityMetadata($other);
   -      return $this;
   +      $result = static::neutral();
        }
   +    $result->mergeCacheabilityMetadata($this);
   +    if (!$this->isForbidden()) {
   +      $result->mergeCacheabilityMetadata($other);
   +    }
   +    return $result;
      }
    
      /**
       * {@inheritdoc}
       */
      public function andIf(AccessResultInterface $other) {
   -    // If this AccessResult already is forbidden or is merely not explicitly
   -    // allowed, then that already is the conclusion. We can completely disregard
   -    // $other.
   -    if ($this->isForbidden() || !$this->isAllowed()) {
   -      return $this;
   +    if ($this->isForbidden() || $other->isForbidden()) {
   +      $result = static::forbidden();
   +    }
   +    elseif ($this->isAllowed() && $other->isAllowed()) {
   +      $result = static::allowed();
        }
   -    // Otherwise, we make this AccessResult forbidden if the other is, or not
   -    // explicitly allowed if the other isn't, and we merge in the cacheability
   -    // metadata if the other access result also has cacheability metadata.
        else {
   -      if ($other->isForbidden()) {
   -        $this->forbid();
   -      }
   -      else if (!$other->isAllowed()) {
   -        $this->resetAccess();
   -      }
   -      $this->mergeCacheabilityMetadata($other);
   -      return $this;
   +      $result = static::neutral();
   ...
      }
   

   10 times better!

2. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
   @@ -76,7 +76,7 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
   -    if (!$return->isAllowed() && !$return->isForbidden()) {
   +    if ($return->isNeutral()) {
   

   MUCH BETTER!

3. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
   @@ -201,36 +137,69 @@ public function testAndIf() {
   -    $access = clone $allowed;
   -    $access->andIf($forbidden);
   +    $access = $allowed->andIf($forbidden);
   +    $this->assertFalse($access->isAllowed());
   +    $this->assertTrue($access->isForbidden());
   +    $this->assertDefaultCacheability($access);
   +
   +    // DENY && ALLOW == DENY
   +    $access = $no_opinion->andIf($allowed);
   +    $this->assertFalse($access->isAllowed());
   +    $this->assertFalse($access->isForbidden());
   +    $this->assertDefaultCacheability($access);
   +
   +    // DENY && DENY === DENY.
   +    $access = $no_opinion->andIf($no_opinion);
   +    $this->assertFalse($access->isAllowed());
   +    $this->assertFalse($access->isForbidden());
   +    $this->assertDefaultCacheability($access);
   +
   +    // DENY && KILL === KILL.
   +    $access = $no_opinion->andIf($forbidden);
   +    $this->assertFalse($access->isAllowed());
   +    $this->assertTrue($access->isForbidden());
   +    $this->assertDefaultCacheability($access);
   +
   +    // KILL && ALLOW = KILL
   +    $access = $forbidden->andif($allowed);
   +    $this->assertFalse($access->isAllowed());
   +    $this->assertTrue($access->isForbidden());
   +    $this->assertDefaultCacheability($access);
   +
   +    // KILL && DENY = KILL
   +    $access = $forbidden->andif($no_opinion);
   +    $this->assertFalse($access->isAllowed());
   +    $this->assertTrue($access->isForbidden());
   +    $this->assertDefaultCacheability($access);
   +
   +    // KILL && KILL = KILL
   +    $access = $forbidden->andif($forbidden);
        $this->assertFalse($access->isAllowed());
   

   Did we considered to update the docs here to talk about neutral? and $neutral?

I only skimmed this so far, but I like what I see. I'll leave it to Wim and dawehner to review in-depth and RTBC though.

+++ b/core/modules/contact/src/Access/ContactPageAccess.php
@@ -71,9 +71,10 @@ public function access(UserInterface $user, AccountInterface $account) {
+    if ($permission_access->allowed()) {

Shouldn't that be $permission_access->isAllowed()?

Code review

Overall this patch looks excellent:

1. public function isNeutral() {
   

   YAY! I thought this might be contentious, which is why I didn't add it previously. I like this much better though :)

2. +++ b/core/lib/Drupal/Core/Access/AccessResultInterface.php
   @@ -36,43 +34,56 @@
   +   * This is a kill switch -- both orIf and andIf will result in isForbidden()
   +   * if either results are isForbidden().
   ...
       * - isForbidden() in either ⇒ isForbidden()
   -   * - isAllowed() in both ⇒ isAllowed()
   -   * - neither isForbidden() nor isAllowed() => !isAllowed() && !isForbidden()
   +   * - otherwise, if isAllowed() in both ⇒ isAllowed()
   +   * - otherwise, one of them is isNeutral()  ⇒ isNeutral()
   

   Good clarifications :)

3. +++ b/core/modules/content_translation/src/Access/ContentTranslationOverviewAccess.php
   @@ -74,10 +74,10 @@ public function access(RouteMatchInterface $route_match, AccountInterface $accou
   -      return $access->allowIfHasPermission($account, $permission);
   +      return $access->orIf(AccessResult::allowedIfHasPermission($account, $permission));
   

   I'm not a big fan of these, but it allows for a smaller (and hence simpler) API, so I think that's more than worth it :)
   It also more clearly shows what's going on; it stresses the use of orIf() and andIf() more, so that's great.
   Taking that fact into consideration, I *am* a fan of this :)

4. +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php
   @@ -55,23 +55,21 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   -          $access = AccessResult::create()->cachePerRole()->cacheUntilEntityChanges($entity);
              // If there is a URL, this is an external link so always accessible.
   -          if ($entity->getUrl()) {
   -            return $access->allow();
   -          }
   -          else {
   +          $access = AccessResult::allowed()->cachePerRole()->cacheUntilEntityChanges($entity);
   +          if (!$entity->getUrl()) {
                // We allow access, but only if the link is accessible as well.
                $link_access = $this->accessManager->checkNamedRoute($entity->getRouteName(), $entity->getRouteParameters(), $account, TRUE);
   -            return $access->allow()->andIf($link_access);
   +            return $access->andIf($link_access);
              }
   +          return $access;
   

   Much clearer :)

5. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
   @@ -337,7 +337,7 @@ function entity_test_entity_field_access($operation, FieldDefinitionInterface $f
   -    $grants[':default']->forbid()->cacheUntilEntityChanges($context['items']->getEntity());
   +    $grants[':default'] = AccessResult::forbidden()->cacheUntilEntityChanges($context['items']->getEntity());
   

   Problematic: cacheability metadata lost!

6.     // andIf(); 1st doesn't implement CacheableInterface, 2nd does.
       $access = $access_without_cacheability->andIf(AccessResult::allowed());
       $this->assertTrue($access->isAllowed());
       $this->assertFalse($access->isForbidden());
       $this->assertFalse($access->isNeutral());
       $this->assertFalse(method_exists($access, 'isCacheable'));
   

   This test was not updated and is therefore now subtly broken. Fixed in this reroll.

In this reroll

From the IS:

1. Write out the nine orIf tests.
2. Add more to AccessManagerTest -- right now removing $result->andIf() doesn't fail it.
3. Add more asserts for caching -- we now have every case covered, surely we can test that cache merging works as we wanted. That will also serve as documentation.
4. I think we should remove the DENY word -- now it only exists on the test, mind you. The constant is gone.
5. Review the changes to ContentTranslationOverviewAccess in #26 -- is the cache propagation right?

1. Done.
2. Reproduced, but as per our discussion in IRC: that's out of scope. AccessManager test coverage is apparently weak, but that's not caused by #2287071: Add cacheability metadata to access checks.
3. Good call. In doing so, I discovered that your changes were wrong. But I also discovered that my original code was similarly, but differently flawed. It was "less wrong", but still wrong. This shows once again that you really can't have too much test coverage of the low-level/"fundamental" code. I have to say that the fact you noticed this was a formal system, called Kleene's weak three-valued logic (http://en.wikipedia.org/wiki/Many-valued_logic) really helped with this. The "contagiousness" aspect is very important, because it naturally also applies to cacheability metadata.
   I'm glad my patch (#2287071: Add cacheability metadata to access checks) triggered your critical eye, because I merely translated what already was there and made everything use a single consistent system. I think this explicit formalness really helps understand the access system much better. It also gives peace of mind, knowing that this isn't something ad-hoc, but something solid :)
   I present to you, all 72 different permutations, times two operators, for a grand total of 144 test cases.
4. Agreed; done. That was s/DENY/NEUTRAL/. Also did s/KILL/FORBIDDEN/ and s/ALLOW/ALLOWED/.
5. There's indeed a problem with the cache propagation there. Now fixed, by renaming mergeCacheabilityMetadata() to inheritCacheability(), making it public, and using it here. It will only be necessary in rare cases (the only other case in core is in point 5 of my dreditor review: entity_test_entity_field_access(), which is in a test module, and a similar example in entity.api.php). Having the cacheability inheriting code as a public instance method also is very consistent with the cleaned up AccessResult: all instance methods are related to cacheability now!

Besides that, I also did:

1. Clean up docs.
2. Add isNeutral() assertions wherever there are isAllowed() and isForbidden() assertions.

The "very" is a bit hyperbole — I've had several people thank me for "such a nice API".

14 fails, 3 exceptions. 13 of those due to something very stupid: no return $this; in inheritCacheability() — fixed and added test coverage. Off to dinner and a green patch!

#32: :)

Great work! Especially the expanded test coverage!

From AccessManager.php:

  protected function checkAny(array $checks, ArgumentsResolverInterface $arguments_resolver) {
    // No opinion by default.
    $result = AccessResult::neutral();

    foreach ($checks as $service_id) {
      if (empty($this->checks[$service_id])) {
        $this->loadCheck($service_id);
      }
      $result = $result->orIf($this->performCheck($service_id, $arguments_resolver));
      // Stop as soon as the first forbidden check is encountered.
      if ($result->isForbidden()) {
        break;
      }
    }

    return $result;
  }

It is not obvious for me, whether you can actually break in cases of OR like that. Do you maybe have to take into account all of them, in order to have all cache information stored? Some line of documentation in case you don't would be cool, why is that possible?

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -17,31 +17,7 @@
   +abstract class AccessResult implements AccessResultInterface, CacheableInterface {
   

   It would be great if we could clearly document that the access specific (non-cache) information of these access result are immutable.

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -153,83 +131,38 @@ public static function forbiddenIf($condition) {
      /**
       * {@inheritdoc}
   +   *
   +   * @see \Drupal\Core\Access\AccessResultAllowed
       */
      public function isAllowed() {
   ...
   +    return FALSE;
      }
   ...
       *
   ...
   +   * \Drupal\Core\Access\AccessResultForbidden
       */
   ...
   +  public function isForbidden() {
   +    return FALSE;
      }
    
   

   Let's put an @see on both of them.

3. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -392,76 +325,88 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
      public function orIf(AccessResultInterface $other) {
   ...
   +    // The other access result's cacheability metadata is merged if $merge_other
   

   Small detail, if you would use "The $other" it would be a little bit better to understand.

4. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -392,76 +325,88 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
   +    // 1. The other access result is used.
   

   Just from reading this comment it seems not clear what means used here (either it is forbidden or allowed, right?)

5. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -392,76 +325,88 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
        else {
   -      if ($other->isForbidden()) {
   -        $this->forbid();
   +      $result = static::neutral();
   +      if (!$this->isNeutral()) {
   +        $merge_other = TRUE;
          }
   -      else if (!$other->isAllowed()) {
   -        $this->resetAccess();
   +    }
   

   I don't understand why we don't have to merge cache info, if $this is neutral and $other is Allowed

6. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -392,76 +325,88 @@ public function cacheUntilEntityChanges(EntityInterface $entity) {
   +    // Only when this access result already was isAllowed(), the second
   +    $result->inheritCacheability($this);
   

   Docs seems to stop in the middle of

7. +++ b/core/lib/Drupal/Core/Access/AccessResultAllowed.php
   @@ -0,0 +1,19 @@
   +
   +class AccessResultAllowed extends AccessResult {
   +
   
   +++ b/core/lib/Drupal/Core/Access/AccessResultForbidden.php
   @@ -0,0 +1,19 @@
   +
   +class AccessResultForbidden extends AccessResult {
   +
   

   Nitpick: One-liner needed.

8. +++ b/core/lib/Drupal/Core/Access/AccessResultInterface.php
   @@ -23,12 +23,10 @@
   - * @endcode
   + * Objects implementing this interface are using Kleene's weak three-valued
   + * logic with the isAllowed() state being TRUE, the isForbidden() state being
   + * the intermediate truth value and isNeutral() being FALSE. See
   + * http://en.wikipedia.org/wiki/Many-valued_logic for more.
     */
    interface AccessResultInterface {
   

   #2157541: Views sets access to ANY on routes - could result in information disclosure could use a 4th state which returns AccessResultAllowed for all cases (AND/OR) but I guess we need to find a different solution for that.

9. +++ b/core/lib/Drupal/Core/Access/AccessResultInterface.php
   @@ -36,43 +34,56 @@
   +  /**
       * Combine this access result with another using OR.
       *
   ...
   +   * @return \Drupal\Core\Access\AccessResultInterface
       */
      public function orIf(AccessResultInterface $other);
   ...
      /**
   ...
       *
   -   * @return $this
   +   * @return \Drupal\Core\Access\AccessResultInterface
       */
      public function andIf(AccessResultInterface $other);
   

   You can use @return static here

10. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
    @@ -49,17 +49,16 @@ public function __construct(CsrfTokenGenerator $csrf_token) {
    +    // Not cacheable because the CSRF token is highly dynamic.
    +    return $result->setCacheable(FALSE);
    

    Technical this is by session and URL, right? Do we have a cache context of a session already?

11. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
    @@ -159,80 +120,101 @@ public function testAccessConditionallyAllowed() {
       public function testAndIf() {
    
    @@ -240,60 +222,81 @@ public function testAndIf() {
       public function testOrIf() {
    

    Did we considered to convert those to use a data provider as well? It lets you provide the available combinations in a better readable way.

12. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
    @@ -159,80 +120,101 @@ public function testAccessConditionallyAllowed() {
         $unused_access_result_due_to_lazy_evaluation = $this->getMock('\Drupal\Core\Access\AccessResultInterface');
         $unused_access_result_due_to_lazy_evaluation->expects($this->never())
           ->method($this->anything());
     
    ...
    +    // FORBIDDEN && * === FORBIDDEN: lazy evaluation verification.
    +    $access = $forbidden->andIf($unused_access_result_due_to_lazy_evaluation);
    +    $this->assertFalse($access->isAllowed());
    

    Awesome!

13. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
    @@ -494,39 +478,404 @@ public function testCacheabilityMerging() {
    +  public function testAndOrCacheabilityPropagationProvider() {
    

    Isn't this method executed as test, because of the prefix "test"? We use providerTestAndOr somewhere else ...

14. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
    @@ -494,39 +478,404 @@ public function testCacheabilityMerging() {
    +    $allowed_un = new UncacheableTestAccessResult('ALLOWED');
    ...
    +    $neutral_un = new UncacheableTestAccessResult('NEUTRAL');
    ...
    +      [$allowed_un, 'OR', $allowed_ct, FALSE, NULL],
    +      [$allowed_un, 'OR', $allowed_cf, FALSE, NULL],
    +      [$allowed_un, 'OR', $allowed_un, FALSE, NULL],
    ...
    +      $result = $first->orIf($second);
    ...
    +      $result = $first->andIf($second);
    

    Mh, here we actually test the UncacheableTestAcessResult not AccessResult itself. I do understand why, but can we make this explicit?

15. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
    @@ -494,39 +478,404 @@ public function testCacheabilityMerging() {
    +    else {
    +      throw new \LogicException('Invalid operator specified');
    +    }
    

    Do we still need this? Just curious.

16. +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
    @@ -494,39 +478,404 @@ public function testCacheabilityMerging() {
    +      $this->assertTrue($result instanceof CacheableInterface, 'Result is an instance of CacheableInterface.');
    ...
    +      $this->assertFalse($result instanceof CacheableInterface, 'Result is not an instance of CacheableInterface.');
    

    Note: There is assertInstanceOf

It is not obvious for me, whether you can actually break in cases of OR like that. Do you maybe have to take into account all of them, in order to have all cache information stored? Some line of documentation in case you don't would be cool, why is that possible?

OOohhh!!! Excellent point! That's true. That's also why I didn't do this in my original patch. AccessManager should just use AccessResultInterface::(andIf|orIf)(), which have comprehensive test coverage. If we try to do introduce such optimizations in AccessManager, we have to repeat that entire test coverage for AccessManager again. And as chx has shown already, AccessManager's test coverage is weak, so I've removed this optimization for now. We can restore it once #2341501: AccessManagerTest coverage is weak lands.

1. Done.
2. Hah, oops, fixed.
3. Done.
4. Improved.
5. If we enter the third if-branch, the combined result is "neutral". We only merge $other's cacheability metadata if it actually affected the combined result. By definition, this branch means the combined result is "neutral". Therefore, we must only check if $this->isNeutral() is FALSE, because if and only if that's the case, we've used $other to come to the conclusion that the combined result is "neutral".
   Therefore: if $this is "neutral" and $other is "allowed", then we did not use $other to conclude that the combined result must be "neutral" and hence we must also not merge its cacheability metadata.
6. Hehe :) Removed, was no longer necessary.
7. Fixed, also for the two other subclasses.
8. … yeah that's out of scope, sorry.
9. Oh! I did not know that. Thanks. Fixed!
10. No, we don't have a "session" cache context, but we could definitely add that. Let's do that in a follow-up? Filed #2341991: Add SessionCacheContext, use it in CsrfAccessCheck.
11. This was already there, I just made it explicit in the comment now :)
12. :)
13. ROFL :D :D :D You're absolutely right! This was being executed as a test, but one with zero test cases. Amazing catch :)
14. Good point, documented.
15. It's not strictly necessary, but there's also no harm. Better be strict than sorry. :)
16. Oh! I wanted to update the patch to use that, but the downside is that you then have to specify the interface as a string, which is less handy than this. So keeping this as is.

We have some follows ups and the current progress makes things surprisingly better to understand!

chx: oops, I forgot to check if there was a similar optimization there. Thanks!

Yay for progress and simplicity!

Great work here; let's try to get this one in soon because BC breaks in this API are a big deal.

Just a minor sechole :p

What about negation...

 * Objects implementing this interface are using Kleene's weak three-valued
 * logic with the isAllowed() state being TRUE, the isForbidden() state being
 * the intermediate truth value and isNeutral() being FALSE. See
 * http://en.wikipedia.org/wiki/Many-valued_logic for more.

This means the opposite of isForbidden is isForbidden??? Seems odd to me.

Basically, these are the truth tables.

Definitions

1. A === isAllowed === "TRUE"
2. N === isNeutral === "FALSE"
3. F === isForbidden === "SUPER EXTREME MEGA FALSE" AKA "BLACK HOLE FALSE"

AND

  |A N F
--+-----
A |A N F
N |N N F
F |F F F

OR

  |A N F
--+-----
A |A A F
N |A N F
F |F F F

As you can tell: boolean logic for the A and the N, but with F being "contagious".

This indeed equals http://en.wikipedia.org/wiki/Many-valued_logic#Bochvar.27s_internal_thre.... If we use Wikipedia's notation, then:

1. T (TRUE) === A == isAllowed()
2. F (FALSE) === N == isNeutral()
3. I (INDETERMINATE) === F === isForbidden()

Given this, This means the opposite of isForbidden is isForbidden??? Seems odd to me. suddenly perfectly matches what Kleene's week three-valued logic says: the negation of the indeterminate is the indeterminate, i.e. the negation of isForbidden() is isForbidden(). This is once again the "contagious" aspect. Wikipedia specifically calls this out also:

The intermediate truth value in Bochvar's "internal" logic can be described as "contagious" because it propagates in a formula regardless of the value of any other variable.[4]

Thanks for taking answering my question :)

Committed ff035fc and pushed to 8.0.x. Thanks!

We need to update https://www.drupal.org/node/2337377 and link it to this issue.

+++ b/core/lib/Drupal/Core/Access/AccessResultInterface.php
@@ -23,12 +23,10 @@
+ * Objects implementing this interface are using Kleene's weak three-valued
+ * logic with the isAllowed() state being TRUE, the isForbidden() state being
+ * the intermediate truth value and isNeutral() being FALSE. See
+ * http://en.wikipedia.org/wiki/Many-valued_logic for more.

Related to #42, what purpose is this doc serving? Objects implementing this interface ARE NOT implementing Kleene logic. They are implementing a subset of that logic only: just conjunction and disjunction. I think we should either remove the doc entirely (since the andIf() and orIf() methods are themselves documented), or else tweak the doc to be more precise. Especially because calling isForbidden() "indeterminate" is confusing: it's not indeterminate, it's just contagious. Not opening an issue for that yet, but if someone wants to, go for it.

I also provided the truth tables in #44. I personally also think that would be clearer, the Wikipedia article is not so great.