Problem/Motivation

Spin-off from #3504114: Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants. This adds the node grants cache context along side the user cache context when node's view access handler is short-circuited, in the specific situation where a node is unpublished and the user has 'view own unpublished nodes' permission, because this never reaches the node grants code.

The original issue is trying to implement a more comprehensive solution that would not require manually adding the cache context here, but it has to introduce a new API and concept in order to do so - hence splitting the quick fix and test coverage out to this issue.

Steps to reproduce

NA

Proposed resolution

Had cache context 'user.node_grants:view' when node_grant used

Remaining tasks

Review

User interface changes

NA

Introduced terminology

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

Issue fork drupal-3516477

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch credited ericgsmith.

catch credited kristiaanvandeneynde.

catch credited mxr576.

catch’s picture

catch
he/him
Primary language English
commented
catch’s picture

catch
he/him
Primary language English
commented
Title: Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants » Avoid cache redirect error when using 'view own unpublished content' permission alongside node grants
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3504114: Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants

catch opened merge request !11690

catch’s picture

catch
he/him
Primary language English
commented

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.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

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

catch’s picture

catch
he/him
Primary language English
commented

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 a forbid prevents node grants running (it can't undo a forbid), but you can't bypass it with a grant except for the view own unpublished permissions check.

ericgsmith’s picture

ericgsmith commented

Thank you @catch and @kristiaanvandeneynde for all the action so far here and on #3504114: Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants. 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 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.

acbramley made their first commit to this issue’s fork.

acbramley’s picture

acbramley commented
Status: Needs review » Needs work

NW based on the comments in the test.

catch’s picture

catch
he/him
Primary language English
commented

@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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

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?

ericgsmith’s picture

ericgsmith commented

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

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative 
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

  • alexpott committed 9a34ed55 on 11.1.x 
    Issue #3516477 by catch, ericgsmith, acbramley, kristiaanvandeneynde,...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed a66d9a73 on 11.2.x 
    Issue #3516477 by catch, ericgsmith, acbramley, kristiaanvandeneynde,...

  • alexpott committed 7d1a9ca9 on 11.x 
    Issue #3516477 by catch, ericgsmith, acbramley, kristiaanvandeneynde,...

Status: Fixed » Closed (fixed)

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