Remove workflow type checkWorkflowAcess() & "view content moderation" permission

+++ b/core/modules/content_moderation/content_moderation.permissions.yml
@@ -2,10 +2,6 @@ view any unpublished content:
-view content moderation:

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -95,16 +95,6 @@ public static function create(ContainerInterface $container, array $configuratio
-  public function checkWorkflowAccess(WorkflowInterface $entity, $operation, AccountInterface $account) {

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -51,13 +51,6 @@ public function label() {
-  public function checkWorkflowAccess(WorkflowInterface $entity, $operation, AccountInterface $account) {

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -30,22 +30,6 @@
-  public function checkWorkflowAccess(WorkflowInterface $entity, $operation, AccountInterface $account);

This is an API change, and therefore must happen before beta, otherwise these can only be @deprecated.

Should this be tagged WI Critical?

+++ b/core/modules/workflows/src/WorkflowAccessControlHandler.php
@@ -62,15 +62,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
-    else {
-      $admin_access = parent::checkAccess($entity, $operation, $account);
-    }

This also addresses #2899553-3: Architectural review of the Workflows module (documentation cleanups)'s \Drupal\workflows\WorkflowAccessControlHandler.3 remark — yay!

Once there's a change record in place, this is RTBC as far as I'm concerned.

Thanks for the CR.

I know I said this would be RTBC, but:

+++ b/core/modules/workflows/src/WorkflowAccessControlHandler.php
@@ -62,15 +62,13 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
-      $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);

These represent massive changes in access checking though. Are we sure we're not regressing? This is why I raised in #2899553-3: Architectural review of the Workflows module (documentation cleanups) that it's frightening that there's zero test coverage for WorkflowAccessControlHandlerTest.

For peace of mind, we'd probably want those tests to be added here, in this issue.

This issue seems to be at odds with #2897148: Remove @internal from workflowHasData/workflowStateHasData and use those methods for access control and configuration validation.. Unless we moved the approach in that issue to a hook_entity_access()?

Big +1 to removing the "view content moderation" permission though.

NW for the tests based on #6.

I'd say it's fine to proceed with this approach. We have hooks for altering access, which could be more understandable from a reviewers perspective ie, hooks are a pattern that contribute an existing access result vs the plugin method which doesn't come with any such prior understanding. #2897148 can implement the hook directly if consensus is reached about the outcome of the issue.

Marking as critical because of the api change.

Adding a test for this, I think this covers most of the scenarios in the access controller.

Am I correct in assuming we'll need a functional test for asserting the cacheability metadata? From what I can tell, they only kick in once rendering a response and don't act directly at an API level.

@timmilwood++
@Sam152++

Am I correct in assuming we'll need a functional test for asserting the cacheability metadata? From what I can tell, they only kick in once rendering a response and don't act directly at an API level.

Thankfully you're not correct :) See \Drupal\Tests\system\Kernel\DateFormatAccessControlHandlerTest + \Drupal\Tests\system\Kernel\MenuAccessControlHandlerTest for examples.

Tagging major because this reduces the possibility that access checking will be misunderstood, wrongly extended or altered, which means it's a security improvement!

Reviewed the #11 interdiff. This is looking very very very very good already! 😀 With minor additional work, this will be perfect.

1. +++ b/core/modules/workflows/src/WorkflowAccessControlHandler.php
   @@ -79,7 +79,9 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   -    return $admin_access->andIf(AccessResult::allowedIf($workflow_types_count > 0))->addCacheTags(['config:core.extension']);
   ...
   +      ->addCacheTags(['config:core.extension', 'workflow_type_plugins']);
   
   +++ b/core/modules/workflows/src/WorkflowTypeManager.php
   @@ -34,7 +34,7 @@ class WorkflowTypeManager extends DefaultPluginManager {
   -    $this->setCacheBackend($cache_backend, 'workflow_type_info');
   +    $this->setCacheBackend($cache_backend, 'workflow_type_info', ['workflow_type_plugins']);
   

   Why both of these cache tags? Why not only workflow_type_plugins or config:core.extension?

   It's my comment at #2899553-3: Architectural review of the Workflows module (documentation cleanups) point 5 for the access control handler that spawned this.

   In fact, using config:core.extension is definitely wrong upon closer inspection, because class WorkflowTypeManager extends DefaultPluginManager and that inherits \Drupal\Core\Plugin\DefaultPluginManager::clearCachedDefinitions() which automatically performs tag-based invalidation, and that is guaranteed to be called automatically during module (un)installation thanks to \Drupal\Core\Plugin\PluginManagerPass, which \Drupal\Core\Extension\ModuleInstaller::(un)install() relies on when it does:

           // Clear plugin manager caches.
           \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions();
   

   In fact, you don't need to specify a cache tag there at all, unless you have other things you want to be able to rely on it.

2. +++ b/core/modules/workflows/tests/modules/workflow_type_test/workflow_type_test.module
   @@ -0,0 +1,28 @@
   +  \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions();
   

   This clears all plugin manager caches rather than just the one for our plugin manager.

3. +++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php
   @@ -0,0 +1,116 @@
   +class WorkflowAccessControlHandlerTest extends KernelTestBase {
   

   👍

4. +++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php
   @@ -0,0 +1,116 @@
   +  public function testCheckAccess() {
   ...
   +    // Admins can administer workflows, while users cannot.
   +    $this->assertTrue($this->accessControlHandler->access($workflow, 'view', $this->adminUser));
   +    $this->assertTrue($this->accessControlHandler->access($workflow, 'update', $this->adminUser));
   +    $this->assertTrue($this->accessControlHandler->access($workflow, 'delete', $this->adminUser));
   +    $this->assertFalse($this->accessControlHandler->access($workflow, 'view', $this->user));
   

   The asserting of TRUE or FALSE should be changed to asserting the actual return value.

   Again, see \Drupal\Tests\system\Kernel\MenuAccessControlHandlerTest::testAccess() for an example.

Having a look at the feedback, thanks for the review.

1. Our access check relies on it, so we need to keep it right?
2. Fixed.
3. :)
4. Done.

Whoops, removed unused class.

1. Oops, right!
2. 👍
3. 👍
4. 👍

Review:

1. +++ b/core/modules/workflows/tests/modules/workflow_type_test/workflow_type_test.module
   @@ -5,6 +5,8 @@
   +use Drupal\Core\Cache\Cache;
   

   Nit: Unused, can be deleted.

2. +++ b/core/modules/workflows/tests/modules/workflow_type_test/workflow_type_test.module
   @@ -0,0 +1,30 @@
   + * Set the type plugin definitions override and clear the cache.
   

   Supernit: s/Set/Sets/

3. +++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php
   @@ -0,0 +1,210 @@
   +   * @var array
   

   Nit: {@inheritdoc}

4. +++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php
   @@ -0,0 +1,210 @@
   +   * @var \Drupal\Core\Entity\EntityAccessControlHandlerInterface
   

   Nit: could be typehinted to \Drupal\workflows\WorkflowAccessControlHandler

5. +++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php
   @@ -0,0 +1,210 @@
   +    $this->assertEquals(AccessResult::allowed()->addCacheContexts(['user.permissions'])->addCacheTags(['workflow_type_plugins']), $this->accessControlHandler->createAccess(NULL, $this->adminUser, [], TRUE));
   

   This is just duplicating the previous assertion, likely a c/p remnant.

6. +++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php
   @@ -0,0 +1,210 @@
   +  public function testWorkflowAdministration($user, $operation, $result) {
   ...
   +      'Admin view' => [
   ...
   +      'User view' => [
   

   The name here is misleading, because this is testing more than only admin user test cases.

7. +++ b/core/modules/workflows/tests/src/Kernel/WorkflowAccessControlHandlerTest.php
   @@ -0,0 +1,210 @@
   +  public function testStateDeleteAccess() {
   ...
   +      $this->accessControlHandler->access($workflow, 'delete-state:foo', $this->adminUser, TRUE)
   

   This is really just testing another operation, so should be merged with the above test function.

   Furthermore, this is only testing as the admin user, not as the non-admin user.

Fixed all my remarks myself. The bulk of the work (the critical portion) was done by @Sam152, I just refactored it slightly to be more future-proof.

RTBC!

Rolling in ::testStateDeleteAccess into the data providers looks awesome. +1 RTBC.

Like I said, you did the bulk of the work, the critical portion. I just had to shuffle things around. I've had others (I think dawehner?) show me this is the better way, just spreading the knowledge! :)

So this permission was originally added in #2708941: Allow roles to view but not administer moderation state back in the workbench moderation days.

It was needed because if you added the 'state' to a view, you couldn't see it unless you had permission to administer the content moderation state entities.

Now that state is a string field on the ContentModerationState entity, this isn't needed.

If someone needs to implement plugin-level access controls, they can always switch out the access handler. But removing it here gives us less code to maintain and handles the 80% case.

There are a few unused use statements now - can we fix those as follows - thanks

Checking changed files...

FILE: ...content_moderation/src/Plugin/WorkflowType/ContentModeration.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
  6 | WARNING | [x] Unused use statement
 12 | WARNING | [x] Unused use statement
----------------------------------------------------------------------

FILE: ...re/drupal/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 6 | WARNING | [x] Unused use statement
 8 | WARNING | [x] Unused use statement
----------------------------------------------------------------------

FILE: ...core/drupal/core/modules/workflows/src/WorkflowTypeInterface.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 8 | WARNING | [x] Unused use statement
----------------------------------------------------------------------

Committed 10a1e15 and pushed to 8.5.x.

Cherry-picked as c3dda3d and pushed to 8.4.x.

🎉

Looks like the change record was never published for this. Have just updated it now.