Problem/Motivation
Every time I (and some of my colleagues have echoed the same) try to work on an issue that requires changes to any tests extending from
Drupal\Tests\jsonapi\Functional\ResourceTestBase I find it very difficult to figure out what is going on and where something needs to be changed.
🐛
Information disclosure access bypass for revision log fields when the JSON:API module is enabled
Fixed
is one such issue. Even with Xdebug, it's very difficult to figure out what should be happening at each step of the test, who's logged in, and what's actually being tested. This makes fixing the failures in those tests very hard, and in the past has completely put me off continuing with trying to get an issue over the line.
For example, it took me ~20 minutes to find out why BlockContentTest::testGetIndividual was failing in 3395404.
It was failing because the expected document wasn't correct: The 'data' member was not as expected. that's on line 805 of ResourceTestBase, with the following call stack:
/data/app/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:805
/data/app/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:724
/data/app/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:988
Going back up to line 988 and working my way backwards, I can't see why a user would have access to view the revision log there. That requires with view revision, view all revisions, or update operation access based on the fix in that issue. When looking in the setUp it looks like the user under test has no roles and I confirmed that via debugging.
However, I missed this line $this->setUpAuthorization('GET');
In there we grant permissions directly to the authenticated user role.
To find that out, I had to debug right down into the BlockContentAccessControlHandler (after failing to find the right place to breakpoint several times in the test) and discovered the user had administer blocks why does a user have that when testing a view operation? Turns out setUpAuthorization grants that permission for GET methods.
Great! It seems like administer blocks really shouldn't be used for GET methods (access block library is the only permission required for viewing a block_content entity that's published), so let's only grant that permission for GET methods.
But GET methods cover testGetIndividual, testCollection, AND testRevisions. Those are 3 very different things, and the latter 2 do require the administer (or other more specific) permissions.
You can see where I'm going with this hopefully....every time I have to touch these tests it feels like I'm balancing 17 sticks with broken plates on top of them and trying to make everything work just right but it never seems to be easy meaning there's weird workarounds littered through the tests for special conditions, etc.
Proposed resolution
Decouple tests a lot, moving things out of ResourceTestBase into their own classes, making it easier to understand what's really being tested for each entity type.
Remaining tasks
Agree
Plan
Code
