diff --git a/core/lib/Drupal/Core/Access/AccessManager.php b/core/lib/Drupal/Core/Access/AccessManager.php index 625d8916b84..44af37e6d51 100644 --- a/core/lib/Drupal/Core/Access/AccessManager.php +++ b/core/lib/Drupal/Core/Access/AccessManager.php @@ -229,7 +229,7 @@ class AccessManager implements ContainerAwareInterface, AccessManagerInterface { $checks = array_diff($checks, $this->checkNeedsRequest); } - $result = AccessResult::create(); + $result = AccessResult::neutral(); if (!empty($checks)) { $arguments_resolver = $this->argumentsResolverFactory->getArgumentsResolver($route_match, $account, $request); if ($conjunction == static::ACCESS_MODE_ALL) { @@ -256,34 +256,18 @@ class AccessManager implements ContainerAwareInterface, AccessManagerInterface { * @see \Drupal\Core\Access\AccessResultInterface::andIf() */ protected function checkAll(array $checks, ArgumentsResolverInterface $arguments_resolver) { - $results = array(); - + // Without checks no opinion can be formed. + if (!$checks) { + return AccessResult::neutral(); + } + $result = AccessResult::allowed(); foreach ($checks as $service_id) { if (empty($this->checks[$service_id])) { $this->loadCheck($service_id); } - - $result = $this->performCheck($service_id, $arguments_resolver); - $results[] = $result; - - // Stop as soon as the first non-allowed check is encountered. - if (!$result->isAllowed()) { - break; - } - } - - if (empty($results)) { - // No opinion. - return AccessResult::create(); - } - else { - /** @var \Drupal\Core\Access\AccessResultInterface $result */ - $result = array_shift($results); - foreach ($results as $other) { - $result->andIf($other); - } - return $result; + $result = $result->andIf($this->performCheck($service_id, $arguments_resolver)); } + return $result; } /** @@ -301,7 +285,7 @@ class AccessManager implements ContainerAwareInterface, AccessManagerInterface { */ protected function checkAny(array $checks, ArgumentsResolverInterface $arguments_resolver) { // No opinion by default. - $result = AccessResult::create(); + $result = AccessResult::neutral(); foreach ($checks as $service_id) { if (empty($this->checks[$service_id])) { diff --git a/core/lib/Drupal/Core/Access/AccessResult.php b/core/lib/Drupal/Core/Access/AccessResult.php index c87b80ff478..91804356543 100644 --- a/core/lib/Drupal/Core/Access/AccessResult.php +++ b/core/lib/Drupal/Core/Access/AccessResult.php @@ -14,34 +14,18 @@ use Drupal\Core\Session\AccountInterface; /** * Value object for passing an access result with cacheability metadata. * + * The access result itself — excluding the cacheability metadata — is + * immutable. There are subclasses for each of the three possible access results + * themselves: + * + * @see \Drupal\Core\Access\AccessResultAllowed + * @see \Drupal\Core\Access\AccessResultForbidden + * @see \Drupal\Core\Access\AccessResultNeutral + * * When using ::orIf() and ::andIf(), cacheability metadata will be merged * accordingly as well. */ -class AccessResult implements AccessResultInterface, CacheableInterface { - - /** - * The value that explicitly allows access. - */ - const ALLOW = 'ALLOW'; - - /** - * The value that neither explicitly allows nor explicitly forbids access. - */ - const DENY = 'DENY'; - - /** - * The value that explicitly forbids access. - */ - const KILL = 'KILL'; - - /** - * The access result value. - * - * A \Drupal\Core\Access\AccessResultInterface constant value. - * - * @var string - */ - protected $value; +abstract class AccessResult implements AccessResultInterface, CacheableInterface { /** * Whether the access result is cacheable. @@ -78,7 +62,6 @@ class AccessResult implements AccessResultInterface, CacheableInterface { * Constructs a new AccessResult object. */ public function __construct() { - $this->resetAccess(); $this->setCacheable(TRUE) ->resetCacheContexts() ->resetCacheTags() @@ -88,64 +71,67 @@ class AccessResult implements AccessResultInterface, CacheableInterface { } /** - * Instantiates a new AccessResult object. - * - * This factory method exists to improve DX; it allows developers to fluently - * create access results. - * - * Defaults to a cacheable access result that neither explicitly allows nor - * explicitly forbids access. + * Creates an AccessResultInterface object with isNeutral() === TRUE. * * @return \Drupal\Core\Access\AccessResult + * isNeutral() will be TRUE. */ - public static function create() { - return new static(); + public static function neutral() { + return new AccessResultNeutral(); } /** - * Convenience method, creates an AccessResult object and calls allow(). + * Creates an AccessResultInterface object with isAllowed() === TRUE. * * @return \Drupal\Core\Access\AccessResult + * isAllowed() will be TRUE. */ public static function allowed() { - return static::create()->allow(); + return new AccessResultAllowed(); } /** - * Convenience method, creates an AccessResult object and calls forbid(). + * Creates an AccessResultInterface object with isForbidden() === TRUE. * * @return \Drupal\Core\Access\AccessResult + * isForbidden() will be TRUE. */ public static function forbidden() { - return static::create()->forbid(); + return new AccessResultForbidden(); } /** - * Convenience method, creates an AccessResult object and calls allowIf(). + * Creates an allowed or neutral access result. * * @param bool $condition - * The condition to evaluate. If TRUE, ::allow() will be called. + * The condition to evaluate. * * @return \Drupal\Core\Access\AccessResult + * If $condition is TRUE, isAllowed() will be TRUE, otherwise isNeutral() + * will be TRUE. */ public static function allowedIf($condition) { - return static::create()->allowIf($condition); + return $condition ? static::allowed() : static::neutral(); } /** - * Convenience method, creates an AccessResult object and calls forbiddenIf(). + * Creates a forbidden or neutral access result. * * @param bool $condition - * The condition to evaluate. If TRUE, ::forbid() will be called. + * The condition to evaluate. * * @return \Drupal\Core\Access\AccessResult + * If $condition is TRUE, isForbidden() will be TRUE, otherwise isNeutral() + * will be TRUE. */ public static function forbiddenIf($condition) { - return static::create()->forbidIf($condition); + return $condition ? static::forbidden(): static::neutral(); } /** - * Convenience method, creates an AccessResult object and calls allowIfHasPermission(). + * Creates an allowed access result if the permission is present, neutral otherwise. + * + * Convenience method, checks the permission and calls ::cachePerRole(). * * @param \Drupal\Core\Session\AccountInterface $account * The account for which to check a permission. @@ -153,83 +139,38 @@ class AccessResult implements AccessResultInterface, CacheableInterface { * The permission to check for. * * @return \Drupal\Core\Access\AccessResult + * If the account has the permission, isAlowed() will be TRUE, otherwise + * isNeutral() will be TRUE. */ public static function allowedIfHasPermission(AccountInterface $account, $permission) { - return static::create()->allowIfHasPermission($account, $permission); + return static::allowedIf($account->hasPermission($permission))->cachePerRole(); } /** * {@inheritdoc} + * + * @see \Drupal\Core\Access\AccessResultAllowed */ public function isAllowed() { - return $this->value === static::ALLOW; + return FALSE; } /** * {@inheritdoc} + * + * @see \Drupal\Core\Access\AccessResultForbidden */ public function isForbidden() { - return $this->value === static::KILL; + return FALSE; } /** - * Explicitly allows access. + * {@inheritdoc} * - * @return $this + * @see \Drupal\Core\Access\AccessResultNeutral */ - public function allow() { - $this->value = static::ALLOW; - return $this; - } - - /** - * Explicitly forbids access. - * - * @return $this - */ - public function forbid() { - $this->value = static::KILL; - return $this; - } - - /** - * Neither explicitly allows nor explicitly forbids access. - * - * @return $this - */ - public function resetAccess() { - $this->value = static::DENY; - return $this; - } - - /** - * Conditionally calls ::allow(). - * - * @param bool $condition - * The condition to evaluate. If TRUE, ::allow() will be called. - * - * @return $this - */ - public function allowIf($condition) { - if ($condition) { - $this->allow(); - } - return $this; - } - - /** - * Conditionally calls ::forbid(). - * - * @param bool $condition - * The condition to evaluate. If TRUE, ::forbid() will be called. - * - * @return $this - */ - public function forbidIf($condition) { - if ($condition) { - $this->forbid(); - } - return $this; + public function isNeutral() { + return FALSE; } /** @@ -391,77 +332,89 @@ class AccessResult implements AccessResultInterface, CacheableInterface { return $this; } - /** - * Convenience method, checks permission and calls ::cachePerRole(). - * - * @param \Drupal\Core\Session\AccountInterface $account - * The account for which to check a permission. - * @param string $permission - * The permission to check for. - * - * @return $this - */ - public function allowIfHasPermission(AccountInterface $account, $permission) { - $this->allowIf($account->hasPermission($permission))->cachePerRole(); - return $this; - } - /** * {@inheritdoc} */ public function orIf(AccessResultInterface $other) { - // If this AccessResult already is forbidden, then that already is the - // conclusion. We can completely disregard $other. - if ($this->isForbidden()) { - return $this; + // $other's cacheability metadata is merged if $merge_other gets set to TRUE + // and this happens in two cases: + // 1. $other's access result is the one that determines the combined access + // result. + // 2. This access result is not cacheable and $other's access result is the + // same. i.e. attempt to return a cacheable access result. + $merge_other = FALSE; + if ($this->isForbidden() || $other->isForbidden()) { + $result = static::forbidden(); + if (!$this->isForbidden() || (!$this->isCacheable() && $other->isForbidden())) { + $merge_other = TRUE; + } + } + elseif ($this->isAllowed() || $other->isAllowed()) { + $result = static::allowed(); + if (!$this->isAllowed() || (!$this->isCacheable() && $other->isAllowed())) { + $merge_other = TRUE; + } } - // Otherwise, we make this AccessResult forbidden if the other is, or - // allowed if the other is, and we merge in the cacheability metadata if the - // other access result also has cacheability metadata. else { - if ($other->isForbidden()) { - $this->forbid(); + $result = static::neutral(); + if (!$this->isNeutral() || (!$this->isCacheable() && $other->isNeutral())) { + $merge_other = TRUE; } - else if ($other->isAllowed()) { - $this->allow(); - } - $this->mergeCacheabilityMetadata($other); - return $this; } + $result->inheritCacheability($this); + if ($merge_other) { + $result->inheritCacheability($other); + } + return $result; } /** * {@inheritdoc} */ public function andIf(AccessResultInterface $other) { - // If this AccessResult already is forbidden or is merely not explicitly - // allowed, then that already is the conclusion. We can completely disregard - // $other. - if ($this->isForbidden() || !$this->isAllowed()) { - return $this; + // The other access result's cacheability metadata is merged if $merge_other + // gets set to TRUE. It gets set to TRUE in one case: if the other access + // result is used. + $merge_other = FALSE; + if ($this->isForbidden() || $other->isForbidden()) { + $result = static::forbidden(); + if (!$this->isForbidden()) { + $merge_other = TRUE; + } + } + elseif ($this->isAllowed() && $other->isAllowed()) { + $result = static::allowed(); + $merge_other = TRUE; } - // Otherwise, we make this AccessResult forbidden if the other is, or not - // explicitly allowed if the other isn't, and we merge in the cacheability - // metadata if the other access result also has cacheability metadata. else { - if ($other->isForbidden()) { - $this->forbid(); + $result = static::neutral(); + if (!$this->isNeutral()) { + $merge_other = TRUE; } - else if (!$other->isAllowed()) { - $this->resetAccess(); - } - $this->mergeCacheabilityMetadata($other); - return $this; } + $result->inheritCacheability($this); + if ($merge_other) { + $result->inheritCacheability($other); + // If this access result is not cacheable, then an AND with another access + // result must also not be cacheable, except if the other access result + // has isForbidden() === TRUE. isForbidden() access results are contagious + // in that they propagate regardless of the other value. + if (!$this->isCacheable() && !$result->isForbidden()) { + $result->setCacheable(FALSE); + } + } + return $result; } /** - * Merges the cacheability metadata of the other access result, if any. + * Inherits the cacheability of the other access result, if any. * * @param \Drupal\Core\Access\AccessResultInterface $other - * The other access result, whose cacheability data (if any) to merge. + * The other access result, whose cacheability (if any) to inherit. + * + * @return $this */ - protected function mergeCacheabilityMetadata(AccessResultInterface $other) { + public function inheritCacheability(AccessResultInterface $other) { if ($other instanceof CacheableInterface) { $this->setCacheable($other->isCacheable()); $this->addCacheContexts($other->getCacheKeys()); @@ -481,6 +434,7 @@ class AccessResult implements AccessResultInterface, CacheableInterface { else { $this->setCacheable(FALSE); } + return $this; } } diff --git a/core/lib/Drupal/Core/Access/AccessResultAllowed.php b/core/lib/Drupal/Core/Access/AccessResultAllowed.php new file mode 100644 index 00000000000..abc214762f5 --- /dev/null +++ b/core/lib/Drupal/Core/Access/AccessResultAllowed.php @@ -0,0 +1,22 @@ +isAllowed() && !$access->isForbidden(); - * @endcode + * Objects implementing this interface are using Kleene's weak three-valued + * logic with the isAllowed() state being TRUE, the isForbidden() state being + * the intermediate truth value and isNeutral() being FALSE. See + * http://en.wikipedia.org/wiki/Many-valued_logic for more. */ interface AccessResultInterface { @@ -36,43 +34,56 @@ interface AccessResultInterface { * Checks whether this access result indicates access is explicitly allowed. * * @return bool + * When TRUE then isForbidden() and isNeutral() are FALSE. */ public function isAllowed(); /** * Checks whether this access result indicates access is explicitly forbidden. * + * This is a kill switch — both orIf() and andIf() will result in + * isForbidden() if either results are isForbidden(). + * * @return bool + * When TRUE then isAllowed() and isNeutral() are FALSE. */ public function isForbidden(); + /** + * Checks whether this access result indicates access is not yet determined. + * + * @return bool + * When TRUE then isAllowed() and isForbidden() are FALSE. + */ + public function isNeutral(); + /** * Combine this access result with another using OR. * * When OR-ing two access results, the result is: * - isForbidden() in either ⇒ isForbidden() - * - isAllowed() in either ⇒ isAllowed() - * - neither isForbidden() nor isAllowed() => !isAllowed() && !isForbidden() + * - otherwise if isAllowed() in either ⇒ isAllowed() + * - otherwise both must be isNeutral() ⇒ isNeutral() * * @param \Drupal\Core\Access\AccessResultInterface $other * The other access result to OR this one with. * - * @return $this + * @return static */ public function orIf(AccessResultInterface $other); /** * Combine this access result with another using AND. * - * When OR-ing two access results, the result is: + * When AND-ing two access results, the result is: * - isForbidden() in either ⇒ isForbidden() - * - isAllowed() in both ⇒ isAllowed() - * - neither isForbidden() nor isAllowed() => !isAllowed() && !isForbidden() + * - otherwise, if isAllowed() in both ⇒ isAllowed() + * - otherwise, one of them is isNeutral() ⇒ isNeutral() * * @param \Drupal\Core\Access\AccessResultInterface $other * The other access result to AND this one with. * - * @return $this + * @return static */ public function andIf(AccessResultInterface $other); diff --git a/core/lib/Drupal/Core/Access/AccessResultNeutral.php b/core/lib/Drupal/Core/Access/AccessResultNeutral.php new file mode 100644 index 00000000000..2b38806902e --- /dev/null +++ b/core/lib/Drupal/Core/Access/AccessResultNeutral.php @@ -0,0 +1,22 @@ +setCacheable(FALSE); // @todo Remove dependency on the internal _system_path attribute: // https://www.drupal.org/node/2293501. if ($this->csrfToken->validate($request->query->get('token'), $request->attributes->get('_system_path'))) { - $access->allow(); + $result = AccessResult::allowed(); } else { - $access->forbid(); + $result = AccessResult::forbidden(); } - return $access; + // Not cacheable because the CSRF token is highly dynamic. + return $result->setCacheable(FALSE); } } diff --git a/core/lib/Drupal/Core/Access/DefaultAccessCheck.php b/core/lib/Drupal/Core/Access/DefaultAccessCheck.php index 6e27a5141ce..6f5b1c6c43d 100644 --- a/core/lib/Drupal/Core/Access/DefaultAccessCheck.php +++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.php @@ -32,7 +32,7 @@ class DefaultAccessCheck implements RoutingAccessInterface { return AccessResult::forbidden(); } else { - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/lib/Drupal/Core/Block/BlockBase.php b/core/lib/Drupal/Core/Block/BlockBase.php index 3768c327305..d76d5228cf6 100644 --- a/core/lib/Drupal/Core/Block/BlockBase.php +++ b/core/lib/Drupal/Core/Block/BlockBase.php @@ -170,18 +170,13 @@ abstract class BlockBase extends ContextAwarePluginBase implements BlockPluginIn // in order to fix that, we need condition plugins to return cache contexts, // otherwise it will be impossible to determine by which cache contexts the // result should be varied. - $access = AccessResult::create()->setCacheable(FALSE); - if ($this->resolveConditions($conditions, 'and', $contexts, $mappings) === FALSE) { - $access->forbid(); - return $access; - } - if ($this->blockAccess($account)) { - $access->allow(); + if ($this->resolveConditions($conditions, 'and', $contexts, $mappings) !== FALSE && $this->blockAccess($account)) { + $access = AccessResult::allowed(); } else { - $access->forbid(); + $access = AccessResult::forbidden(); } - return $access; + return $access->setCacheable(FALSE); } /** diff --git a/core/lib/Drupal/Core/Entity/EntityAccessCheck.php b/core/lib/Drupal/Core/Entity/EntityAccessCheck.php index 855be68c014..04412e94d4b 100644 --- a/core/lib/Drupal/Core/Entity/EntityAccessCheck.php +++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.php @@ -55,7 +55,7 @@ class EntityAccessCheck implements AccessInterface { } // No opinion, so other access checks should decide if access should be // allowed or not. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php index e655880030b..3def84ac573 100644 --- a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php @@ -76,10 +76,10 @@ class EntityAccessControlHandler extends EntityHandlerBase implements EntityAcce ); $return = $this->processAccessHookResults($access); - if (!$return->isAllowed() && !$return->isForbidden()) { + if ($return->isNeutral()) { // No module had an opinion about the access, so let's the access // handler check access. - $return->orIf($this->checkAccess($entity, $operation, $langcode, $account)); + $return = $return->orIf($this->checkAccess($entity, $operation, $langcode, $account)); } $result = $this->setCache($return, $entity->uuid(), $operation, $langcode, $account); return $return_as_object ? $result : $result->isAllowed(); @@ -102,7 +102,7 @@ class EntityAccessControlHandler extends EntityHandlerBase implements EntityAcce protected function processAccessHookResults(array $access) { // No results means no opinion. if (empty($access)) { - return AccessResult::create(); + return AccessResult::neutral(); } /** @var \Drupal\Core\Access\AccessResultInterface $result */ @@ -141,7 +141,7 @@ class EntityAccessControlHandler extends EntityHandlerBase implements EntityAcce } else { // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } @@ -231,10 +231,10 @@ class EntityAccessControlHandler extends EntityHandlerBase implements EntityAcce ); $return = $this->processAccessHookResults($access); - if (!$return->isAllowed() && !$return->isForbidden()) { + if ($return->isNeutral()) { // No module had an opinion about the access, so let's the access // handler check create access. - $return->orIf($this->checkCreateAccess($account, $context, $entity_bundle)); + $return = $return->orIf($this->checkCreateAccess($account, $context, $entity_bundle)); } $result = $this->setCache($return, $cid, 'create', $context['langcode'], $account); return $return_as_object ? $result : $result->isAllowed(); @@ -263,7 +263,7 @@ class EntityAccessControlHandler extends EntityHandlerBase implements EntityAcce } else { // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php index 830cf8f849b..d183fa2e9ba 100644 --- a/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php +++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php @@ -67,7 +67,7 @@ class EntityCreateAccessCheck implements AccessInterface { } // If we were unable to replace all placeholders, deny access. if (strpos($bundle, '{') !== FALSE) { - return AccessResult::create(); + return AccessResult::neutral(); } } return $this->entityManager->getAccessControlHandler($entity_type)->createAccess($bundle, $account, [], TRUE); diff --git a/core/modules/block/block.api.php b/core/modules/block/block.api.php index 43f03e9db63..954402ab982 100644 --- a/core/modules/block/block.api.php +++ b/core/modules/block/block.api.php @@ -159,7 +159,7 @@ function hook_block_access(\Drupal\block\Entity\Block $block, $operation, \Drupa } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** diff --git a/core/modules/comment/src/CommentAccessControlHandler.php b/core/modules/comment/src/CommentAccessControlHandler.php index d22a0e41ee0..b78c4b0638b 100644 --- a/core/modules/comment/src/CommentAccessControlHandler.php +++ b/core/modules/comment/src/CommentAccessControlHandler.php @@ -42,7 +42,7 @@ class CommentAccessControlHandler extends EntityAccessControlHandler { default: // No opinion. - return AccessResult::create()->cachePerRole(); + return AccessResult::neutral()->cachePerRole(); } } diff --git a/core/modules/contact/src/Access/ContactPageAccess.php b/core/modules/contact/src/Access/ContactPageAccess.php index 0a7eae74506..b0574da99dd 100644 --- a/core/modules/contact/src/Access/ContactPageAccess.php +++ b/core/modules/contact/src/Access/ContactPageAccess.php @@ -71,9 +71,10 @@ class ContactPageAccess implements AccessInterface { } // User administrators should always have access to personal contact forms. - $access = AccessResult::create()->cachePerRole(); - if ($account->hasPermission('administer users')) { - return $access->allow(); + $access = AccessResult::neutral()->cachePerRole(); + $permission_access = AccessResult::allowedIfHasPermission($account, 'administer users'); + if ($permission_access->isAllowed()) { + return $access->orIf($permission_access); } // If requested user has been blocked, do not allow users to contact them. @@ -94,7 +95,7 @@ class ContactPageAccess implements AccessInterface { return $access; } - return $access->allowIfHasPermission($account, 'access user contact forms'); + return $access->orIf(AccessResult::allowedIfHasPermission($account, 'access user contact forms')); } } diff --git a/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php index 37349b9388e..40d4ca63b10 100644 --- a/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php +++ b/core/modules/content_translation/src/Access/ContentTranslationManageAccessCheck.php @@ -106,7 +106,7 @@ class ContentTranslationManageAccessCheck implements AccessInterface { } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/content_translation/src/Access/ContentTranslationOverviewAccess.php b/core/modules/content_translation/src/Access/ContentTranslationOverviewAccess.php index 2151c8235a9..d79b7f5e9fe 100644 --- a/core/modules/content_translation/src/Access/ContentTranslationOverviewAccess.php +++ b/core/modules/content_translation/src/Access/ContentTranslationOverviewAccess.php @@ -66,7 +66,7 @@ class ContentTranslationOverviewAccess implements AccessInterface { // Check "translate any entity" permission. if ($account->hasPermission('translate any entity')) { - return $access->allow()->cachePerRole(); + return AccessResult::allowed()->cachePerRole()->inheritCacheability($access); } // Check per entity permission. @@ -74,10 +74,10 @@ class ContentTranslationOverviewAccess implements AccessInterface { if ($definition->getPermissionGranularity() == 'bundle') { $permission = "translate {$bundle} {$entity_type_id}"; } - return $access->allowIfHasPermission($account, $permission); + return AccessResult::allowedIfHasPermission($account, $permission)->inheritCacheability($access); } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/field_ui/src/Access/FormModeAccessCheck.php b/core/modules/field_ui/src/Access/FormModeAccessCheck.php index c0afbb488ef..df7edf3fbb2 100644 --- a/core/modules/field_ui/src/Access/FormModeAccessCheck.php +++ b/core/modules/field_ui/src/Access/FormModeAccessCheck.php @@ -62,6 +62,7 @@ class FormModeAccessCheck implements AccessInterface { * The access result. */ public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account, $form_mode_name = 'default', $bundle = NULL) { + $access = AccessResult::neutral(); if ($entity_type_id = $route->getDefault('entity_type_id')) { if (!isset($bundle)) { $entity_type = $this->entityManager->getDefinition($entity_type_id); @@ -76,21 +77,16 @@ class FormModeAccessCheck implements AccessInterface { $visibility = $entity_display->status(); } - $access = AccessResult::create(); if ($form_mode_name != 'default' && $entity_display) { $access->cacheUntilEntityChanges($entity_display); } if ($visibility) { $permission = $route->getRequirement('_field_ui_form_mode_access'); - $access->allowIfHasPermission($account, $permission); + $access = $access->orIf(AccessResult::allowedIfHasPermission($account, $permission)); } - return $access; - } - else { - // No opinion. - return AccessResult::create(); } + return $access; } } diff --git a/core/modules/field_ui/src/Access/ViewModeAccessCheck.php b/core/modules/field_ui/src/Access/ViewModeAccessCheck.php index 3fe035fdeea..7ab02a3d6de 100644 --- a/core/modules/field_ui/src/Access/ViewModeAccessCheck.php +++ b/core/modules/field_ui/src/Access/ViewModeAccessCheck.php @@ -62,6 +62,7 @@ class ViewModeAccessCheck implements AccessInterface { * The access result. */ public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account, $view_mode_name = 'default', $bundle = NULL) { + $access = AccessResult::neutral(); if ($entity_type_id = $route->getDefault('entity_type_id')) { if (!isset($bundle)) { $entity_type = $this->entityManager->getDefinition($entity_type_id); @@ -76,21 +77,16 @@ class ViewModeAccessCheck implements AccessInterface { $visibility = $entity_display->status(); } - $access = AccessResult::create(); if ($view_mode_name != 'default' && $entity_display) { $access->cacheUntilEntityChanges($entity_display); } if ($visibility) { $permission = $route->getRequirement('_field_ui_view_mode_access'); - $access->allowIfHasPermission($account, $permission); + $access = $access->orIf(AccessResult::allowedIfHasPermission($account, $permission)); } - return $access; - } - else { - // No opinion. - return AccessResult::create(); } + return $access; } } diff --git a/core/modules/file/src/FileAccessControlHandler.php b/core/modules/file/src/FileAccessControlHandler.php index cd87b193380..c6183dae406 100644 --- a/core/modules/file/src/FileAccessControlHandler.php +++ b/core/modules/file/src/FileAccessControlHandler.php @@ -38,7 +38,7 @@ class FileAccessControlHandler extends EntityAccessControlHandler { } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** diff --git a/core/modules/filter/src/FilterFormatAccessControlHandler.php b/core/modules/filter/src/FilterFormatAccessControlHandler.php index 025cb870941..337dcf7de46 100644 --- a/core/modules/filter/src/FilterFormatAccessControlHandler.php +++ b/core/modules/filter/src/FilterFormatAccessControlHandler.php @@ -51,7 +51,7 @@ class FilterFormatAccessControlHandler extends EntityAccessControlHandler { } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/filter/src/Tests/FilterFormatAccessTest.php b/core/modules/filter/src/Tests/FilterFormatAccessTest.php index b3d7b5556a2..580e60c0340 100644 --- a/core/modules/filter/src/Tests/FilterFormatAccessTest.php +++ b/core/modules/filter/src/Tests/FilterFormatAccessTest.php @@ -125,7 +125,7 @@ class FilterFormatAccessTest extends WebTestBase { $this->assertTrue($this->allowed_format->access('use', $this->web_user), 'A regular user has access to use a text format they were granted access to.'); $this->assertEqual(AccessResult::allowed()->cachePerRole(), $this->allowed_format->access('use', $this->web_user, TRUE), 'A regular user has access to use a text format they were granted access to.'); $this->assertFalse($this->disallowed_format->access('use', $this->web_user), 'A regular user does not have access to use a text format they were not granted access to.'); - $this->assertEqual(AccessResult::create()->cachePerRole(), $this->disallowed_format->access('use', $this->web_user, TRUE), 'A regular user does not have access to use a text format they were not granted access to.'); + $this->assertEqual(AccessResult::neutral(), $this->disallowed_format->access('use', $this->web_user, TRUE)); //, 'A regular user does not have access to use a text format they were not granted access to.'); $this->assertTrue($fallback_format->access('use', $this->web_user), 'A regular user has access to use the fallback format.'); $this->assertEqual(AccessResult::allowed(), $fallback_format->access('use', $this->web_user, TRUE), 'A regular user has access to use the fallback format.'); diff --git a/core/modules/language/src/LanguageAccessControlHandler.php b/core/modules/language/src/LanguageAccessControlHandler.php index e0d0aceb2e9..4322c178c15 100644 --- a/core/modules/language/src/LanguageAccessControlHandler.php +++ b/core/modules/language/src/LanguageAccessControlHandler.php @@ -31,7 +31,7 @@ class LanguageAccessControlHandler extends EntityAccessControlHandler { default: // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php index f562cfbb148..61245272afb 100644 --- a/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessControlHandler.php @@ -55,23 +55,21 @@ class MenuLinkContentAccessControlHandler extends EntityAccessControlHandler imp switch ($operation) { case 'view': // There is no direct view. - return AccessResult::create(); + return AccessResult::neutral(); case 'update': if (!$account->hasPermission('administer menu')) { - return AccessResult::create()->cachePerRole(); + return AccessResult::neutral()->cachePerRole(); } else { - $access = AccessResult::create()->cachePerRole()->cacheUntilEntityChanges($entity); // If there is a URL, this is an external link so always accessible. - if ($entity->getUrl()) { - return $access->allow(); - } - else { + $access = AccessResult::allowed()->cachePerRole()->cacheUntilEntityChanges($entity); + if (!$entity->getUrl()) { // We allow access, but only if the link is accessible as well. $link_access = $this->accessManager->checkNamedRoute($entity->getRouteName(), $entity->getRouteParameters(), $account, TRUE); - return $access->allow()->andIf($link_access); + return $access->andIf($link_access); } + return $access; } case 'delete': diff --git a/core/modules/node/node.api.php b/core/modules/node/node.api.php index 3fecffa4664..d42f74811da 100644 --- a/core/modules/node/node.api.php +++ b/core/modules/node/node.api.php @@ -348,7 +348,7 @@ function hook_node_access(\Drupal\node\NodeInterface $node, $op, \Drupal\Core\Se default: // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/node/node.module b/core/modules/node/node.module index 159a2308459..091d6018a28 100644 --- a/core/modules/node/node.module +++ b/core/modules/node/node.module @@ -1042,7 +1042,7 @@ function node_node_access(NodeInterface $node, $op, $account) { default: // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/node/src/Access/NodeAddAccessCheck.php b/core/modules/node/src/Access/NodeAddAccessCheck.php index 5d82dac135d..25c5557ce01 100644 --- a/core/modules/node/src/Access/NodeAddAccessCheck.php +++ b/core/modules/node/src/Access/NodeAddAccessCheck.php @@ -61,7 +61,7 @@ class NodeAddAccessCheck implements AccessInterface { } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php index 550447b59ff..0cb51c40c8e 100644 --- a/core/modules/node/src/NodeAccessControlHandler.php +++ b/core/modules/node/src/NodeAccessControlHandler.php @@ -123,7 +123,7 @@ class NodeAccessControlHandler extends EntityAccessControlHandler implements Nod } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** diff --git a/core/modules/node/src/NodeGrantDatabaseStorage.php b/core/modules/node/src/NodeGrantDatabaseStorage.php index 78b96b5ea7e..7ea330781fc 100644 --- a/core/modules/node/src/NodeGrantDatabaseStorage.php +++ b/core/modules/node/src/NodeGrantDatabaseStorage.php @@ -59,7 +59,7 @@ class NodeGrantDatabaseStorage implements NodeGrantDatabaseStorageInterface { // no point in querying the database for access grants. if (!$this->moduleHandler->getImplementations('node_grants') || !$node->id()) { // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } // Check the database for potential access grants. diff --git a/core/modules/node/tests/modules/node_access_test/node_access_test.module b/core/modules/node/tests/modules/node_access_test/node_access_test.module index fd9dd14e4be..8cc226e2219 100644 --- a/core/modules/node/tests/modules/node_access_test/node_access_test.module +++ b/core/modules/node/tests/modules/node_access_test/node_access_test.module @@ -151,5 +151,5 @@ function node_access_test_node_access(\Drupal\node\NodeInterface $node, $op, \Dr return AccessResult::forbidden()->setCacheable(FALSE); } // No opinion. - return AccessResult::create()->setCacheable(FALSE); + return AccessResult::neutral()->setCacheable(FALSE); } diff --git a/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php b/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php index 7f5c4aa0b10..7c3eaf4427c 100644 --- a/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php +++ b/core/modules/quickedit/tests/src/Unit/Access/EditEntityFieldAccessCheckTest.php @@ -49,7 +49,7 @@ class EditEntityFieldAccessCheckTest extends UnitTestCase { $non_editable_entity = $this->createMockEntity(); $non_editable_entity->expects($this->any()) ->method('access') - ->will($this->returnValue(AccessResult::create()->cachePerRole())); + ->will($this->returnValue(AccessResult::neutral()->cachePerRole())); $field_storage_with_access = $this->getMockBuilder('Drupal\field\Entity\FieldStorageConfig') ->disableOriginalConstructor() @@ -62,13 +62,13 @@ class EditEntityFieldAccessCheckTest extends UnitTestCase { ->getMock(); $field_storage_without_access->expects($this->any()) ->method('access') - ->will($this->returnValue(AccessResult::create())); + ->will($this->returnValue(AccessResult::neutral())); $data = array(); $data[] = array($editable_entity, $field_storage_with_access, AccessResult::allowed()->cachePerRole()); - $data[] = array($non_editable_entity, $field_storage_with_access, AccessResult::create()->cachePerRole()); - $data[] = array($editable_entity, $field_storage_without_access, AccessResult::create()->cachePerRole()); - $data[] = array($non_editable_entity, $field_storage_without_access, AccessResult::create()->cachePerRole()); + $data[] = array($non_editable_entity, $field_storage_with_access, AccessResult::neutral()->cachePerRole()); + $data[] = array($editable_entity, $field_storage_without_access, AccessResult::neutral()->cachePerRole()); + $data[] = array($non_editable_entity, $field_storage_without_access, AccessResult::neutral()->cachePerRole()); return $data; } diff --git a/core/modules/shortcut/shortcut.module b/core/modules/shortcut/shortcut.module index 61fc8957526..2df3a314f2a 100644 --- a/core/modules/shortcut/shortcut.module +++ b/core/modules/shortcut/shortcut.module @@ -90,12 +90,12 @@ function shortcut_set_switch_access($account = NULL) { if (!$user->hasPermission('access shortcuts')) { // The user has no permission to use shortcuts. - return AccessResult::create()->cachePerRole(); + return AccessResult::neutral()->cachePerRole(); } if (!$user->hasPermission('switch shortcut sets')) { // The user has no permission to switch anyone's shortcut set. - return AccessResult::create()->cachePerRole(); + return AccessResult::neutral()->cachePerRole(); } // Users with the 'switch shortcut sets' permission can switch their own @@ -108,7 +108,7 @@ function shortcut_set_switch_access($account = NULL) { } // No opinion. - return AccessResult::create()->cachePerRole(); + return AccessResult::neutral()->cachePerRole(); } /** diff --git a/core/modules/shortcut/src/ShortcutAccessControlHandler.php b/core/modules/shortcut/src/ShortcutAccessControlHandler.php index aee649c6b24..fa52c6004cd 100644 --- a/core/modules/shortcut/src/ShortcutAccessControlHandler.php +++ b/core/modules/shortcut/src/ShortcutAccessControlHandler.php @@ -61,7 +61,7 @@ class ShortcutAccessControlHandler extends EntityAccessControlHandler implements } // @todo Fix this bizarre code: how can a shortcut exist without a shortcut // set? The above if-test is unnecessary. See https://www.drupal.org/node/2339903. - return AccessResult::create()->cacheUntilEntityChanges($entity); + return AccessResult::neutral()->cacheUntilEntityChanges($entity); } /** @@ -73,7 +73,7 @@ class ShortcutAccessControlHandler extends EntityAccessControlHandler implements } // @todo Fix this bizarre code: how can a shortcut exist without a shortcut // set? The above if-test is unnecessary. See https://www.drupal.org/node/2339903. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php b/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php index 4968f040f6e..37da46f4853 100644 --- a/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php +++ b/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php @@ -29,7 +29,7 @@ class ShortcutSetAccessControlHandler extends EntityAccessControlHandler { return AccessResult::allowed()->cachePerRole(); } if (!$account->hasPermission('access shortcuts')) { - return AccessResult::create()->cachePerRole(); + return AccessResult::neutral()->cachePerRole(); } return AccessResult::allowedIf($account->hasPermission('customize shortcut links') && $entity == shortcut_current_displayed_set($account))->cachePerRole()->cacheUntilEntityChanges($entity); @@ -38,7 +38,7 @@ class ShortcutSetAccessControlHandler extends EntityAccessControlHandler { default: // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/system/entity.api.php b/core/modules/system/entity.api.php index 09860a614a2..18d9828c7fe 100644 --- a/core/modules/system/entity.api.php +++ b/core/modules/system/entity.api.php @@ -532,7 +532,7 @@ use Drupal\Core\Render\Element; */ function hook_entity_access(\Drupal\Core\Entity\EntityInterface $entity, $operation, \Drupal\Core\Session\AccountInterface $account, $langcode) { // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** @@ -558,7 +558,7 @@ function hook_entity_access(\Drupal\Core\Entity\EntityInterface $entity, $operat */ function hook_ENTITY_TYPE_access(\Drupal\Core\Entity\EntityInterface $entity, $operation, \Drupal\Core\Session\AccountInterface $account, $langcode) { // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** @@ -584,7 +584,7 @@ function hook_ENTITY_TYPE_access(\Drupal\Core\Entity\EntityInterface $entity, $o */ function hook_entity_create_access(\Drupal\Core\Session\AccountInterface $account, array $context, $entity_bundle) { // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** @@ -610,7 +610,7 @@ function hook_entity_create_access(\Drupal\Core\Session\AccountInterface $accoun */ function hook_ENTITY_TYPE_create_access(\Drupal\Core\Session\AccountInterface $account, array $context, $entity_bundle) { // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** @@ -1868,7 +1868,7 @@ function hook_entity_field_access_alter(array &$grants, array $context) { // don't want to switch node module's grant to // AccessResultInterface::isAllowed() , because the grants of other modules // should still decide on their own if this field is accessible or not - $grants['node']->resetAccess(); + $grants['node'] = AccessResult::neutral()->inheritCacheability($grants['node']); } } diff --git a/core/modules/system/tests/modules/entity_test/entity_test.module b/core/modules/system/tests/modules/entity_test/entity_test.module index fd9d29420e9..bf0cded921c 100644 --- a/core/modules/system/tests/modules/entity_test/entity_test.module +++ b/core/modules/system/tests/modules/entity_test/entity_test.module @@ -331,7 +331,7 @@ function entity_test_entity_field_access($operation, FieldDefinitionInterface $f } } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** @@ -341,7 +341,7 @@ function entity_test_entity_field_access($operation, FieldDefinitionInterface $f */ function entity_test_entity_field_access_alter(array &$grants, array $context) { if ($context['field_definition']->getName() == 'field_test_text' && $context['items'][0]->value == 'access alter value') { - $grants[':default']->forbid()->cacheUntilEntityChanges($context['items']->getEntity()); + $grants[':default'] = AccessResult::forbidden()->inheritCacheability($grants[':default'])->cacheUntilEntityChanges($context['items']->getEntity()); } } @@ -488,7 +488,7 @@ function entity_test_entity_test_access(EntityInterface $entity, $operation, Acc \Drupal::state()->set('entity_test_entity_test_access', TRUE); // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** @@ -498,7 +498,7 @@ function entity_test_entity_create_access(AccountInterface $account, $context, $ \Drupal::state()->set('entity_test_entity_create_access', TRUE); // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } /** @@ -508,5 +508,5 @@ function entity_test_entity_test_create_access(AccountInterface $account, $conte \Drupal::state()->set('entity_test_entity_test_create_access', TRUE); // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } diff --git a/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php b/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php index 95efd80a983..2ae39ea556f 100644 --- a/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php +++ b/core/modules/system/tests/modules/entity_test/src/EntityTestAccessControlHandler.php @@ -41,7 +41,7 @@ class EntityTestAccessControlHandler extends EntityAccessControlHandler { } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } diff --git a/core/modules/system/tests/modules/router_test_directory/src/Access/DefinedTestAccessCheck.php b/core/modules/system/tests/modules/router_test_directory/src/Access/DefinedTestAccessCheck.php index b435a398e8b..9a362ae90cd 100644 --- a/core/modules/system/tests/modules/router_test_directory/src/Access/DefinedTestAccessCheck.php +++ b/core/modules/system/tests/modules/router_test_directory/src/Access/DefinedTestAccessCheck.php @@ -33,7 +33,7 @@ class DefinedTestAccessCheck implements AccessInterface { return AccessResult::forbidden(); } else { - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/system/tests/modules/router_test_directory/src/Access/TestAccessCheck.php b/core/modules/system/tests/modules/router_test_directory/src/Access/TestAccessCheck.php index 45ab4b5237f..d4686bfd193 100644 --- a/core/modules/system/tests/modules/router_test_directory/src/Access/TestAccessCheck.php +++ b/core/modules/system/tests/modules/router_test_directory/src/Access/TestAccessCheck.php @@ -24,6 +24,6 @@ class TestAccessCheck implements AccessInterface { public function access() { // No opinion, so other access checks should decide if access should be // allowed or not. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/taxonomy/src/TermAccessControlHandler.php b/core/modules/taxonomy/src/TermAccessControlHandler.php index 2ecd3a6f967..a4783731c0b 100644 --- a/core/modules/taxonomy/src/TermAccessControlHandler.php +++ b/core/modules/taxonomy/src/TermAccessControlHandler.php @@ -38,7 +38,7 @@ class TermAccessControlHandler extends EntityAccessControlHandler { default: // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/modules/user/src/Access/RoleAccessCheck.php b/core/modules/user/src/Access/RoleAccessCheck.php index ebfaa808a40..7d7b3062f9d 100644 --- a/core/modules/user/src/Access/RoleAccessCheck.php +++ b/core/modules/user/src/Access/RoleAccessCheck.php @@ -52,7 +52,7 @@ class RoleAccessCheck implements AccessInterface { } // If there is no allowed role, give other access checks a chance. - return AccessResult::create()->cachePerRole(); + return AccessResult::neutral()->cachePerRole(); } } diff --git a/core/modules/user/src/UserAccessControlHandler.php b/core/modules/user/src/UserAccessControlHandler.php index aab5f3bcc82..49a76203159 100644 --- a/core/modules/user/src/UserAccessControlHandler.php +++ b/core/modules/user/src/UserAccessControlHandler.php @@ -57,7 +57,7 @@ class UserAccessControlHandler extends EntityAccessControlHandler { } // No opinion. - return AccessResult::create(); + return AccessResult::neutral(); } } diff --git a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php index 717e8fc8d09..3a8c4c3af86 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php @@ -186,7 +186,7 @@ class AccessManagerTest extends UnitTestCase { // Check route access without any access checker defined yet. foreach ($route_matches as $route_match) { $this->assertEquals(FALSE, $this->accessManager->check($route_match, $this->account)); - $this->assertEquals(AccessResult::create(), $this->accessManager->check($route_match, $this->account, NULL, TRUE)); + $this->assertEquals(AccessResult::neutral(), $this->accessManager->check($route_match, $this->account, NULL, TRUE)); } $this->setupAccessChecker(); @@ -195,7 +195,7 @@ class AccessManagerTest extends UnitTestCase { // setChecks. foreach ($route_matches as $route_match) { $this->assertEquals(FALSE, $this->accessManager->check($route_match, $this->account)); - $this->assertEquals(AccessResult::create(), $this->accessManager->check($route_match, $this->account, NULL, TRUE)); + $this->assertEquals(AccessResult::neutral(), $this->accessManager->check($route_match, $this->account, NULL, TRUE)); } // Now applicable access checks have been saved on each route object. @@ -206,7 +206,7 @@ class AccessManagerTest extends UnitTestCase { $this->assertEquals(TRUE, $this->accessManager->check($route_matches['test_route_2'], $this->account)); $this->assertEquals(FALSE, $this->accessManager->check($route_matches['test_route_3'], $this->account)); $this->assertEquals(TRUE, $this->accessManager->check($route_matches['test_route_4'], $this->account)); - $this->assertEquals(AccessResult::create(), $this->accessManager->check($route_matches['test_route_1'], $this->account, NULL, TRUE)); + $this->assertEquals(AccessResult::neutral(), $this->accessManager->check($route_matches['test_route_1'], $this->account, NULL, TRUE)); $this->assertEquals(AccessResult::allowed(), $this->accessManager->check($route_matches['test_route_2'], $this->account, NULL, TRUE)); $this->assertEquals(AccessResult::forbidden(), $this->accessManager->check($route_matches['test_route_3'], $this->account, NULL, TRUE)); $this->assertEquals(AccessResult::allowed(), $this->accessManager->check($route_matches['test_route_4'], $this->account, NULL, TRUE)); @@ -242,7 +242,7 @@ class AccessManagerTest extends UnitTestCase { */ public function providerTestCheckConjunctions() { $access_allow = AccessResult::allowed(); - $access_deny = AccessResult::create(); + $access_deny = AccessResult::neutral(); $access_kill = AccessResult::forbidden(); $access_configurations = array(); diff --git a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php index 67d25d5be41..15b8599317a 100644 --- a/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php +++ b/core/tests/Drupal/Tests/Core/Access/AccessResultTest.php @@ -8,7 +8,10 @@ namespace Drupal\Tests\Core\Access; use Drupal\Core\Access\AccessResult; +use Drupal\Core\Access\AccessResultInterface; +use Drupal\Core\Access\AccessResultNeutral; use Drupal\Core\Cache\Cache; +use Drupal\Core\Cache\CacheableInterface; use Drupal\Tests\UnitTestCase; /** @@ -36,15 +39,16 @@ class AccessResultTest extends UnitTestCase { $verify = function (AccessResult $access) { $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertTrue($access->isNeutral()); $this->assertDefaultCacheability($access); }; // Verify the object when using the constructor. - $a = new AccessResult(); + $a = new AccessResultNeutral(); $verify($a); // Verify the object when using the ::create() convenience method. - $b = AccessResult::create(); + $b = AccessResult::neutral(); $verify($b); $this->assertEquals($a, $b); @@ -55,23 +59,19 @@ class AccessResultTest extends UnitTestCase { * @covers ::allowed * @covers ::isAllowed * @covers ::isForbidden + * @covers ::isNeutral */ public function testAccessAllowed() { $verify = function (AccessResult $access) { $this->assertTrue($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); }; - // Verify the object when using the ::allow() instance method. - $a = AccessResult::create()->allow(); - $verify($a); - // Verify the object when using the ::allowed() convenience static method. $b = AccessResult::allowed(); $verify($b); - - $this->assertEquals($a, $b); } /** @@ -79,44 +79,19 @@ class AccessResultTest extends UnitTestCase { * @covers ::forbidden * @covers ::isAllowed * @covers ::isForbidden + * @covers ::isNeutral */ public function testAccessForbidden() { $verify = function (AccessResult $access) { $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); }; - // Verify the object when using the ::forbid() instance method. - $a = AccessResult::create()->forbid(); - $verify($a); - // Verify the object when using the ::forbidden() convenience static method. $b = AccessResult::forbidden(); $verify($b); - - $this->assertEquals($a, $b); - } - - /** - * @covers ::reset - * @covers ::isAllowed - * @covers ::isForbidden - */ - public function testAccessReset() { - $verify = function (AccessResult $access) { - $this->assertFalse($access->isAllowed()); - $this->assertFalse($access->isForbidden()); - $this->assertDefaultCacheability($access); - }; - - $a = AccessResult::allowed()->resetAccess(); - $verify($a); - - $b = AccessResult::forbidden()->resetAccess(); - $verify($b); - - $this->assertEquals($a, $b); } /** @@ -124,34 +99,20 @@ class AccessResultTest extends UnitTestCase { * @covers ::allowedIf * @covers ::isAllowed * @covers ::isForbidden + * @covers ::isNeutral */ public function testAccessConditionallyAllowed() { - $verify = function (AccessResult $access, $allowed, $forbidden = FALSE) { + $verify = function (AccessResult $access, $allowed) { $this->assertSame($allowed, $access->isAllowed()); - $this->assertSame($forbidden, $access->isForbidden()); + $this->assertFalse($access->isForbidden()); + $this->assertSame(!$allowed, $access->isNeutral()); $this->assertDefaultCacheability($access); }; - // Verify the object when using the ::allowIf() instance method. - $a1 = AccessResult::create()->allowIf(TRUE); - $verify($a1, TRUE); - $a2 = AccessResult::create()->allowIf(FALSE); - $verify($a2, FALSE); - $b1 = AccessResult::allowedIf(TRUE); $verify($b1, TRUE); $b2 = AccessResult::allowedIf(FALSE); $verify($b2, FALSE); - - $this->assertEquals($a1, $b1); - $this->assertEquals($a2, $b2); - - // Verify that ::allowIf() does not overwrite an existing value when the - // condition does not evaluate to TRUE. - $a1 = AccessResult::forbidden()->allowIf(TRUE); - $verify($a1, TRUE); - $a2 = AccessResult::forbidden()->allowIf(FALSE); - $verify($a2, FALSE, TRUE); } /** @@ -159,80 +120,101 @@ class AccessResultTest extends UnitTestCase { * @covers ::forbiddenIf * @covers ::isAllowed * @covers ::isForbidden + * @covers ::isNeutral */ public function testAccessConditionallyForbidden() { - $verify = function (AccessResult $access, $forbidden, $allowed = FALSE) { - $this->assertSame($allowed, $access->isAllowed()); + $verify = function (AccessResult $access, $forbidden) { + $this->assertFalse($access->isAllowed()); $this->assertSame($forbidden, $access->isForbidden()); + $this->assertSame(!$forbidden, $access->isNeutral()); $this->assertDefaultCacheability($access); }; - // Verify the object when using the ::allowIf() instance method. - $a1 = AccessResult::create()->forbidIf(TRUE); - $verify($a1, TRUE); - $a2 = AccessResult::create()->forbidIf(FALSE); - $verify($a2, FALSE); - $b1 = AccessResult::forbiddenIf(TRUE); $verify($b1, TRUE); $b2 = AccessResult::forbiddenIf(FALSE); $verify($b2, FALSE); - - $this->assertEquals($a1, $b1); - $this->assertEquals($a2, $b2); - - // Verify that ::forbidIf() does not overwrite an existing value when the - // condition does not evaluate to TRUE. - $a1 = AccessResult::allowed()->forbidIf(TRUE); - $verify($a1, TRUE); - $a2 = AccessResult::allowed()->forbidIf(FALSE); - $verify($a2, FALSE, TRUE); } /** * @covers ::andIf */ public function testAndIf() { - $no_opinion = AccessResult::create(); + $neutral = AccessResult::neutral(); $allowed = AccessResult::allowed(); $forbidden = AccessResult::forbidden(); $unused_access_result_due_to_lazy_evaluation = $this->getMock('\Drupal\Core\Access\AccessResultInterface'); $unused_access_result_due_to_lazy_evaluation->expects($this->never()) ->method($this->anything()); - // ALLOW && ALLOW === ALLOW. - $access = clone $allowed; - $access->andIf($allowed); + // ALLOWED && ALLOWED === ALLOWED. + $access = $allowed->andIf($allowed); $this->assertTrue($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); - // ALLOW && DENY === DENY. - $access = clone $allowed; - $access->andIf($no_opinion); + // ALLOWED && NEUTRAL === NEUTRAL. + $access = $allowed->andIf($neutral); $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertTrue($access->isNeutral()); $this->assertDefaultCacheability($access); - // ALLOW && KILL === KILL. - $access = clone $allowed; - $access->andIf($forbidden); + // ALLOWED && FORBIDDEN === FORBIDDEN. + $access = $allowed->andIf($forbidden); $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); - // DENY && * === DENY. - $access = clone $no_opinion; - $access->andIf($unused_access_result_due_to_lazy_evaluation); + // NEUTRAL && ALLOW == NEUTRAL + $access = $neutral->andIf($allowed); $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertTrue($access->isNeutral()); $this->assertDefaultCacheability($access); - // KILL && * === KILL. - $access = clone $forbidden; - $access->andIf($unused_access_result_due_to_lazy_evaluation); + // NEUTRAL && NEUTRAL === NEUTRAL. + $access = $neutral->andIf($neutral); + $this->assertFalse($access->isAllowed()); + $this->assertFalse($access->isForbidden()); + $this->assertTrue($access->isNeutral()); + $this->assertDefaultCacheability($access); + + // NEUTRAL && FORBIDDEN === FORBIDDEN. + $access = $neutral->andIf($forbidden); $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); + $this->assertDefaultCacheability($access); + + // FORBIDDEN && ALLOWED = FORBIDDEN + $access = $forbidden->andif($allowed); + $this->assertFalse($access->isAllowed()); + $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); + $this->assertDefaultCacheability($access); + + // FORBIDDEN && NEUTRAL = FORBIDDEN + $access = $forbidden->andif($neutral); + $this->assertFalse($access->isAllowed()); + $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); + $this->assertDefaultCacheability($access); + + // FORBIDDEN && FORBIDDEN = FORBIDDEN + $access = $forbidden->andif($forbidden); + $this->assertFalse($access->isAllowed()); + $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); + $this->assertDefaultCacheability($access); + + // FORBIDDEN && * === FORBIDDEN: lazy evaluation verification. + $access = $forbidden->andIf($unused_access_result_due_to_lazy_evaluation); + $this->assertFalse($access->isAllowed()); + $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); } @@ -240,60 +222,81 @@ class AccessResultTest extends UnitTestCase { * @covers ::orIf */ public function testOrIf() { - $no_opinion = AccessResult::create(); + $neutral = AccessResult::neutral(); $allowed = AccessResult::allowed(); $forbidden = AccessResult::forbidden(); $unused_access_result_due_to_lazy_evaluation = $this->getMock('\Drupal\Core\Access\AccessResultInterface'); $unused_access_result_due_to_lazy_evaluation->expects($this->never()) ->method($this->anything()); - // ALLOW || ALLOW === ALLOW. - $access = clone $allowed; - $access->orIf($allowed); + // ALLOWED || ALLOWED === ALLOWED. + $access = $allowed->orIf($allowed); $this->assertTrue($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); - // ALLOW || DENY === ALLOW. - $access = clone $allowed; - $access->orIf($no_opinion); + // ALLOWED || NEUTRAL === ALLOWED. + $access = $allowed->orIf($neutral); $this->assertTrue($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); - // ALLOW || KILL === KILL. - $access = clone $allowed; - $access->orIf($forbidden); + // ALLOWED || FORBIDDEN === FORBIDDEN. + $access = $allowed->orIf($forbidden); $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); - // DENY || DENY === DENY. - $access = clone $no_opinion; - $access->orIf($no_opinion); + // NEUTRAL || NEUTRAL === NEUTRAL. + $access = $neutral->orIf($neutral); $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertTrue($access->isNeutral()); $this->assertDefaultCacheability($access); - // DENY || ALLOW === ALLOW. - $access = clone $no_opinion; - $access->orIf($allowed); + // NEUTRAL || ALLOWED === ALLOWED. + $access = $neutral->orIf($allowed); $this->assertTrue($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); - // DENY || KILL === KILL. - $access = clone $no_opinion; - $access->orIf($forbidden); + // NEUTRAL || FORBIDDEN === FORBIDDEN. + $access = $neutral->orIf($forbidden); $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); - // KILL || * === KILL. - $access = clone $forbidden; - $access->orIf($unused_access_result_due_to_lazy_evaluation); + // FORBIDDEN || ALLOWED === FORBIDDEN. + $access = $forbidden->orIf($allowed); $this->assertFalse($access->isAllowed()); $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); + $this->assertDefaultCacheability($access); + + // FORBIDDEN || NEUTRAL === FORBIDDEN. + $access = $forbidden->orIf($allowed); + $this->assertFalse($access->isAllowed()); + $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); + $this->assertDefaultCacheability($access); + + // FORBIDDEN || FORBIDDEN === FORBIDDEN. + $access = $forbidden->orIf($allowed); + $this->assertFalse($access->isAllowed()); + $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); + $this->assertDefaultCacheability($access); + + // FORBIDDEN || * === FORBIDDEN. + $access = $forbidden->orIf($unused_access_result_due_to_lazy_evaluation); + $this->assertFalse($access->isAllowed()); + $this->assertTrue($access->isForbidden()); + $this->assertFalse($access->isNeutral()); $this->assertDefaultCacheability($access); } @@ -302,9 +305,9 @@ class AccessResultTest extends UnitTestCase { * @covers ::isCacheable */ public function testCacheable() { - $this->assertTrue(AccessResult::create()->isCacheable()); - $this->assertTrue(AccessResult::create()->setCacheable(TRUE)->isCacheable()); - $this->assertFalse(AccessResult::create()->setCacheable(FALSE)->isCacheable()); + $this->assertTrue(AccessResult::neutral()->isCacheable()); + $this->assertTrue(AccessResult::neutral()->setCacheable(TRUE)->isCacheable()); + $this->assertFalse(AccessResult::neutral()->setCacheable(FALSE)->isCacheable()); } /** @@ -312,8 +315,8 @@ class AccessResultTest extends UnitTestCase { * @covers ::getCacheMaxAge */ public function testCacheMaxAge() { - $this->assertSame(Cache::PERMANENT, AccessResult::create()->getCacheMaxAge()); - $this->assertSame(1337, AccessResult::create()->setCacheMaxAge(1337)->getCacheMaxAge()); + $this->assertSame(Cache::PERMANENT, AccessResult::neutral()->getCacheMaxAge()); + $this->assertSame(1337, AccessResult::neutral()->setCacheMaxAge(1337)->getCacheMaxAge()); } /** @@ -329,6 +332,7 @@ class AccessResultTest extends UnitTestCase { $verify = function (AccessResult $access, array $contexts) { $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertTrue($access->isNeutral()); $this->assertTrue($access->isCacheable()); $this->assertSame('default', $access->getCacheBin()); $this->assertSame(Cache::PERMANENT, $access->getCacheMaxAge()); @@ -336,7 +340,7 @@ class AccessResultTest extends UnitTestCase { $this->assertSame([], $access->getCacheTags()); }; - $access = AccessResult::create()->addCacheContexts(['cache_context.foo']); + $access = AccessResult::neutral()->addCacheContexts(['cache_context.foo']); $verify($access, ['cache_context.foo']); // Verify resetting works. $access->resetCacheContexts(); @@ -357,27 +361,27 @@ class AccessResultTest extends UnitTestCase { // ::cachePerRole() convenience method. $contexts = array('cache_context.user.roles'); - $a = AccessResult::create()->addCacheContexts($contexts); + $a = AccessResult::neutral()->addCacheContexts($contexts); $verify($a, $contexts); - $b = AccessResult::create()->cachePerRole(); + $b = AccessResult::neutral()->cachePerRole(); $verify($b, $contexts); $this->assertEquals($a, $b); // ::cachePerUser() convenience method. $contexts = array('cache_context.user'); - $a = AccessResult::create()->addCacheContexts($contexts); + $a = AccessResult::neutral()->addCacheContexts($contexts); $verify($a, $contexts); - $b = AccessResult::create()->cachePerUser(); + $b = AccessResult::neutral()->cachePerUser(); $verify($b, $contexts); $this->assertEquals($a, $b); // Both. $contexts = array('cache_context.user', 'cache_context.user.roles'); - $a = AccessResult::create()->addCacheContexts($contexts); + $a = AccessResult::neutral()->addCacheContexts($contexts); $verify($a, $contexts); - $b = AccessResult::create()->cachePerRole()->cachePerUser(); + $b = AccessResult::neutral()->cachePerRole()->cachePerUser(); $verify($b, $contexts); - $c = AccessResult::create()->cachePerUser()->cachePerRole(); + $c = AccessResult::neutral()->cachePerUser()->cachePerRole(); $verify($c, $contexts); $this->assertEquals($a, $b); $this->assertEquals($a, $c); @@ -390,17 +394,10 @@ class AccessResultTest extends UnitTestCase { ->will($this->returnValue(FALSE)); $contexts = array('cache_context.user.roles'); - // Verify the object when using the ::allowIfHasPermission() convenience - // instance method. - $a = AccessResult::create()->allowIfHasPermission($account, 'may herd llamas'); - $verify($a, $contexts); - // Verify the object when using the ::allowedIfHasPermission() convenience // static method. $b = AccessResult::allowedIfHasPermission($account, 'may herd llamas'); $verify($b, $contexts); - - $this->assertEquals($a, $b); } /** @@ -413,6 +410,7 @@ class AccessResultTest extends UnitTestCase { $verify = function (AccessResult $access, array $tags) { $this->assertFalse($access->isAllowed()); $this->assertFalse($access->isForbidden()); + $this->assertTrue($access->isNeutral()); $this->assertTrue($access->isCacheable()); $this->assertSame('default', $access->getCacheBin()); $this->assertSame(Cache::PERMANENT, $access->getCacheMaxAge()); @@ -420,7 +418,7 @@ class AccessResultTest extends UnitTestCase { $this->assertSame($tags, $access->getCacheTags()); }; - $access = AccessResult::create()->addCacheTags(['foo' => ['bar']]); + $access = AccessResult::neutral()->addCacheTags(['foo' => ['bar']]); $verify($access, ['foo' => ['bar' => 'bar']]); // Verify resetting works. $access->resetCacheTags(); @@ -458,35 +456,21 @@ class AccessResultTest extends UnitTestCase { ->method('getCacheTag') ->will($this->returnValue(array('node' => array(20011988)))); $tags = array('node' => array(20011988 => 20011988)); - $a = AccessResult::create()->addCacheTags($tags); + $a = AccessResult::neutral()->addCacheTags($tags); $verify($a, $tags); - $b = AccessResult::create()->cacheUntilEntityChanges($node); + $b = AccessResult::neutral()->cacheUntilEntityChanges($node); $verify($b, $tags); $this->assertEquals($a, $b); } /** - * @covers ::andIf - * @covers ::orIf - * @covers ::mergeCacheabilityMetadata + * @covers ::inheritCacheability */ - public function testCacheabilityMerging() { - $access_without_cacheability = $this->getMock('\Drupal\Core\Access\AccessResultInterface'); - $access_without_cacheability->expects($this->exactly(2)) - ->method('isAllowed') - ->willReturn(TRUE); - $access_without_cacheability->expects($this->exactly(2)) - ->method('isForbidden') - ->willReturn(FALSE); - $access_without_cacheability->expects($this->once()) - ->method('andIf') - ->will($this->returnSelf()); - + public function testInheritCacheability() { // andIf(); 1st has defaults, 2nd has custom tags, contexts and max-age. - $access = AccessResult::allowed() - ->andIf(AccessResult::allowed()->setCacheMaxAge(1500)->cachePerRole()->addCacheTags(['node' => [20011988]])); - $this->assertTrue($access->isAllowed()); - $this->assertFalse($access->isForbidden()); + $access = AccessResult::allowed(); + $other = AccessResult::allowed()->setCacheMaxAge(1500)->cachePerRole()->addCacheTags(['node' => [20011988]]); + $this->assertTrue($access->inheritCacheability($other) instanceof AccessResult); $this->assertTrue($access->isCacheable()); $this->assertSame(['cache_context.user.roles'], $access->getCacheKeys()); $this->assertSame(['node' => [20011988 => 20011988]], $access->getCacheTags()); @@ -494,39 +478,410 @@ class AccessResultTest extends UnitTestCase { $this->assertSame(1500, $access->getCacheMaxAge()); // andIf(); 1st has custom tags, max-age, 2nd has custom contexts and max-age. - $access = AccessResult::allowed()->cachePerUser()->setCacheMaxAge(43200) - ->andIf(AccessResult::forbidden()->addCacheTags(['node' => [14031991]])->setCacheMaxAge(86400)); - $this->assertFalse($access->isAllowed()); - $this->assertTrue($access->isForbidden()); + $access = AccessResult::allowed()->cachePerUser()->setCacheMaxAge(43200); + $other = AccessResult::forbidden()->addCacheTags(['node' => [14031991]])->setCacheMaxAge(86400); + $this->assertTrue($access->inheritCacheability($other) instanceof AccessResult); $this->assertTrue($access->isCacheable()); $this->assertSame(['cache_context.user'], $access->getCacheKeys()); $this->assertSame(['node' => [14031991 => 14031991]], $access->getCacheTags()); $this->assertSame('default', $access->getCacheBin()); $this->assertSame(43200, $access->getCacheMaxAge()); + } - // orIf(); 1st is cacheable, 2nd isn't. - $access = AccessResult::allowed()->orIf(AccessResult::allowed()->setCacheable(FALSE)); - $this->assertTrue($access->isAllowed()); - $this->assertFalse($access->isForbidden()); - $this->assertFalse($access->isCacheable()); + /** + * Provides a list of access result pairs and operations to test. + * + * This tests the propagation of cacheability metadata. Rather than testing + * every single bit of cacheability metadata, which would lead to a mind- + * boggling number of permutations, in this test, we only consider the + * permutations of all pairs of the following set: + * 1. Allowed, implements cacheable interface, is cacheable + * 2. Allowed, implements cacheable interface, is not cacheable + * 3. Allowed, does not implement cacheable interface (hence not cacheable) + * 4. Forbidden, implements cacheable interface, is cacheable + * 5. Forbidden, implements cacheable interface, is not cacheable + * 6. Forbidden, does not implement cacheable interface (hence not cacheable) + * 7. Neutral, implements cacheable interface, is cacheable + * 8. Neutral, implements cacheable interface, is not cacheable + * 9. Neutral, does not implement cacheable interface (hence not cacheable) + * + * This leads to 72 permutations (9!/(9-2)! = 9*8 = 72) per operation. There + * are two operations to test (AND and OR), so that leads to a grand total of + * 144 permutations, all of which are tested. + * + * There are two "contagious" patterns: + * 1. Any operation with a forbidden access result yields a forbidden result. + * This therefore also applies to the cacheability metadata associated with + * a forbidden result. + * This is the case for items 4, 5 and 6 in the set above. + * 2. Any operation yields an access result object that is of the same class + * (implementation) as the first operand. This is because operations are + * invoked on the first operand. Therefore, if the first implementation + * does not implement CacheableInterface, then the result won't either. + * This is the case for items 3, 6 and 9 in the set above. + */ + public function andOrCacheabilityPropagationProvider() { + // ct: cacheable=true, cf: cacheable=false, un: uncacheable. + // Note: the test cases that have a "un" access result as the first operand + // test UncacheableTestAccessResult, not AccessResult. However, we + // definitely want to verify that AccessResult's orIf() and andIf() methods + // work correctly when given an AccessResultInterface implementation that + // does not implement CacheableInterface, and we want to test the full gamut + // of permutations, so that's not a problem. + $allowed_ct = AccessResult::allowed(); + $allowed_cf = AccessResult::allowed()->setCacheable(FALSE); + $allowed_un = new UncacheableTestAccessResult('ALLOWED'); + $forbidden_ct = AccessResult::forbidden(); + $forbidden_cf = AccessResult::forbidden()->setCacheable(FALSE); + $forbidden_un = new UncacheableTestAccessResult('FORBIDDEN'); + $neutral_ct = AccessResult::neutral(); + $neutral_cf = AccessResult::neutral()->setCacheable(FALSE); + $neutral_un = new UncacheableTestAccessResult('NEUTRAL'); - // andIf(); 1st is cacheable, 2nd isn't. - $access = AccessResult::allowed()->andIf(AccessResult::allowed()->setCacheable(FALSE)); - $this->assertTrue($access->isAllowed()); - $this->assertFalse($access->isForbidden()); - $this->assertFalse($access->isCacheable()); + // Structure: + // - First column: first access result. + // - Second column: operator ('OR' or 'AND'). + // - Third column: second access result. + // - Fourth column: whether the result implements CacheableInterface + // - Fifth column: whether the result is cacheable (if column 4 is TRUE) + return [ + // Allowed (ct) OR allowed (ct,cf,un). + [$allowed_ct, 'OR', $allowed_ct, TRUE, TRUE], + [$allowed_ct, 'OR', $allowed_cf, TRUE, TRUE], + [$allowed_ct, 'OR', $allowed_un, TRUE, TRUE], + // Allowed (cf) OR allowed (ct,cf,un). + [$allowed_cf, 'OR', $allowed_ct, TRUE, TRUE], + [$allowed_cf, 'OR', $allowed_cf, TRUE, FALSE], + [$allowed_cf, 'OR', $allowed_un, TRUE, FALSE], + // Allowed (un) OR allowed (ct,cf,un). + [$allowed_un, 'OR', $allowed_ct, FALSE, NULL], + [$allowed_un, 'OR', $allowed_cf, FALSE, NULL], + [$allowed_un, 'OR', $allowed_un, FALSE, NULL], - // andIf(); 1st implements CacheableInterface, 2nd doesn't. - $access = AccessResult::allowed()->andIf($access_without_cacheability); - $this->assertTrue($access->isAllowed()); - $this->assertFalse($access->isForbidden()); - $this->assertFalse($access->isCacheable()); + // Allowed (ct) OR forbidden (ct,cf,un). + [$allowed_ct, 'OR', $forbidden_ct, TRUE, TRUE], + [$allowed_ct, 'OR', $forbidden_cf, TRUE, FALSE], + [$allowed_ct, 'OR', $forbidden_un, TRUE, FALSE], + // Allowed (cf) OR forbidden (ct,cf,un). + [$allowed_cf, 'OR', $forbidden_ct, TRUE, TRUE], + [$allowed_cf, 'OR', $forbidden_cf, TRUE, FALSE], + [$allowed_cf, 'OR', $forbidden_un, TRUE, FALSE], + // Allowed (un) OR forbidden (ct,cf,un). + [$allowed_un, 'OR', $forbidden_ct, FALSE, NULL], + [$allowed_un, 'OR', $forbidden_cf, FALSE, NULL], + [$allowed_un, 'OR', $forbidden_un, FALSE, NULL], - // andIf(); 1st doesn't implement CacheableInterface, 2nd does. - $access = $access_without_cacheability->andIf(AccessResult::allowed()); - $this->assertTrue($access->isAllowed()); - $this->assertFalse($access->isForbidden()); - $this->assertFalse(method_exists($access, 'isCacheable')); + // Allowed (ct) OR neutral (ct,cf,un). + [$allowed_ct, 'OR', $neutral_ct, TRUE, TRUE], + [$allowed_ct, 'OR', $neutral_cf, TRUE, TRUE], + [$allowed_ct, 'OR', $neutral_un, TRUE, TRUE], + // Allowed (cf) OR neutral (ct,cf,un). + [$allowed_cf, 'OR', $neutral_ct, TRUE, FALSE], + [$allowed_cf, 'OR', $neutral_cf, TRUE, FALSE], + [$allowed_cf, 'OR', $neutral_un, TRUE, FALSE], + // Allowed (un) OR neutral (ct,cf,un). + [$allowed_un, 'OR', $neutral_ct, FALSE, NULL], + [$allowed_un, 'OR', $neutral_cf, FALSE, NULL], + [$allowed_un, 'OR', $neutral_un, FALSE, NULL], + + + // Forbidden (ct) OR allowed (ct,cf,un). + [$forbidden_ct, 'OR', $allowed_ct, TRUE, TRUE], + [$forbidden_ct, 'OR', $allowed_cf, TRUE, TRUE], + [$forbidden_ct, 'OR', $allowed_un, TRUE, TRUE], + // Forbidden (cf) OR allowed (ct,cf,un). + [$forbidden_cf, 'OR', $allowed_ct, TRUE, FALSE], + [$forbidden_cf, 'OR', $allowed_cf, TRUE, FALSE], + [$forbidden_cf, 'OR', $allowed_un, TRUE, FALSE], + // Forbidden (un) OR allowed (ct,cf,un). + [$forbidden_un, 'OR', $allowed_ct, FALSE, NULL], + [$forbidden_un, 'OR', $allowed_cf, FALSE, NULL], + [$forbidden_un, 'OR', $allowed_un, FALSE, NULL], + + // Forbidden (ct) OR neutral (ct,cf,un). + [$forbidden_ct, 'OR', $neutral_ct, TRUE, TRUE], + [$forbidden_ct, 'OR', $neutral_cf, TRUE, TRUE], + [$forbidden_ct, 'OR', $neutral_un, TRUE, TRUE], + // Forbidden (cf) OR neutral (ct,cf,un). + [$forbidden_cf, 'OR', $neutral_ct, TRUE, FALSE], + [$forbidden_cf, 'OR', $neutral_cf, TRUE, FALSE], + [$forbidden_cf, 'OR', $neutral_un, TRUE, FALSE], + // Forbidden (un) OR neutral (ct,cf,un). + [$forbidden_un, 'OR', $neutral_ct, FALSE, NULL], + [$forbidden_un, 'OR', $neutral_cf, FALSE, NULL], + [$forbidden_un, 'OR', $neutral_un, FALSE, NULL], + + // Forbidden (ct) OR forbidden (ct,cf,un). + [$forbidden_ct, 'OR', $forbidden_ct, TRUE, TRUE], + [$forbidden_ct, 'OR', $forbidden_cf, TRUE, TRUE], + [$forbidden_ct, 'OR', $forbidden_un, TRUE, TRUE], + // Forbidden (cf) OR forbidden (ct,cf,un). + [$forbidden_cf, 'OR', $forbidden_ct, TRUE, TRUE], + [$forbidden_cf, 'OR', $forbidden_cf, TRUE, FALSE], + [$forbidden_cf, 'OR', $forbidden_un, TRUE, FALSE], + // Forbidden (un) OR forbidden (ct,cf,un). + [$forbidden_un, 'OR', $forbidden_ct, FALSE, NULL], + [$forbidden_un, 'OR', $forbidden_cf, FALSE, NULL], + [$forbidden_un, 'OR', $forbidden_un, FALSE, NULL], + + + // Neutral (ct) OR allowed (ct,cf,un). + [$neutral_ct, 'OR', $allowed_ct, TRUE, TRUE], + [$neutral_ct, 'OR', $allowed_cf, TRUE, FALSE], + [$neutral_ct, 'OR', $allowed_un, TRUE, FALSE], + // Neutral (cf) OR allowed (ct,cf,un). + [$neutral_cf, 'OR', $allowed_ct, TRUE, TRUE], + [$neutral_cf, 'OR', $allowed_cf, TRUE, FALSE], + [$neutral_cf, 'OR', $allowed_un, TRUE, FALSE], + // Neutral (un) OR allowed (ct,cf,un). + [$neutral_un, 'OR', $allowed_ct, FALSE, NULL], + [$neutral_un, 'OR', $allowed_cf, FALSE, NULL], + [$neutral_un, 'OR', $allowed_un, FALSE, NULL], + + // Neutral (ct) OR neutral (ct,cf,un). + [$neutral_ct, 'OR', $neutral_ct, TRUE, TRUE], + [$neutral_ct, 'OR', $neutral_cf, TRUE, TRUE], + [$neutral_ct, 'OR', $neutral_un, TRUE, TRUE], + // Neutral (cf) OR neutral (ct,cf,un). + [$neutral_cf, 'OR', $neutral_ct, TRUE, TRUE], + [$neutral_cf, 'OR', $neutral_cf, TRUE, FALSE], + [$neutral_cf, 'OR', $neutral_un, TRUE, FALSE], + // Neutral (un) OR neutral (ct,cf,un). + [$neutral_un, 'OR', $neutral_ct, FALSE, NULL], + [$neutral_un, 'OR', $neutral_cf, FALSE, NULL], + [$neutral_un, 'OR', $neutral_un, FALSE, NULL], + + // Neutral (ct) OR forbidden (ct,cf,un). + [$neutral_ct, 'OR', $forbidden_ct, TRUE, TRUE], + [$neutral_ct, 'OR', $forbidden_cf, TRUE, FALSE], + [$neutral_ct, 'OR', $forbidden_un, TRUE, FALSE], + // Neutral (cf) OR forbidden (ct,cf,un). + [$neutral_cf, 'OR', $forbidden_ct, TRUE, TRUE], + [$neutral_cf, 'OR', $forbidden_cf, TRUE, FALSE], + [$neutral_cf, 'OR', $forbidden_un, TRUE, FALSE], + // Neutral (un) OR forbidden (ct,cf,un). + [$neutral_un, 'OR', $forbidden_ct, FALSE, NULL], + [$neutral_un, 'OR', $forbidden_cf, FALSE, NULL], + [$neutral_un, 'OR', $forbidden_un, FALSE, NULL], + + + + + // Allowed (ct) AND allowed (ct,cf,un). + [$allowed_ct, 'AND', $allowed_ct, TRUE, TRUE], + [$allowed_ct, 'AND', $allowed_cf, TRUE, FALSE], + [$allowed_ct, 'AND', $allowed_un, TRUE, FALSE], + // Allowed (cf) AND allowed (ct,cf,un). + [$allowed_cf, 'AND', $allowed_ct, TRUE, FALSE], + [$allowed_cf, 'AND', $allowed_cf, TRUE, FALSE], + [$allowed_cf, 'AND', $allowed_un, TRUE, FALSE], + // Allowed (un) AND allowed (ct,cf,un). + [$allowed_un, 'AND', $allowed_ct, FALSE, NULL], + [$allowed_un, 'AND', $allowed_cf, FALSE, NULL], + [$allowed_un, 'AND', $allowed_un, FALSE, NULL], + + // Allowed (ct) AND forbidden (ct,cf,un). + [$allowed_ct, 'AND', $forbidden_ct, TRUE, TRUE], + [$allowed_ct, 'AND', $forbidden_cf, TRUE, FALSE], + [$allowed_ct, 'AND', $forbidden_un, TRUE, FALSE], + // Allowed (cf) AND forbidden (ct,cf,un). + [$allowed_cf, 'AND', $forbidden_ct, TRUE, TRUE], + [$allowed_cf, 'AND', $forbidden_cf, TRUE, FALSE], + [$allowed_cf, 'AND', $forbidden_un, TRUE, FALSE], + // Allowed (un) AND forbidden (ct,cf,un). + [$allowed_un, 'AND', $forbidden_ct, FALSE, NULL], + [$allowed_un, 'AND', $forbidden_cf, FALSE, NULL], + [$allowed_un, 'AND', $forbidden_un, FALSE, NULL], + + // Allowed (ct) AND neutral (ct,cf,un). + [$allowed_ct, 'AND', $neutral_ct, TRUE, TRUE], + [$allowed_ct, 'AND', $neutral_cf, TRUE, FALSE], + [$allowed_ct, 'AND', $neutral_un, TRUE, FALSE], + // Allowed (cf) AND neutral (ct,cf,un). + [$allowed_cf, 'AND', $neutral_ct, TRUE, FALSE], + [$allowed_cf, 'AND', $neutral_cf, TRUE, FALSE], + [$allowed_cf, 'AND', $neutral_un, TRUE, FALSE], + // Allowed (un) AND neutral (ct,cf,un). + [$allowed_un, 'AND', $neutral_ct, FALSE, NULL], + [$allowed_un, 'AND', $neutral_cf, FALSE, NULL], + [$allowed_un, 'AND', $neutral_un, FALSE, NULL], + + + // Forbidden (ct) AND allowed (ct,cf,un). + [$forbidden_ct, 'AND', $allowed_ct, TRUE, TRUE], + [$forbidden_ct, 'AND', $allowed_cf, TRUE, TRUE], + [$forbidden_ct, 'AND', $allowed_un, TRUE, TRUE], + // Forbidden (cf) AND allowed (ct,cf,un). + [$forbidden_cf, 'AND', $allowed_ct, TRUE, FALSE], + [$forbidden_cf, 'AND', $allowed_cf, TRUE, FALSE], + [$forbidden_cf, 'AND', $allowed_un, TRUE, FALSE], + // Forbidden (un) AND allowed (ct,cf,un). + [$forbidden_un, 'AND', $allowed_ct, FALSE, NULL], + [$forbidden_un, 'AND', $allowed_cf, FALSE, NULL], + [$forbidden_un, 'AND', $allowed_un, FALSE, NULL], + + // Forbidden (ct) AND neutral (ct,cf,un). + [$forbidden_ct, 'AND', $neutral_ct, TRUE, TRUE], + [$forbidden_ct, 'AND', $neutral_cf, TRUE, TRUE], + [$forbidden_ct, 'AND', $neutral_un, TRUE, TRUE], + // Forbidden (cf) AND neutral (ct,cf,un). + [$forbidden_cf, 'AND', $neutral_ct, TRUE, FALSE], + [$forbidden_cf, 'AND', $neutral_cf, TRUE, FALSE], + [$forbidden_cf, 'AND', $neutral_un, TRUE, FALSE], + // Forbidden (un) AND neutral (ct,cf,un). + [$forbidden_un, 'AND', $neutral_ct, FALSE, NULL], + [$forbidden_un, 'AND', $neutral_cf, FALSE, NULL], + [$forbidden_un, 'AND', $neutral_un, FALSE, NULL], + + // Forbidden (ct) AND forbidden (ct,cf,un). + [$forbidden_ct, 'AND', $forbidden_ct, TRUE, TRUE], + [$forbidden_ct, 'AND', $forbidden_cf, TRUE, TRUE], + [$forbidden_ct, 'AND', $forbidden_un, TRUE, TRUE], + // Forbidden (cf) AND forbidden (ct,cf,un). + [$forbidden_cf, 'AND', $forbidden_ct, TRUE, FALSE], + [$forbidden_cf, 'AND', $forbidden_cf, TRUE, FALSE], + [$forbidden_cf, 'AND', $forbidden_un, TRUE, FALSE], + // Forbidden (un) AND forbidden (ct,cf,un). + [$forbidden_un, 'AND', $forbidden_ct, FALSE, NULL], + [$forbidden_un, 'AND', $forbidden_cf, FALSE, NULL], + [$forbidden_un, 'AND', $forbidden_un, FALSE, NULL], + + + // Neutral (ct) AND allowed (ct,cf,un). + [$neutral_ct, 'AND', $allowed_ct, TRUE, TRUE], + [$neutral_ct, 'AND', $allowed_cf, TRUE, TRUE], + [$neutral_ct, 'AND', $allowed_un, TRUE, TRUE], + // Neutral (cf) AND allowed (ct,cf,un). + [$neutral_cf, 'AND', $allowed_ct, TRUE, FALSE], + [$neutral_cf, 'AND', $allowed_cf, TRUE, FALSE], + [$neutral_cf, 'AND', $allowed_un, TRUE, FALSE], + // Neutral (un) AND allowed (ct,cf,un). + [$neutral_un, 'AND', $allowed_ct, FALSE, NULL], + [$neutral_un, 'AND', $allowed_cf, FALSE, NULL], + [$neutral_un, 'AND', $allowed_un, FALSE, NULL], + + // Neutral (ct) AND neutral (ct,cf,un). + [$neutral_ct, 'AND', $neutral_ct, TRUE, TRUE], + [$neutral_ct, 'AND', $neutral_cf, TRUE, TRUE], + [$neutral_ct, 'AND', $neutral_un, TRUE, TRUE], + // Neutral (cf) AND neutral (ct,cf,un). + [$neutral_cf, 'AND', $neutral_ct, TRUE, FALSE], + [$neutral_cf, 'AND', $neutral_cf, TRUE, FALSE], + [$neutral_cf, 'AND', $neutral_un, TRUE, FALSE], + // Neutral (un) AND neutral (ct,cf,un). + [$neutral_un, 'AND', $neutral_ct, FALSE, NULL], + [$neutral_un, 'AND', $neutral_cf, FALSE, NULL], + [$neutral_un, 'AND', $neutral_un, FALSE, NULL], + + // Neutral (ct) AND forbidden (ct,cf,un). + [$neutral_ct, 'AND', $forbidden_ct, TRUE, TRUE], + [$neutral_ct, 'AND', $forbidden_cf, TRUE, FALSE], + [$neutral_ct, 'AND', $forbidden_un, TRUE, FALSE], + // Neutral (cf) AND forbidden (ct,cf,un). + [$neutral_cf, 'AND', $forbidden_ct, TRUE, TRUE], + [$neutral_cf, 'AND', $forbidden_cf, TRUE, FALSE], + [$neutral_cf, 'AND', $forbidden_un, TRUE, FALSE], + // Neutral (un) AND forbidden (ct,cf,un). + [$neutral_un, 'AND', $forbidden_ct, FALSE, NULL], + [$neutral_un, 'AND', $forbidden_cf, FALSE, NULL], + [$neutral_un, 'AND', $forbidden_un, FALSE, NULL], + ]; + } + + /** + * @covers ::andIf + * @covers ::orIf + * @covers ::inheritCacheability + * + * @dataProvider andOrCacheabilityPropagationProvider + */ + public function testAndOrCacheabilityPropagation(AccessResultInterface $first, $op, AccessResultInterface $second, $implements_cacheable_interface, $is_cacheable) { + if ($op === 'OR') { + $result = $first->orIf($second); + } + else if ($op === 'AND') { + $result = $first->andIf($second); + } + else { + throw new \LogicException('Invalid operator specified'); + } + if ($implements_cacheable_interface) { + $this->assertTrue($result instanceof CacheableInterface, 'Result is an instance of CacheableInterface.'); + if ($result instanceof CacheableInterface) { + $this->assertSame($is_cacheable, $result->isCacheable(), 'isCacheable() matches expectations.'); + } + } + else { + $this->assertFalse($result instanceof CacheableInterface, 'Result is not an instance of CacheableInterface.'); + } + } + +} + +class UncacheableTestAccessResult implements AccessResultInterface { + + /** + * The access result value. 'ALLOWED', 'FORBIDDEN' or 'NEUTRAL'. + * + * @var string + */ + protected $value; + + /** + * Constructs a new UncacheableTestAccessResult object. + */ + public function __construct($value) { + $this->value = $value; + } + /** + * {@inheritdoc} + */ + public function isAllowed() { + return $this->value === 'ALLOWED'; + } + + /** + * {@inheritdoc} + */ + public function isForbidden() { + return $this->value === 'FORBIDDEN'; + } + + /** + * {@inheritdoc} + */ + public function isNeutral() { + return $this->value === 'NEUTRAL'; + } + + /** + * {@inheritdoc} + */ + public function orIf(AccessResultInterface $other) { + if ($this->isForbidden() || $other->isForbidden()) { + return new static('FORBIDDEN'); + } + elseif ($this->isAllowed() || $other->isAllowed()) { + return new static('ALLOWED'); + } + else { + return new static('NEUTRAL'); + } + } + + /** + * {@inheritdoc} + */ + public function andIf(AccessResultInterface $other) { + if ($this->isForbidden() || $other->isForbidden()) { + return new static('FORBIDDEN'); + } + elseif ($this->isAllowed() && $other->isAllowed()) { + return new static('ALLOWED'); + } + else { + return new static('NEUTRAL'); + } } } diff --git a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php index bea19c0e712..36d330de37d 100644 --- a/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php @@ -97,7 +97,7 @@ class CustomAccessCheckTest extends UnitTestCase { $route = new Route('/test-route', array(), array('_custom_access' => '\Drupal\Tests\Core\Access\TestController::accessDeny')); $account = $this->getMock('Drupal\Core\Session\AccountInterface'); - $this->assertEquals(AccessResult::create(), $this->accessChecker->access($route, $route_match, $account)); + $this->assertEquals(AccessResult::neutral(), $this->accessChecker->access($route, $route_match, $account)); $route = new Route('/test-route', array(), array('_custom_access' => '\Drupal\Tests\Core\Access\TestController::accessAllow')); $this->assertEquals(AccessResult::allowed(), $this->accessChecker->access($route, $route_match, $account)); @@ -115,7 +115,7 @@ class TestController { } public function accessDeny() { - return AccessResult::create(); + return AccessResult::neutral(); } public function accessParameter($parameter) { diff --git a/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php index c7bc1dffc05..8bf0013a04a 100644 --- a/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Access/DefaultAccessCheckTest.php @@ -50,7 +50,7 @@ class DefaultAccessCheckTest extends UnitTestCase { $request = new Request(array()); $route = new Route('/test-route', array(), array('_access' => 'NULL')); - $this->assertEquals(AccessResult::create(), $this->accessChecker->access($route, $request, $this->account)); + $this->assertEquals(AccessResult::neutral(), $this->accessChecker->access($route, $request, $this->account)); $route = new Route('/test-route', array(), array('_access' => 'FALSE')); $this->assertEquals(AccessResult::forbidden(), $this->accessChecker->access($route, $request, $this->account)); diff --git a/core/tests/Drupal/Tests/Core/Entity/EntityCreateAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Entity/EntityCreateAccessCheckTest.php index da04fdd04d2..1ee9e1b84f0 100644 --- a/core/tests/Drupal/Tests/Core/Entity/EntityCreateAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Entity/EntityCreateAccessCheckTest.php @@ -40,9 +40,9 @@ class EntityCreateAccessCheckTest extends UnitTestCase { * @return array */ public function providerTestAccess() { - $no_access = AccessResult::create()->cachePerRole(); + $no_access = AccessResult::neutral()->cachePerRole(); $access = AccessResult::allowed()->cachePerRole(); - $no_access_due_to_errors = AccessResult::create(); + $no_access_due_to_errors = AccessResult::neutral(); return array( array('', 'entity_test', $no_access, $no_access), diff --git a/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php b/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php index 5fc71238dcf..ad5b5773926 100644 --- a/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php +++ b/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php @@ -155,7 +155,7 @@ class RoleAccessCheckTest extends UnitTestCase { foreach ($deny_accounts as $account) { $message = sprintf('Access denied for user %s with the roles %s on path: %s', $account->id(), implode(', ', $account->getRoles()), $path); $has_access = $role_access_check->access($collection->get($path), $account); - $this->assertEquals(AccessResult::create()->cachePerRole(), $has_access, $message); + $this->assertEquals(AccessResult::neutral()->cachePerRole(), $has_access, $message); } }