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

I don't remember where, but I'm ~90% sure that we at some discussed the route access thing, I think because I investigated some missing cacheablity on a response and was surprised to not see it because it was there on the final response. And it was because it was only added by route access. And you told me that is by design because of some reason. No idea where it was but I think I do remember the reason.

DPC caches based on the *route*, so we need to have that. And route matching and access happens at the same time (AccessAwareRouter) for security reasons. Which means even if DPC wanted to run before checking route access, it couldn't. Which also means that it would be a waste to consider route access cacheablity as that would just introduce the possibility of more variations that are not actually relevant (or should not be).

and I think I have just run into this here: https://git.drupalcode.org/project/drupal/-/merge_requests/8198#note_317834

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"'

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?

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.

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.

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. Never mind, forgot that anyone can view /user/[id] given the right permissions and that said page doesn't vary by user that way.

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.

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

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

The #3278759: Access cacheability is not correct when "view own unpublished content" is in use 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)

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?

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 #3278759: Access cacheability is not correct when "view own unpublished content" is in use.

Opened #3452181: VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects, 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.

#2973356-26: Cacheability information from route access checker access results are ignored by dynamic_page_cache now has a dedicated issue: #3452426: Insufficient cacheability information bubbled up by UserAccessControlHandler

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

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.

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

According to this comment, this is blocking that issue.

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

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.

Added tag for #39

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

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.

Rebased to make NR bot happy.

Let's rebase to see if anything new reports an error before VariationCache improvement gets merged and unblocks this issue...

kewl! Nothing got broken

Okay, so unless exit codes are lost somewhere, the upcoming VariationCache improvement won't spot any new issues in Drupal core after this change. Fingers crossed...

https://git.drupalcode.org/issue/drupal-2973356/-/commit/8aef8dd16ce8ec4...

Rebasing since #3452181: VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects is in 11.x now \o/

No conflicts. hopefully this gets smooth...

Created two change records for this issue.

Found 2 nits, everything else looks okay. The only thing that still has me worried a bit is the jsonapi tests becoming uncacheable. It might very well be intentional and therefore harmless, but it would be great if we could git blame, find the original issue and then ask around to make sure why a NULL access result is allowed and added as a dependency (therefore setting max age to 0).

Seems good enough for me. Could you please inline the variable still, though?

Gitlab complained about a merge conflict so I had to rebase, No manual steps were needed locally.

Is there a follow-up issue for \Drupal\jsonapi\Controller\FileUpload::handleFileUploadForExistingResource()?

I haven't opened it yet, I was consider it, but that issue is two folded, one is the sub-request, but the first blocker is \Drupal\Core\PageCache\RequestPolicy\CommandLineOrUnsafeMethod that makes POST requests uncacheable dynamic cache. (IMO does not change anything on this https://www.drupal.org/node/3420901)

https://git.drupalcode.org/project/drupal/-/merge_requests/8221#note_368194

So I can register this finding as an issue, but that is going to be blocked by making POST requests cacheable by Dynamic Page Cache, which I do not know yet if it is on the roadmap or not.

It hasn't been discussed for dynamic page cache, but we added read-only render caching for post requests in general, and I think we could probably apply similar to dynamic page cache too - would be worth a try! #3395776: Make POST requests render cacheable.

Went ahead and opened #3473158: Read-only render caching on POST requests for dynamic page cache.

Committed/pushed to 11.x and cherry-picked to 10.4.x, thanks!

This would probably be committable to a patch release too if it wasn't for the service priority changes, given those, leaving in the two minor releases.

Thanks @catch for the merge and delaying with opening the follow up issue.

FYI, devel's tests started failing after this merge. Its not clear to me why, since devel uses lazy builders. https://gitlab.com/drupalspoons/devel/-/issues/541.