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:

  1. Remove the cache context.
  2. Only add the context if the current user is the passed in account
  3. ???

Comments

kristiaanvandeneynde created an issue. See original summary.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

Issue summary: View changes
kristiaanvandeneynde’s picture

Issue summary: View changes
Wim Leers’s picture

Priority: Normal » Major
Issue tags: +Needs tests

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

chx’s picture

kristiaanvandeneynde’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Not sure why this is a major bug, as long as user.permissions is a required cache context anyway, this would make zero difference.

Wim Leers’s picture

Priority: Major » Normal
kristiaanvandeneynde’s picture

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

Berdir’s picture

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

kristiaanvandeneynde’s picture

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

Berdir’s picture

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

Status: Active » Closed (duplicate)

Knowing 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 :)