EntityResource: Provide comprehensive test coverage for BlockContent entity

Working on this.

This is work in progress. Posting it here as reference for #2851905: BlockContent entities are accessible to anonymous users when a REST endpoint is enabled.

I think we can let testbot check it, no?

Thanks @Wim Leers.

As a result I believe we can't really use \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testGet to test BlockContentResourceTestBase since we are seeing 200 everywhere?

We're not seeing 200 everywhere. From the test output:

Drupal\Core\Entity\EntityStorageException: 'block_content_type' entity with ID 'basic' already exists.

I think the solution is to remove this:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/BlockContent/BlockContentResourceTestBase.php
@@ -0,0 +1,147 @@
+    $block_content_type = BlockContentType::create(array(
+      'id' => 'basic',
+      'label' => 'basic',
+      'revision' => FALSE
+    ));
+    $block_content_type->save();

Also, the whole point is that we should not be seeing 200 everywhere. The tests are fine, what we need to fix is BlockAccessControlHandler. For that, we have #2851905: BlockContent entities are accessible to anonymous users when a REST endpoint is enabled. I think it makes sense to merge this patch with #2851905: BlockContent entities are accessible to anonymous users when a REST endpoint is enabled, i.e. to mark this as a duplicate of #2851905.

"Everywhere" was the wrong word choice here :)

I meant we're seeing 200 where 403 is expected in \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testGet for BlockContentJsonAnonTest

Yep, that makes sense :)

Thanks for your help @vaplas. See this related issue #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" about the access.

Thanks for the update @vaplas. I'll send a new patch.

Postponing this on #2843754: EntityResource: Provide comprehensive test coverage for Feed entity per #17.

#2843754: EntityResource: Provide comprehensive test coverage for Feed entity landed, this is now unblocked!

Let's get this rerolled, see #17.

+++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
@@ -19,6 +19,7 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
+      return AccessResult::allowedIfHasPermission($account, 'administer blocks');

This is … a huge problem. Because \Drupal\block_content\Plugin\Block\BlockContentBlock::blockAccess() does this:

  protected function blockAccess(AccountInterface $account) {
    if ($this->getEntity()) {
      return $this->getEntity()->access('view', $account, TRUE);
    }
    return AccessResult::forbidden();
  }

Therefore this change causes "custom blocks (block_content blocks) to only be visible for users that have the administer blocks permission.

That's obviously a massive BC break.

It's closely related to #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity".

I think the only acceptable solution here is to require the access content permission, per #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST.

+++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
@@ -19,9 +19,15 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
   protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
     if ($operation === 'view') {
-      return AccessResult::allowed();
+      $result = AccessResult::allowedIfHasPermission($account, 'access content');
     }

Same as in the file issue, this introduces a hidden dependency on the node module and I can definitely imagine situations here where this would break stuff.

Simple example. A site does not give access content to anon/auth/some users but they can see blocks that explain that they need to e.g. register to be able to see the content.

I don't know. Many of our entity types are designed for specific use cases, in case of block_content, it's being displayed in blocks. They don't need view access checks there because that's what block visibility conditions are used for. Except we can't reverse-check them for REST because we don't know the connection and there could also be different blocks with different logic.

What I mean is that a if a user creates a custom block and displays it with visibility condition so that only admins can see it, would he except that to be respected in REST as well? :)

I feel like we're trying to make a change here for the only purpose of being able to test it with our standard framework. From Wim very early in this issue:

> Also, the whole point is that we should not be seeing 200 everywhere.

But why not? Just so our tests are happy? Tests have to follow the actual use case, not the other way round :) Either we actually respect the real way acess/visibility is done for block_content (which I don't think we can, as we would actually need to access block entities and check access for those, which means we need to respect things like the URL context) or we shouldn't bother at all.

I have to warn you, I've already had quite a few discussions with Wim and others on REST and API First and so on. And I'm somewhat critical of the whole REST-for-decoupled sites approach as you'll see in a moment. This is your last chance to hide :)

This issue title could be translated to "Lets blindly write test coverage for block_content following the general assumptions of ResourceTestBase."

But there is a reason we have different entity types, they all have their things they are special about and specific use cases, if not, we could just make everything nodes.

So lets take a step back and figure out what exactly API/REST access means for block_content entities. IMHO, there are different major use cases that come up:

1. Data synchronisation between sites, where you want the raw data.
This is IMHO the use case that rest.module was written for and works (worked*) quite well for that. But in such a case, you likely want to have direct access to REST limited to your server processes and not everyone in the internet. IMHO that's why rest.module used to define special rest-specific permissions that we removed. So, it doesn't really matter for most sites if viewing those entities over REST requires no permission or access content, both are actually too open and we kind of broke that use case already a while ago.

2. Decoupled sites that want to show content from those blocks to users, requested from normal/anon users
I think whether or not your users have access content is pretty irrelevant for deciding if a block_content entity should be shown or not. What matters is the block visibility settings in case of standard block placement. But how do you even know which blocks you should display? On normal Drupal sites, that's done through a block placement, with URL/language/role/node type.. visibility rules. What you really need IMHO for this use case is integration with block entities and possibly even a service that basically exposes BlockRepository and gives you a way to get all blocks that should be displayed for a certain URL/User and their content. As Wim said, this is related to #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" (there is nothing config entity or block specific about that issue actually, that applies to all entities). I have actually no idea how decoupled sites do that today, but my guess is that they don't bother with the block system at all and just do all of that on the client side?

So by making this change and adding those tests, we actually don't get closer to either use case as far as I see? Tests should implement the actual use cases and verify those work, Assumptions in a test base class shouldn't be a justification to making changes to something that also affects things outside of REST. Also, all of those REST for $entity_type test groups result in a considerable test overhead that we also shouldn't blindly add to every single entity type we have without asking ourself what the benefit is.

*: I'm a bit worried that by trying to make REST better for decoupled sites, we make it worse for the original use case and end up with something that can't do either of those two use cases well.

Thanks for your excellent comment @Berdir! :)

1. I don't think we're making it work worse. Also, the permission changes that we made at #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources were A) approved by REST maintainer @klausi, B) "sync" was mentioned zero times on that issue, C) the issue (and the IS) points to comments at the time where the REST module was being added, where it's clearly being stated that those permissions were a temporary work-around.

   So I think that some group of people saw REST as primarily developed for sync use cases, others saw it for the general decoupled use case. That's an unfortunate misunderstanding, or lack of shared agreement/vision. I'm not sure how to change that now :(

   So, it doesn't really matter for most sites if viewing those entities over REST requires no permission or access content, both are actually too open and we kind of broke that use case already a while ago.

   Do you mean that you think it's okay for block_content content entities to be legible by the entire world? But then I'm still not sure what you mean by both are actually too open and we kind of broke that use case a while ago — could you clarify those?

2. Block visibility settings being arguably more important: very interesting! I've got a few thoughts:
   block visibility conditions live in block config entities ("Placed blocks"), not block_content content entities — so theoretically irrelevant on this issue, but I agree that in practice it's closely enough related to be relevant.
3. I think that for many decoupled use cases, block visibility conditions don't matter, because block placement is often controlled in template/JS code. Only the more advanced decoupled sites need block placement and therefore visibility conditions.
4. OTOH, I do think that just about every visibility condition can actually be evaluated on the client side, which means that there is no need for a BlockRepository-like service on the client side.

RE: #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" actually applying to all entity types — I can see why you'd interpret it that way, but I think that loses very important nuance. There's a key difference IMHO: nodes, comments, users, then view == "read the entity" == "render on page" (because the rendered representation is equivalent with being able to read all fields). But in the case of block entities (config entities), what's actually happening is not rendering the data stored in that entity — it's interpreting that data to render the associated block plugin in a particular place on the page. That's what makes block config entities special in this way.

So by making this change and adding those tests, we actually don't get closer to either use case as far as I see?

What this patch is doing from my POV:

1. it is not changing anything about use case 1 (data sync), except making it safer — because nothing in the normalization is being modified
2. it is making use case 2 safer

Also, all of those REST for $entity_type test groups result in a considerable test overhead that we also shouldn't blindly add to every single entity type we have without asking ourself what the benefit is.

The benefit is that we

1. A) know that every entity type actually is "API-First" — it's accessible via an API. Drupal 8 shipped claiming that, but as the many many dozens of bugfixes over the past year have proven: it does not remotely work for all entity types
2. B) know when we break something — the test coverage that was added in Q4 2016 and has been growing since has already prevented many regressions

Let's continue in a constructive manner. Can you explain how this patch is negatively impacting the data sync use case? I really would love to understand that better.

Do you mean that you think it's okay for block_content content entities to be legible by the entire world? But then I'm still not sure what you mean by both are actually too open and we kind of broke that use case a while ago — could you clarify those?

In many ways, block_content is like a backend entity that is not viewed directly like a node for example. As you said yourself, the block configs with their (visibility) configuration are the interaction point to the site, the block_content entity just provides the content inside.

For the sync use case, what you want is problably a list of all block_content entities and being able to fetch the complete raw data, not a formatted output. But that's not something that you want to make public to anyone, just your sync user/role, because normal visitors should not be able to fetch the raw data. Right now, there is no permission, which means it's not possible without using the deprecated BC option. This adds the "access content" permission, which does introduce certain problems, like breaking valid use cases where users are allowed to see blocks but not nodes. But it's still not safe enough for the data sync use case.

I didn't know that the permissions still exist as a BC fallback. Maybe we should simply discuss (in a separate issue obviously) to un-deprecate that setting and make it an official feature, that by default is off, then we already have much better protection for data sync use cases?

Block visibility settings being arguably more important: very interesting! I've got a few thoughts:
block visibility conditions live in block config entities ("Placed blocks"), not block_content content entities — so theoretically irrelevant on this issue, but I agree that in practice it's closely enough related to be relevant.

Yes, aware of that problem, which is why block_content is kind of a backend-thing that doesn't have its own access control as written above, I think I also mentioned the in a decoupled site, you might want to expose block entities and not block_content.

I think that for many decoupled use cases, block visibility conditions don't matter, because block placement is often controlled in template/JS code. Only the more advanced decoupled sites need block placement and therefore visibility conditions.

That cames back to my note that I actually have no clue how a decoupled site is handling blocks. If they don't care about block visibility (and block placement), then how do they know which block_content entities they need to load and display? As I wrote, unlike node, you never go to block_content/1 and they are not shown in lists based on references/type. block placement, block theme and block visibility configuration is the only thing that defines where they show up.

The only thing I can think of when not using those things is that you hardcode UUID's in the frontend and then fetch their data, so it's kind of configurable for editors. But then you don't really need block_content entities, you could do the same with nodes or terms or a custom entity type. This is IMHO again the conflict between We-Want-Generic-Always-Same-REST-API-For-All-Entities vs. the fact that different entity types are not the same thing and work differently, otherwise we would not need to bother with even having them and everything can be done with nodes :)

OTOH, I do think that just about every visibility condition can actually be evaluated on the client side, which means that there is no need for a BlockRepository-like service on the client side.

That could likely be done, but is that secure? If I configure role-based visibility conditions in a block (or in a view for example) then I would expect that really only those users can see that content and it's not always sent out and evaluated client side?

it is not changing anything about use case 1 (data sync), except making it safer — because nothing in the normalization is being modified
[...]
Let's continue in a constructive manner. Can you explain how this patch is negatively impacting the data sync use case? I really would love to understand that better.

I might have been a bit unclear on that. Yes, as I wrote above, this issue doesn't break use case 1, I meant that already kind of happened before like in the permission issue. But I also don't think it really makes it safer, as outlined above, on most sites, anonymous users have access content, which means their raw block_entity data is wide open whether we add the permission or not (just like node, just that the expectation of users might be different due to block visibility configuration).

it is making use case 2 safer

How exactly? I think it just breaks certain use cases. If you have a site where anonymous users have access content, then it makes zero difference and is not safer in any way. But if your site is different and anon users can't see the content, then I can totally imagine that you have put blocks on your page that are visibile to anonymous where you for example explain how to sign up. And by adding the permission check we don't make their site safer, we break it, because now that block is gone. Not just in REST, they don't even need to have that enabled, always, for everyone. So IMHO, adding the permission check here is a BC break.

My recommendation: Adjust the tests accordingly to not check for an access denied message, because block_content doesn't have that. And maye open a separate issue to make sense of blocks, block_content on decoupled sites, as a start, collect information of how developers are doing that now/what they need/how they think it should work. And then figure out a solution for that instead of trying to press everything into the One-Size-Fits-All-REST-Test.

PS: I'll respond to the part about #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" in that issue.

#2820848: Make BlockContent entities publishable landed.

In #32 we started discussing whether relying on the access content permission is okay or not. For the general problem of modules other than node using that permission, we now have #2907121: Move 'access content' permission to system module. And to improve the access handling of blocks, we have #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity".

#2820848: Make BlockContent entities publishable has given us a way forward here, allowing us not to need to change block's access control logic. In other words: tighter scope.

Let's finally get this done!

I removed the changes to BlockContentAccessControlHandler from the patch, because it conflicted heavily with changes in HEAD. No more changes are being made to that code.

The interdiff reflects all changes except those to BlockContentAccessControlHandler, which in the last patch (#31) was being modified, and is now no longer being modified. That also explains the size of the interdiff: many of the tests that needed to be updated due to the access control changes now no longer need to be updated!

Work for me assuming the tests pass, glad we found a solution we can agree on here ;) Doesn't solve all the fun things we started discussing but we can't solve all the problems at once.

❤️

Doesn't solve all the fun things we started discussing but we can't solve all the problems at once.

Exactly!

Looking back on this issue, the scope was simply too big! I should've seen that sooner, but I didn't want to see it, because it'd mean blocking this on something far more complex. My apologies.

Oh and #2907121: Move 'access content' permission to system module was marked as a duplicate of #2892821: Core modules check node module's "access content" permission for displaying things that have nothing to do with nodes. Updating list of related issues.

+class BlockContentHalJsonBasicAuthTest extends BlockContentHalJsonAnonTest {
+class BlockContentHalJsonCookieTest extends BlockContentHalJsonAnonTest {

Wouldn't a BlockContentHalJsonTestBase make more sense? Looks like in HEAD, we have both patterns (e.g., we have a CommentHalJsonTestBase but not a NodeHalJsonTestBase), but for anything new we add, let's pick a pattern we like and stick to it.

This is the pattern used most widely already.

We can still change it later if we want. These are just tests.

IIRC Comment is the only exception (in typing this on my phone).

The exceptions are: Comment, Feed and Item. Those 3 have a *HalJsonTestBase class. All others extend the *HalJsonAnonTest class.

That makes for dozens versus 3, so this is definitely the most widely used pattern.

Adding review credit to @Berdir.

All others extend the *HalJsonAnonTest class...That makes for dozens versus 3

class ActionHalJsonBasicAuthTest extends ActionResourceTestBase {
class BaseFieldOverrideHalJsonBasicAuthTest extends BaseFieldOverrideResourceTestBase {
class BlockHalJsonBasicAuthTest extends BlockResourceTestBase {
class BlockContentTypeHalJsonBasicAuthTest extends BlockContentTypeResourceTestBase {
... (and many more)

A basic auth test extending an anon test looks like a much rarer pattern to me. But we can leave consistency (in whichever direction we choose to apply it) for a followup, so I pushed #41 to 8.5.x.

As this is only test coverage, it seems like it would be a good addition to 8.4 as well, but #41 shows failures for that, so setting this to "Patch (to be ported)".

#2820848: Make BlockContent entities publishable only landed in 8.5.x, and per #40, we need that to be able to proceed here. Once the super tricky #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" is solved, this could theoretically be committed to 8.4.x, but in practice that is unlikely to happen.

Therefore, this is only realistically committable to 8.5.x, therefore marking this fixed :)