Access cacheability is not correct when "view own unpublished content" is in use

I think the fix will have to look something like this, although since we are returning early that may break other things but I don't really see any way around it. Side note - these "own" permissions a pretty problematic wrt. caching!

Obviously needs tests and probably breaks some existing stuff, perhaps we just detect the presence of the "own" permission and always add the user cache context, then keep the rest as is.

We can also remove the cachePerPermissions since cachePerUser is higher in the hierarchy.

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: The Content overview page filters out unpublished nodes when a node access module is enabled)

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 #2982770: NodeAccessControlHandler::checkAccess() does not add a necessary cache context 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 #2973356: Cacheability information from route access checker access results are ignored by dynamic_page_cache first is a blocker of this issue.

Updated the issue summary

Fixing this issue probably also resolves this one.

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

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 #2973356: Cacheability information from route access checker access results are ignored by dynamic_page_cache 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 #2973356: Cacheability information from route access checker access results are ignored by dynamic_page_cache 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 #2973356: Cacheability information from route access checker access results are ignored by dynamic_page_cache.

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

Opened #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests 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 #3452631: Introduce AccessResult::andAllowedIf(), orAllowedIf(), andForbiddenIf() and orForbiddenIf() to eventually replace orIf() and andIf() 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.

#3473374: Improve Dynamic Page Cache header assertions in JSON:API tests 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

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 or @larowlan 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. I like to use that method so that when we improve the typing there we get that for free here.

If we can update that I'll rtbc again.

isNew() can be tricked, it does not just check the value of the id, it also 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 #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests 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

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

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 #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests 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 #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests which as a task isn't eligible for backport, so marking this as fixed.

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

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 #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests 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.

And #3473374 was only born to keep this issue clean from test framework fixes and speed up the code review.

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

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

I believe this has caused a regression / BC break: #3495703: Loss of access to unpublished nodes.

Should be NW now since it was reverted?

Have tried to see where this is at.

The comment in #99 links to the 10.5.x pipeline which we can see is broken - as far as I can tell the 10.4.x pipeline from the same commit is https://git.drupalcode.org/project/drupal/-/pipelines/358805 which did fail but looks like an unrelated failure.

I've reapplied the commit from 11.x to the 10.4.x and it applies cleanly and appears to still pass https://git.drupalcode.org/project/drupal/-/merge_requests/12272/pipelines

I've done the same for 10.5.x here and its failing with what was reported before it was reverted https://git.drupalcode.org/issue/drupal-3278759/-/jobs/5409866#L412

The reason appears to be what was outlined here in #91 - except that the related issue #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests was committed to 10.4.x but does not appear to be on 10.5.x. That issue was closed as fixed - but my assumption here (have not tested) is that if that issue is backported to 10.5.x then this should pass.

I'm a bit confused on the timeline of when branches were open - but should this be postposed on #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests - and should #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests be reopened so that it can be applied to 10.5.x? Seems very unexpected that we have something in 10.4.x but not in 10.5.x?

Edit - I've reopened that issue and added a MR for 10.5.x

Reviewed merge request #12272 - diff is identical to the 11.x commit, pipeline is green, so I believe this is RTBC. Setting version accordingly.

I believe this should also be ported to the 10.5.x. and 10.6.x once #3473374: Improve Dynamic Page Cache header assertions in JSON:API tests is committed on those branches.

So currently this is in all supported 11.x versions but not 10.x.

#3473374: Improve Dynamic Page Cache header assertions in JSON:API tests landed in 10.6.x and 10.5.x finally, I've rebased MR !12273 against 10.5.x and if this is green now then this means this can be committed to both those branches.

I'm also closing the 11.0.x and 10.4.x MRs as they are no longer eligible for backports.

!12273 is green now so this should mean no test failures once this is committed to 10.5.x and 10.6.x. Thanks for the patience and assistance with the backports here.

Committed and pushed 86faa69f084 to 10.6.x and 53f117df5f0 to 10.5.x. Thanks!