After implementing node grants, it becomes impossible to return a CacheableResponse from a controller which performs a query with 'node_access' specified. The code in node_query_node_access (see link below) grabs the current request stack and a renderer and renders an array with node access cache tags. This causes EarlyRenderingControllerWrapperSubscriber (linked below) to throw a LogicException.

https://api.drupal.org/api/drupal/core%21modules%21node%21node.module/fu...
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21EventSubs... (see source, line 154)

Comments

gabesullice created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2557815: Automatically bubble the "user.node_grants:$op" cache context in node_query_node_access_alter()

Where is the controller code?

We added that auto-cache-context-bubbling thing to node_query_node_access_alter() in #2557815: Automatically bubble the "user.node_grants:$op" cache context in node_query_node_access_alter(), and it comes with test coverage that uses a controller. So I'm surprised it doesn't work for you, because it does work in the test.

gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented

@Wim Leers I believe the reason those tests pass is because they are returning a renderable array, the issue is with returning a CacheableResponse. See comment here: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/EventSubscri...

This is effectively the code http://cgit.drupalcode.org/jsonapi/tree/src/Resource/EntityResource.php#...

Wim Leers’s picture

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

The problem is that node_query_node_access_alter() runs during the controller, you want it to run lazily, then this is not a problem.

(Sorry had this response sitting here from before #3, lost this browser tab somewhere.)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice commented
Status: Active » Closed (outdated)
Issue tags: +API-First Initiative

Whoa, blast from the past. This is outdated since #2984964: JSON API + hook_node_grants() implementations: accessing /jsonapi/node/article as non-admin user results in a cacheability metadata leak

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
Status: Closed (outdated) » Closed (works as designed)

So glad to have found this issue as I was debugging the problem. I have a custom controller that returns a CacheableJsonResponse directly and I thought I was capturing all the relevant metadata until I executed a query with node_access cache tag. I got the logic exception!

Note that #2984964: JSON API + hook_node_grants() implementations: accessing /jsonapi/node/article as non-admin user results in a cacheability metadata leak doesn't make this issue outdated exactly. That issue fixed the problem specifically for JSON API only. For any custom module developers that return cacheable JSON, they need to wrap the query being executed in a render context and pop the cache metadata off of it!

See the patch in #2984964 for example.