- Issue created by @mxr576
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Are you sure though? AccessResult::andIf() has 3 branches, we are dealing with the third.
if ($this->isForbidden() || $other->isForbidden()) { } elseif ($this->isAllowed() && $other->isAllowed()) { } else { // WE ARE HERE WHEN ALLOWED+NEUTRAL. }
In said branch, we pass the following check:
if (!$this->isNeutral()) { $merge_other = TRUE; if ($other instanceof AccessResultReasonInterface) { $result->setReason($other->getReason()); } }
So when you have allowed and use andIf to feed in a neutral, you should get the cacheability from the neutral access result. This makes sense because A+N could turn into A+A under different cache context values, and that would lead to a different outcome (Allowed).
However, when you have neutral and feed in allowed, then the second one doesn't get merged in. But that makes sense as the first one is N and we know the second one can't be F. N+N and N+A both return N based on the first N, so why even bother with the second result?
So I don't see a bug in the code. Am I missing something?
- ππΊHungary mxr576 Hungary
You are right, I went back time, made the code more explicit and I we hit the N+A scenario that was not clearly visible on the original screenshot.
At first I could not reproduce with the latest, more explicit code in https://git.drupalcode.org/project/drupal/-/merge_requests/8285
// Only allow view access if the account is active. $result = AccessResult::allowedIfHasPermission($account, 'access user profiles'); if ($result->isAllowed()) { $result = $result->andIf( AccessResult::allowedIf($entity->isActive())->addCacheableDependency($entity) ); if ($result instanceof AccessResultReasonInterface) { $result->setReason("The 'access user profiles' permission is required and the user must be active."); } if ($result->isAllowed()) { return $result; } } // Users can view own profiles at all times. return $result->orIf(AccessResult::allowedIf($account->id() == $entity->id())->addCacheContexts(['user']));
and I realized why... thanks to the more explicit code that only handles the A+N scenario and does not expect the the
user:3
cache tag appear in every cases.protected function getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) { // @see \Drupal\user\UserAccessControlHandler::checkAccess() - return parent::getExpectedUnauthorizedEntityAccessCacheability($is_authenticated) - ->addCacheTags(['user:3']);
Although, this could also mean that A+A scenario is not tested, at least not part of the User related REST tests.
If you agree, we can close this ticket.
- Status changed to Closed: works as designed
3 months ago 1:06pm 5 June 2024 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Yup, mistakes happen. Glad we figured it out.