Media Types missing access control handler result in empty column in media overview page

Thanks for reporting.

This is in fact a bug because the column shouldn't be empty if the user has permission to access the view in the first place.

Also, IMHO we shouldn't be exposing "Source" as the column title, once the bundle entity label is "Media Type".

The patch attached should fix both points.

OK, let's try a less drastic approach.

Also, realized that we have no test coverage whatsoever for the media overview page. I believe it would be weird to test just the label we are fixing here, and also out-of-scope to test everything in this issue, so I just opened #2932756: [PP-1] Add test coverage for media overview page to create a test that tests everything on that page.

1. +++ b/core/modules/media/config/optional/views.view.media.yml
   @@ -614,7 +614,7 @@ display:
              expose:
                operator_id: bundle_op
   -            label: Source
   +            label: 'Media Type'
                description: ''
                use_operator: false
                operator: bundle_op
   

   Do we need "Media Type" or would just "Type" be enough since we are listing media entities?

2. +++ b/core/modules/media/src/MediaTypeAccessControlHandler.php
   @@ -0,0 +1,36 @@
   +  /**
   +   * Allows to grant access to just the labels.
   +   *
   +   * @var bool
   +   */
   +  protected $viewLabelOperation = TRUE;
   

   I think we usually just use @inheritdoc on the propertiy

IMHO, I'd just combine this and tests into a single issue. Committing fixes without tests seems wrong, and if we need some tests anyway?

Thanks @Berdir for reviewing!

IMHO, I'd just combine this and tests into a single issue. Committing fixes without tests seems wrong, and if we need some tests anyway?

Fair enough. I've closed the other one as duplicate and added some general tests for the view on this patch. Please let me know if something important is missing coverage.

Also, including a test-only patch to demonstrate the bug.

Thanks!

Oh irony :)

This looks good to me. @Berdirs feedback was addressed, we have a failing patch to show the issue and a working patch to fix it.

Let's do this.

1. +++ b/core/modules/media/config/optional/views.view.media.yml
   @@ -252,7 +252,7 @@ display:
   +          label: 'Type'
   
   @@ -614,7 +614,7 @@ display:
   +            label: 'Type'
   

   Very minor nit but since this would cause an unnecessary difference between config supplied by module and that which is exported from a site probably worth addressing. The quotes are unnecessary and would not be added by the config exporter.

   Also this feels like an unrelated change and whilst correct probably worth its own issue. I.e. it is not necessary for the access fix.

2. +++ b/core/modules/media/src/MediaTypeAccessControlHandler.php
   @@ -0,0 +1,34 @@
   +    if ($operation === 'view label' && $entity instanceof MediaTypeInterface) {
   

   Given this is the MediaTypeAccessControlHandler checking the type of $entity seems overkill. We don't check in \Drupal\user\UserAccessControlHandler() for example.

3. +++ b/core/modules/media/src/MediaTypeAccessControlHandler.php
   @@ -0,0 +1,34 @@
   +      return AccessResult::allowedIfHasPermission($account, 'view media');
   

   I think this should also be allowed if the user has the admin permission "administer media types". Although I do wonder if we should just allow access here - like date formats, menus etc... I'm not sure. Also is the permission correct. To see the view you need the permission 'access media overview' not 'view media'.

4. If we are restricting access to the label then, as far as I can see, the current test does not cover the case where the user does not have access to the label.

Thanks for reviewing @alexpott!

1. Fixed, moved this change to #2934649: Fix wrong header "Source" on media overview page

2. Fixed

3.

Although I do wonder if we should just allow access here - like date formats, menus etc... I'm not sure. Also is the permission correct. To see the view you need the permission 'access media overview' not 'view media'.

We've gone with view media because this is the less restrictive permission, i.e., it would be unlikely to have a site where users can access media overview or administer media types but not view media. And it's not the same permission as the view (access media overviewbecause I'm not sure if the label access could be checked in other scenarios as well? Not sure though.
On the other hand, I wouldn't blindly grant access here... I do think there may be legitimate use cases of sites where "all things media-related" may be separate from some parts of the site / users.

4. Fixed.

Looks fine to me. Having the media type label available for the view media permission makes sense to me.

The cache clear call is a a strange workaround. I'd recommend to just create a second user without the permission and pass each user explicitly to ->access().

Yeah, I couldn't find a better way of clearing the access cache. But I agree an approach with 2 users seems cleaner.
Also, I moved that test to a kernel test, because it seemed to me it makes more sense there (instead of inside MediaOverviewPageTest, which is not strictly about testing the access handler).

Thanks!

+++ b/core/modules/media/tests/src/Kernel/MediaCreationTest.php
@@ -33,6 +35,27 @@ public function testMediaTypeCreation() {
+    $user1 = User::create([
+      'name' => 'username1',
+      'status' => 1,
+    ]);
+    $user1->save();
+    $user2 = User::create([

users in kernel tests are a bit tricky because there is no user by default, so the first user is uid 1 and has full access to everything.

That's not the case here though because the base class already creates a user, so we should be OK.

Still, can you upload a test-only patch that fails for both the overview and this to be sure?

+1 on moving this to a kernel test, was thinking about the same.

Thanks @Berdir for the feedback!

Here's a test-only patch that should fail.

Great, failed as expected, #17 is RTBC imho.

@balsama is going to add a couple screenshots here.

Quick manual test.

1. Created a couple of media entities (one File and one Image)
2. Created a role + user with the "Access media overview" permission only
3. Logged in with said user and visited "/admin/content/media".
4. Result, "Source" column exists and is empty:
   
   
5. Applied patch from #17.
6. Result, "Source" column exists and is populated with the bundle name:
   
   

All set with the ol' screenshots.

Great test coverage! And thanks @balsama, those screenshots are just what I was looking for.

Committed and pushed to 8.5.x. Thanks!

This patch appears to have fixed an issue I had in 8.4.4 whereby Views Grouping field Nr.2 headers were seen only by the administrator. Now solved in 8.5.0-beta1. Thank you very much.