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:

This was a bugsmash target today. This looks well in hand. I have added the 'blocker' tag to 📌 Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review because of comment #31. It is good to see the the issue this is postponed on is in the remaining tasks.

Rebased and also cherry-picked the latest MR state from 📌 Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review .

Added the latest version of https://git.drupalcode.org/project/drupal/-/merge_requests/8221 to the top of the commit change.
https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di...

No blockers anymore! \o/ just need to rebase this branch tomorrow.

Opened ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review for the improvements that I made on \Drupal\Tests\jsonapi\Functional\ResourceTestBase1 to ensure the test suite is less fragile and more intuitive to use in context of Dynamic Page Cache assertions.

I am going to reorganize existing changes in MR#8198 to separate changes that belongs to here from those that belongs to 3473374.

This should be it... now we have 3 meaningful commits in the branch.
https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di...

I may saw some other areas in ResourceTestBase still with hardcoded Dynamic Cache Header values... checking those.

Yep, those three could be still automated. https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di...

Cacheable metadata, most notably cache contexts, is not being carried over to the next checks in the access control handler. This can lead to cache misses or worst case information disclosure.

Until 📌 Introduce AccessResult::andAllowedIf(), orAllowedIf(), andForbiddenIf() and orForbiddenIf() to eventually replace orIf() and andIf() Needs review lands, I wouldn't use andIf() like you do. It means we are running code when we could have already exited early.

Gradually build a $cacheable_metadata object and add that to $result whenever you want to return it. That still allows for early returns, but does not lose important cacheable metadata.

E.g.:

$cacheable_metadata->addCacheContexts(['foo']);
if (is_foo()) {
  return AccessResult::allowed()->addCacheableDependency($cacheable_metadata);
}

$cacheable_metadata->addCacheContexts(['bar']);
if (is_bar()) {
  return AccessResult::allowed()->addCacheableDependency($cacheable_metadata);
}

// etc.

Do you have concrete suggestions for code adjustments? I know those new methods will bring improvements but with the current tooling I tried hard in the latest commits to return as early as possible, but also keeping collected cacheability instead of ignoring them. But the chance is there that something can be still further improved

user.roles:authenticated context bugs me since the latest change... maybe I can delay that and first check instead whether the user has the given permission, if yes, I can double check whether it is anonymous or not and only add that context, otherwise just vary per permissions.

✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review got an RTBC \o/

I think I noticed something off about the MR, will comment in detail during contrib time at DrupalCon

The cache per user needs to be up higher where I left the remark. Changes are looking really good otherwise! Will do a full review once you've updated it.

The issue here is that we're dealing with a passed in account rather than the current user so any user.foo cache context is technically not correct here.

YES...YES...YES!!! And I always feel bad when I have to bubble up cache per current user in anyways in an entity access handler method just to ensure things "do not get broken" but semantically that is completely incorrect. That said, this is what we have currently... :(

Thanks for the review, to be honest, I have lost a bit now... :) Could you please commit the expected change to the MR?

And before you do that, let's remember that we try to avoid cache per user as long as we can, this is the reason why user.roles:antonymous or user.roles:authenticated (in the current version) s the primary driver to differentiate access for logged in and non-logged in users instead of returning something that varies per user. We only do this as long as we can, so when a node_access module enabled than we have add user.node_grants: that basically makes every result vary by user in a different way.

Thanks for the review, to be honest, I have lost a bit now... :) Could you please commit the expected change to the MR?

It's late so could have made a mistake, but I stayed closer to the original code while making sure cacheable metadata is passed down after every check. Please review.

Did a check on the changes and I generally agree with those. I hope nobody is going to raise scope creep just because for consistency reasons not on the view operation was touched.

There are some tests that needs to be fixed, I am going to working on that.

My changes after Kristiaan's changes: https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?st...

I take the opportunity that after so many back and forth code reviews with Kristiaan, mark this as RTBC. My changes since his last changes mainly impacted failing tests after his changes, that were changed by me previously.

Rebased and brought in latest changes from [##3473374] which also had to be rebased, but we found other regressions in 11.x in that: https://git.drupalcode.org/project/drupal/-/merge_requests/9465#note_386330

Left some comments on the MR

I'm wondering if the cure is worse than the symptom here

Thanks for the review, let me start with a the rebase since there is work to be done it seems.

❯ git rebase origin/11.x
Successfully rebased and updated refs/heads/3278759-access-cacheability.

Thanks for the review @larowlan!

I'll discuss this with other core committers

I wonder if we can expand the description of the permission to convey that there is a performance impact of granting this permission. And whether we should also have a hook_requirements that shows a warning if this permission has been granted. Will report back on other committers' thoughts.

Not fully caught up with the implementation, but there are plenty of valid cases to use own permissions, it's only wrong when granted unnecessarily. But the situation around unpublished view and edit access is messy, we can't really point to a different permission that might not actually exist depending on the modules you use. So I don't think we can remove or warn about its usage.

So I don't think we can remove or warn about its usage.

+1

For changing the world, please join this other issue to brainstorm on how to make node grants and entity access work better together (less confusing to use): https://www.drupal.org/project/drupal/issues/3452449 ✨ Grant query level access to own unpublished nodes Active

I'm wondering if the cure is worse than the symptom here

The symptom is a potentially unstable or even insecure website, so the cure is definitely not worse :)

Appears to have some open threads.

Thanks for the additional feedbacks, I have pushed back on some with answers and accepted one. Back to review

Okay so I saw @bbrala's comment about isNew() now, but I'd still say we should change the $node->id() boolean checks to a version of isNew(). Reason being that if isNew() ever gets updated to be more strict, we immediately benefit from that change. If we keep manual ID to boolean conversions, then we may end up lagging behind on core changes.

MR looks good to me now for the parts that I can review confidently (so anything but jsonapi tests). Don't let my isNew() comment hold you back, although I think it'd be a nice change.

Test fails seem to be random ones, so if you can ping @bbrala for a final approval then it's RTBC in my book.

Yes they were random failures, restart make them disappear.

Test fails seem to be random ones, so if you can ping @bbrala or @larowlan for a final approval then it's RTBC in my book.

+1

Really only see the isnew issue. The response to larowlans comment seems valid, and personally I agree with mrx on that.

If we can update that I'll rtbc again.

isNew() can be tricked, it most likely does not help PHPStan to narrow down the actual type of $node->id(), but I agree, let's follow the Drupal Way here -- funny, I wanted to use that originally, but I ditched the idea for some reasons, I hope not because tests started to fail :)

  public function isNew() {
    return !empty($this->enforceIsNew) || !$this->id();
  }

Only open thread is about cache hit ratio and I agree with @mxr576 that security trumps that. We can open a follow-up to further discuss those ramifications if need be.

All aboard the RTBC train, choo choo!

Now that ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review is in 11.x, this needs a rebase because the MR does not apply anymore, because it had a cherry picked version. Maybe I just drop the cherry picked commit and see what happens...

❯ git rebase origin/11.x
Auto-merging core/.phpstan-baseline.php
Auto-merging core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
CONFLICT (content): Merge conflict in core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
error: could not apply 07987b49a5... Cherry-picked "Improve Dynamic Page Cache header assertions in JSON:API tests"

I am going to use git merge origin/11.x instead of rebase in a hope that we do not need to restart the review process here.

Looks good so far, we only have those changes in the MR that we should be, no changes on ResourceTestBase that was fixed in the spin-off issue.

Rebased again, due to random test failure fixed in upstream: https://www.drupal.org/project/drupal/issues/3478621#comment-15875919 📌 Add a filecache-backed attribute parser Active

Green again! Kept it on RTBC...

Thanks for updating the MR. Can't immediately see any changes outside of the resolved conflict, so fine with keeping RTBC.

Updating issue credits

larowlan → closed merge request !8198

Committed to 11.x and backported to 11.1.x

This doesn't apply to 11.0.x/10.5.x or 10.4.x so needs reroll.

Flagging as patch (to be ported) on that basis.

Thanks everyone for the work here

I tried to create a new 11.0.x version of this patch, but there is something a little messy in there, perhaps something else did not get a backport, im not too sure. I'm not sharp enough right now to find out.

It seems ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review has not been backported to 11.0.x yet, so that should be backported first.

10.4.x backport is ready in the related MR.

Backported to 10.5.x and 10.4.x

A backport to 10.3.x or 11.0.x would also require a backport of ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review which as a task isn't eligible for backport, so marking this as fixed.

Thanks all, especially @mxr576 for sticking with this one.

larowlan → closed merge request !10409

Thanks all, especially @mxr576 for sticking with this one.

:beers:

A backport to 10.3.x or 11.0.x would also require a backport of ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review which as a task isn't eligible for backport, so marking this as fixed.

Does this highlight an edge case in the backporting policy? Here’s the dilemma:
This fix cannot be backported to 11.0.x due to its dependency on a non-backportable issue. As a result, the upgrade path from 10.4.x/10.5.x to 11.0.x could lead to unexpected and unforeseeable feature or bugfix losses unless projects skip directly to 11.1.x.

Discussed with @catch and he agreed it made sense to backport ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review

mxr576 → closed merge request !10451

This broke HEAD on 10.5 and 10.4.x, see https://git.drupalcode.org/project/drupal/-/jobs/3585472. Reverted for now.