(Re)move _json_api_params from JsonApiParamEnhancer and do not use denormalization for them

Thanks for creating this issue. This was originally done in https://security.drupal.org/node/166860#comment-143269, where I wrote:

As luck would have it, after months of waiting for the RTBC patch https://www.drupal.org/project/drupal/issues/2869426 to land, it landed today, and of course it ended up causing some disruption for JSON:API (fixed in https://www.drupal.org/project/jsonapi/issues/3021311), but as it turns out, also for this security patch.

This security patch was affected because … the JSON:API module has been using the routing system in a not-quite-as-intended way for some time: it's using a RouteEnhancerInterface implementation to not enhance route defaults, but map URL query args into rich value objects. And if that fails, an exception is thrown. This security patch merely adds one more case where an exception is thrown: a 403 in case filtering is not allowed.

(The JSON:API module is hardly to blame here; it took 2.5 years for the core REST module to fix its very wrong use of the routing system in https://www.drupal.org/project/drupal/issues/2737751, and that just happened to get finished today!)

As of today, throwing those exceptions that early is causing failures in 8.7.

This reroll changes the place where the filter URL query arg is mapped into rich value objects to a later time in the request; to the actual place it belongs. It makes the minimal changes necessary (it only does this for the filter query arg, sorting/pager are left for a future public issue.)

(That future public issue is this one.)

Came up again at #2986169-12: [PP-1] Support sortable, paginated and filterable related resources..

Pointed #3031036: Why are sort and page denormalized in JsonApiParamEnhancer while filter is denormalized in the collection controller? here.

While there may well be good reasons to move all those out of the enhancer, I'm confused by what made it a necessity for 'filter', so uploading this patch to see what tests fails.

Or rather, this.

Ok, so the problem is that if you throw an exception from within a route enhancer, then that happens before any of the parameters/$defaults (that have been determined by this enhancer or any earlier routing code) have been transferred to $request attributes. That includes the '_route_object' attribute. Which means that exception handlers that have conditional logic based on which route was matched will process the exception as if no route was matched. Which in the case of #6, confuses AuthenticationManager::defaultFilter() into treating the AccessDeniedHttpException exception thrown due to lack of authorization to use a JSON:API filter as something that instead occurred due to a lack of authorization to use Basic Authentication (which is the authentication mechanism used by the test).

I think this is a bug with AccessAwareRouter, and will open a core issue to fix it.

But in the meantime, doing what this issue suggests and moving the sort and page denormalization from the enhancer to where the filter one is seems sensible.

I think this is a bug with AccessAwareRouter, and will open a core issue to fix it.

I'm fine with that. But I'm not sure it's a bug in there. The real problem here is not a bug in one place or the other, but the fact that our routing system design/architecture is not unambiguously defined. It's not clear whether this should or should not work!

Opening that core issue would spark the necessary discussion around this. For the scope of the security issue we could not get blocked on that though.

But in the meantime, doing what this issue suggests and moving the sort and page denormalization from the enhancer to where the filter one is seems sensible.

+1

#10: will review tomorrow. But … EIGHTY SIX KILOBYTES?!

Yeah, currently reading this on a mobile device, unfit for patch review. Will do so in the morning.

Great to see progress here in any case :)

@effulgentsia++
@gabesullice++

Thanks for #11, that was a big help :) I definitely like the massively increased clarity and simplicity in EntityResource: less magic.

1. 😍21 files changed, 669 insertions, 1102 deletions. — I'm not surprised!

2. +++ b/jsonapi.services.yml
   @@ -7,28 +7,6 @@ services:
   -  serializer.normalizer.sort.jsonapi:
   ...
   -  serializer.normalizer.offset_page.jsonapi:
   ...
   -  serializer.normalizer.entity_condition.jsonapi:
   ...
   -  serializer.normalizer.entity_condition_group.jsonapi:
   ...
   -  serializer.normalizer.filter.jsonapi:
   

   --- a/src/Controller/EntityResource.php
   +++ b/src/Controller/EntityResource.php
   …
   

   👍 This all made sense thanks to #11.1 especially.

3. +++ b/src/LinkManager/LinkManager.php
   @@ -93,33 +93,24 @@ class LinkManager {
   +   * @param \Drupal\jsonapi\Query\OffsetPage $page_param
   +   *   The current pagination parameter for the requested collection.
   ...
   -  public function getPagerLinks(Request $request, array $link_context = []) {
   +  public function getPagerLinks(Request $request, OffsetPage $page_param, array $link_context = []) {
   

   👍 This is now injected for the reason in #11.3. As a side effect, it also results in less magic because less reliance on semi-globals (IMHO Request is the new global in Drupal 8…)

4. +++ /dev/null
   @@ -1,111 +0,0 @@
   -  const PATH_KEY = 'path';
   ...
   -  protected function validate($data) {
   
   +++ b/src/Query/EntityCondition.php
   @@ -9,6 +12,27 @@ namespace Drupal\jsonapi\Query;
   +  const PATH_KEY = 'path';
   
   @@ -87,4 +111,71 @@ class EntityCondition {
   +  protected static function validate($parameter) {
   

   👍 Validation and constants are moved out of the former normalizers into the value objects that we're keeping. This means we're turning formerly anemic domain models into objects that contain their associated logic.

5. +++ b/src/Query/Filter.php
   @@ -45,10 +73,10 @@ class Filter {
   -   * @param \Drupal\Entity\Query\QueryInterface $query
   +   * @param \Drupal\Core\Entity\Query\QueryInterface $query
   

   👍 Oh hah, this must always have been wrong then! Seems reasonable to change it here, unless somebody objects.

6. +++ b/jsonapi.services.yml
   @@ -105,8 +83,8 @@ services:
   +  jsonapi.deserialization.enhancer:
   +    class: Drupal\jsonapi\Routing\DeserializationEnhancer
   

   🤔 This … I was confused by.

   +++ b/src/Routing/DeserializationEnhancer.php
   @@ -19,7 +17,7 @@ use Symfony\Component\Serializer\Exception\UnexpectedValueException;
   -class JsonApiParamEnhancer implements EnhancerInterface, ContainerAwareInterface {
   +class DeserializationEnhancer implements EnhancerInterface, ContainerAwareInterface {
   
   @@ -51,22 +49,7 @@ class JsonApiParamEnhancer implements EnhancerInterface, ContainerAwareInterface
        if (isset($defaults['serialization_class']) && !$request->isMethodSafe(FALSE)) {
          // Deserialize incoming data if available.
   

   😨 Apparently I helped make this happen in #2987608: Move deserialization from RequestHandler to JsonApiParamEnhancer, but I'm now quite confused why we did that. As long as we were doing denormalization of filter/page/sort params in a route enhancer (which we're getting rid of here), it sort of was internally consistent, which is a justification. But it still seems less than ideal, less than clear.

   I'm relieved to see that at the end of #2987608-14: Move deserialization from RequestHandler to JsonApiParamEnhancer, I expressed this same concern. I'm less impressed by my former self's not seeing any problems with this, and actually being in favor of this 😔

   In #2987608-16: Move deserialization from RequestHandler to JsonApiParamEnhancer, @gabesullice did a big refactor of the patch in that issue to introduce this. In hindsight, I think it might've been better to keep EntityResource::deserialize() instead of this.

   Even though changing this is out of scope for this issue, I wonder what @effulgentsia thinks about this? Should we move this logic into EntityResource instead, hence removing this service altogether? We could do that in a separate issue.

7. --- a/src/Routing/JsonApiParamEnhancer.php
   +++ b/src/Routing/DeserializationEnhancer.php
   

   🤔 Following up on the previous point. For now, I think RequestBodyEnhancer would be a much clearer name. Thoughts?

8. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -217,18 +213,8 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -    $sort = new Sort([['path' => 'nid', 'direction' => 'ASC']]);
        $request = Request::create('/jsonapi/node/article');
   -    $request->attributes = new ParameterBag([
   -      '_route_params' => [
   -        '_json_api_params' => [
   -          'sort' => $sort,
   -        ],
   -      ],
   -      '_json_api_params' => [
   -        'sort' => $sort,
   -      ],
   -    ]);
   +    $request->query = new ParameterBag(['sort' => 'nid']);
   

   👏 I hadn't even thought about this, but this madness just disappears, NICE! 👏

   (This is where kernel tests fell disappointingly short: we had to replicate much/most of the implementation details, making the tests highly coupled to implementation details…)

9. --- a/tests/src/Kernel/Normalizer/FilterNormalizerTest.php
   +++ /dev/null
   @@ -1,160 +0,0 @@
   -  public function denormalizeProvider() {
   ...
   -  public function testDenormalizeNested() {
   
   +++ b/tests/src/Kernel/Query/FilterTest.php
   @@ -336,4 +293,115 @@ class FilterTest extends JsonapiKernelTestBase {
   +  public function parameterProvider() {
   ...
   +  public function testCreateFromQueryParameterNested() {
   

   👍 This test coverage was not lost, but moved.

10. +++ b/tests/src/Kernel/Query/FilterTest.php
    @@ -142,13 +112,8 @@ class FilterTest extends JsonapiKernelTestBase {
    -    $normalized = [
    -      'colors.foobar' => '',
    -    ];
    -    $this->normalizer->denormalize($normalized, Filter::class, NULL, [
    -      'entity_type_id' => 'node',
    -      'bundle' => 'painting',
    -    ]);
    +    $resource_type = new ResourceType('node', 'painting', NULL);
    +    Filter::createFromQueryParameter(['colors.foobar' => ''], $resource_type, $this->fieldResolver);
    

    👍 These tests are also much easier to understand now, wonderful!

11. +++ b/tests/src/Kernel/Query/SortTest.php
    @@ -58,18 +41,21 @@ class SortNormalizerTest extends KernelTestBase {
    -      ['lorem', [['path' => 'foo', 'direction' => 'ASC', 'langcode' => NULL]]],
    ...
    +      ['lorem', [['path' => 'lorem', 'direction' => 'ASC', 'langcode' => NULL]]],
    

    👍 … another case where our existing tests were giving false assurances because they were relying on too much magic.

    Actually, thanks to this refactor, we can convert this to a unit test! 👍 In fact, we can do that for all of these, except for FilterTest, because Filter is the only one that has a dependency on another service. Done!

I forgot #21's interdiff 😊

Confirmed that #16 still works: both JSON:API Extras and its "Defaults" submodules pass tests with it. And it actually became simpler 😀

What is this for?

Remove it and re-run that unit test. You'll see that PHPUnit complains about it because there were zero assertions :) It's obvious in hindsight: in the previous patches and in HEAD, we're only doing an assertion in case of an invalid input.

WFM!

I wrote in #25:

Confirmed that #16 still works: both JSON:API Extras and its "Defaults" submodules pass tests with it. And it actually became simpler 😀

I just checked, and with this applied, https://www.drupal.org/project/consumer_image_styles/releases/8.x-3.x-dev also still passes tests, without any changes 👍

Thanks for this! +1