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

Created on 4 May 2022, over 2 years ago
Updated 13 September 2024, 2 months ago

Problem/Motivation

Access cacheability is not correct when the "view own unpublished content" is in use, leading to improperly cached render arrays.

Steps to reproduce

(See even more minimalist reproduction steps is MR !8198)

1. Standard install
2. Add an entity reference field to the Page content type called "Related Articles" where article content can be referenced.
3. Configure the "Related Articles" field to display as a rendered entity.
4. Create Content Editor named "Dan"
5. Log in as Dan
6. Create an Article named "Dan's Article".
7. Create a Page named "Test Page" and add "Dan's Article" as a Related Article.
8. As the admin, unpublish "Dan's Article"
9. As Dan, View "Test Page". You will see "Dan's Article" rendered in pink. Good.
10. Create a new Content Editor named Flan.
11. Log in as Flan.
12. As Flan, view "Test Page". You will NOT see "Dan's Article". Good.
13. Clear Caches.
14. As Flan, view "Test Page". You will NOT see "Dan's Article". Good.
15. As Dan, view "Test Page". You will NOT see "Dan's Article". This is not correct.

Note that you will never see MORE than you are supposed to see. This is not an access bypass problem. Rather you will potentially see less than you are supposed to see.

In this particular case, the incorrect cacheable metadata is being created within EntityReferenceFormatterBase::getEntitiesToView:

$access = $this->checkAccess($entity);
// Add the access result's cacheability, ::view() needs it.
$item->_accessCacheability = CacheableMetadata::createFromObject($access);

Proposed resolution

Bubble up user cache context when there is no other option, since the lack of proper cache context on the final render result causes this problem.

Remaining tasks

  1. Fix πŸ“Œ Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review because it is currently a blocker of fixing this one, see more details in https://git.drupalcode.org/project/drupal/-/merge_requests/8198#note_317834
  2. Consider if improvements introduced by ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review (originally invented in this issue) should be merged first or can be merged as part of this issue.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/A

πŸ› Bug report
Status

Needs review

Version

11.0 πŸ”₯

Component
Node systemΒ  β†’

Last updated 3 days ago

No maintainer
Created by

πŸ‡ΊπŸ‡ΈUnited States danflanagan8 St. Louis, US

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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.

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
    -    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 β†’ .

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

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

  • Pipeline finished with Failed
    6 months ago
    Total: 556s
    #183346
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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.

  • Pipeline finished with Failed
    6 months ago
    Total: 642s
    #183361
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    Hide patch #8 based on #11.

  • Pipeline finished with Failed
    6 months ago
    Total: 589s
    #183398
  • Pipeline finished with Failed
    6 months ago
    Total: 590s
    #183431
  • Pipeline finished with Failed
    6 months ago
    Total: 187s
    #183458
  • Pipeline finished with Failed
    6 months ago
    Total: 606s
    #183464
  • Pipeline finished with Failed
    6 months ago
    Total: 660s
    #183588
  • Pipeline finished with Failed
    6 months ago
    Total: 576s
    #183600
  • Pipeline finished with Failed
    6 months ago
    Total: 573s
    #183617
  • Pipeline finished with Failed
    6 months ago
    Total: 191s
    #184099
  • Pipeline finished with Failed
    6 months ago
    Total: 619s
    #184184
  • Pipeline finished with Failed
    6 months ago
    Total: 242s
    #184914
  • Pipeline finished with Failed
    6 months ago
    Total: 185s
    #184940
  • Pipeline finished with Failed
    6 months ago
    Total: 211s
    #184955
  • Pipeline finished with Failed
    6 months ago
    Total: 609s
    #184967
  • Pipeline finished with Failed
    6 months ago
    Total: 701s
    #184986
  • Pipeline finished with Failed
    6 months ago
    Total: 646s
    #185144
  • Pipeline finished with Failed
    6 months ago
    Total: 170s
    #185180
  • Pipeline finished with Failed
    6 months ago
    Total: 180s
    #185183
  • Pipeline finished with Failed
    6 months ago
    Total: 1189s
    #185187
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Pipeline finished with Success
    6 months ago
    Total: 923s
    #185839
  • Pipeline finished with Failed
    6 months ago
    Total: 2213s
    #185898
  • Pipeline finished with Failed
    6 months ago
    Total: 547s
    #186540
  • Pipeline finished with Failed
    6 months ago
    Total: 860s
    #186595
  • Pipeline finished with Success
    6 months ago
    Total: 585s
    #186602
  • Pipeline finished with Success
    5 months ago
    Total: 658s
    #190037
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    After a spin-off

  • Pipeline finished with Success
    5 months ago
    Total: 568s
    #190547
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    Fixing this issue probably also resolves this one.

  • Status changed to Needs review 5 months ago
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

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

  • Status changed to Needs work 5 months ago
  • 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.

  • Status changed to Needs review 5 months ago
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Pipeline finished with Failed
    5 months ago
    Total: 563s
    #196626
  • Pipeline finished with Success
    5 months ago
    Total: 652s
    #196640
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Pipeline finished with Success
    5 months ago
    Total: 541s
    #197014
  • Status changed to Needs work 5 months ago
  • πŸ‡³πŸ‡±Netherlands bbrala Netherlands

    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.

  • Pipeline finished with Success
    5 months ago
    Total: 615s
    #198716
  • Pipeline finished with Success
    5 months ago
    Total: 553s
    #198738
  • Status changed to Postponed 5 months ago
  • πŸ‡³πŸ‡±Netherlands bbrala Netherlands

    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.

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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:

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    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.

  • Pipeline finished with Failed
    3 months ago
    Total: 589s
    #260280
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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 .

  • Pipeline finished with Failed
    3 months ago
    Total: 447s
    #260285
  • Pipeline finished with Success
    3 months ago
    Total: 544s
    #260501
  • Pipeline finished with Canceled
    2 months ago
    Total: 172s
    #277866
  • Pipeline finished with Failed
    2 months ago
    Total: 601s
    #277869
  • Pipeline finished with Success
    2 months ago
    Total: 646s
    #277880
  • Assigned to mxr576
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

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

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Issue was unassigned.
  • Status changed to Needs review 2 months ago
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Pipeline finished with Canceled
    2 months ago
    Total: 210s
    #278551
  • Pipeline finished with Success
    2 months ago
    Total: 428s
    #278558
  • Pipeline finished with Running
    2 months ago
    #278946
  • Pipeline finished with Success
    2 months ago
    Total: 649s
    #278968
  • Pipeline finished with Success
    2 months ago
    Total: 6846s
    #279349
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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.

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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.

  • Pipeline finished with Success
    2 months ago
    Total: 1297s
    #279539
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Pipeline finished with Success
    2 months ago
    Total: 465s
    #279560
  • Status changed to Needs work 2 months ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    2 months ago
    Total: 548s
    #282118
  • Pipeline finished with Success
    2 months ago
    Total: 1046s
    #282150
  • Status changed to Needs review 2 months ago
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Status changed to Needs work 2 months ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.
    
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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.

  • Pipeline finished with Failed
    2 months ago
    Total: 566s
    #282265
  • Pipeline finished with Canceled
    2 months ago
    Total: 2505s
    #282297
  • Pipeline finished with Success
    2 months ago
    Total: 492s
    #282352
  • Status changed to Needs review 2 months ago
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary
  • Pipeline finished with Success
    about 2 months ago
    Total: 443s
    #284260
  • Pipeline finished with Success
    about 2 months ago
    Total: 720s
    #284279
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 184s
    #292748
  • Pipeline finished with Failed
    about 2 months ago
    Total: 1074s
    #293032
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 454s
    #293294
  • Pipeline finished with Failed
    about 2 months ago
    Total: 159s
    #293320
  • Pipeline finished with Success
    about 2 months ago
    Total: 430s
    #293326
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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.

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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

  • Pipeline finished with Success
    about 1 month ago
    Total: 442s
    #300847
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Left some comments on the MR

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

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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.
    
  • Pipeline finished with Failed
    22 days ago
    Total: 565s
    #319075
  • Pipeline finished with Success
    22 days ago
    Total: 591s
    #319098
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    Thanks for the review @larowlan!

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    I'll discuss this with other core committers

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    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.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    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.

  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

    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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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 :)

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Appears to have some open threads.

  • Pipeline finished with Failed
    3 days ago
    Total: 293s
    #336364
  • Pipeline finished with Success
    3 days ago
    Total: 1266s
    #336371
  • πŸ‡­πŸ‡ΊHungary mxr576 Hungary

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

Production build 0.71.5 2024