Media items should not be available at /media/{id} to users with 'view media' permission by default

Here is a first patch. There are a couple of things that should be adjusted:

* We need an update path to set the checkbox for existing sites.
* Changing the setting, doesn't clear the entity type cache. Not sure about this.
* Some documentation in the settings form why it is risky to expose the canonical URL.

Finally, let's see how much tests are failing.

1. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -11,6 +11,9 @@ media.settings:
   +    expose_canonical:
   

   Let's prefix this with bc_, just like we did in #2751325: All serialized values are strings, should be integers/booleans when appropriate.

   @chr.fritsch pointed out that this is not actually solely for BC, some sites may have an explicit need for this.

2. +++ b/core/modules/media/media.links.task.yml
   @@ -1,18 +1,6 @@
   -entity.media.edit_form:
   -  title: Edit
   -  route_name: entity.media.edit_form
   -  base_route: entity.media.canonical
   -
   -entity.media.delete_form:
   
   +++ b/core/modules/media/src/Plugin/Derivative/DynamicLocalTasks.php
   @@ -0,0 +1,79 @@
   +    $this->derivatives["entity.media.edit_form"] = [
   +      'route_name' => "entity.media.edit_form",
   +      'title' => $this->t('Edit'),
   +      'base_route' => $parent,
   +    ] + $base_plugin_definition;
   +
   +    $this->derivatives["entity.media.delete_form"] = [
   

   Why did we move these?

3. +++ b/core/modules/media/media.links.task.yml
   @@ -1,18 +1,6 @@
   -  weight: 10
   
   +++ b/core/modules/media/src/Plugin/Derivative/DynamicLocalTasks.php
   @@ -0,0 +1,79 @@
   +      'route_name' => "entity.media.delete_form",
   +      'title' => $this->t('Delete'),
   +      'base_route' => $parent,
   +    ] + $base_plugin_definition;
   

   We lost the weight here.

4. +++ b/core/modules/media/media.module
   @@ -347,3 +347,16 @@ function _media_get_add_url($allowed_bundles) {
   +  if (\Drupal::config('media.settings')->get('expose_canonical')) {
   

   This means that a config change needs to invalidate entity type definitions, i.e. the entity_types cache tag.

   It also means routes need to be rebuilt, so you'll want to invalidate the routes cache tag too AFAICT.

   See \Drupal\rest\EventSubscriber\RestConfigSubscriber and \Drupal\system\EventSubscriber\ConfigCacheTag::onSave() an example.

5. +++ b/core/modules/media/src/Entity/Media.php
   @@ -531,4 +529,19 @@ public static function getRequestTime() {
   +    if (in_array($rel, ['canonical', 'revision']) && !$this->hasLinkTemplate($rel)) {
   

   Let's put in an @see to the configuration here, and explain why we even need this.

6. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -91,6 +91,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#title' => $this->t('Expose canonical media URL'),
   +      '#default_value' => $this->config('media.settings')->get('expose_canonical'),
   

   I suggest s/expose_canonical/expose_canonical_routes/ and "Make media items accessible via their own pages"

7. +++ b/core/modules/media/src/Plugin/Derivative/DynamicLocalTasks.php
   @@ -0,0 +1,79 @@
   +   * Creates an DynamicLocalTasks object.
   

   Übernit: s/an/a/

+++ b/core/modules/media/src/Form/MediaSettingsForm.php
@@ -91,6 +91,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#description' => $this->t('We NEED a description'),

Suggestion: By default, published !media-entities are only accessible to end users if they're shown (referenced) by other entities. On most sites, access to individual !media-entities (via guessable URLs) is unexpected. If it makes sense for your site to have all !media-entities be individually accessible, turn this setting on.

(For !media-entities, use label_plural from the entity type definition.)

That's quite verbose and perhaps a bit scary, but I think that's warranted in this case.

Regarding #4

2. Because we have to set a different parent
3. Fixed
4. We have still have the problem that media_entity_type_alter is called after DefaultHtmlRouteProvider::getRoutes()
5. Fixed
6. Fixed
7. Fixed

Also added the suggestion from #5

1. +++ b/core/modules/media/media.links.task.yml
   @@ -1,18 +1,6 @@
   +  weight: 100
   

   Why this weight? Let's document the reason.

2. +++ b/core/modules/media/media.services.yml
   @@ -19,3 +19,8 @@ services:
   +    tags:
   +    - { name: event_subscriber }
   

   Übernit: the last line needs an extra 2 spaces of leading indentation.

3. +++ b/core/modules/media/src/Entity/Media.php
   @@ -531,4 +529,22 @@ public static function getRequestTime() {
   +    // Canonical and revision URL's should always point to the edit form or
   

   Nit: s/URL's/URLs/

4. +++ b/core/modules/media/src/Entity/Media.php
   @@ -531,4 +529,22 @@ public static function getRequestTime() {
   +      if ($this->access('edit')) {
   +        $rel = 'edit-form';
   +      }
   +      else {
   +        $rel = 'collection';
   +      }
   

   🤔 Hm, this could be tricky. This means that the URL that is returned depends on the user (which permissions do they have?) and the configuration (which permissions does each role have?).

   I'd say that we should merge the cacheability of the access result with the resulting Url object, but … the \Drupal\Core\Url object does not carry cacheability.

   \Drupal\Core\GeneratedUrl does … but that's not what this method returns.

   Do we really need to conditionally return different routes?

5. +++ b/core/modules/media/src/EventSubscriber/MediaConfigSubscriber.php
   @@ -0,0 +1,68 @@
   +   * The router builder.
   ...
   +  protected $routerBuilder;
   ...
   +   *   The router builder service.
   ...
   +   * Informs the router builder a rebuild is needed when necessary.
   

   s/router/route/

   (And yes, \Drupal\rest\EventSubscriber\RestConfigSubscriber got this wrong too 😅)

6. +++ b/core/modules/media/src/EventSubscriber/MediaConfigSubscriber.php
   @@ -0,0 +1,68 @@
   +   * Constructs the RestConfigSubscriber.
   

   C/P leftover :)

7. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -22,6 +23,13 @@ class MediaSettingsForm extends ConfigFormBase {
   +   * The entity manager service.
   
   @@ -29,10 +37,13 @@ class MediaSettingsForm extends ConfigFormBase {
   +   *   The entity manager service.
   

   Nit: entity type manager.

This patch fixes the caching issue we had when changing the setting.

It also adds a super-simple update path to set the setting for existing sites.

I will now start to address #9

1. +++ b/core/modules/media/media.post_update.php
   @@ -18,3 +18,13 @@ function media_post_update_collection_route() {
   + * Enable canonical routes for the Media entity type.
   

   Keeps canonical routes for Media entities enabled on existing sites.

2. +++ b/core/modules/media/src/EventSubscriber/MediaConfigSubscriber.php
   @@ -27,6 +28,13 @@ class MediaConfigSubscriber implements EventSubscriberInterface {
   +   * The entity manager service.
   
   @@ -34,10 +42,13 @@ class MediaConfigSubscriber implements EventSubscriberInterface {
   +   *   The entity manager service.
   

   entity type manager

3. +++ b/core/modules/media/src/EventSubscriber/MediaConfigSubscriber.php
   @@ -50,9 +61,9 @@ public function __construct(RouteBuilderInterface $router_builder, CacheTagsInva
   +      $this->cacheTagsInvalidator->invalidateTags(['rendered']);
   

   ✅ Needs a comment. Added that. Explanation is similar to #2241275: DateFormat cache tag: don't set the cache tag, but invalidate the entire render cache?.

4. +++ b/core/modules/media/src/Plugin/Derivative/DynamicLocalTasks.php
   @@ -56,6 +56,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +        'weight' => 1,
   
   @@ -65,6 +66,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +      'weight' => 2,
   

   These would guarantee the correct order, but why was this not necessary before, but is now?

This fixes #9 and #11

All HAL REST GET tests are failing (Testing Drupal\Tests\media\Functional\Hal\MediaHalJsonAnonTest and friends), even after adding something like

  public function testGet() {
    \Drupal::configFactory()
      ->getEditable('media.settings')
      ->set('expose_canonical_routes', TRUE)
      ->save(TRUE);

    $this->rebuildAll();
    \Drupal::service('router.builder')->rebuild();
    $this->resourceConfigStorage->resetCache();

    return parent::testGet();
  }

It's then failing like this:

1) Drupal\Tests\media\Functional\Hal\MediaHalJsonAnonTest::testGet
Drupal\Core\Config\ConfigDuplicateUUIDException: Attempt to save a configuration entity 'entity.media' with UUID '3d1b92f9-010b-425f-8f04-44b5a30e9ad4' when this entity already exists with UUID 'e7ffc9c7-1e9c-4e0d-b227-2f8dd6b16e28'

That's triggered by this line:

$this->resourceConfigStorage->load(static::$resourceConfigId)->disable()->save();

Manually stepping through this confirms this seemingly impossible thing, that loading something by ID succeeds and simply calling disable()->save() somehow causes the loaded config entity to have a UUID that differs from the original config entity. Calling $this->resourceConfigStorage->resetCache([static::$resourceConfigId]); doesn't help.

If this is not scary enough, what's worse is that this happens only for HAL tests. Why? How? I have no clue. I already spent about 45 minutes debugging this. I'm moving on to something else.

I changed the implementation to stay with the canonical link template. Now we are setting the canonical URL to the edit URL.

Let's see how many tests will fail now.

So let's start fixing tests.

BTW: I don't think that the related issue is affecting us, because our route has no optional arguments.

+++ b/core/modules/media/tests/src/Functional/Rest/MediaResourceTestBase.php
@@ -41,6 +41,24 @@
+    $this->serializer = $this->container->get('serializer');
+    $this->resourceConfigStorage = $this->container->get('entity_type.manager')->getStorage('rest_resource_config');

🎉😭🙈We spent so much time debugging this! Great find :)

This should fix the remaining test fails

> We spent so much time debugging this! Great find :)

That's exactly why I think that using $this->container and properties with services is a bad idea in tests, should either just use \Drupal::() or alternatively methods. See #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests.

+++ b/core/modules/media/src/Entity/Media.php
@@ -75,7 +75,7 @@
- *     "canonical" = "/media/{media}",
+ *     "canonical" = "/media/{media}/edit",

After reading the title, I came here to suggest this (the same as BlockContent) instead of actually removing it. +1!

1. I talked to @alexpott and @Wim Leers and we decided to rename the setting to standalone_media_url
2. Nice catch, $this->container->get('router.builder')->rebuild(); is enough for us to work
3. Added a comment
4. Actually it's not needed.

Awesome work. I think this patch is an excellent fix for a pretty serious WTF in the media system. One request, though:

+++ b/core/modules/media/config/schema/media.schema.yml
@@ -11,6 +11,9 @@ media.settings:
+    standalone_media_url:
+      type: boolean
+      label: 'Standalone Media URL'

Having the word "media" in this setting name is redundant. Can we just call this standalone_url? And can the description be changed to "Whether media items can be accessed at standalone URLs"?

I addressed #25 and #26

Addressing #28

1. Changed
2. Added an update test.
3. I removed it. Let the testbot answer that question :)

This patch needed some test changes after #2882473: Hide the media name basefield from the entity form by default landed.

Let's kick them.

👍🏻

👍

#33 +1

Agreed, +1!

Ok, here is the Caga Tio. Let's proceed  😁

Only found nits (see below), and fixed them all. Let's get this done.

1. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -11,6 +11,9 @@ media.settings:
   +      label: 'Allow media entities to be viewed at media/ID'
   

   Allow media items to be viewed standalone at /media/{id}.

2. +++ b/core/modules/media/media.post_update.php
   @@ -18,3 +18,13 @@ function media_post_update_collection_route() {
   + * Keep media entities viewable at media/{id}.
   

   Nit: Missing leading slash.
   s/entities/items/

3. +++ b/core/modules/media/src/EventSubscriber/MediaConfigSubscriber.php
   @@ -0,0 +1,97 @@
   +   * The router builder.
   

   s/router/route/

4. +++ b/core/modules/media/src/EventSubscriber/MediaConfigSubscriber.php
   @@ -0,0 +1,97 @@
   +    $this->entityTypeManager = $entity_type_manager;
   +
   +  }
   

   Nit.

5. +++ b/core/modules/media/src/EventSubscriber/MediaConfigSubscriber.php
   @@ -0,0 +1,97 @@
   +   * This method is executed on all config save events.
   +   *
   +   * Informs the router builder a rebuild is needed and clears the entity types
   +   * cache when necessary.
   

   The first line is kinda pointless. The second is great. Let's make it fit on a single line.

6. +++ b/core/modules/media/src/EventSubscriber/MediaConfigSubscriber.php
   @@ -0,0 +1,97 @@
   +      $this->routerBuilder->setRebuildNeeded();
   +
   +    }
   

   Nit.

7. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -22,6 +23,13 @@ class MediaSettingsForm extends ConfigFormBase {
   +   * The entity type manager service.
   
   @@ -29,10 +37,13 @@ class MediaSettingsForm extends ConfigFormBase {
   +   *   The entity type manager service.
   

   s/service//

8. +++ b/core/modules/media/src/Form/MediaSettingsForm.php
   @@ -91,6 +103,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t("Allow users to access @media-entities on media/{id}.", ['@media-entities' => $this->entityTypeManager->getDefinition('media')->getPluralLabel()]),
   

   s/on/at/

   s/media//media/

9. +++ b/core/modules/media/src/Plugin/Derivative/DynamicLocalTasks.php
   @@ -0,0 +1,94 @@
   + * Generates media local tasks.
   

   s/media/media-related/

10. +++ b/core/modules/media/src/Plugin/Derivative/DynamicLocalTasks.php
    @@ -0,0 +1,94 @@
    +   * The entity manager service.
    ...
    +   *   The entity manager service.
    ...
    +   *   The config factory service.
    

    s/service//

11. +++ b/core/modules/media/tests/src/Functional/Update/MediaUpdateTest.php
    @@ -80,4 +84,34 @@ public function testOwnerEntityKey() {
    +    $mediaType = $this->createMediaType('test');
    

    Should not use camelCase.

12. … and many more nits, all fixed.

Ugh, right interdiff, wrong patch.

CR reviewed; added some small clarifications.

+++ b/core/modules/media/media.post_update.php
@@ -18,3 +18,13 @@ function media_post_update_collection_route() {
+/**
+ * Keep media items viewable at /media/{id}.
+ */
+function media_post_update_enable_standalone_url() {
+  \Drupal::configFactory()
+    ->getEditable('media.settings')
+    ->set('standalone_url', TRUE)
+    ->save(TRUE);

Why was this done a as a post update, this is just a regular plain config save?

It being a post update makes it impossible for me to also set that to FALSE in existing installations of my installation profile.

IMHO, we should either change it to a regular update function or check it against NULL, so that it is only set if NULL. Then I can do my own update or post update function and the execution order doesn't matter, mine will win.

Created #3036787: media_post_update_enable_standalone_url() should only set TRUE if not already set.

Note: This breaks linkit media integration as that no longer exposes a matcher plugin for media entities. Created an issue there #3040749: Drupal 8.7 no longer has a dedicated canonical route for media entities, breaks entity matcher deriver.

Is it a longer term plan to remove all ability to access the fields and content of media outside of nodes? We have been building a semantic data repository on Drupal 8 and testing the move from 8.6.10 to 8.7.x had me worried. I see the canonical route for media entities can be re-enabled. But I am just wondering what the longer term plan for media entities is?

I don't think there are plans beyond this issue. Most sites don't need/want them to be exposed, but you can still chose to do so if that makes sense for you.

Thanks for your quick response, I also noticed that the media REST endpoint seems to have been changed as well from the old media/{id} to media/{id}/edit. I don't think it is from this ticket. but I might have missed that change or is that a completely separate ticket?

That was most likely caused by this and probably not something that we expected to happen. Not seeing any test changes related to that, so I think our test coverage is a bit too generic to catch that :-/. I'd suggest you open an new issue (bug?) about that. A bit tricky now, because changing it again could be seen as yet another change.

Wow, great catch, @whikloj! And yes, @Berdir is right, this is a consequence of our REST test coverage being generic. OTOH it's pretty exceptional to have foo/{…}/edit as a canonical route, so I'm not too concerned about the REST test coverage.

The fix should be easy, could you file that issue that @Berdir asked for, @whikloj? 🙏

@Berdir @Wim Leers... cancel that. It was a PEBKAC error, I thought I had cleared my cache but apparently I had not.

**edit**
To clarify the REST route does change to media/{id}/edit due to this change, but if you check the checkbox in Media Settings it reverts to media/{id}.

It took me a minute to discover this checkbox was at /admin/config/media/media-settings as I didn't really think of it as a "security setting". I know this issue is fixed, but does anyone else think it might have a better home on the edit page for each media type (i.e. at /admin/structure/media/manage/audio > Publishing options) so there is more fine grained control per media type?

Re. #58 - can you file a new issue to propose this? I think it's a good idea, though it'll need discussion. It sounds similar to what the Rabbit Hole contrib module does.

@andrewmacpherson As suggested: #3081741: Allow media to expose a standalone URL on a per-bundle basis

We overlooked the path alias: #3067944: Follow-up for #3017935: Media entities should no longer have path alias support by default.