Problem/Motivation

Discovered in #2938035-6: When PATCHing a field is disallowed, no reason is given for *why* this happens. This blocks that issue.

#2808233: REST 403 responses don't tell the user *why* access is not granted: requires deep Drupal understanding to figure out introduced \Drupal\Core\Access\AccessResultReasonInterface. It also introduced the inheriting of a the reason through ->orIf() and ->andIf() combinations.

I just noticed that if you do

$a = AccessResult::neutral('You do not have the FOO permission.');
$b = AccessResult::neutral();
$reason = $a->orIf($b)->getReason();

Then $reason === NULL… which means you've just lost the reason. This occurs in real-world circumstances. Which means, for example, on any site with hook_entity_field_access(), the reason for some field access operation being denied … will be erased. Resulting in a bad DX.

Proposed resolution

  1. Add tests.
  2. Fix.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#13 2938053-13.patch5.68 KBWim Leers
#13 interdiff-12-13.txt2.94 KBWim Leers
#12 2938053-12.patch3.86 KBWim Leers
#12 interdiff-11-12.txt1.12 KBWim Leers
#11 2938053-11.patch3.13 KBWim Leers
#11 interdiff-8-11.txt1.53 KBWim Leers
#8 2938053-8.patch2.73 KBWim Leers
#8 interdiff-3-8.txt811 bytesWim Leers
#3 2938053-3.patch2.63 KBWim Leers
#3 interdiff-2-3.txt1.3 KBWim Leers
#2 2938053-2.patch1.34 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2938053-2.patch1.34 KB

Expanding test coverage. This fails.

(Lifted from #2938035-6: When PATCHing a field is disallowed, no reason is given for *why* this happens.)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff-2-3.txt1.3 KB
2938053-3.patch2.63 KB

And this fixes it.

(Lifted from #2938035-7: When PATCHing a field is disallowed, no reason is given for *why* this happens.)

The last submitted patch, 2: 2938053-2.patch, failed testing. View results

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Priority: Minor » Major

If this is indirectly blocking #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only, which in turn blocks JsonApi from being added to core, then raising this to Major. If it ends up not needing to block any of that, then we can re-evaluate priority.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#5: correct, this is indirectly blocking #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only, and will soon be directly blocking it.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

This looks good to me. I'm hesitant to RTBC because I'd like to add an extra assertion $this->assertNull($neutral_reasonless->orIf($neutral_reasonless)->getReason());. But that's a very minor observation.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff-3-8.txt811 bytes
2938053-8.patch2.73 KB

Good call, done.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Needs review » Reviewed & tested by the community

This is RTBC for me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Needs work

I wanted to make sure we had test coverage for both $forbidden->orIf($neutral) and $neutral->orIf($forbidden); and it turns out we have an important typo in the test. We need to make the following change:

diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
index d9af59890f..7912f64237 100644
--- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
+++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php
@@ -337,7 +337,7 @@ public function testOrIf() {
     $this->assertDefaultCacheability($access);

     // FORBIDDEN || NEUTRAL === FORBIDDEN.
-    $access = $forbidden->orIf($allowed);
+    $access = $forbidden->orIf($neutral);
     $this->assertFalse($access->isAllowed());
     $this->assertTrue($access->isForbidden());
     $this->assertFalse($access->isNeutral());

Another thing I pondered was if we should test for reason precedence. Ie.

$neutral = AccessResult::neutral('neutral message');
$neutral_other = AccessResult::neutral('other neutral message');
$access = $neutral->orIf($neutral_other);
$this->assertEquals('neutral message', $access->getReason());
$access = $neutral_other->orIf($neutral);
$this->assertEquals('other neutral message', $access->getReason());

We don't have any other tests for this for the FORBIDDEN && FORBIDDEN where the same situation comes up. I also wonder if we should care about the information loss.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-8-11.txt1.53 KB
2938053-11.patch3.13 KB

Another thing I pondered was if we should test for reason precedence.

+1. Done! I should've done that in the original issue.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff-11-12.txt1.12 KB
2938053-12.patch3.86 KB

it turns out we have an important typo in the test

… and there was one more, even!

Fixed both.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
interdiff-12-13.txt2.94 KB
2938053-13.patch5.68 KB

And #10 made me realize that the problem this issue describes occurs not only for NEUTRAL || NEUTRAL, but also for FORBIDDEN || FORBIDDEN. A similar bug existed there (the reason could get lost). The fix was similar. In fact, now the ::orIf() logic has identical code for handling reason inheritance in both the "merge NEUTRAL || NEUTRAL" and the "merge FORBIDDEN || FORBIDDEN" cases, and both have complete test coverage now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Title: AccessResult::orIf() fails to retain the reason if both operands are neutral, but the first contains a reason and the second one does not » AccessResult::orIf() fails to retain the reason if both operands are neutral or forbidden, but the first contains a reason and the second one does not

Updating title accordingly.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Oops, I wanted to do that, but obviously forgot!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only

The JSON API module just ran into this same core bug in its test coverage, as predicted. See #2930028-61: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

This all looks very solid. @alexpott made some remarks in #10 and as far as I can tell, those have all been resolved.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed b6242dab05 to 8.6.x and f1d3e4a07c to 8.5.x. Thanks!

Crediting myself for a review that affected the shape of the patch.

  • alexpott committed b6242da on 8.6.x 
    Issue #2938053 by Wim Leers, alexpott: AccessResult::orIf() fails to...

  • alexpott committed f1d3e4a on 8.5.x 
    Issue #2938053 by Wim Leers, alexpott: AccessResult::orIf() fails to...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2954847: Core bug #2938053 fixed, now JSON API's strict assertions fail

This unblocked #2938035 — rebased patches at #2938035-28: When PATCHing a field is disallowed, no reason is given for *why* this happens ready!

It also unblocked #2954847, a similar issue for the contrib JSON API module!

Status: Fixed » Closed (fixed)

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