Follow-up to SA-CONTRIB-2018-081: Restore JSON:API filter access

This patch looks great. I reviewed the permission provider work a few months ago, but it's probably more efficient to let @bojanz review this in detail.

+++ b/entity.module
@@ -118,3 +125,93 @@ function entity_views_query_alter(ViewExecutable $view, QueryPluginBase $query)
+  if (!is_a($access_class, EntityAccessControlHandlerBase::class, TRUE)) {
+    return;
+  }
+  $reflector = new ReflectionMethod($access_class, 'checkAccess');
+  if ($reflector->getDeclaringClass()->getName() !== EntityAccessControlHandlerBase::class) {
+    return;
+  }

Fancy 🤓😀

Unfortunately, the thing here is: this module provides entity query access. It sounds like the JSON:API module's workaround for core's missing entity query access is this new set of hooks, which now needs a new hook in this module that duplicates permission logic and the concepts of entity query access?

I agree fixing it in this module is better than various one-offs in Drupal Commerce (see #3022231-3: Add in new JSONAPI hooks to allow filtering of products via API.

However, I imagine a better priority be entity query access in Drupal core, which would have mitigated this whole problem?

However, I imagine a better priority be entity query access in Drupal core, which would have mitigated this whole problem?

100% agreed.

But doing that to fix the security problem was not an option: a security issue can never add major new public APIs that need public scrutiny. Let alone a security issue for a contrib module.

It took hundreds of hours of work over the span of all of 2018 to get this security release fixed. Please believe me when I say: I wish we wouldn't have had to do that 🙏🙏🙏

I get your frustration. We've been there too.

P.S.: We even actively explored doing exactly what you suggest, and even worked with your colleague @bojanz on this, but it became clear doing that was impossible, since we'd never get the amount of attention and scrutiny such an issue deserves without public scrutiny. (We're currently blocked on the security team for certain aspects of creating a public issue.)

but I don't see where the ER portion is implemented.

Because selection plugins use an entity query. I'm on mobile and cannot find the code, but I'm pretty sure the entity query access fixed some of our marketplace user problems around valid store references once this landed.

Selecting entity references is one thing (this uses @EntityReferenceSelection plugins which modify entity queries).

We're talking about entity queries that involve joins based on entity references, and checking access on those.

Just now we got the very first issue filed asking why they needed to grant the anonymous user admin permissions to be able to filter a list of commerce products: #3028210: Applying filter to collection of Commerce Product entities requires admin privileges. I explained it and pointed to this issue, recommending that they apply this patch for now.

I was directed here from the above related issue. I have a custom entity that I can no longer filter on unless I grant the anonymous user administrative privileges. I tried the entity-jsonapi.patch first, but that didn't seem to work and throws a bunch of PHP warnings. The second patch, entity-jsonapi.patch takes, but doesn't fix the api results.

It seems the second patch would require me to implement query_access on the custom entity, but I haven't been able to find any documentation on how to do that. Pardon my ignorance, but could you direct me on what I'd need to do with the entity in order to get this patch to work?

This is the access control logic for the custom entity:

protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {

  $is_owner = ($account->id() && $account->id() == $entity->getOwnerId()) ? TRUE : FALSE;
  switch ($operation) {
    case 'view':
      if(in_array('administrator', $account->getRoles())) {
        return AccessResult::allowed();
      } else {
        return AccessResult::allowedIf((($account->hasPermission('view shareable') || $account->hasPermission('view shareable ' . $entity->bundle())) && $entity->status->value));
      }
    case 'update':
      return AccessResult::allowedIf((($account->hasPermission('update shareable') && $is_owner) || ($account->hasPermission('update shareable ' . $entity->bundle()) && $is_owner)) || $account->hasPermission('update any shareable') || $account->hasPermission('update any shareable ' . $entity->bundle()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity);

    case 'delete':
      return AccessResult::allowedIf((($account->hasPermission('delete shareable') && $is_owner) || ($account->hasPermission('delete shareable ' . $entity->bundle()) && $is_owner)) ||  $account->hasPermission('delete any shareable') || $account->hasPermission('delete any shareable ' . $entity->bundle()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity);
  }

  // No opinion.
  return AccessResult::neutral()->cachePerPermissions();
}

Hi,

I updated my version of JSONAPI module to 2.3 and after that I can't filter my custom Entities by id, as mentioned here it's a permission issue, I can retrieve it as admin, but nos as anonymous as I usually did.

I can't apply this patch since its for 1.x version, is there a way I could for 2.x version ?. Thanks in advance.

I also have a `checkAccess` custom access handler implementation on my module, but when an anonymous user uses a filter on my JSON endpoint it doesn't even enters the function, but if do not use the filter it does.

I have a quiz entity and if I GET `/jsonapi/quiz/quiz/` as anonymous I get all Quiz entities, but if I add a filter by ID `jsonapi/quiz/quiz/?filter[id]=id_number_goes_here` it does not work, it doesn't pass through the function, I can't do anything.

Any help is well received since i'm not being able to capture the entity request

What a non-ideal situation.

Code-wise I like #9 more than #2, but I'm worried that using query_access won't provide wide enough coverage. Many more entity types use the generic access control handlers, and extending an access control handler is better understood than extending a query access handler (which custom entity types would often have to do). However, the comment in #11 indicates that the #2 patch potentially has issues.

This means, that we'll need to do some manual testing and exploration of the fix. Unfortunately, I won't have time to do so in the near future. Maybe April. In the meantime, it's up to the community to push this forward.

I came across this problem while testing jsonapi filters and was unable to get them to work without using an admin user. @wim-leers pointed me to this issue after asking about it on Slack.

Example filter:
/jsonapi/commerce_product/default?filter[test][condition][path]=product_category.drupal_internal__tid&filter[test][condition][operator]=%3D&filter[test][condition][value]=4

It turns out that additional access checks is applied onto the collection query for non-privileged user accounts and one of them was:

AND ((commerce_product_field_data.product_id < 1) and (commerce_product_field_data.product_id > 1))

Which effectively invalidates the query.

Patch #9 seems to be working fine for me, but does no longer apply cleanly.

Thanks for the reroll!

RC3 just shipped. Without this. :(

What do you need to happen for this to ship in a release?

I just ran into this and spun for a few minutes until I uncovered the always false condition. However, I have only tested READ operations.

This patch worked for me when retrieving filtered commerce products and variations. However, it did not work when retrieving product attributes. Not sure why. Permissions to view product attributes are checked for anonymous.

I'm sorry if I'm pointing out the obvious, but after reviewing this patch and the previous comments I feel like the solution offered here is building further on the foundation we regrettably had to add in JSON:API because we couldn't drastically alter core when trying to fix this security issue (see #5).

Now that we have a security release with the new JSON:API hooks, we can ask ourselves: Do we really want to continue building upon something we had to ship with a gun to our head? Correct me if I'm wrong, but the real problem seems to be that Entity API only applies query access to an entity query's base table and not any joins that are added to it (see #8).

So if we go ahead with the patch listed here, won't that simply kick the can further down the road? Yes, it might solve the issue for JSON:API, but any other module adding a join to an entity query will still not be able to rely on query access handlers.

How about we steer the conversation towards trying to figure out how we can support joins? The fewer commonly used modules implement these new JSON:API hooks, the easier it will become to add entity query access to core and get rid of this band-aid solution in JSON:API?

This issue is the only one remaining before the maintainers can #3034677: Release 8.x-1.0, but based on the most recent comments it seems as though the status for this issue should move back to needs work.

Patch of #16 didn't works for me.

Tested environment:
Core: Drupal8.7.8 and Drupal8.8.x
Commerce: 2.x-dev and 8.x-2.14
Entity: 1.0.0-rc3 .

Test steps: Fresh install drupal core and commerce, then add a product and two product variations. Give enough permission to anonymous.
then use anonymous user to request

/jsonapi/commerce_product_variation/default?filter[product_id.id][value]=eb5186f7-de97-4569-9d15-371231180a9b

Empty result:
{"jsonapi":{"version":"1.0","meta":{"links":{"self":{"href":"http:\/\/jsonapi.org\/format\/1.0\/"}}}},"data":[],"links":{"self":{"href":"http:\/\/drupal88x.ld\/jsonapi\/commerce_product_variation\/default?filter%5Bproduct_id.id%5D%5Bvalue%5D=eb5186f7-de97-4569-9d15-371231180a9b"}}}

Patch #16 worked for me, to be able to get a collection of products, filtered by categories :)

FWIW I have #16 each Commerce API project. We should just get this committed since a "proper" core fix won't be coming (any time soon, easily, etc.)

Okay, sounds like we're proceeding with #16.

Here's a slightly tweaked version (docblocks, guard clauses, consistent return types to make PhpStorm happy).

mglaman confirmed that the new patch still works. Thanks, everyone!