Problem/Motivation
UserAccessControlHandler::checkAccess() has this code:
else if ($account->id() == $entity->id()) {
return AccessResult::allowed()->cachePerUser();
}
...
return AccessResult::neutral();
The problem with this is that the cachePerUser() is not added when there is no match (checking whether user1 can view user2's profile), which allows the neutral() result to be cached across users, but then the cache can be primed with user1, and then user2 can't view his own profile.
Proposed resolution
Use the allowedIf() syntax which allows the ->cachePerUser();
to be associated in either case. This is the same approach as is done for the 'update' and 'delete' operations.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#21 | 2614230-21.patch | 1.66 KB | _utsavsharma |
#21 | interdiff_18-21.txt | 826 bytes | _utsavsharma |
#18 | 2614230-18.patch | 1.65 KB | Gauravvvv |
#3 | interdiff.txt | 685 bytes | effulgentsia |
#3 | user-access-control-3.patch | 1.63 KB | effulgentsia |
Issue fork drupal-2614230
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:
Comments
Comment #2
Wim LeersLooks correct! Great catch.
Comment #3
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI expected the original patch to have the same 2 test failures as in #2558599-84: Automatically assign user cache contexts/tags when using current_user service, but it didn't. However, this one does. I think this means we need to fix the tests to expect the 'user' cache context, or else have a code flow that doesn't need it.
Comment #17
catchStill valid, sent the patch for a re-test since it looks like it'd still apply, we'll still need to update those tests if it's the same two test failures.
Comment #18
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedI have attached a patch for 11.x. Patch #3, does not apply to 11.x so not adding interdiff.
Comment #19
smustgrave CreditAttribution: smustgrave at Mobomo commentedStill need a test showing the bug.
Comment #20
catchThe code flow used to be a switch, but is now an if/elseif, so I don't think this will work the same way at all - it's the final else that needs the user cache content adding, and the condition added here will always be true.
Comment #21
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedTried to address the failures in #18.