Inform users that media items don't inherit access control from parents

I think that it makes a lot of sense to inform/warn users about this, because the mismatch in expectations can lead to unintended disclosures (even though the behavior is by design).

A couple thoughts about the current approach:

1. We need to be careful with status report warnings because they can lead the site owner to think something is malfunctioning even when it's working as designed and there's no actionable step to take, which is problematic for usability reasons. In general, we're actually trying to reduce the number of status report warnings we have. (See #2877128: [META] Evaluate status report warnings and determine which should be reduced to info, removed, etc. for more information on all that.) An info message might be more appropriate for the status report.
    
2. Having the message on Manage fields is definitely a good idea, but I think we might want it to be just hook_help() text added to that route rather than a warning.
    
3. I think it'd also be a good idea to provide a list or audit of where the media type is referenced.
    
4. Possibly (as a followup) there could be a filter or display for the Media view/Media Library that actually allows the site owner to look over private file media items themselves and to verify their access restrictions, so that the site owner can fix misconfigured media in response to the message.
    
5. The message wording is very Drupally and probably hard for many site builders to understand. A first draft at improving it:
   

   Media type File uses the private file storage and can be used on the following content:

   • Content types: Article, Recipe
   • Taxonomy vocabularies: Food

   By default, the media items will be accessible to any users with access to File media, even if the user does not have access to content that references it. More information on configuring media access restrictions.

   Note that I didn't say "Media type(s)"; we should probably use formatPlural() for that for better translation support.
    

6. I think it might be better to inform the user in the context in which they are actually adding the Media. E.g., the media add form, the node form, or especially the media library widget once that's added. Otherwise, the information is disconnected from the place that's affected, and content authors or site builders who need to know the information might not ever see it.
    
7. I think we should also have a section in hook_help() about this, possibly just a short section that links the proposed handbook page. (Also let's at least get minimal content on that page before we commit this, since Media is stable and so it's covered by the docs gate.)
    
8. I can see the case for the developer-only configuration option about the warning to mitigate things like my points 1 and 2 above, but ideally we'll come up with a solution that someone doesn't need to turn off. :) Note for other reviewers: the patch does not expose said configuration option in the UI (and should not).
    

Thanks! I think this is a really good step to straightening out the tangle of access expectations about Media.

1. +++ b/core/modules/media/config/install/media.settings.yml
   @@ -1,3 +1,4 @@
   +show_private_scheme_warning: TRUE
   

   TRUE should be lowercase true. Also, can we rename this to show_private_file_warning? 'scheme' is a weird word in this context.

2. +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -11,6 +11,9 @@ media.settings:
   +      label: 'Whether the site should show a warning on the UI when the source field is configured to use the private scheme'
   

   Can we adjust this: "...is configured to use private file storage"?

3. +++ b/core/modules/media/media.install
   @@ -107,6 +107,35 @@ function media_requirements($phase) {
   +      $types_to_warn = [];
   +      foreach ($media_types as $type) {
   +        $source_definition = $type->getSource()->getSourceFieldDefinition($type);
   +        if (!in_array($source_definition->getType(), ['file', 'image'])) {
   +          continue;
   +        }
   +        if ($source_definition->getSetting('uri_scheme') === 'private') {
   +          $types_to_warn[$type->id()] = $type->label();
   +        }
   +      }
   

   This seems like a good place to use array_filter(). Additionally, I think it'd be better to check if the source field item list is, or extends, FileFieldItemList rather than rely on a field type ID.

4. +++ b/core/modules/media/media.module
   @@ -58,7 +58,30 @@ function media_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('Media items behave differently than files and images regarding restricted access. While files and images, when configured to use the private storage, will inherit access permissions from the content where they are attached to, Media items have their own access restrictions. This usually means that by default users could have access to files on Media items even though they do not have access to the content that references the Media items. <a href="@doc_url">More information on configuring media access restrictions</a>.', [
   

   Nit: I think "Media items" should be lowercased to "media items".

5. +++ b/core/modules/media/media.module
   @@ -58,7 +58,30 @@ function media_help($route_name, RouteMatchInterface $route_match) {
   +      $source_definition = $media_type->getSource()->getSourceFieldDefinition($media_type);
   +      if (!in_array($source_definition->getType(), ['file', 'image'])) {
   +        return;
   +      }
   

   This repeats code from hook_install(); seems like we should have a static method somewhere to determine this. How about a static method on the File source plugin to determine if a given media type is compatible with it?

Looks really damn good. Everything I found, to be honest, is relatively nitpicky.

1. +++ b/core/modules/media/media.module
   @@ -58,7 +59,30 @@ function media_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('Media items behave differently than files and images regarding restricted access. While files and images, when configured to use the private storage, will inherit access permissions from the content where they are attached to, media items have their own access restrictions. This usually means that by default users could have access to files on media items even though they do not have access to the content that references the media items. <a href="@doc_url">More information on configuring media access restrictions</a>.', [
   

   Can we rephrase this a bit "...differently than files and images regarding restricted access" might read better as "...differently than files and images where restricted access is concerned."

2. +++ b/core/modules/media/media.module
   @@ -58,7 +59,30 @@ function media_help($route_name, RouteMatchInterface $route_match) {
   +      $source_definition = $media_type->getSource()->getSourceFieldDefinition($media_type);
   +      if (!is_a($source_definition->getClass(), FileFieldItemList::class, TRUE)) {
   +        return;
   +      }
   

   So this is the second time in this patch that we have repeated the "check if this media type uses files" logic. Media Library also does this, which convinces me that this check needs to be a public static method of the File source plugin.

3. +++ b/core/modules/media/media.module
   @@ -347,3 +371,22 @@ function _media_get_add_url($allowed_bundles) {
   +  $changed_from_private = $field->getSetting('uri_scheme') === 'public' && $field->original->getSetting('uri_scheme') === 'private';
   

   If this is a new field, $field->original might not be set, which will cause this code to fatal, no?

4. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -171,4 +174,47 @@ public function testMediaAccess() {
   +    $this->drupalGet("/admin/structure/media/manage/{$media_type->id()}/fields");
   

   Let's just call $this->getSession()->reload() here, rather than repeat the URL.

5. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -171,4 +174,47 @@ public function testMediaAccess() {
   +    $this->drupalGet("/admin/structure/media/manage/{$media_type->id()}/fields");
   

   Nit: Any chance of using Url::fromRoute() to generate the URL?

+++ b/core/modules/media/media.install
@@ -107,6 +108,31 @@ function media_requirements($phase) {
+      $types_to_warn = array_filter($media_types, function (MediaTypeInterface $type) {
+        return File::hasFileSourceField($type);
+      });

Nit: This can be one line: $types_to_warn = array_filter($media_types, [File::class, 'hasFileSourceField']).

1. +++ b/core/modules/media/media.install
   @@ -107,6 +108,31 @@ function media_requirements($phase) {
   +              '@doc_url' => 'https://www.drupal.org/docs/8/core/modules/media/setting-up-private-access-to-media-items',
   
   +++ b/core/modules/media/media.module
   @@ -58,7 +59,30 @@ function media_help($route_name, RouteMatchInterface $route_match) {
   +        '@doc_url' => 'https://www.drupal.org/docs/8/core/modules/media/setting-up-private-access-to-media-items',
   ...
   +          '@doc_url' => 'https://www.drupal.org/docs/8/core/modules/media/setting-up-private-access-to-media-items',
   

   :doc_url

2. +++ b/core/modules/media/media.module
   @@ -347,3 +371,22 @@ function _media_get_add_url($allowed_bundles) {
   +function media_field_storage_config_presave(Drupal\Core\Entity\EntityInterface $field) {
   

   Rather than doing this in a presave hook, this should be done in a config subscriber. See #3017935: Media items should not be available at /media/{id} to users with 'view media' permission by default for an example.

3. +++ b/core/modules/media/media.module
   @@ -347,3 +371,22 @@ function _media_get_add_url($allowed_bundles) {
   +    // @todo There may be a more fine-grained way of doing this.
   +    \Drupal::service('cache.render')->invalidateAll();
   

   Cache::invalidateTags(['rendered']) is that cleaner way :)

   But are you sure we actually need to invalidate this?

4. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -124,4 +125,19 @@ public function createSourceField(MediaTypeInterface $type) {
   +   *   TRUE if the type uses a source plugin with a file/image as a source
   +   *   field, FALSE otherwise.
   ...
   +  public static function hasFileSourceField(MediaTypeInterface $type) {
   ...
   +    return is_a($source_field_definition->getClass(), FileFieldItemList::class, TRUE);
   

   Based on the name, I'd expect
   return $source_field_definition->getType() === 'file';

   But I get it; you want to make sure that @FieldType=image and other subclasses are also detected.

   So let's make that more clear, because it's a bit surprising.

   Also, I don't think the fact that it uses a @FieldType=file field is the problem here, I think the problem is that it's referencing a File entity. Wouldn't it be safer to check whether one of the target types is File? Then it'd also work for https://www.drupal.org/project/dynamic_entity_reference.

1. +++ b/core/modules/media/media.module
   @@ -71,18 +72,28 @@ function media_help($route_name, RouteMatchInterface $route_match) {
   +      $output = [
   ...
   +      return $output;
   

   Übernit: we usually call a variable containing a render array $build.

2. +++ b/core/modules/media/media.module
   @@ -371,22 +382,3 @@ function _media_get_add_url($allowed_bundles) {
   -function media_field_storage_config_presave(Drupal\Core\Entity\EntityInterface $field) {
   

   🎉

3. +++ b/core/modules/media/tests/src/Functional/MediaAccessTest.php
   @@ -201,8 +201,11 @@ public function testMediaAccessWarnings() {
   +    $role = Role::load(RoleInterface::AUTHENTICATED_ID);
   +    $role->grantPermission('administer site configuration');
   +    $role->save();
   

   Übernit: can be chained.

1. +++ b/core/modules/media/media.module
   @@ -58,7 +60,40 @@ function media_help($route_name, RouteMatchInterface $route_match) {
   +        $warning = '<p>' . t('The source field is configured to use the private file storage. By default, the media items will be accessible to any users with access to %type media, even if the user does not have access to content that references it. <a href=":doc_url">More information on configuring media access restrictions</a>.', [
   +            '%type' => $media_type->label(),
   +            ':doc_url' => 'https://www.drupal.org/docs/8/core/modules/media/setting-up-private-access-to-media-items',
   +          ]) . '</p>';
   

   Übernit: small indentation problem here: the last 3 quoted lines have two leading spaces too many.

2. +++ b/core/modules/media/media.module
   @@ -58,7 +60,40 @@ function media_help($route_name, RouteMatchInterface $route_match) {
   +        $build['#markup'] = $warning;
   

   I was concerned that this warning doesn't look like a warning, because it's just help text on the page that is easily missed. But @marcoscano pointed out that this is exactly what was requested by @xjm, because it's a warning that cannot be acted upon/resolved. Tricky to make a decision here, but I get it.

   This was my only remaining concern.

1. It's great that there is a way to disable the warning. On many of the existing warnings people have asked for this.

2. +++ b/core/modules/media/config/install/media.settings.yml
   @@ -1,3 +1,4 @@
   +show_private_file_warning: true
   
   +++ b/core/modules/media/config/schema/media.schema.yml
   @@ -11,6 +11,9 @@ media.settings:
   +    show_private_file_warning:
   +      type: boolean
   +      label: 'Whether the site should show a warning on the UI when the source field is configured to use private file storage'
   

   We need an update path to add this for existing sites. Also I think in the update we should return text telling users that they have this situation in case they are unaware.

3. +++ b/core/modules/media/media.install
   @@ -107,6 +108,29 @@ function media_requirements($phase) {
   +              '%types' => implode(", ", $types_to_warn),
   

   This doesn't really work for translatable strings. We should use an inline item list and render it.

   An inline item list looks like:

         $requires = [
           '#theme' => 'item_list',
           '#items' => $module['#requires'],
           '#context' => ['list_style' => 'comma-list'],
         ];
   

   Also requirements can be render arrays. See code in system_requirements()

4. +++ b/core/modules/media/src/Plugin/media/Source/File.php
   @@ -124,4 +125,24 @@ public function createSourceField(MediaTypeInterface $type) {
   +    $target_types = is_array($target_type) ? $target_type : [$target_type];
   +    return in_array('file', $target_types, TRUE);
   

   I think this can be abbreviated to

   return in_array('file', (array) $target_type, TRUE);
   

5. Given the unexpectedness of how media behaves with private files I wonder if we should consider hiding the private file option when adding file fields to new media types. Because it is extremely hard to get right and extremely easy to get wrong. Perhaps this can be a followup.

Re-roll patch created for 9.1.x

This is certainly a critical issue for the completion of the Media Initiative, so I'm bumping the priority.

Usability review

We looked at this issue at the #3173646: Drupal Usability Meeting 2020-09-29.

Generally, the messages seem clear. Of course, the new documentation page has to be completed.

I am removing the word "warning" from the issue summary.

The only message that seems to need some attention is the one on the help page:

• Remove "to" from "where they are attached to".
• Refer to fields in the first sentence: "Media reference fields" and "file and image fields".
• Break up the long second sentence into two parts, and consider changing the order. Something like "Media items have their own access restrictions. When file and image fields are configured ..., they inherit ...".

I agree with #38 and as said in #37 this is still quite important for Drupal Security and Triaged Media Initiative. So should we incorporate the changes from #38 or is this to be discussed first?

Looking at security and priority we should get this more or simple "fix" in asap, shouldn't we?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

@benjifisher sorry to ask this 3 years later, but do you know which patch was reviewed? #36?

If it's just these wording changes, I'd create a MR against 10.1.x and push things forward here.
Sad to see that https://www.drupal.org/project/media_private_access has no Drupal 9/10 release, while this is still problematic.

I applied patch #36 manually on 10.1.x and changed the wording proposed in #38.

Please review!

Thanks @Grevil! Just one wording change, then all points from #38 are addressed.
See my comment in GitLab. Or am I wrong?

Remove "to" from "where they are attached to".

Well done! As all points from #38 have been addressed, I'm setting this RTBC for maintainers to have a look.

What about "Needs documentation updates" tag?
Also note #4.4 as possible followup. Do we need a discussion on that point?

I think it's most important to get this merged asap.

Looks like tests are failing on coding standards checks.

My apologies, my dev environment is not suited for core issue... Should definitely fix that!

Edit: Will fix that tomorrow.