Add missing REST and JSON:API test coverage for the workspace entity type

+++ b/core/modules/workspaces/src/WorkspaceAccessControlHandler.php
@@ -19,23 +19,23 @@ class WorkspaceAccessControlHandler extends EntityAccessControlHandler {
    */
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
     /** @var \Drupal\workspaces\WorkspaceInterface $entity */
-    if ($account->hasPermission('administer workspaces')) {
-      return AccessResult::allowed()->cachePerPermissions();
-    }
+    $access_result = AccessResult::allowedIfHasPermission($account, 'administer workspaces');
 
     // @todo Consider adding explicit "publish any|own workspace" permissions in
     //   https://www.drupal.org/project/drupal/issues/3084260.
     $permission_operation = ($operation === 'update' || $operation === 'publish') ? 'edit' : $operation;
 
     // Check if the user has permission to access all workspaces.
-    $access_result = AccessResult::allowedIfHasPermission($account, $permission_operation . ' any workspace');
+    $any_access_result = AccessResult::allowedIfHasPermission($account, $permission_operation . ' any workspace');
+    $access_result->orIf($any_access_result);

Weirdly (at least to me) it looks like the $access_result->orIf($any_access_result); returns a AccessResultNeutral even when $access_result is AccessResultNeutral and $any_access_result is AccessResultAllowed.

I (and it looks like the code also) was expecting an AccessResultAllowed which would prevent the current fails in this test: Drupal\workspaces\WorkspaceAccessException: The user does not have permission to view that workspace.

Note to self:
No more posting on d.o before at least 2 coffees.

Let's see how this goes.

Only the non-combined patch for starters (seeing my previous "success" above, let's do baby steps...)

1. +++ b/core/modules/jsonapi/jsonapi.module
   @@ -319,7 +319,7 @@ function jsonapi_jsonapi_user_filter_access(EntityTypeInterface $entity_type, Ac
   -function jsonapi_jsonapi_workspace_filter_access(EntityTypeInterface $entity_type, $published, $owner, AccountInterface $account) {
   +function jsonapi_jsonapi_workspace_filter_access(EntityTypeInterface $entity_type, AccountInterface $account) {
   

   This … must mean that we're lacking test coverage 😨

2. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -920,7 +920,9 @@ public function testGetIndividual() {
   -      $this->assertResourceErrorResponse(403, $message, $url, $response, '/data', $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), FALSE, 'MISS');
   +      // MISS or UNCACHEABLE depends on data. It must not be HIT.
   +      $dynamic_cache_header_value = !empty(array_intersect(['user', 'session'], $expected_403_cacheability->getCacheContexts())) ? 'UNCACHEABLE' : 'MISS';
   +      $this->assertResourceErrorResponse(403, $message, $url, $response, '/data', $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), FALSE, $dynamic_cache_header_value);
   

   👍 We have this pattern already in many other places in this test.

3. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -1076,7 +1078,12 @@ public function testCollection() {
        // MISS or UNCACHEABLE depends on the collection data. It must not be HIT.
   -    $dynamic_cache = $expected_cacheability->getCacheMaxAge() === 0 ? 'UNCACHEABLE' : 'MISS';
   +    if (!empty(array_intersect(['user', 'session'], $expected_cacheability->getCacheContexts()))) {
   +      $dynamic_cache = 'UNCACHEABLE';
   +    }
   +    else {
   +      $dynamic_cache = $expected_cacheability->getCacheMaxAge() === 0 ? 'UNCACHEABLE' : 'MISS';
   +    }
   
   @@ -1089,6 +1096,13 @@ public function testCollection() {
   +    // MISS or UNCACHEABLE depends on the collection data. It must not be HIT.
   +    if (!empty(array_intersect(['user', 'session'], $expected_cacheability->getCacheContexts()))) {
   +      $dynamic_cache = 'UNCACHEABLE';
   +    }
   +    else {
   +      $dynamic_cache = $expected_cacheability->getCacheMaxAge() === 0 ? 'UNCACHEABLE' : 'MISS';
   +    }
   
   @@ -1098,6 +1112,13 @@ public function testCollection() {
   +    // MISS or UNCACHEABLE depends on the collection data. It must not be HIT.
   +    if (!empty(array_intersect(['user', 'session'], $expected_cacheability->getCacheContexts()))) {
   +      $dynamic_cache = 'UNCACHEABLE';
   +    }
   +    else {
   +      $dynamic_cache = $expected_cacheability->getCacheMaxAge() === 0 ? 'UNCACHEABLE' : 'MISS';
   +    }
   

   We use

       // MISS or UNCACHEABLE depends on the collection data. It must not be HIT.
       $dynamic_cache = $expected_cacheability->getCacheMaxAge() === 0 || !empty(array_intersect(['user', 'session'], $expected_cacheability->getCacheContexts())) ? 'UNCACHEABLE' : 'MISS';
   

   elsewhere in this test.

   🙏 Can you copy/paste that same code in these places, for consistency sake?

4. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -2001,6 +2024,13 @@ public function testPostIndividual() {
   +        // Entity types with string IDs have to send the entity ID when creating
   +        // the document, but our normalization excludes it from the list of data
   +        // attributes.
   +        if ($field_name === $created_entity->getEntityType()->getKey('id')) {
   +          continue;
   +        }
   

   🤔😅 I … don't understand this. Is "our normalization" the JSON:API normalization or the normalization that the Workspace JSON:API test happens to send in a POST request? It seems like it's the latter? If so, this seems like a hack? Why is it not? (I am sincerely asking, I am having a hard time understanding this — please bear with me.)

5. +++ b/core/modules/jsonapi/tests/src/Functional/WorkspaceTest.php
   @@ -0,0 +1,209 @@
   +  protected static $firstCreatedEntityId = 'updated_campaign';
   ...
   +  protected static $secondCreatedEntityId = 'updated_campaign';
   

   🤔 Why are these the same? This is especially strange because \Drupal\Tests\workspaces\Functional\Rest\WorkspaceResourceTestBase uses

     protected static $firstCreatedEntityId = 'running_on_faith';
   
     protected static $secondCreatedEntityId = 'running_on_faith_2';
   

6. +++ b/core/modules/jsonapi/tests/src/Functional/WorkspaceTest.php
   @@ -0,0 +1,209 @@
   +  protected function setUpAuthorization($method) {
   +    $this->grantPermissionsToTestedRole(['administer workspaces']);
   +  }
   

   🤔 Why doesn't this grant only the create workspace permission in case of POST?

7. +++ b/core/modules/jsonapi/tests/src/Functional/WorkspaceTest.php
   @@ -0,0 +1,209 @@
   +  protected function getPatchDocument() {
   +    $patch_document = parent::getPatchDocument();
   +    unset($patch_document['data']['attributes']['id']);
   +    return $patch_document;
   +  }
   

   🤔 Why does this need to be unset? This is the only entity type that needs this. What makes workspace entities so special?

8. +++ b/core/modules/jsonapi/tests/src/Functional/WorkspaceTest.php
   @@ -0,0 +1,209 @@
   +    $modified['data']['attributes']['id'] = strtolower($this->randomMachineName());
   

   Similar question here.

9. +++ b/core/modules/jsonapi/tests/src/Functional/WorkspaceTest.php
   @@ -0,0 +1,209 @@
   +    // @see \Drupal\media\MediaAccessControlHandler::checkAccess()
   

   🤓 Wrong @see.

10. +++ b/core/modules/workspaces/src/EntityTypeInfo.php
    @@ -86,6 +86,9 @@ public function entityTypeBuild(array &$entity_types) {
    +    // Workspace entities can not be moderated because they use string IDs.
    +    $entity_types['workspace']->setHandlerClass('moderation', '');
    

    🤔 Can we add an @see here? I can't find any other entity type with such a comment, so it's hard to figure out why string IDs are a problem.

11. +++ b/core/modules/workspaces/src/WorkspaceAccessControlHandler.php
    @@ -19,23 +19,23 @@ class WorkspaceAccessControlHandler extends EntityAccessControlHandler {
    -    if ($account->hasPermission('administer workspaces')) {
    -      return AccessResult::allowed()->cachePerPermissions();
    -    }
    +    $access_result = AccessResult::allowedIfHasPermission($account, 'administer workspaces');
     
         // @todo Consider adding explicit "publish any|own workspace" permissions in
         //   https://www.drupal.org/project/drupal/issues/3084260.
         $permission_operation = ($operation === 'update' || $operation === 'publish') ? 'edit' : $operation;
     
         // Check if the user has permission to access all workspaces.
    -    $access_result = AccessResult::allowedIfHasPermission($account, $permission_operation . ' any workspace');
    +    $any_access_result = AccessResult::allowedIfHasPermission($account, $permission_operation . ' any workspace');
    +    $access_result = $access_result->orIf($any_access_result);
     
         // Check if it's their own workspace, and they have permission to access
         // their own workspace.
         if ($access_result->isNeutral() && $account->isAuthenticated() && $account->id() === $entity->getOwnerId()) {
    -      $access_result = AccessResult::allowedIfHasPermission($account, $permission_operation . ' own workspace')
    +      $own_access_result = AccessResult::allowedIfHasPermission($account, $permission_operation . ' own workspace')
             ->cachePerUser()
             ->addCacheableDependency($entity);
    +      $access_result = $access_result->orIf($own_access_result);
         }
    

    🤔 These changes look great! But they also seem out of scope. This makes me suspect that this is being changed to fix a (cacheability) bug in the existing workspace access control handler. If so, that should have explicit test coverage. If you disagree: I'd love to hear why, but the argument needs to especially convince a core committer, not me :)

12. +++ b/core/modules/workspaces/src/WorkspaceAccessControlHandler.php
    @@ -45,7 +45,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
    -    return AccessResult::allowedIfHasPermission($account, 'create workspace');
    +    return AccessResult::allowedIfHasPermissions($account, ['administer workspaces', 'create workspace'], 'OR');
       }
    

    ✅ This seems like a trivial enough bugfix/improvement to be included here.

    🤔 But … it also makes me wonder why administer workspaces does not have restrict access: true set in workspaces.permissions.yml?

13. +++ b/core/modules/workspaces/tests/src/Functional/Hal/WorkspaceHalJsonTestBase.php
    @@ -0,0 +1,70 @@
    + * Base hal_json test class for the path_alias entity type.
    

    🤓🐛 path_alias → workspace

    Other than that nitpick these Workspace HAL+JSON tests looks great 👍

14. +++ b/core/modules/workspaces/tests/src/Functional/Rest/WorkspaceResourceTestBase.php
    @@ -178,16 +172,12 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
         switch ($method) {
           case 'GET':
    -        return "The 'view any workspace' permission is required.";
    -      break;
    -      case 'POST':
    -        return "The 'create workspace' permission is required.";
    -      break;
           case 'PATCH':
    -        return "The 'edit any workspace' permission is required.";
    -      break;
           case 'DELETE':
    -        return "The 'delete any workspace' permission is required.";
    +        return "The 'administer workspaces' permission is required.";
    +      break;
    +      case 'POST':
    +        return "The following permissions are required: 'administer workspaces' OR 'create workspace'.";
           break;
         }
    

    🤔 These changes seem unnecessary?

This blocks #3088643: Mark Workspaces as a stable core module.

RE #10.4: Worked with @amateescu in Drupal Slack. I now finally understand why this was there. It's because the id field was aliased to drupal_internal__id and he didn't know JSON:API would also respect aliases on POST requests. (If it didn't, that'd make for one hell of a confusing user experience.)

Just making the POST request send that allows the code I was concerned about to be deleted.

That makes this patch a lot easier to understand :)

+++ b/core/modules/jsonapi/tests/src/Functional/WorkspaceTest.php
@@ -0,0 +1,227 @@
+  protected function getModifiedEntityForPostTesting() {
+    $modified = parent::getModifiedEntityForPostTesting();
+    $modified['data']['attributes']['drupal_internal__id'] = strtolower($this->randomMachineName());
+    return $modified;
+  }

The previous interdiff changed this. But we shouldn't need this at all. getModifiedEntityForPostTesting() is smart enough to not generate an entity with the same value for a field that has a uniqueness constraint.

But we need to tell the base test class what fields need to be unique. So doing that. This requires knowing the JSON:API test infrastructure, @amateescu could only have figured this out by diving deeper into the existing tests. So did it for him :)

#12:

IDK about "always" but I sure try :D

1. 👍 Right! I think it'd be helpful to post a patch without this one hunk — it should fail. That'll prove the test coverage covers this.

3. ✅
4. See #15 + #16.
5. ✅
6. See #15 + #16.
7. See #15 + #16.
8. 🙏 Ahhhh! Thanks!
9. Sorry to hear that. 🤐 Yeah this test infrastructure is not trivial to understand, because it requires knowing both Entity/Field API intricacies and assumptions, and JSON:API spec assumptions. We then generically test across entity types where those different assumptions clash, and verify that all things are possible from both of those perspectives.
10. ✅
11. ✅

More detailed patch review based on having dealt with #15 and #16

With #15 and #16, \Drupal\Tests\jsonapi\Functional\WorkspaceTest looks much simpler, making it easier to maintain in the future. That was my goal 🙂 👍

I spotted two more things thanks to that work, and they don't require knowing this test infrastructure in detail:

1. +++ b/core/modules/jsonapi/tests/src/Functional/WorkspaceTest.php
   @@ -0,0 +1,227 @@
   +          'drupal_internal__id' => 'updated_campaign',
   +          'label' => 'Updated campaign',
   

   It's confusing that the keyword "updated" is used in here, because this is used to create a new workspace. Let's rename that?

2. We should be able to remove
   

   +++ b/core/modules/jsonapi/tests/src/Functional/WorkspaceTest.php
   @@ -0,0 +1,223 @@
   +  protected function createAnotherEntity($key) {
   +    $duplicate = $this->getEntityDuplicate($this->entity, $key);
   +    $duplicate->set('id', 'duplicate');
   +    $duplicate->set('field_rest_test', 'Second collection entity');
   +    $duplicate->save();
   +    return $duplicate;
   +  }
   

   because the only reason we have this, is to ->set('id', 'duplicate').

   The proper way to fix this is by adding a createDuplicate() override to \Drupal\workspaces\Entity\Workspace. It should get something like this:

   public function createDuplicate() {
       $duplicate = parent::createDuplicate();
       $duplicate->id->value = $this->id->value . '_duplicate';
       return $duplicate;
     }
   

   That way, it works for all Entity API users, of which the JSON:API module is just one!

Perfect: my last remarks were fixed and we have a failing test-only patch without the most tricky change.

_{Regarding that tricky change: The fact that that hasn't been reported yet means that literally nobody has ever tried to access workspace entities using JSON:API 🙃}

Fun stuff, can anybody explain the drupal:9.0.0 part of these failures whilst were testing against 8.9.x?

Drupal\Tests\BrowserTestBase::$defaultTheme is required in drupal:9.0.0 when using an install profile that does not set a default theme. See https://www.drupal.org/node/2352949, which includes recommendations on which theme to use.

@amateescu Nice catch, was pondering what was going on there.

As mentioned in comment #30.

Patch in #27 no longer applies - Needs Reroll.

Rerolled patch from #27.

1. +++ b/core/modules/workspaces/src/Entity/Workspace.php
   @@ -185,4 +185,13 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function createDuplicate() {
   +    $duplicate = parent::createDuplicate();
   +    $duplicate->id->value = $this->id() . '_duplicate';
   +    return $duplicate;
   +  }
   

   This change deserve it's own blocking issue. There's a decent concern we should have about the id() becoming too long - there's a max length of 128. There's also a question about whether we should fix this for all string ID entity types in \Drupal\Core\Entity\ContentEntityBase::createDuplicate()

2. +++ b/core/modules/workspaces/src/EntityTypeInfo.php
   @@ -83,6 +83,11 @@ public function entityTypeBuild(array &$entity_types) {
   +    // Workspace entities can not be moderated because they use string IDs.
   +    // @see \Drupal\content_moderation\Entity\ContentModerationState::baseFieldDefinitions()
   +    // where the target entity ID is defined as an integer.
   +    $entity_types['workspace']->setHandlerClass('moderation', '');
   

   This seems to be of wider scope than this issue - should it be in its own issue so it can be tested?

3. +++ b/core/modules/workspaces/workspaces.permissions.yml
   @@ -1,5 +1,6 @@
    administer workspaces:
      title: Administer workspaces
   +  restrict access: TRUE
   

   I think this should be in its own issue - if this wasn't an experimental module this would be a security issue.

This was postponed on two issues that has noe been closed, changing status.

Using block_content json tests as references and a few other. The test coverage seems to line up with what we've done in others.

LGTM!

I looked at this two or three times and the UNCACHEABLE/MISS changes put me off committing it just yet, but... I can't think of anything else we can do here, and this is in the base class for test coverage that even the subclasses don't directly interact with, it'll make it less likely that the next entity type has to do anything special, then noticed Wim pointing out we already do the same thing elsewhere already in the test coverage. So... committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!