EntityResource: Provide comprehensive test coverage for DateFormat entity

I was looking at \Drupal\system\DateFormatAccessControlHandler::checkAccess and noticed we have the following code which makes DateFormat entities always accessible.

   // There are no restrictions on viewing a date format.
    if ($operation == 'view') {
      return AccessResult::allowed();
    }

The following test in \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testGet will probably fail:

// DX: 403 when unauthorized.
$response = $this->request('GET', $url, $request_options);
$this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response);
$this->assertArrayNotHasKey('Link', $response->getHeaders());

Should we patch EntityResourceTestBase.php for entities which are always 200?

Here's a patch to confirm this.

Here's the exposed JSON data:

{
  "uuid": "8ea8f25f-d2f5-4332-93bc-b79ac5ee4cd2",
  "langcode": "en",
  "status": true,
  "dependencies": [],
  "_core": {
    "default_config_hash": "M7yqicYkU36hRy5p9drAaGBBihhUD1OyujFrAaQ93ZE"
  },
  "id": "html_time",
  "label": "HTML Time",
  "locked": true,
  "pattern": "H:i:s"
}

#3: Interesting. I think we can use the same approach as #2808217-27: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission:

I wonder if we don't want to have some permission to obey? The Block, Comment, File and System modules all use the access content permission to do this, despite not depending on the node module.

So I propose we do the same here. If we fix that at some point, we'll need to move the access content permission to the System module and provide an upgrade path anyway.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/DateFormat/DateFormatResourceTestBase.php
@@ -0,0 +1,94 @@
+    // Remove the 'config:user.role.anonymous' cache tag.
+    return array_values(array_filter(parent::getExpectedCacheTags(), function ($tag) {
+      return $tag !== 'config:user.role.anonymous';
+    }));

Using array_diff() would be more elegant.

OK let's see what fails here.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/DateFormat/DateFormatResourceTestBase.php
@@ -0,0 +1,98 @@
+  protected function getExpectedCacheContexts() {
+    // @see ::createEntity()
+    return [
+      'url.site',
+      'user.permissions',
+    ];
+  }

This matches the parent implementation. So now we can simply delete this method altogether :)

After that's removed, this is RTBC!

Thanks. Updated.

CI is crashing again.

Re-testing.

+++ b/core/modules/system/src/DateFormatAccessControlHandler.php
@@ -20,7 +20,7 @@ class DateFormatAccessControlHandler extends EntityAccessControlHandler {
     // There are no restrictions on viewing a date format.
     if ($operation == 'view') {
-      return AccessResult::allowed();
+      return AccessResult::allowedIfHasPermission($account, 'access content');
     }

This is a bit weird. The examples given to justify this change are all content entities - and so 'access content' makes some sense for them - but not here imo. Why are we not just delegating to the parent in the case of view?

Why are we not just delegating to the parent in the case of view?

Because \Drupal\Core\Entity\EntityAccessControlHandler::checkAccess() does:

  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    if ($operation == 'delete' && $entity->isNew()) {
      return AccessResult::forbidden()->addCacheableDependency($entity);
    }
    if ($admin_permission = $this->entityType->getAdminPermission()) {
      return AccessResult::allowedIfHasPermission($account, $this->entityType->getAdminPermission());
    }
    else {
      // No opinion.
      return AccessResult::neutral();
    }
  }

And the admin_permission for DateFormat is 'administer site configuration'. So delegating to the parent would require that far too broad permission just to view date_format config entities.

This is blocked on #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST reaching consensus.

Consensus was achieved! Quoting #2870018-35: Should the 'access content' permission be used to control access to viewing configuration entities via REST:

1. config entities that do not yet have an access control handler nor an admin_permission (RdfMapping, Tour): add an admin_permission.
2. update all access control handlers for entity types that currently grant access without requiring any permission (View, Menu, File, DateFormat): change them to require the entity type's admin_permission or the entity-type specific "view" permission (yet to be added), so for example: AccessResult::allowedIfHasPermissions($account, ['view menu', 'administer menu'], 'OR').

+++ b/core/modules/system/src/DateFormatAccessControlHandler.php
@@ -18,12 +18,8 @@ class DateFormatAccessControlHandler extends EntityAccessControlHandler {
-    // There are no restrictions on viewing a date format.
-    if ($operation == 'view') {
-      return AccessResult::allowed();
-    }

Is this not an API change? Can you elaborate on the consequences of this?

It is tightening security, for something that should never have provided access blindly to everybody.

At the same time, it is extremely unlikely to affect anybody, because nothing is actually checking "view" access to DateFormat config entities today. REST is the first scenario where that is actually being checked, and so we are improving the access control handler here.

I share your concern about a theoretical BC break. I raised the same in the aforementioned issue. But dawehner and others convinced me that this is the safer, correct way to deal with this.

Some things are checking view access, for example entity reference fields as mentioned in the other issue. I'm not sure if there is a site out there with a date format entity reference, possibly not, but there are a number of config entities with perfectly valid use cases or where we're even doing it by default, like NodeType.

As I kind of suggested, if we do this, we should consider to at least keep the previous access for the view label operation.

Also, what's interesting is that viewing doesn't really mean the same for REST as it does for other kinds of viewing. It's perfectly valid to "view" a block in a traditional way, meaning, passing it through BlockViewBuilder. but viewing for rest is printing out its raw configuration, which is a very different thing.

For content entities, we have the field access as an additional layer of security. I'm pretty sure that many sites are accidently exposing a lot of internal data/fields through REST because usually those kind of fields are hidden by simply not printing them in the node template/view display configuration. But there we can at least claim that we *have* an API to solve that problem :)

As I kind of suggested, if we do this, we should consider to at least keep the previous access for the view label operation.

So you're saying that if this existed:

if ($operation === 'view') {
  return AccessResult::allowed();
}

then we must at least do this:

if ($operation === 'view label') {
  return AccessResult::allowed();
}

?

Also, what's interesting is that viewing doesn't really mean the same for REST as it does for other kinds of viewing. It's perfectly valid to "view" a block in a traditional way, meaning, passing it through BlockViewBuilder. but viewing for rest is printing out its raw configuration, which is a very different thing.

Yes, this is a huge design flaw in the current system. But fortunately, the problem is limited to pretty much just block config entities. Block config entities really describe how a block should be rendered. I already opened #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" for this more than half a year ago.

#20/#21 looks like it needs more discussion to me.

I migrated/quoted #20 + #21 to/in #2870018-43: Should the 'access content' permission be used to control access to viewing configuration entities via REST. I agree.

These are now the changes to the access control handler:

    * {@inheritdoc}
    */
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
-    // There are no restrictions on viewing a date format.
-    if ($operation == 'view') {
+    // There are no restrictions on viewing the label of a date format.
+    if ($operation === 'view label') {
       return AccessResult::allowed();
     }
     // Locked date formats cannot be updated or deleted.

In #2870018-47: Should the 'access content' permission be used to control access to viewing configuration entities via REST, Berdir pointed to https://www.drupal.org/node/2661092. Fixed.

And here is comprehensive test coverage for DateFormatAccessControlHandler, per @Berdir's request at #2870018-47: Should the 'access content' permission be used to control access to viewing configuration entities via REST.

Ugh, c/p remnant, which I didn't notice because I ran the test with the PHPUnit test runner instead of run-tests.sh…

Similar test coverage added to #2843783-14: EntityResource: Provide comprehensive test coverage for Menu entity.

Looks good to me! RTBC!

Committed 3a6bbc8 and pushed to 8.4.x. Thanks!

Thanks!

This patch introduced some tests which do explicit checking for UID 1. Can we please stop doing that? I'm trying to remove the special casing for UID 1 from core in #540008: Add a container parameter that can remove the special behavior of UID#1.

That said, we should avoid having tests rely on arcane voodoo (in this case UID 1) altogether. What does UID 1 tell a non-Drupal person who is looking at your tests to understand your code? Absolutely nothing.

Don't get me wrong, it's not my intention to discredit the work that has been done here. I'm just trying to raise awareness about tests needing to be explicit about their requirements. That my comment also shamelessly draws attention to the UID 1 issue is a nice side-effect :)

I understand you want to draw attention to that issue. I agree and applaud with the rationale/effort behind that issue.

But please look at the patch more closely, because:

+  public function testAccess($which_user, $which_entity, $view_label_access_result, $view_access_result, $update_access_result, $delete_access_result, $create_access_result) {
+    // We must always create user 1, so that a "normal" user has a ID >1.
+    $root_user = $this->drupalCreateUser();
+
+    if ($which_user === 'user1') {
+      $user = $root_user;
+    }
+    else {
+      $permissions = ($which_user === 'admin')
+        ? ['administer site configuration']
+        : [];
+      $user = $this->drupalCreateUser($permissions);
+    }
…
+  }
+
+  public function testAccessProvider() {
…
+
+    return [
+      'permissionless + unlocked' => [
+        'permissionless',
+        'unlocked',
…
+      ],
+      'permissionless + locked' => [
+        'permissionless',
+        'locked',
…
+      ],
+      'admin + unlocked' => [
+        'admin',
+        'unlocked',
…
+      ],
+      'admin + locked' => [
+        'admin',
+        'locked',
…
+      ],
+      'user1 + unlocked' => [
+        'user1',
+        'unlocked',
…
+      ],
+      'user1 + locked' => [
+        'user1',
+        'locked',
…
+      ],
+    ];
+  }

This issue is actually making things easier for #540008: Add a container parameter that can remove the special behavior of UID#1. It's NOT adding test coverage tied to user 1. It's doing that in addition to an "admin" user that has the necessary permissions explicitly assigned. So, in #540008, you will be able to remove those two last use cases and simplify the setup!

After having read the tests I was planning to do exactly that :)

My first instinct when seeing these new tests break the UID 1 issue was "damage control" as I noticed both came from an issue that is part of a larger whole. My bad for trying to act quickly. I'll simplify the tests for the UID 1 issue.

Thanks Wim!

np :)