Issue #2900320 by Sam152, timmillwood, Wim Leers: Remove workflow type...

Issue #2900320 by Sam152, timmillwood, Wim Leers: Remove workflow type checkWorkflowAcess() & "view content moderation" permission

core/modules/content_moderation/content_moderation.permissions.yml

@@ -2,10 +2,6 @@ view any unpublished content:
   title: 'View any unpublished content'
   description: 'This permission is necessary for any users that may moderate content.'
 
-view content moderation:
-  title: 'View content moderation'
-  description: 'View content moderation.'
-
 view latest version:
   title: 'View the latest version'
   description: 'View the latest version of an entity. (Also requires "View any unpublished content" permission)'

core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php

@@ -3,13 +3,11 @@
 namespace Drupal\content_moderation\Plugin\WorkflowType;
 
 use Drupal\content_moderation\ModerationInformationInterface;
-use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Entity\ContentEntityInterface;
 use Drupal\Core\Entity\EntityTypeBundleInfoInterface;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Entity\EntityPublishedInterface;
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
-use Drupal\Core\Session\AccountInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\content_moderation\ContentModerationState;
 use Drupal\workflows\Plugin\WorkflowTypeBase;
@@ -93,16 +91,6 @@ public static function create(ContainerInterface $container, array $configuratio
     );
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function checkWorkflowAccess(WorkflowInterface $entity, $operation, AccountInterface $account) {
-    if ($operation === 'view') {
-      return AccessResult::allowedIfHasPermission($account, 'view content moderation');
-    }
-    return parent::checkWorkflowAccess($entity, $operation, $account);
-  }
-
   /**
    * {@inheritdoc}
    */

core/modules/content_moderation/tests/src/Functional/ModerationRevisionRevertTest.php

@@ -43,7 +43,6 @@ public function setUp() {
       'administer nodes',
       'bypass node access',
       'view all revisions',
-      'view content moderation',
       'use editorial transition create_new_draft',
       'use editorial transition publish',
     ]);

core/modules/content_moderation/tests/src/Functional/ModerationStateAccessTest.php

@@ -32,7 +32,6 @@ public function testViewShowsCorrectStates() {
     $permissions = [
       'access content',
       'view all revisions',
-      'view content moderation',
     ];
     $editor1 = $this->drupalCreateUser($permissions);
     $this->drupalLogin($editor1);

core/modules/workflows/src/Plugin/WorkflowTypeBase.php

@@ -3,9 +3,7 @@
 namespace Drupal\workflows\Plugin;
 
 use Drupal\Component\Plugin\PluginBase;
-use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Plugin\PluginWithFormsTrait;
-use Drupal\Core\Session\AccountInterface;
 use Drupal\workflows\State;
 use Drupal\workflows\StateInterface;
 use Drupal\workflows\Transition;
@@ -48,13 +46,6 @@ public function label() {
     return $definition['label'];
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  public function checkWorkflowAccess(WorkflowInterface $entity, $operation, AccountInterface $account) {
-    return AccessResult::neutral();
-  }
-
   /**
    * {@inheritdoc}
    */

core/modules/workflows/src/WorkflowAccessControlHandler.php

@@ -12,7 +12,7 @@
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
- * Access controller for the Moderation State entity.
+ * Access controller for the Workflow entity.
  *
  * @see \Drupal\workflows\Entity\Workflow.
  *
@@ -62,15 +62,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       list(, $state_id) = explode(':', $operation, 2);
       // Deleting a state is editing a workflow, but also we should forbid
       // access if there is only one state.
-      $admin_access = AccessResult::allowedIf(count($entity->getTypePlugin()->getStates()) > 1)
+      return AccessResult::allowedIf(count($entity->getTypePlugin()->getStates()) > 1)
         ->andIf(parent::checkAccess($entity, 'edit', $account))
         ->andIf(AccessResult::allowedIf(!in_array($state_id, $workflow_type->getRequiredStates(), TRUE)))
         ->addCacheableDependency($entity);
     }
-    else {
-      $admin_access = parent::checkAccess($entity, $operation, $account);
-    }
-    return $workflow_type->checkWorkflowAccess($entity, $operation, $account)->orIf($admin_access);
+
+    return parent::checkAccess($entity, $operation, $account);
   }
 
   /**
@@ -81,7 +79,9 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
     $admin_access = parent::checkCreateAccess($account, $context, $entity_bundle);
     // Allow access if there is at least one workflow type. Since workflow types
     // are provided by modules this is cacheable until extensions change.
-    return $admin_access->andIf(AccessResult::allowedIf($workflow_types_count > 0))->addCacheTags(['config:core.extension']);
+    return $admin_access
+      ->andIf(AccessResult::allowedIf($workflow_types_count > 0))
+      ->addCacheTags(['workflow_type_plugins']);
   }
 
 }

core/modules/workflows/src/WorkflowTypeInterface.php

@@ -5,7 +5,6 @@
 use Drupal\Component\Plugin\ConfigurablePluginInterface;
 use Drupal\Component\Plugin\DerivativeInspectionInterface;
 use Drupal\Core\Plugin\PluginWithFormsInterface;
-use Drupal\Core\Session\AccountInterface;
 
 /**
  * An interface for Workflow type plugins.
@@ -29,22 +28,6 @@ interface WorkflowTypeInterface extends PluginWithFormsInterface, DerivativeInsp
    */
   public function label();
 
-  /**
-   * Performs access checks.
-   *
-   * @param \Drupal\workflows\WorkflowInterface $entity
-   *   The workflow entity for which to check access.
-   * @param string $operation
-   *   The entity operation. Usually one of 'view', 'view label', 'update' or
-   *   'delete'.
-   * @param \Drupal\Core\Session\AccountInterface $account
-   *   The user for which to check access.
-   *
-   * @return \Drupal\Core\Access\AccessResultInterface
-   *   The access result.
-   */
-  public function checkWorkflowAccess(WorkflowInterface $entity, $operation, AccountInterface $account);
-
   /**
    * Determines if the workflow is being has data associated with it.
    *

core/modules/workflows/src/WorkflowTypeManager.php

@@ -34,7 +34,7 @@ class WorkflowTypeManager extends DefaultPluginManager {
   public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
     parent::__construct('Plugin/WorkflowType', $namespaces, $module_handler, WorkflowTypeInterface::class, WorkflowType::class);
     $this->alterInfo('workflow_type_info');
-    $this->setCacheBackend($cache_backend, 'workflow_type_info');
+    $this->setCacheBackend($cache_backend, 'workflow_type_info', ['workflow_type_plugins']);
   }
 
 }

core/modules/workflows/tests/modules/workflow_type_test/workflow_type_test.module

+<?php
+
+/**
+ * @file
+ * Module file for workflow_type_test.
+ */
+
+/**
+ * Implements hook_workflow_type_info_alter().
+ */
+function workflow_type_test_workflow_type_info_alter(&$definitions) {
+  // Allow tests to override the workflow type definitions.
+  $state = \Drupal::state();
+  if ($state->get('workflow_type_test.plugin_definitions') !== NULL) {
+    $definitions = $state->get('workflow_type_test.plugin_definitions');
+  }
+}
+
+/**
+ * Sets the type plugin definitions override and clear the cache.
+ *
+ * @param array $definitions
+ *   Definitions to set.
+ */
+function workflow_type_test_set_definitions($definitions) {
+  \Drupal::state()->set('workflow_type_test.plugin_definitions', $definitions);
+  \Drupal::service('plugin.manager.workflows.type')->clearCachedDefinitions();
+}

core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php

+<?php
+
+namespace Drupal\Tests\workflows\Kernel;
+
+use Drupal\Core\Access\AccessResult;
+use Drupal\Core\Cache\Context\CacheContextsManager;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\KernelTests\KernelTestBase;
+use Drupal\simpletest\UserCreationTrait;
+use Drupal\workflows\Entity\Workflow;
+
+/**
+ * @coversDefaultClass \Drupal\workflows\WorkflowAccessControlHandler
+ * @group workflows
+ */
+class WorkflowAccessControlHandlerTest extends KernelTestBase {
+
+  use UserCreationTrait;
+
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = [
+    'workflows',
+    'workflow_type_test',
+    'system',
+    'user',
+  ];
+
+  /**
+   * The workflow access control handler.
+   *
+   * @var \Drupal\workflows\WorkflowAccessControlHandler
+   */
+  protected $accessControlHandler;
+
+  /**
+   * A test admin user.
+   *
+   * @var \Drupal\Core\Session\AccountInterface
+   */
+  protected $adminUser;
+
+  /**
+   * A non-privileged user.
+   *
+   * @var \Drupal\Core\Session\AccountInterface
+   */
+  protected $user;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    $this->installEntitySchema('workflow');
+    $this->installEntitySchema('user');
+    $this->installSchema('system', ['sequences']);
+
+    $this->accessControlHandler = $this->container->get('entity_type.manager')->getAccessControlHandler('workflow');
+
+    // Create and discard user 1, which is special and bypasses all access
+    // checking.
+    $this->createUser([]);
+    $this->user = $this->createUser([]);
+    $this->adminUser = $this->createUser(['administer workflows']);
+  }
+
+  /**
+   * @covers ::checkCreateAccess
+   */
+  public function testCheckCreateAccess() {
+    // A user must have the correct permission to create a workflow.
+    $this->assertEquals(
+      AccessResult::neutral()
+        ->addCacheContexts(['user.permissions'])
+        ->setReason("The 'administer workflows' permission is required.")
+        ->addCacheTags(['workflow_type_plugins']),
+      $this->accessControlHandler->createAccess(NULL, $this->user, [], TRUE)
+    );
+    $this->assertEquals(
+      AccessResult::allowed()
+        ->addCacheContexts(['user.permissions'])
+        ->addCacheTags(['workflow_type_plugins']),
+      $this->accessControlHandler->createAccess(NULL, $this->adminUser, [], TRUE)
+    );
+
+    // Remove all plugin types and ensure not even the admin user is allowed to
+    // create a workflow.
+    workflow_type_test_set_definitions([]);
+    $this->accessControlHandler->resetCache();
+    $this->assertEquals(
+      AccessResult::neutral()
+        ->addCacheContexts(['user.permissions'])
+        ->addCacheTags(['workflow_type_plugins']),
+      $this->accessControlHandler->createAccess(NULL, $this->adminUser, [], TRUE)
+    );
+  }
+
+  /**
+   * @covers ::checkAccess
+   * @dataProvider checkAccessProvider
+   */
+  public function testCheckAccess($user, $operation, $result, $states_to_create = []) {
+    $workflow = Workflow::create([
+      'type' => 'workflow_type_test',
+      'id' => 'test_workflow',
+    ]);
+    $workflow->save();
+    $workflow_type = $workflow->getTypePlugin();
+    foreach ($states_to_create as $state_id => $is_required) {
+      $workflow_type->addState($state_id, $this->randomString());
+    }
+    \Drupal::state()->set('workflow_type_test.required_states', array_filter($states_to_create));
+    $this->assertEquals($result, $this->accessControlHandler->access($workflow, $operation, $this->{$user}, TRUE));
+  }
+
+  /**
+   * Data provider for ::testCheckAccess.
+   *
+   * @return array
+   */
+  public function checkAccessProvider() {
+    $container = new ContainerBuilder();
+    $cache_contexts_manager = $this->prophesize(CacheContextsManager::class);
+    $cache_contexts_manager->assertValidTokens()->willReturn(TRUE);
+    $cache_contexts_manager->reveal();
+    $container->set('cache_contexts_manager', $cache_contexts_manager);
+    \Drupal::setContainer($container);
+
+    return [
+      'Admin view' => [
+        'adminUser',
+        'view',
+        AccessResult::allowed()->addCacheContexts(['user.permissions']),
+      ],
+      'Admin update' => [
+        'adminUser',
+        'update',
+        AccessResult::allowed()->addCacheContexts(['user.permissions']),
+      ],
+      'Admin delete' => [
+        'adminUser',
+        'delete',
+        AccessResult::allowed()->addCacheContexts(['user.permissions']),
+      ],
+      'Admin delete only state' => [
+        'adminUser',
+        'delete-state:foo',
+        AccessResult::neutral()->addCacheTags(['config:workflows.workflow.test_workflow']),
+        ['foo' => FALSE],
+      ],
+      'Admin delete one of two states' => [
+        'adminUser',
+        'delete-state:foo',
+        AccessResult::allowed()
+          ->addCacheTags(['config:workflows.workflow.test_workflow'])
+          ->addCacheContexts(['user.permissions']),
+        ['foo' => FALSE, 'bar' => FALSE],
+      ],
+      'Admin delete required state when there are >1 states' => [
+        'adminUser',
+        'delete-state:foo',
+        AccessResult::allowed()
+          ->addCacheTags(['config:workflows.workflow.test_workflow'])
+          ->addCacheContexts(['user.permissions']),
+        ['foo' => TRUE, 'bar' => FALSE],
+      ],
+      'User view' => [
+        'user',
+        'view',
+        AccessResult::neutral()
+          ->addCacheContexts(['user.permissions'])
+          ->setReason("The 'administer workflows' permission is required."),
+      ],
+      'User update' => [
+        'user',
+        'update',
+        AccessResult::neutral()
+          ->addCacheContexts(['user.permissions'])
+          ->setReason("The 'administer workflows' permission is required."),
+      ],
+      'User delete' => [
+        'user',
+        'delete',
+        AccessResult::neutral()
+          ->addCacheContexts(['user.permissions'])
+          ->setReason("The 'administer workflows' permission is required."),
+      ],
+      'User delete only state' => [
+        'user',
+        'delete-state:foo',
+        AccessResult::neutral()->addCacheTags(['config:workflows.workflow.test_workflow']),
+        ['foo' => FALSE],
+      ],
+      'User delete one of two states' => [
+        'user',
+        'delete-state:foo',
+        AccessResult::neutral()
+          ->addCacheTags(['config:workflows.workflow.test_workflow'])
+          ->addCacheContexts(['user.permissions'])
+          ->setReason("The 'administer workflows' permission is required."),
+        ['foo' => FALSE, 'bar' => FALSE],
+      ],
+      'User delete required state when there are >1 states' => [
+        'user',
+        'delete-state:foo',
+        AccessResult::neutral()
+          ->addCacheTags(['config:workflows.workflow.test_workflow'])
+          ->addCacheContexts(['user.permissions'])
+          ->setReason("The 'administer workflows' permission is required."),
+        ['foo' => TRUE, 'bar' => FALSE],
+      ],
+    ];
+  }
+
+}