Closed (fixed)
Project:
Drupal core
Version:
10.3.x-dev
Component:
user system
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
4 Jun 2024 at 10:35 UTC
Updated:
25 Jun 2024 at 15:44 UTC
Jump to comment: Most recent
Problem/Motivation
This is a spin-off issue of #2973356: Cacheability information from route access checker access results are ignored by dynamic_page_cache where as part of the review, @kristiaanvde's spidey senses identified an incorrect cacheability information definition in the access result returned by the UserAccessControlHandler that could lead to unexpected scenarios (like access loss) when access results returned by route access checkers finally get considered by Dynamic Page Cache module.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3452426
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
mxr576I'll submit a patch for this later today.
Comment #3
kristiaanvandeneyndeIf the intent was to manually optimize 'user.permissions' into 'user', don't. VariationCache needs all of the cache contexts, even if they will eventually be optimized away because, during said optimization (cache context folding), extra cache tags might surface that need to be added to the cache entry.
So in this case, the first if-statement can vary by user.permissions, the second by ['user', 'user.permissions'] and the third one too. Again, do not manually optimize this, you're introducing bugs and potential security issues.
Comment #5
mxr576Comment #7
kristiaanvandeneyndeChanges look great, would RTBC but I think we should explain why the seemingly unrelated tests were changed. If you could add a few notes with your findings there, then all good to me.
Comment #8
mxr576I double checked it and I haven't found any unrelated tests, those are test base classes that used by the user related REST/JSONAPI endpoint tests.
Comment #9
kristiaanvandeneyndeSorry I should have been more specific: Test code that shouldn't at first sight have been affected.
Could you briefly explain why this is needed now?
Could you also elaborate on the mitigating lines? Why weren't they needed before and are they now?
What about the fix you copied from REST?
Comment #10
mxr576I assume you were referring to this one, which was copied from JSONAPI: https://git.drupalcode.org/project/drupal/-/merge_requests/8285#note_321681
It seems there are no tests in REST - which concerns me - that would have hit the simple "let's normalize
user.permission usertouser" requirement so far.Comment #11
kristiaanvandeneyndeThanks, looks good to me.
Comment #16
catchLooks good to me too, both the actual fixes and trying to keep the scope appropriate vs. the other pre-existing and newly opened issues. Also thanks for the annotations on the MR for the test changes, I probably would have asked about at least one so saved a round trip of issue statuses.
Committed/pushed to 11.x, cherry-picked to 11.0.x, 10.4.x, 10.3.x, thanks!