Add per-media type creation permissions for media

Here's an initial patch from Drupalcon.

Crabcakes!

YAY!! Thank you, @sunset_bill, for the patch :)

This is a bit premature, but I am 100% in favour of having this functionality in core Media. I can see many potential use cases for it in complex, media-heavy sites.

It will still need tests, and we'll need to reroll it once #2831274: Bring Media entity module to core as Media module lands. This is also a somewhat lower-priority issue for the Media Initiative. So I'm postponing this for now.

#2831274: Bring Media entity module to core as Media module landed, so this is now active!

Here's the straight reroll of the patch.

Thank you, @naveenvalecha!

1. +++ b/core/modules/media/media.permissions.yml
   @@ -23,3 +23,6 @@ delete any media:
   +permission_callbacks:
   +  - \Drupal\media\MediaPermissions::mediaTypePermissions
   \ No newline at end of file
   

   Let us add a newline so as not to enrage the Git gods...

2. +++ b/core/modules/media/src/MediaPermissions.php
   @@ -0,0 +1,77 @@
   + * Provides dynamic permissions for medias of different types.
   

   "medias" should be "media items".

3. +++ b/core/modules/media/src/MediaPermissions.php
   @@ -0,0 +1,77 @@
   +        'title' => $this->t('%type_name: Create new content', $type_params),
   +      ],
   +      "edit own $type_id content" => [
   +        'title' => $this->t('%type_name: Edit own content', $type_params),
   

   These permissions should use the word "media", not "content". I think this is copypasta from Node.

On top of this, we'll definitely need tests.

will work on this issue during this week-end

Worked together with @hazong on this patch. It was an tricky issue because user_role_revoke_permissions load a new instance of the role object, but ::grantPermissions uses the already instantiated object from the very beginning of the test.

Please give @hazong commit credit on this issue. Thanks!

EDIT: Sorry, made a mistake.

Er...the patch in #14 only has changes to MediaAccessTest, but apparently no changes to the permissions?!

Thats the interdiff hehe :)

Looks great! Tons of test coverage! I have but a few minor things before it's RTBC...

1. +++ b/core/modules/media/src/MediaPermissions.php
   @@ -0,0 +1,64 @@
   +    foreach (MediaType::loadMultiple() as $type) {
   

   If we make MediaPermissions implement ContainerInjectionInterface, we can inject the entity type manager instead :)

2. +++ b/core/modules/media/src/MediaPermissions.php
   @@ -0,0 +1,64 @@
   +   * @param \Drupal\media\Entity\MediaType $type
   

   This should be MediaTypeInterface.

3. +++ b/core/modules/media/src/MediaPermissions.php
   @@ -0,0 +1,64 @@
   +  protected function buildPermissions(MediaType $type) {
   

   So should this.

This will need a change record of its own, but we will also need to update https://www.drupal.org/node/2863992 so that it refers to the new change record, for people porting their contrib or custom modules from Media Entity to core Media, who may need to update their permissions.

Here is a new patch, to address all the comments from #20

The patch looks great! We just need the change records fixed up as per #20, then this is RTBC.

In the new patch i fixed the failing tests.

I am still not sure about the assertCacheContext changes i did. Can someone please verify it.

-> "Needs review" to trigger testbot, but change records are still missing

Yay, it passed! Thank you, @chr.fritsch. Kicking back for change record stuff, though... :)

I never wrote a CR before, so here is the first draft: #2895605: Introduce per-bundle permissions for media types in preparation to deprecate existing ones

This introduces a slight problem with allowing roles to administer various specific bundles in that you lose the ability to allow that role to view unpublished media entities. This also exists now in that to view an unpublished media entity the role can also create/edit/delete it (since it's tied exclusively to the administer media permission) but it's slightly less obvious since it's fairly standard to just grant the administer media permission anyway since all CRUD operations are common place for an editor role on media.

There's a few options to tackle this:
1) Adding a single "view unpublished media" permission
2) Adding a "view own unpublished media" (similar to what node does)
3) Adding "view unpublished {bundle} media" permissions

I'm not sure if this should be done here, or in a follow up issue but just some food for thought :)

Regarding #28: Good points, but I think we should defer all that to a follow-up issue. Will you open that?

The patch looks good at a glance, but, now that Media is stable, I think this will need an update path. So unfortunately I have to kick this one back yet again...

Here comes the update hook. The update path tests are still missing. I never wrote one. Have to take a look

Here comes a first draft of update hook test. Doesn't work locally for me, but i guess it's a problem with my machine.

That should be appliable

Famous last words coming from me, but this oughta fix the tests :) Turns out the fixture was missing a few tables.

No interdiff because it's like 60KB, 99% of which is changes to the fixture. Sorry!

Added a bit of code to revoke the old, defunct permissions.

Manually tested the patch and it looks good overall. Few questions/notes:

1. Missing Revision related permissions (e.g. Delete revisions) per bundle.
2. Is it worth having a base permission class/trait to have common functionality abstracted from NodePermission and MediaPermission?
3. Created #2911421: Add per-bundle permission system to core part of reviewing this issue.
4. Revoked "create media" permission with bundle specific on update_N and we are trying to test here with "create media"?

+++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
@@ -67,46 +67,106 @@ public function testMediaAccess() {
+    $permissions = ['create media'];

Not sure if those needs work. So leaving it needs review for now.

1:
Since media is revisonable, we should also add revsion related permission

2:
Because MediaPermissions and NodePermissions are looking more or less the same, we could introduce some base class. But that could be done in a followup

4:

+++ b/core/modules/media/media.install
@@ -77,3 +79,56 @@ function media_requirements($phase) {
+      $role->revokePermission('create media');

That should be removed, because the permission stays as it is

Talked with @vijaycs85 about 1. and we agreed that we should remove the "edit own media", "edit any media", "delete own media", "delete any media" and "create media" permissions, because we have them all as bundle permissions. Then we are on the same track as node. Also we will leave the revisions stuff out, because we currently don't have any UI for revisions support. That should all be done in a follow-up.

Talked to @seanB and he also agreed on #40. So here comes the patch

Tagging per #40.

Patch in #41 looks good. +1 to RTBC!

@chr.fritsch we might need to update the contentEntity definition as well?

diff --git a/core/modules/media/src/Entity/Media.php b/core/modules/media/src/Entity/Media.php
index 1f8c8d3..8a7bce9 100644
--- a/core/modules/media/src/Entity/Media.php
+++ b/core/modules/media/src/Entity/Media.php
@@ -66,7 +66,7 @@
  *     "revision_log_message" = "revision_log_message",
  *   },
  *   bundle_entity_type = "media_type",
- *   permission_granularity = "entity_type",
+ *   permission_granularity = "bundle",
  *   admin_permission = "administer media",
  *   field_ui_base_route = "entity.media_type.edit_form",
  *   common_reference_target = TRUE,

Nice catch

Interdiff looks good. +1 to RTBC.

Adding new permissions needs OK from product manager.
Can we get a screenshot of the permissions page showing the permissions?
Thanks

Updated issue summary with screenshot.

Thanks for the screenshots.

For issues where we add permissions, we want to (a) decide whether the permission is an 80% permission that belongs in core, versus something that can be provided by contrib for sites that need more granularity, and (b) make sure that the permission list is manageable.

So those screenshots are what it's like with the default core media types out of the box. How many different media types are on a typical site? What's an example scenario where per-media type permissions are needed?

Also, this issue recommends for per-media type permissions. However, my assumption would have been that the permissions would be per source, rather than per media type. With a typical site, would we end up with a huge list of both kinds of permissions?

Generally with issues like this the product team would like to have them get usability signoff, rather than the product team which acts at a more strategic level (like whether we will add a given module at all, or what its MVP is). So switching to "Needs usability review".

Thanks!

Having permissions at the source level makes zero sense to me, architecturally speaking.

The source is almost entirely under the hood. It is only exposed to site builders, when creating or editing media types. It is an intrinsic part of each media type, but it doesn't really affect the authoring experience. Authors will not generally even be aware of them. They are API-level, not UI-level. Therefore, we should not entangle the source plugins with permissions. Not only do I believe it would be extremely confusing to users, I think it would introduce a substantial amount of weirdness to the Media API. So I would strongly oppose such an idea.

+1 to @seanB's estimate that most sites will have 5-6 media types. Some sites will have more, others will have less, but I envision 5-6 as being the 90% case.

It is also worth mentioning that the current permissions in the Media module are way too broad. If we don't have per-bundle permissions in core, I guarantee this issue will be reopened. Putting it in contrib will only serve to inconvenience people. Media types are more analogous to node types than any other bundle entity type in core, and we have per-type permissions for node types.

So although it will make the Permissions page even more crowded (which is a legitimate UX problem, but well out of Media's scope), we will be doing our users a favor by having Media's permissions act consistently with the behavior people already expect from node types.

I think we're clear on not doing this on the source level. The main question is whether to do this at all (vs. keeping the existing 1 set of permissions that applies to all media types).

My initial response is that we should follow indeed the model we have for content types and have separate permissions per media type. So that Role X can create Cat media and Role Y can create Dog media and Role Z can create Cat and Dog media. Or: authenticated users can create Review Image media to use in their review of the product they bought. Only editors can create the Official Product Image media.

I also think we're already beyond the point of a manageable permissions page. We do have to eventually focus on improving the permissions page situation. And while it's not necessarily up to Media to fix this, given the estimated 5 or 6 Media types does result in 25-30 additional rows of checkboxes, which is a pretty damn big addition. (How many types do we plan to ship with default core?)

Consider this a still undecided-but-leaning-towards-core-adding-the-per-type-permissions :)

How many types do we plan to ship with default core?

Right now, I think the goal is to ship three media types with Standard. We already include File and Image, and once #2831944: Implement media source plugin for remote video via oEmbed lands, another one for remote video will be included.

I'd like to add another reason why we should do this now, rather than later (because, as I said, it will be requested): it is much easier to migrate the permissions now, when fewer people are using Media, than later, when thousands of people may be using it in ways we hadn't anticipated. The future maintenance burden is far lower if we do this now, and that's gotta count for something :)

Maybe this could be the issue that catalyzes a rebuild/redesign of the Permissions page. ;)

I totally agree with phenaproxima and seanB. In Thunder for example we ship with 6 different media types and I think we cover the majority of use-cases.

That this would flood the permissions page is out of scope. So I added #1765576: Redesign the permissions page as a related issue.

It's not really out of scope if this patch introduces the actual flooding, no? We're not adding "just" a single row of checkboxes.

3 Media types introduces 15 additional rows of checkboxes. I enabled every core module and count about 150 rows of checkboxes. So a 10 percent increase in that case, even more with only Standard profile modules enabled. If this was a performance decrease, no way would we let this happen. And in fact it is a performance decrease because people will spend that much more time finding what they need on the permissions page.

I know it's not fair to block this on a system page that has had a broken model for years, but doing nothing about it and just keep adding (a lot of) stuff is not a viable strategy either.

For block content we proposed (still not fixed) #1975064: Add more granular block content permissions that there was a checkbox on the type form that said 'expose additional permissions' or something to that effect.

If that was checked, then separate permissions would exist and be used, if not, it would just use a base set.

We proposed the same for comment module too #1903138: Move global comment permissions to comment-type level

Maybe its something we should seriously explore.

Node is part of the problem too, it has so many by default.

I note that Node already has a permission handler, perhaps we need a base permission handler for bundleable entities where the bundles come from a config entity. This base handler would provide this function by default.

Then media, block_content and comment could use it. And with time, node could too.

Thoughts?

Link direct to that discussion - https://www.drupal.org/node/1975064#comment-11372113

Screenshot of potential UI proposed from @yoroy

I think having the permissions on the bundle form is a future enhancement.

I'm proposing that checking 'custom permissions' makes those permissions exist on the permissions page.

Unchecking it means the default global permissions for media/node/comment/block content/et al are used.

I think #61, although it would be a new pattern in core, is a good and cool idea. However, I can imagine a fair bit of complexity arising in the implementation.

Another, simpler compromise might be to introduce per-bundle permissions now, but fewer of them. So rather than "edit own", "delete any", etc. for each media type, we'd simply have three permissions per type: create, edit, delete. This would still provide site builders with a reasonable level of flexibility, without choking the permissions page. We could add more granular (own/any) permissions in a follow-up issue once the UX problems have been ironed out, and although there would be a migration path involved, it would not be as complicated.

I wouldn't add just add the half of them. Maybe it's best to go with something like #61, because this 5 permissions are still not the end. We are currently completely missing the revisions support. #2887106: Add generic base classes for content entity revisions would probably introduce 'view all revisions', 'revert all revisions' and 'delete all revisions'.

Yoroy has provided a review.

#64: I totally expected to get corrected on my math :-)

Thought about this a bit more. We should just add the extra permissions. I wanted to stress that we need to get serious about finding a better model for the permissions UI but that should in itself not block this issue from going in. It would be better to have these more fine grained permissions directly in core from the start then having to add them later.

I opened #2919636: Design a better model for managing permissions in the ideas queue. Carry on in here!

Just a reroll.

Trying to fix those test failures.

If I'm reading those new test failures right, they're happening for reasons outside the scope of this patch, right?

OK, after much conversation with phenaproxima, I'm documenting where I'm stuck on this issue.

The final failing test is failing because MediaUpdateTest::testBundlePermission() calls $this->runUpdates(), which defaults to its parent class's method, i.e. UpdatePathBaseTest::runUpdates(). We see the test failure in that method, specifically here:

$this->assertFalse($needs_updates, 'After all updates ran, entity schema is up to date.');

However, that obscures the real problem. If you comment that line out and re-run the test, you get a more useful failure:

1) Drupal\Tests\media\Functional\Update\MediaUpdateTest::testBundlePermission
The <em class="placeholder">media.field_media_image</em> field needs to be uninstalled.
/[d8 root]/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:383
/[d8 root]/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php:41

When I chased this down to EntityDefinitionUpdateManager::getChangeList() and xdebugged that method, I found that it thought that both field_media_file and field_media_image had been deleted. Corroborating this was the fact that my test's database and my functioning D8 media site looked different as you see in the images. (That is, there's no evidence in the test database that those two fields exist.)

So, I'm stumped. Can anybody suggest reasons why, in the test, those fields would either have been deleted, or never been installed?

I think the problem is #2883813: Move File/Image media type into Standard once Media is stable. In drupal-8.media-enabled.php we try to load configuration form media/config/install. The files are not there anymore, because they were move into standard profile.

So maybe we should base drupal-8.media-enabled.php on media_test_type configuration.

I thought a bit more about it and i think it's better to load the files form the standard profile. Now drupal-8.media-enabled.php does what it's name supposed to be. It enables media module like it would be in standard profile.

Looks quite good. Only two nits, but neither should block RTBC. I'm not qualified, because I've worked on the patch too recently, but I see no reason not to proceed.

1. +++ b/core/modules/media/src/MediaPermissions.php
   @@ -0,0 +1,92 @@
   +  public function __construct(EntityTypeManagerInterface $entityTypeManager) {
   +    $this->entityTypeManager = $entityTypeManager;
   

   Nit: $entityTypeManager should be snake_case, not camelCase.

2. +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
   @@ -0,0 +1,63 @@
   +    // Run updates.
   +    $this->runUpdates();
   

   Nit: This comment is superfluous. :)

Fixed the nits

All seems super obvious and straight forward. RTBC

Eclipse

Is there a way to manually test this and verify through the UI that it works as intended?

Create multiple roles and grant them access to different operations per media type. Then create some users with a role and test if you are able to execute the operations

Right, but how to create media types in core? There's no media module to enable etc.

drush en media or remove the hidden key from media.info.yml file. With enabling media module you will get a file and a image type.

Hopefully that helps.

See, it's not that obvious :-) Yes, that does help, thanks for the instructions. I applied the patch locally and yes, the permissions are there:

Seems like the necessary follow-ups are in place as well. There's still a "needs change record updates" tag though, has that been taken care of? I see there's https://www.drupal.org/node/2895605, but can't tell if it correctly reflects discussion in #20, #21.

I opened #2923347: Migrate generic media permissions to per-type permissions as a follow-up for the media_entity upgrade path.

I read over all the discussion since my last review. I still have some hesitations about the expanded number of permissions (by 5-10% or whatever) but @yoroy has signed off so I'll follow his recommendation. The per-bundle permissions do seem more integral to the bundle than for comments or block types, and the various usecases illustrating why they're useful also make a good case for why we want these permissions in core.

1. +++ b/core/modules/media/media.permissions.yml
   @@ -1,33 +1,16 @@
      restrict access: TRUE
   -
    administer media types:
      title: 'Administer media types'
      restrict access: TRUE
   -
    view media:
   ...
      description: 'To view a revision, you also need permission to view the media item.'
   -
    access media overview:
   

   These hunks are all out of scope.

2. +++ b/core/modules/media/media.permissions.yml
   @@ -1,33 +1,16 @@
   -
   -update media:
   -  title: 'Update own media'
   -
   -update any media:
   -  title: 'Update any media'
   -
   -delete media:
   -  title:  'Delete own media'
   -
   -delete any media:
   -  title:  'Delete any media'
   -
   -create media:
   -  title: 'Create media'
   

   Thinking about it, this is a BC break. I agree with not showing these permissions in the UI when we have the per-bundle varieties, but we need to not break BC for anything that might already be checking for them

   I filed #2924785: Provide a mechanism to deprecate permissions to provide a way to deprecate permissions. I'd suggest just removing this hunk from the patch, adding a @todo, and addressing the deprecation in a followup (even though it will add five more permissions than needed until they can be deprecated).

Okay, that all sounds good to me. The loss of permissions does represent a BC break, despite the presence of an update path, so let's open a follow-up to deprecate the old permissions (which we can postpone on #2924785: Provide a mechanism to deprecate permissions), fix up media.permissions.yml, and then I think we are back to RTBC.

Okay, i brought the old permissions back. I also removed the revocation of the old permissions. This will be done in #2925459: Deprecate generic media permissions, which i created as a follow-up.

I think we are good to go here. All of @xjm's comments have been addressed, all follow-ups are filed, and all T's are dotted and I's are crossed :) Let's do this.

+++ b/core/modules/media/media.permissions.yml
@@ -6,6 +6,7 @@ administer media types:
+# @todo: Remove this in https://www.drupal.org/project/drupal/issues/2925459
 view media:
   title: 'View media'

Minor issue here -- I think the "View media" permission is not going to be removed, right? So the @todo should be after it, or it should say something like "Deprecate some permissions in..."

Also, do we have a followup yet for the proposal to provide a UI and API pattern for per-bundle permissions? I don't think that's covered by #1765576: Redesign the permissions page or #2919636: Design a better model for managing permissions which both have a much wider scope.

Fixed the minor issue

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -32,22 +33,22 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
-        if ($account->hasPermission('update any media')) {
+        if ($account->hasPermission('edit any ' . $type . ' media')) {
           return AccessResult::allowed()->cachePerPermissions();
         }
-        return AccessResult::allowedIf($account->hasPermission('update media') && $is_owner)
-          ->cachePerPermissions()
-          ->cachePerUser()
-          ->addCacheableDependency($entity);
+        if ($account->hasPermission('edit own ' . $type . ' media') && $is_owner) {
+          return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity);
+        }
+        return AccessResult::neutral()->cachePerPermissions();
 
       case 'delete':
-        if ($account->hasPermission('delete any media')) {
+        if ($account->hasPermission('delete any ' . $type . ' media')) {
           return AccessResult::allowed()->cachePerPermissions();
         }
-        return AccessResult::allowedIf($account->hasPermission('delete media') && $is_owner)
-          ->cachePerPermissions()
-          ->cachePerUser()
-          ->addCacheableDependency($entity);
+        if ($account->hasPermission('delete own ' . $type . ' media') && $is_owner) {
+          return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($entity);
+        }
+        return AccessResult::neutral()->cachePerPermissions();

So I think not removing the permissions also means retaining the code paths for them; otherwise they're just strings in a file that don't do anything. So I think we need to add the new code paths here but not delete the previous ones, and then in #2925459: Deprecate generic media permissions we'd add @trigger_error() to the old code paths along with the deprecation flag in the permissions file.

Brought back the code path and added a @todo

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -32,22 +33,36 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+        // @todo: Deprecate this permissions in https://www.drupal.org/project/drupal/issues/2925459
...
+        // @todo: Deprecate this permissions in https://www.drupal.org/project/drupal/issues/2925459

Oh, these need to be wrapped as per: https://www.drupal.org/node/1354#todo. The sentence should also probably have a period on it.

Fixed the @todo

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -39,7 +39,8 @@
+        // @todo Deprecate this permissions in

The tiniest nitpick: "permissions" should be "permission" (singular).

Fixable on commit, though. So back to RTBC!

No, wait. Back to NR for the follow-up @xjm mentioned in #92.

When trying to apply this patch to 8.5, got the following error:

1 out of 1 hunk FAILED -- saving rejects to file core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php.rej

Rerolled this patch for 8.5.

changed plural comments to singular.

Alright, for those following at home #101 patch is too small. What happened was I was doing doing the git diff strategy to make a patch. Three of those files were new and not captured in the diff as those files were in my working tree and not committed. #103 is too big, I accidentally committed files that were not part of the patch.

Now, the goldilocks patch that is just the right size.

I think I know what's going on here. :)

This is the test failure:

--- Expected
+++ Actual
@@ @@
-{"message":"Unprocessable Entity: validation failed.\nname: Name: this field cannot hold more than 1 values.\n"}
+{"message":"Unprocessable Entity: validation failed.\nname: Name: this field cannot hold more than 1 values.\nthumbnail.0: You do not have access to the referenced entity (file: 2).\nfield_media_file.0: You do not have access to the referenced entity (file: 1).\n"}

This is the merge conflict I get when I start to rebase the patch from #97:

diff --cc core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
index 1875d004a4,07d8c47bfa..0000000000
--- a/core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
@@@ -49,9 -49,7 +49,13 @@@ abstract class MediaResourceTestBase ex
          break;
  
        case 'PATCH':
++<<<<<<< HEAD
 +        $this->grantPermissionsToTestedRole(['update any media']);
 +        // @todo Remove this in https://www.drupal.org/node/2824851.
 +        $this->grantPermissionsToTestedRole(['access content']);
++=======
+         $this->grantPermissionsToTestedRole(['edit any camelids media']);
++>>>>>>> patch
          break;

From #97:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
@@ -45,15 +45,15 @@ protected function setUpAuthorization($method) {
 
       case 'POST':
-        $this->grantPermissionsToTestedRole(['create media']);
+        $this->grantPermissionsToTestedRole(['create camelids media']);
         break;
 
       case 'PATCH':
-        $this->grantPermissionsToTestedRole(['update any media']);
+        $this->grantPermissionsToTestedRole(['edit any camelids media']);
         break;
 
       case 'DELETE':
-        $this->grantPermissionsToTestedRole(['delete any media']);
+        $this->grantPermissionsToTestedRole(['delete any camelids media']);

From the current patch:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
@@ -45,17 +45,15 @@ protected function setUpAuthorization($method) {
       case 'PATCH':
-        $this->grantPermissionsToTestedRole(['update any media']);
-        // @todo Remove this in https://www.drupal.org/node/2824851.
-        $this->grantPermissionsToTestedRole(['access content']);
+        $this->grantPermissionsToTestedRole(['edit any camelids media']);
         break;

Now, #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior did just get fixed, but it would be out of scope to do the removal in this issue even if the @todo were mere cruft. But it does sound kinda like it would cause that exact test failure. :) So let's try restoring the @todo and the access content grant; the patch should not change them. And then if the tests pass with that fix, then we should open up a separate followup to figure out what needs to happen following #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.

Thanks!

@xjm thanks for the review and great find.

So TLDR; I removed a chunk of code from the reroll and we need to add it back in:

case 'PATCH':
  // @todo Remove this in https://www.drupal.org/node/2824851.
  $this->grantPermissionsToTestedRole(['access content']);

Note: local test still fail at the exact same place but hoping we get further on the test bot.

1) Drupal\Tests\hal\Functional\EntityResource\Media\MediaHalJsonAnonTest::testGet
Failed asserting that Array &0 (.....
2) Drupal\Tests\hal\Functional\EntityResource\Media\MediaHalJsonAnonTest::testPatch
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"No route found for \u0022PATCH media\/1\u0022: Method Not Allowed (Allow: GET, POST, HEAD)"}
+{"message":"No route found for \u0022PATCH \/media\/1\u0022: Method Not Allowed (Allow: GET, POST, HEAD)"}

/Users/rob/projects/drupal/core/modules/rest/tests/src/Functional/ResourceTestBase.php:393
/Users/rob/projects/drupal/core/modules/rest/tests/src/Functional/ResourceTestBase.php:455
/Users/rob/projects/drupal/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:946

3) Drupal\Tests\hal\Functional\EntityResource\Media\MediaHalJsonAnonTest::testDelete
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"No route found for \u0022DELETE media\/1\u0022: Method Not Allowed (Allow: GET, POST, HEAD)"}
+{"message":"No route found for \u0022DELETE \/media\/1\u0022: Method Not Allowed (Allow: GET, POST, HEAD)"}

/Users/rob/projects/drupal/core/modules/rest/tests/src/Functional/ResourceTestBase.php:393
/Users/rob/projects/drupal/core/modules/rest/tests/src/Functional/ResourceTestBase.php:455
/Users/rob/projects/drupal/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1152

Gave this a read-through and it looks correct to me. I think it's ready to go.

I'll file the follow-up requested by @xjm in #92, based on what @larowlan said in #61 and #62.

Filed the follow-up: #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions.

Well and so. We already signed off on adding these permissions to the permission page. But then we added Audio and Video! So now we have:

Before patch with Media enabled: 113 permissions
After patch: 133 permissions

17.6% increase! :D

At the least, this makes #2925459: Deprecate generic media permissions a good deal more important.

Adding a screenshot of all the new media audio and video permissions.

lol

I pinged @yoroy for his thoughts based on the updated screenshot in the IS. Also note that if we add oEmbed and then add a remote media type, there would be another five permissions in core alone.

The counterpoint to the "17.6% increase" risk is that even if we unhide the module, Media will still not be in Standard. So if we are concerned about this based on the latest state, we could make it a Standard blocker instead. I'd say it's up to the UX team which.

2 data points that might be interesting in the context of this and general issues about improving the UI

* One distribution that we have currently has 11 different media types, so that would be about 55 additional permissions. This is probably more than most sites.

* A site based on another distribution that we use that does not use media but has 27 node types, 651 permissions and 25 roles (don't ask me why..). That's 16275 checkboxes :) No, the global permissions page is not working anymore :)

The "winning" modules are..
node: 225 (27 * 8)
field_ui: 61 (did not expect that...apparently the site has 20 entity types with a field UI * 3)
taxonomy: 47 (this is still on 8.3, so this will grow a bit with 8.4)
file_entity: 32
masquerade: 26
profile: 19

Not exactly sure what my point is, probably that media is likely going to be fairly harmless on most sites compared to what node.module is already doing to them :)

One last thought, an alternative option to deprecating the old permissions would be to have a setting to switch between the two modes, either global any permissions or per-type. Then sites don't automatically get a ton of additional permissions. The screens in #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions currently have a per-bundle optional checkbox, but as explained there, that doesn't really make sense, only a per-entity-type setting would, IMHO.

Thanks for the additional examples @Berdir. No objections to adding these permissions here as proposed.

@xjm asked for more usability feedback. I agree with @yoroy :)

My comment in #28 is partially address by #2889855: Unpublished media entity can not be accessed by owner and update any media/delete any media access possibly cached by user, I will create another issue for adding view any unpublished media.

Created the follow up #2936652: Add "view unpublished $bundle media" permissions for each media bundle

Adding review credits

@Berdir for providing real-world testing/insight
@hazong as per #14
@xjm for mentoring/reviews

diff --git a/core/modules/media/media.install b/core/modules/media/media.install
index 8fbde81..90f9d99 100644
--- a/core/modules/media/media.install
+++ b/core/modules/media/media.install
@@ -7,7 +7,6 @@
 
 use Drupal\user\RoleInterface;
 use Drupal\user\Entity\Role;
-use Drupal\media\Entity\MediaType;
 
 /**
  * Implements hook_install().

Fixed on commit

Committed as 4443678 and pushed to 8.6.x.

Cherry-picked as fb74ea4 and pushed to 8.5.x

Published change record

Yay, glad this landed!

FYI: #2989469: MediaUpdateTest is broken