Use the new 'view label' entity access check in the entity reference label formatter

+++ b/core/modules/user/src/Plugin/Field/FieldFormatter/AuthorFormatter.php
@@ -59,9 +59,7 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    */
   protected function checkAccess(EntityInterface $entity) {
-    // Always allow an entity author's username to be read, even if the current
-    // user does not have permission to view the entity author's profile.
-    return AccessResult::allowed();
+    return $entity->access('view label', NULL, TRUE);

Can we extend from EntityReferenceLabelFormatter and remove this method completely?

Unrelated thing that I just noticed: views doesn't implement isApplicable(), which means that every string field shows the username formatter.

As per #5.

Ok, thanks for the explanation. Fair enough, lets keep the extend. AuthorFormatter is a weird thing anyway.

So what effects will this have on existing sites?

I think this is fine for both branches, however it could use a change notice. While we're bringing things into line with how 7.x worked, new 8.x sites don't have that context, and there's a lot of FUD around Drupal and username enumeration so it'd be good to clearly document why we're doing the change and what the expectations are.

CNW for that, patch itself looks fine.

IMO this should be 8.2.x only. The CR looks good.

So I was confused on this issue why we are changing what went into DB dumps that (theoretically at least) represent a specific point in Drupal's history (especially the RC or 8.0.0) to test the upgrade path, rather than changing the assertions when we change a behavior.

Looking at the test failures for the original patch, it looks like there was/is a magical entity 12 that should have access denied. We expect one behavior for the fixture in 8.1.x, and a different in 8.2.x.

It's also a bit confusing to review since there is just one massive dump we are changing and so from the patch we can't remove the specific changes to any generation script/etc. Back in the day we used to have a series of partial dumps for specific upgrade path tests so that we could cumulatively add new test coverage for new upgrade paths.

Part of the issue in HEAD I guess is that the test behavior is based on the specific contents of the fixture that seem kind of arbitrary and are zipped up in a binary file, rather than a part of the test. To some extent it has to be that way I guess, and certainly redoing the test is not in scope here.

I asked @amateescu and catch about this, but leaving at RTBC for now for feedback.

Hope this makes sense; dashing off a quick comment before my flight so I don't forget.

NR for info on #27.

Moving to right component. Back to RTBC to get a response.

RTBC +1

So I am not sure how #4 answers my questions from #27, but another committer could review it as well.

A current do-not-test patch without the binary files would make reviewing this a little more friendly. :)

@xjm I think the change to the db is OK - essentially we added arbitrary content at the time to check access, the expectations after upgrade have changed with that specific data-set, but if we change the dump to include the full entity view formatter then we can test the expectations the test was originally meant to do. If we removed the assertions instead, we'd be losing test coverage for the access after update and something might break it without us knowing later. As long as the new test data is equivalent to what an actual 8.0.x site would have then we've not lost any coverage either.

I'm not sure that the database dumps have been changed correctly. Obviously it is really hard to work with but the new filled file is 5115609 bytes once extracted and the old filled file is 5209878 bytes. This could be a simple as a cleared cache but content seems to have been removed. For example, if you do this command:

gunzip -c core/modules/system/tests/fixtures/update/drupal-8-rc1.filled.standard.php.gz | grep "Drupalaton Day Zero"

you find content in the HEAD version and not in the patched version. I guess this might be RSS feed related. But it is questionable that this change should have such affects on the data.

Back to RTBC then :)

My example scenario is that we allow view access to node types for access content, which on 99% of the sites means anonymous users. That was not really an issue so far, but now we have config entity GET support in rest.module. So if you have that, everyone can read anything our of your node type settings, and there might be third party settings or so in there that you don't want to be public.

That view access is necessary to be able to see the node type label in views for example, similar to the referenced paragraphs issue. That's exactly why we added it in #2561331: When 'administer content types' permission is disabled, content type field is hidden in content manager. With this, we could actually switch this to use the view label operation. Not sure if we want to do that, certainly not in this issue, as it could break something. But we at least allow it then for entity types where the distinction between view and view label is important, like user, so they don't need a custom formatter anymore to suppor that.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Created #2853984: Remove unused use statement in AuthorFormatter.php as follow-up.

I just ran into this issue after hours of debugging wondering why on drupal 8.2.x My labels where displayed alright and drupal 8.3.x the access was denied even for admin users.

I was implementing my own AccessControlHandler for a custom entity and "view label" was not an operation I was aware of.

Can you please reopen so a proper change record can be added?

Also, what do you mean with custom access control handler? *Not* extending the default one? If you did, then this should not have broken anything, as it falls back to the view operation unless a special property is set. If you do not extend from that (which is strongly recommended as you don't get any of the default logic then, like the hooks) then I guess this could break, yes.

Ok now I see.

I guess what we do is not very standard since we set some bundles as read-only so if the operation is not "view" we deny access
http://cgit.drupalcode.org/external_entities/tree/src/ExternalEntityAcce...

Is just a weird case, I guess the change record doesn't need more explanation.

Thanks for the clarification :)

The isForbidden() call in checkAccess() is not needed, as checkAccess() is not even called if there is already a forbidde result.

The forbidden check for non-view operations seems to be duplicated in checkAccess() and access(), which is most likely exactly what breaks it in your case.

If you remove that and move the edit/delete/view check in checkAccess() then I think everything would just work without any view label related changes. But by having that logic in access() you a) break the BC layer we added for the view label operation and b) it also means that hook implementations can not allow access, only deny.

Sorry for digging up this old issue but the people who were involved might be interested in taking a look at #3246579: Entity reference label formatter may render link to inaccessible entity.