Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants

catch → credited ericgsmith → .

catch → credited kristiaanvandeneynde → .

catch → credited mxr576 → .

I also think we need to consider whether this case actually ought to check node grants too e.g. if you're able to view your own unpublished nodes, then maybe there is some reason node grants would still want to deny that? Seems unlikely but theoretically possible.

Off the top of my head: Don't grants also not apply if an access hook already gave a definitive answer?

In ::checkAccess(), if ::checkViewAccess() returns null, it falls back to node grants. The only situation in which that doesn't happen is when the user is allowed to view their own unpublished content.

In EntityAccessControlHandler::access(), it calls checkViewAccess as long as access hasn't been forbidden already.

So you can forbid access and prevent node access running (it can't undo a forbid), but you can't bypass it with a grant except for the view own unpublished permissions check.

Thank you @catch and @kristiaanvandeneynde for all the action so far here and on 🐛 Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants Active . I have been meaning to come back and provide feedback (and thank you for all the explanations!) but needed to find enough time to digest it all.

Looking just at this issue - this change resolves the error I was seeing and seems like a good quick win.

Reviewing the previous comments - I wasn't sure if this comment 🐛 Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants Active from @kristiaanvandeneynde was implying there would still be issue with this approach? I got a little lost tracking which comments were to which MR / approach.

The test I originally wrote as a quick and clunky way to just trigger the error - at a minimum some of the comments need updating - but I wonder if we need to actually assert for something to make it more useful? Or alternatively simplify it to check user.node_grants:view is consistent in the cache metadata returned from node access. I'm happy to take suggestions here and tidy up / work on the tests.

NW based on the comments in the test.

@acbramley I think that comment is outdated, per the discussion on the main issue https://git.drupalcode.org/project/drupal/-/merge_requests/11395/diffs#e...

Going to run the test only job to find out now though. Either way, we need to remove the comment even if that's the only problem.

Here we go: https://git.drupalcode.org/issue/drupal-3516477/-/jobs/4935386

@ericgsmith I did some quick tidy up, but we should probably also make sure we get a 200 response code from the request, and maybe explain what's going on a bit more with the repeated status changes etc. closer to where they happen?

Thanks @catch - tidy up looks good - I've added an additional comment to cover why we are repeating the publish / unpublish check.

1) Drupal\Tests\node\Functional\NodeAccessCacheRedirectWarningTest::testNodeAccessCacheRedirectWarning
Exception: User warning: Trying to overwrite a cache redirect for "entity_view:block:p2869g2u:[languages:language_interface]=en:[route]=entity.node.canonical02d270e527ee2db38f14edbd305a4fea048b224ab212dd8a734fefc49e4a04ab:[theme]=stark:[user.permissions]=7f7368bdcaf1bc7d699ccd93afd47f8507b65e3f5fb80919000513056663b45a" with one that has nothing in common, old one at address "languages:language_interface, theme, user.permissions, route" was pointing to "user.node_grants:view", new one points to "user.roles:authenticated, user".

Test coverage appears to be working

cleaned up IS best I could, but title appears to match the solution. LGTM

Committed and pushed 7d1a9ca93b3 to 11.x and a66d9a73577 to 11.2.x and 9a34ed55ded to 11.1.x. Thanks!

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