Cacheability information from route access checker access results are ignored by dynamic_page_cache

Created on 16 May 2018, about 6 years ago
Updated 11 July 2024, 8 days ago

Problem/Motivation

Dynamic page cache currently ignores cacheability information from route access results which could lead to unexpected edge cases. Imagine a page where the access is decided per user bases but the content does not vary per user. Without adding user context to the render array/response of the page, the dynamic page cache module is not going to cache it per user.

Having this fixed is currently a blocker of another issue and the original fix for this problem were invented there: 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work See also: https://git.drupalcode.org/project/drupal/-/merge_requests/8198#note_317834

Original description

Discovered in #2869426: EntityResource should add _entity_access requirement to REST routes .

See #2869426-28: EntityResource should add _entity_access requirement to REST routes , #2869426-29: EntityResource should add _entity_access requirement to REST routes and #2869426-83: EntityResource should add _entity_access requirement to REST routes in particular.

#29:

I could be wrong, but I remember that we once discussed that it is by design that route access cacheability is *not* considered by dynamic page cache because dynamic page cache runs *after* route access checking.

But maybe I misunderstood something here.

To my surprise 😲 and fear 😱, this was NEVER DISCUSSED! Zero mentions of . #2429617-219: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) mentions it. But doesn't dive in deeper. So I instead looked at every occurrence of . Again nothing mentioned.

But I think @Berdir is right. It's just unfortunate that despite Dynamic Page Cache having such detailed test coverage, this is not something that is explicitly tested. (If it were, then the change made to \Drupal\Core\EventSubscriber\RouteAccessResponseSubscriber::getSubscribedEvents() should trigger a test failure.)

And I think the reason this is only coming up now and not months or years ago, is that the REST module's responses are atypical. Unlike pretty much all other responses, the controller in question (\Drupal\rest\Plugin\rest\resource\EntityResource::get()) has been:

  1. Doing its own route/response-level access checking.
  2. Significantly varying the contents of the response based on that access checking.

(The closest analogies are block and entity rendering, but those are not route/response-level, they're intra-response-level.)

Proposed resolution

TBD

Remaining tasks

  1. Fix 🐛 Insufficient cacheability information bubbled up by UserAccessControlHandler Active that was identified in comment 26 by kristiaanvandeneynde OCD/spidy-sense
  2. Get the 🐛 VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Active so it can spot potential issues revealed by this fix
    1. Fix 🐛 JsonApiRequestValidator does not set cacheable metadata when the filter allows the request RTBC that was discovered in the VariationCache hardening issue
    2. Fix 🐛 BreadcrumbBuilder::applies() mismatch with cacheability metadata Needs review that was discovered in the VariationCache hardening issue

User interface changes

None.

API changes

None.

Data model changes

Yes: Dynamic Page Cache would vary not just by the Response's cacheability, but also by the route access result's cacheability.

📌 Task
Status

Needs review

Version

11.0 🔥

Component
Dynamic page cache 

Last updated 1 day ago

Created by

🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 224s
    #185372
  • Pipeline finished with Failed
    about 2 months ago
    Total: 675s
    #185385
  • Pipeline finished with Failed
    about 2 months ago
    Total: 579s
    #185630
  • Pipeline finished with Failed
    about 2 months ago
    Total: 612s
    #185707
  • Pipeline finished with Failed
    about 2 months ago
    Total: 590s
    #185754
  • Pipeline finished with Failed
    about 2 months ago
    Total: 576s
    #185765
  • Pipeline finished with Success
    about 2 months ago
    Total: 658s
    #185780
  • Pipeline finished with Success
    about 2 months ago
    Total: 573s
    #185828
  • Status changed to Needs review about 2 months ago
  • Pipeline finished with Failed
    about 2 months ago
    Total: 779s
    #185914
  • Pipeline finished with Success
    about 2 months ago
    Total: 598s
    #185924
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Where is this query coming from? Seems unrelated:
    'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"'

  • 🇨🇭Switzerland Berdir Switzerland

    That's the entity query index lookup for the blocks of the current theme, i guess means a dynamic page cache miss that previously was a hit?

  • 🇭🇺Hungary mxr576 Hungary

    That's the entity query index lookup for the blocks of the current theme, i guess means a dynamic page cache miss that previously was a hit?

    Probably, tbh, I have no clue on that.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    That's the entity query index lookup for the blocks of the current theme, i guess means a dynamic page cache miss that previously was a hit?

    Yeah that's what has me a tiny bit worried. The StandardPerformanceTest is supposed to mimic a "normal" page visit on a "regular" Drupal site and gauge its performance. If we get an extra query there, it's highly likely we get said extra query everywhere. If it's required for this bug to go away, sure, but this is something we should double-check. We don't want to regress if we can avoid it.

    I'll see if I can spare a bit of time to have a look.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Okay I just did some digging and this extra query makes sense to me now.

    So because of the StandardPerformanceTest checking for Login, we are seeing a route that used to have a DPC HIT now be UNCACHEABLE. This route being the user page, which @mxr576 correctly pointed out is where you land after login and therefore, with the fix here, triggering a DPC UNCACHEABLE where it used not to.

    Now all of the pieces of the /user/[id] page that could be cached, still are. So the only reasonable thing we can expect to see more queries of is that which is required to build the final response again before it is shipped off to the response subscribers and then the browser.

    Chief among these page building elements that has to run again because there was no DPC cache hit is BlockPageVariant, which calls BlockRepository::getVisibleBlocksPerRegion(), which in turn fires the extra query we're seeing.

    So on that front: All good, this was to be expected and is a side-effect we can't do anything about here. The only thing that has me really worried now is that there's apparently nothing on the user page that also sets the user cache context. Because, if there were, this page would (and should) always have been UNCACHEABLE for the DPC.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Gave the whole MR a decent look and would RTBC if there was an answer to what is going on in those json_api tests. I'm not getting into the onRespond priority as I feel as if I'm not qualified to make a judgement there. I'd have to check all other response subscribers first to confidently sign off on that one.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Okay so this issue made me go down the rabbit hole a bit as I was wondering why DPC would mark the user page as UNCACHEABLE even though sometimes it actually is cacheable. Then I found a very interesting bug which will occur after this MR goes in.

    Check the following code in UserAccessControlHandler::checkAccess():

          case 'view':
            // Only allow view access if the account is active.
            if ($account->hasPermission('access user profiles') && $entity->isActive()) {
              return AccessResult::allowed()->cachePerPermissions()->addCacheableDependency($entity);
            }
            // Users can view own profiles at all times.
            elseif ($account->id() == $entity->id()) {
              return AccessResult::allowed()->cachePerUser();
            }
            else {
              return AccessResultNeutral::neutral("The 'access user profiles' permission is required and the user must be active.")->cachePerPermissions()->addCacheableDependency($entity);
            }
    

    There is a bug here where a user without the 'access user profiles' would be locked out of their own profile. This is because anything after the $account->id() == $entity->id() has to vary by user (and user.permissions, but that gets optimized away), yet the return value at the bottom only varies by user.permissions.

    So if a user without the permission and without looking at their own profile comes along, we return a neutral access result for said profile varying by user.permissions only. Meaning if the user whom the profile belongs to comes along next, and happens to have the same permissions as the previous visitor, they would get the cached response with AccessResultNeutral!

    This currently does not manifest itself because DPC doesn't take the access checks' cacheability into account, but given how user.permissions does not lead to an unceacheable response, DPC could very well be caching an access denied page and serve it to the user who should get access. At least, once this MR gets in.

    Not setting back to NW because I don't necessarily think this is the place to fix said bug, but it will occur once this MR gets accepted.

    The fix would probably look like this:

          case 'view':
            $view_cacheability = (new CacheableMetadata())->addCacheContexts(['user.permissions'])->addCacheableDependency($entity);
            // Only allow view access if the account is active.
            if ($account->hasPermission('access user profiles') && $entity->isActive()) {
              return AccessResult::allowed()->addCacheableDependency($view_cacheability);
            }
            
            // Users can view own profiles at all times.
            $view_cacheability->addCacheContexts(['user']);
            if ($account->id() == $entity->id()) {
              return AccessResult::allowed()->addCacheableDependency($view_cacheability);
            }
            
            return AccessResultNeutral::neutral("The 'access user profiles' permission is required and the user must be active.")->addCacheableDependency($view_cacheability);
    
  • 🇭🇺Hungary mxr576 Hungary

    Thanks @kristiaanvandeneynde for the detailed testing and reporting, another rabbit hole indeed.

    The 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work already has a couple of blockers, another child issue of a child issue would not hurt much ;S

    Would you like to open a new issue for this or check if there is an existing one? (You deserve a (double) credit for this finding)

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    I'll try to open one next week and attach the current code in the MR. Catch requested we use code more similar to ContactPageAccess, but now I'm thinking that class is bugged too. We can update the MR once I'm done researching that (or someone else can jump in and research it too).

    Shall we postpone this issue or somehow mark that it can't be committed in its current state lest we introduce bugs and potentially even security issues?

  • 🇭🇺Hungary mxr576 Hungary

    Shall we postpone this issue or somehow mark that it can't be committed in its current state lest we introduce bugs and potentially even security issues?

    Added a note about this to the issue description, but I hope this does not become a chicken-or-egg story, since for me, this is already a spin-off/prerequisite of fixing 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work .

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Opened 🐛 VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Active , haven't opened an issue for the UserAccessControlHandler bug yet as I'd like to see how many more surface once the VC MR is ready.

  • Pipeline finished with Canceled
    about 2 months ago
    #189958
  • Pipeline finished with Success
    about 2 months ago
    Total: 544s
    #189964
  • Pipeline finished with Success
    about 1 month ago
    Total: 719s
    #192589
  • Pipeline finished with Failed
    about 1 month ago
    Total: 619s
    #196997
  • Pipeline finished with Success
    about 1 month ago
    Total: 543s
    #197013
  • 🇭🇺Hungary mxr576 Hungary

    Synched with @kristiaanvandeneynde on Slack about the plan for getting this closed sooner than later, updated the issue description accordingly.

  • Status changed to Needs work 29 days 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 29 days ago
  • 🇭🇺Hungary mxr576 Hungary
    ❯ git rebase origin/11.x
    Auto-merging core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
    Auto-merging core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
    Auto-merging core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php
    Auto-merging core/modules/workspaces/tests/src/Functional/WorkspaceSwitcherTest.php
    CONFLICT (content): Merge conflict in core/modules/workspaces/tests/src/Functional/WorkspaceSwitcherTest.php
    Auto-merging core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php
    error: could not apply 0804e87ca2... Cherry-picked fix from https://git.drupalcode.org/project/drupal/-/merge_requests/8198
    hint: Resolve all conflicts manually, mark them as resolved with
    hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
    hint: You can instead skip this commit: run "git rebase --skip".
    hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
    Could not apply 0804e87ca2... Cherry-picked fix from https://git.drupalcode.org/project/drupal/-/merge_requests/8198
    
    

    Added void return defs to tests again... PHPStorm could easily resolve that.

  • Pipeline finished with Success
    29 days ago
    Total: 496s
    #203772
  • 🇳🇿New Zealand quietone New Zealand

    According to this comment 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work , this is blocking that issue.

  • 🇭🇺Hungary mxr576 Hungary

    The last second level blockers just got closed, let's make a comment and hope that flushes stale render cache ;P

  • Status changed to Needs work 9 days 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 8 days ago
  • 🇭🇺Hungary mxr576 Hungary
  • Pipeline finished with Success
    8 days ago
    Total: 736s
    #221223
  • 🇮🇳India naveenvalecha New Delhi

    Added tag for #39

  • 🇭🇺Hungary mxr576 Hungary

    @naveenvalecha, review bot was right, Gitlab also complained about the merge issue, that tag should be used wisely.

Production build 0.69.0 2024