EntityResource: Provide comprehensive test coverage for BaseFieldOverride entity + add missing access control handler

This patch also adds an access handler \Drupal\Core\Field\BaseFieldOverrideAccessControlHandler to \Drupal\Core\Field\Entity\BaseFieldOverride. Let's see what fails.

+++ b/core/lib/Drupal/Core/Field/BaseFieldOverrideAccessControlHandler.php
@@ -0,0 +1,22 @@
+  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
+    return AccessResult::allowedIfHasPermission($account, 'administer ' . $entity->getTargetEntityTypeId() . ' fields');
+  }

This is problematic because it doesn't call the parent implementation so it misses e.g. the

if ($operation == 'delete' && $entity->isNew()) {
      return AccessResult::forbidden()->addCacheableDependency($entity);
    }

part. I think it should do $parent_access->orIf(...) or something like that.

First: thanks for your review — I'm very very very glad an Entity API maintainer is reviewing this :)

I was wondering what you meant by "parent". I'm assuming you're referring to the FieldConfig config entity type now. Because:

class BaseFieldOverride extends FieldConfigBase {

But … class FieldConfig extends FieldConfigBase implements FieldConfigInterface {.

IOW: BaseFieldOverride is not a child of FieldConfig, it's a sibling.

Doesn't that mean that we should reuse \Drupal\field\FieldConfigAccessControlHandler here?

Oops, sorry for being so brief. No I actually meant parent as in

parent::checkAccess();

I.e. the default implementation in \Drupal\Core\Entity\EntityAccessControlHandler::checkAccess(). That for example includes a check that you are never allowed to delete an entity if it is new. That is something that we lose by overriding the method without calling the parent.

Thanks for clarifying :)

But my question still stands though: BaseFieldOverride could reuse \Drupal\field\FieldConfigAccessControlHandler, and IMHO/AFAICT it should. What do you think?

Very interesting idea. I checked and that's actually not possible, though. Because the associated field storage definitions of BaseFIeldOverride entities are BaseFieldDefinition objects and not FieldStorageConfig's, so they do not have isLocked().

Ahhh, right :) Thanks for checking!

@arshadcn: So then please change the patch according to @tstoeckler's guidance in #8! :)

@tstoeckler++

@tstoeckler do you mean something like this?

Looks good to me! I guess in theory I could now complain about a missing kernel test for the added base field access control handler, but since it's literally two lines of code and the whole point of this is really extensive Rest test coverage, I hope we can sneak this by the committers without it. On the other hand @arshadcn if you would like to gain some extra credit ;-), feel free to write a kernel test for the new access handler.

It looks like it was a random failure. Returning RTBC status

#13: I'd strongly -1 a KernelTestBase test. We'd want a unit test for this.

And given that 99% of entity access control handlers don't have explicit test coverage, and the test coverage added here is pretty explicit, I think this is still a net step forward :)

Without a doubt this is a Drupal CI infra fail:

00:07:02.781 Build was aborted
00:07:02.781 ERROR: Step ‘Archive the artifacts’ failed: no workspace for drupal_patches #10312
00:07:02.782 Checking console output
00:07:02.904 ERROR: Step ‘Publish JUnit test result report’ failed: no workspace for drupal_patches #10312
00:07:02.922 Finished: ABORTED

Committed 5dbe710 and pushed to 8.4.x. Thanks!

Given the new access control handler I think we need to discuss this before backporting to 8.3.x

diff --git a/core/modules/rest/tests/src/Functional/EntityResource/BaseFieldOverride/BaseFieldOverrideResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/BaseFieldOverride/BaseFieldOverrideResourceTestBase.php
index d3ed8bd..1f4b03a 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/BaseFieldOverride/BaseFieldOverrideResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/BaseFieldOverride/BaseFieldOverrideResourceTestBase.php
@@ -3,8 +3,6 @@
 namespace Drupal\Tests\rest\Functional\EntityResource\BaseFieldOverride;
 
 use Drupal\Core\Field\Entity\BaseFieldOverride;
-use Drupal\field\Entity\FieldConfig;
-use Drupal\field\Entity\FieldStorageConfig;
 use Drupal\node\Entity\NodeType;
 use Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase;
 

Unused uses fixed on commit.

Discussed with @xjm. We agreed that there is an issue with issue scope here. We need a better issue title and issue summary because this is introducing non test changes and fixing missing things. Like the lack of a access control handler for basfieldoverride - is that a bug, security hole (I don;'t think so because I don't think that by enabling rest on basfieldoverride entities you can create them in HEAD) or a task because we adding missing stuff?

a task because we adding missing stuff?

This.

It'a a task, because many (most) config entity types do not specify an access control handler, nor an admin_permission.

See #2870018-6: Should the 'access content' permission be used to control access to viewing configuration entities via REST for more detail.

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').

However, for this particular case (\Drupal\Core\Field\Entity\BaseFieldOverride), an admin_permission does not make sense, just like it does not make sense for \Drupal\field\Entity\FieldStorageConfig (a semantical sibling). So what we do instead, is duplicate the exact same approach as \Drupal\field\FieldStorageConfigAccessControlHandler:

return $access->orIf(AccessResult::allowedIfHasPermission($account, 'administer ' . $entity->getTargetEntityTypeId() . ' fields'));

This should be considered the equivalent of admin_permission for this config entity type. Therefore the latest patch already is actually implementing the consensus from #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST.

Changing to a bug because this is a fixing an access control bug in base field overrides.

Committed and pushed 2623bb6 to 8.4.x and 6695bf0 to 8.3.x. Thanks!

diff --git a/core/modules/rest/tests/src/Functional/EntityResource/BaseFieldOverride/BaseFieldOverrideResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/BaseFieldOverride/BaseFieldOverrideResourceTestBase.php
index d3ed8bd..1f4b03a 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/BaseFieldOverride/BaseFieldOverrideResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/BaseFieldOverride/BaseFieldOverrideResourceTestBase.php
@@ -3,8 +3,6 @@
 namespace Drupal\Tests\rest\Functional\EntityResource\BaseFieldOverride;
 
 use Drupal\Core\Field\Entity\BaseFieldOverride;
-use Drupal\field\Entity\FieldConfig;
-use Drupal\field\Entity\FieldStorageConfig;
 use Drupal\node\Entity\NodeType;
 use Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase;
 

Remove unused uses.

Yay, thanks! Updated #2824572: Write EntityResourceTestBase subclasses for every other entity type..