I noticed the following in \Drupal\Core\Access\AccessResult
:
/**
* Creates an allowed access result if the permission is present, neutral otherwise.
*
* Checks the permission and adds a 'user.permissions' cache context.
*
* @param \Drupal\Core\Session\AccountInterface $account
* The account for which to check a permission.
* @param string $permission
* The permission to check for.
*
* @return \Drupal\Core\Access\AccessResult
* If the account has the permission, isAllowed() will be TRUE, otherwise
* isNeutral() will be TRUE.
*/
public static function allowedIfHasPermission(AccountInterface $account, $permission) {
return static::allowedIf($account->hasPermission($permission))->addCacheContexts(['user.permissions']);
}
This varies the cached result by the permissions hash the currently logged in user has, not the permissions hash the user which was passed in through the $account parameter has. Same goes for \Drupal\Core\Access\AccessResult::allowedIfHasPermissions()
It's an edge case because 99% of the time it will probably be the current user the function is dealing with. But in case someone wants to verify if another user can access something, this will cache the result for the permissions of the current user instead.
Proposed solutions:
- Remove the cache context.
- Only add the context if the current user is the passed in account
- ???
Comments
Comment #3
kristiaanvandeneyndeComment #4
kristiaanvandeneyndeComment #5
Wim LeersThat is a great catch! My initial reaction was the same as your second option: "Only add the context if the current user is the passed in account".
But first, let's create a failing test to show the problem.
Comment #6
chx CreditAttribution: chx at Smartsheet commentedIsn't this a duplicate of good ole' #2628870: Access result caching per user(.permissions) does not check for correct user
Comment #7
kristiaanvandeneyndeThat one seems to touch a whole lot more than what this issue is about. This is merely about varying a cacheable result by the user's permissions, where "the user" is wrongly assumed to always be the current logged in user.
Comment #10
BerdirNot sure why this is a major bug, as long as user.permissions is a required cache context anyway, this would make zero difference.
Comment #11
Wim LeersComment #12
kristiaanvandeneyndeDoes the cache metadata for access checks get added to the page? In that case, the page will indeed still have user.permissions. If it doesn't, then we may end up caching access checks for the wrong user, which could be major.
Comment #13
Berdircaching incorrectly for a wrong user is indeed a problem, but that's what #2628870: Access result caching per user(.permissions) does not check for correct user is about.
Comment #14
kristiaanvandeneyndeIndeed it is. So perhaps this is a duplicate after all?
Any idea as to what the answer is to "Does the cache metadata for access checks get added to the page? ", i.e.: Does it bubble up as well? Would seem odd at first because you may want to check access before trying to build or retrieve a page. Then again, that's just from glancing at it.
Comment #15
BerdirIf those access results are properly attached to the output then yes, it gets attached and bubbled up. See $field_access in \Drupal\Core\Entity\Entity\EntityViewDisplay::buildMultiple() for example.
Comment #19
kristiaanvandeneyndeKnowing what I know now compared to three years ago, I still think this is a tricky issue.
Suppose you have a page where the visibility depends on a permission of the author, not the visitor. Something like: "Owner can display advanced information to the public". Then if a general audience member visits this page when the owner has that permission, the advanced version would get cached and displayed to all audience members. But if later on that owner has a change of permissions, retracting said permission (let's say their premium subscription expired), then said page would still be leaking the advanced information to the general public.
This would be alleviated by only varying by user.permissions if the passed in account is the current user's account and otherwise adding the user's roles cache tags (like we see when user.permissions gets optimized away). Which is exactly what chx's patch in #2628870: Access result caching per user(.permissions) does not check for correct user does.
So this is a duplicate of that issue, but the other issue still needs a proper fix :)