[PP-1] Access cacheability is not correct when "view own unpublished content" is in use

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

As a bug it will need a test case.

Did not review.

-    if ($operation === 'view' && !$status && $account->hasPermission('view own unpublished content') && $account->isAuthenticated() && $account->id() == $uid) {
-      return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
+    if ($operation === 'view' && !$status && $account->hasPermission('view own unpublished content') && $account->isAuthenticated()) {
+      return AccessResult::allowedIf($account->id() == $uid)->cachePerUser()->addCacheableDependency($node);
     }
 

Did some testing today and unfortunately this is not that simple. By removing $account->id() == $uid we eliminate the chance for granting access to an unpublished node created by somebody else. (Got to this conclusion when I worked on the fix for #3449181)

So maybe the only way is what @wim.leers suggested in #2982770-5: NodeAccessControlHandler::checkAccess() does not add a necessary cache context → .

I have spent several hours trying to reproduce this issue using only the available testing tools, aiming for a more minimalist approach than described here.

I attempted to follow the reproduction steps from #2982770-5: NodeAccessControlHandler::checkAccess() does not add a necessary cache context → , but I discovered significant differences between how Views and JSONAPI handle entity access checks. JSONAPI performs entity access checks, including the "view own unpublished content" check, whereas Views relies on hook_node_grants() via query alteration out of the box. (See my failed attempts in the attached unable_to_reproduce.patch).

As a result, I had to create a custom test controller, which I will include in my next merge request (MR).

Closed 🐛 NodeAccessControlHandler::checkAccess() does not add a necessary cache context Needs work as duplicate of this one. Also transferred the latest proposed fix from there.

Issue credits should be transferred.

Hide patch #8 based on #11.

Fixing 📌 Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review first is a blocker of this issue.

After a spin-off

Fixing this issue probably also resolves this one.

hm, this should have been in "needs review" for a while :)

catch → closed merge request !8198

mxr576 → reopened merge request !8198

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Did a review. Caching is hard and hurts my head. Left some comments for clarification and some small changes.

In general this makes a lot of sense and a i see no glaring issues.

Ok, i reviewed this and would RTBC. Although caching is scary we have pretty good proof on the effects. Code looks good, the ->id() issue is fine like this.

Dont think we need a CR here.

Unfortunatly this is postponed on 📌 Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review so we need to postpone on that.

Thanks for the review and the tentative RTBC. @bbrala.

I have synced with @kristiaanvandeneynde on Slack and described the current plan to get the blocker in sooner than later :) See: #2973356-32: Cacheability information from route access checker access results are ignored by dynamic_page_cache → :fingers-crossed: