Expand the entity access model for workflow states and transitions.

I think this would count as api additions, so I don't think it should be a blocker for stable.

It would be good to confirm that adding extra access operations is an API addition only. If that's the case, this can safely be done in 8.5.

Picking this back up, am I correct in saying based on #2300677: JSON:API POST/PATCH support for fully validatable config entities not being completed, introducing a more fleshed out permissions model for states and transitions would currently only need to apply access control at the UI level? Given we don't have any special handling for REST and the delete-state:foo opperation, I think that sounds right.

Here is what I had in mind for this.

I agree, _workflow_access does seem nicer. NW for that.

Good catch! This is looking good to me :)

1. We test the list of paths in the positive and negative here.
   

   +++ b/core/modules/workflows/tests/src/Functional/WorkflowCustomStateTransitionAccessTest.php
   @@ -0,0 +1,89 @@
   +    $forbidden_paths = [
   +      'admin/config/workflow/workflows/manage/test_type/state/cant_delete/delete',
   +      'admin/config/workflow/workflows/manage/test_type/state/cant_update',
   +      'admin/config/workflow/workflows/manage/test_type/transition/cant_update',
   +      'admin/config/workflow/workflows/manage/test_type/transition/cant_delete/delete',
   +      'admin/config/workflow/workflows/manage/test_type/add_state',
   +      'admin/config/workflow/workflows/manage/test_type/add_transition',
   +    ];
   +    // Until the list of forbidden operations have been set, the admin user
   +    // should be able to access all the forbidden paths.
   +    foreach ($forbidden_paths as $forbidden_path) {
   +      $this->drupalGet($forbidden_path);
   +      $this->assertSession()->statusCodeEquals(200);
   +    }
   +
   +    // Update the forbidden operations which deny access to the actions
   +    // represented by the above paths.
   +    $this->container->get('state')->set('workflow_type_test_forbidden_operations', [
   +      'update-state:cant_update',
   +      'delete-state:cant_delete',
   +      'update-transition:cant_update',
   +      'delete-transition:cant_delete',
   +      'add-state',
   +      'add-transition',
   +    ]);
   +    foreach ($forbidden_paths as $forbidden_path) {
   +      $this->drupalGet($forbidden_path);
   +      $this->assertSession()->statusCodeEquals(403);
   +    }
   

2. Good point.
3. Didn't know you could do this for props.

Whoops, missed 1.1. This is what happens when two bits of feedback are put in the one bullet point ;)

Local variable does make this a bit more readable.

I think a change record could mostly be pulled from the docs in the patch, I don't have time to put one together today, but can look at it later in the week.

Thanks for the review!

• #26.1, not a fan of the extra nesting.
• #27.1 it seems you are correct, good catch.
• #27.2, this format looks good.
• #27.3, I'll put a debugger in there sometime this week and figure out why they are uncacheable.
• #27.4, the WorkflowDeleteAccessCheck class itself is marked @internal.

Addressing the code feedback from #27.

For some reason I thought EntityAccessControlHandler had its own caching, but it's just a static cache. I think then I'll have to look at the whole response object, see why it's not cacheable.

I started looking into #27.3. My findings were as follows:

The request to these pages were uncacheable for the following reasons:

1. The CSRF token in the form builder adds a high cardinality cache context, "session".
2. It also adds max-age 0 when rendering the token.
3. The DenyAdminRoutes cache response policy prevents these pages from being cached too.

I implemented the following in an attempt to make the page cacheable:

diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
index be000dcc25..51ef531189 100644
--- a/core/lib/Drupal/Core/Form/FormBuilder.php
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -661,11 +661,11 @@ public function renderPlaceholderFormAction() {
   public function renderFormTokenPlaceholder($placeholder) {
     return [
       '#markup' => $this->csrfToken->get($placeholder),
-      '#cache' => [
-        'contexts' => [
-          'session',
-        ],
-      ],
+//      '#cache' => [
+//        'contexts' => [
+//          'session',
+//        ],
+//      ],
     ];
   }
 
@@ -778,9 +778,9 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
               ]
             ]
           ],
-          '#cache' => [
-            'max-age' => 0,
-          ],
+//          '#cache' => [
+//            'max-age' => 0,
+//          ],
         ];
       }
     }
diff --git a/core/modules/dynamic_page_cache/dynamic_page_cache.services.yml b/core/modules/dynamic_page_cache/dynamic_page_cache.services.yml
index 241999233d..7a7136b026 100644
--- a/core/modules/dynamic_page_cache/dynamic_page_cache.services.yml
+++ b/core/modules/dynamic_page_cache/dynamic_page_cache.services.yml
@@ -21,9 +21,9 @@ services:
     tags:
       - { name: service_collector, tag: dynamic_page_cache_response_policy, call: addPolicy}
     lazy: true
-  dynamic_page_cache_deny_admin_routes:
-    class: Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes
-    arguments: ['@current_route_match']
-    public: false
-    tags:
-      - { name: dynamic_page_cache_response_policy }
+#  dynamic_page_cache_deny_admin_routes:
+#    class: Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes
+#    arguments: ['@current_route_match']
+#    public: false
+#    tags:
+#      - { name: dynamic_page_cache_response_policy }

This created a cache entry in cache_dynamic_page_cache:

mysql> select cid from test93734581cache_dynamic_page_cache where data LIKE '%Are you sure%';
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| cid                                                                                                                                                                                                                                                             |
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| response:[languages:language_interface]=en:[request_format]=html:[route]=entity.workflow.delete_state_formd76ce31db3e6bd858bccb75b8d0f9eef2f51d7caef3f0de1bf0b0476ea295fe3:[theme]=classy:[url.query_args:_wrapper_fnQw7_zLurFxypKu7-AN1Yn-LVVlx638uaJlXebLYP6M |
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

Then in \Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber::onRouteMatch I expected this to be delivered on the second hit. Stepping into render cache from here, into \Drupal\Core\Render\RenderCache::get the cid used to lookup the cache hit was the following:

response:[request_format]=html:[route]=system.40335786c7117b4e38d0f169239752ce71158266ae2f6e4aa230fbbb87bd699c0e3

This obviously doesn't match the entry in the DB, so not sure why it was unable to derive the correct cache ID. I figured that it was possibly due to the fact this is an unconventional code path, ie, the safeguards I commented out probably exist for a reason and undoing them is undefined behaviour. Possibly a bug in render cache or dynamic_page_cache too?

At this point I decided to conclude my adventure, I hope you'll understand :P

@Wim Leers any chance of a sign off on the cacheability part? I think we're back to RTBC if that's all good.

Added the change record here: https://www.drupal.org/node/2929327

1. I tried this and it made the test plugin more confusing because of the naming conflict between state storage and workflow states.
2. I think the entity access system will add all the contexts required to cache these properly. What the new test cases indicate is that nothing beyond the users permissions are currently being used to make a decision about access, so an access check for update-state:foo could be cached for all entities. This isn't the case for some of the existing operations like delete-state:foo. It checks things like the required states, number of states etc, so the Workflow entity is added as a cacheable dependency. As far as caching the access checks per 'route' context, this must be implied somewhere else, given it's not something \Drupal\Core\Entity\EntityAccessCheck::access adds either. Not sure where though.
3. This is a style thing and one that isn't widely accepted. 100 instances of \$(.*)= \$this->assertSession(), 500 instances of $this->assertSession()->.

Anyone interested in reviewing this again? I believe all the points have been addressed.

Tentatively back to RTBC given mostly nits were fixed in #30 and I think I've addressed most of the feedback.

Thanks for taking the time to review this again @larowlan. I've restored the original access check as a BC shim with deprecations.

Re: signing off, I wasn't able to get in touch with @Wim Leers for a thumbs up however I did a deep dive into why our test didn't require any cache clears in #31.

Thanks for looking at this again @Wim Leers.

1. Woohoo!
2. Tried to clear this up.
3. Yep, named captured groups are the best!
4. Using _workflow_access ends up translating into a custom entity operation, it's just a case of if _workflow_access or _entity_access manages that for us. Personally even if both can do the job, I think it's nice to be able to use 'add-state' alongside 'update-state' and 'delete-state' which _entity_access can't do for us.

Does anyone have a strong opinion that _workflow_access should not support "add" operations alongside update and delete if the equivalent also works with _entity_access? Personally I think it reads better from a consistency perspective and it's not a large amount of additional code to maintain.