UserTest::testRelated fails on Drupal 8.5 because no $reason is given in the Access Denied response.

This is critical because it is breaking HEAD on 8.5. This issue became apparent after #3014232: [regression] ResourceTypeRepository is significantly slower in JSON:API 2, becomes noticeable when handling hundreds of interlinked resources was resolved.

CommentFileSizeAuthor
#7 3019506-7.patch1.16 KBwim leers
#4 3019506-4-revert_3014232.patch2.75 KBwim leers
#3 3019506-3.patch824 bytesgabesullice

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
commented

This test should show the failure on HEAD once it completes: https://www.drupal.org/pift-ci-job/1143538

The result:


Drupal\Tests\jsonapi\Functional\UserTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-4.xml:
PHPunit Test failed to complete; Error: PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\jsonapi\Functional\UserTest
.......F....

Time: 1.55 minutes, Memory: 4.00MB

There was 1 failure:

1) Drupal\Tests\jsonapi\Functional\UserTest::testRelated
The 'errors' member was not as expected.
Failed asserting that Array &0 (
    0 => Array &1 (
        'detail' => 'The current user is not allowed to view this relationship.'
        'links' => Array &2 (
            'info' => Array &3 (
                'href' => 'http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4'
            )
            'via' => Array &4 (
                'href' => 'http://php-apache-jenkins-drupal-contrib-298340/subdirectory/jsonapi/user/user/679be96e-f81f-4e5b-afe4-0d668109fb02/roles'
            )
        )
        'status' => 403
        'title' => 'Forbidden'
    )
) is identical to Array &0 (
    0 => Array &1 (
        'detail' => 'The current user is not allowed to view this relationship. The user only has authorization for the 'view label' operation.'
        'links' => Array &2 (
            'info' => Array &3 (
                'href' => 'http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4'
            )
            'via' => Array &4 (
                'href' => 'http://php-apache-jenkins-drupal-contrib-298340/subdirectory/jsonapi/user/user/679be96e-f81f-4e5b-afe4-0d668109fb02/roles'
            )
        )
        'status' => 403
        'title' => 'Forbidden'
    )
).
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
Issue tags: +API-First Initiative
StatusFileSize
new 3019506-3.patch824 bytes

I suspect there's an issue with caching because this would be the appropriate response if the entire entity was forbidden instead of just the related field (which is the case before setUpAuthorization('GET') is called. This should (dis)prove that hunch.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3019506-4-revert_3014232.patch2.75 KB

UserTest::testRelated() still fails with #3.

Let's see if this fails if we revert both commits for #3014232: [regression] ResourceTypeRepository is significantly slower in JSON:API 2, becomes noticeable when handling hundreds of interlinked resources.

The last submitted patch, 3: 3019506-3.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 3019506-4-revert_3014232.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 3019506-7.patch1.16 KB

Alright, #4's failure confirms that this is indeed a regression in 8.5. But git log origin/8.5.x -- /Users/wim.leers/Work/d8/core/modules/user/src/UserAccessControlHandler.php shows the last modification to UserAccessControlHandler was on November 30, 2017.

I dug deeper. And deeper.

I found the root cause.

Commit e7f061d5d40dca7b4aafabf903652fab1cd436ab for #2773645: Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral' modified UserAccessControlHandler subtly in 8.6. There is only a single line change that matters to us here:

       case 'roles':
       case 'status':
       case 'access':
       case 'login':
       case 'init':
-        return AccessResult::forbidden();
+        return AccessResult::neutral();
     }

Making that same change makes JSON:API 2 pass tests on 8.5. In other words, this is an oversight in the ::testRelated() test coverage wrt view label. Because #2956084, and specifically #2956084-87: Impossible to raise an error when an `include` is requested for an inaccessible relationship field. and later were not tested on Drupal 8.5.

Actually, only a subset of that is necessary:

       case 'roles':
+        return AccessResult::neutral();
       case 'status':
       case 'access':
       case 'login':

More generally speaking, this was possible to happen because https://www.drupal.org/node/2723491/qa stopped testing against Drupal 8.5. Because that configuration was either screwed up or because that functionality is just broken on d.o. I know I've set up branch tests in the past. I've set up the necessary additional tests, hopefully they continue to work this time … 😬

So … we end up with this ridiculously hacky work-around. But that's good enough for 8.5. This is a difference in an error message, caused by a core improvement. Not JSON:API's fault.

(This took me ~2 hours 😩)

Status: Needs review » Needs work

The last submitted patch, 7: 3019506-7.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

8.5 is green! 8.6 and 8.7 are red due to a new core change, but that's being fixed in #3019389: MediaTest fails on 8.7 since #2956368. Let's land this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: UserTest::testRelated fails on Drupal 8.5 because no $reason is given in the Access Denied response. » UserTest::testRelated() fails on Drupal 8.5 because no $reason is given in the Access Denied response

  • Wim Leers committed 69b2aa0 on 8.x-2.x 
    Issue #3019506 by Wim Leers, gabesullice: UserTest::testRelated() fails...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Category: Bug report » Task

Status: Fixed » Closed (fixed)

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