Image field default value not shown when upload destination set to private file storage

Created on 8 October 2013, over 10 years ago
Updated 17 February 2024, 8 days ago

Problem/Motivation

In an image field's configuration, you can specify a default image to be displayed as a fallback if the content editor does not upload an image to that field.

When an image field's Upload Destination is set to the Private file storage destination (i.e.: the private:// stream), attempting to view (i.e.: download) the default image returns an HTTP 403 Access Denied, resulting in a broken image on the page.

Steps to reproduce

  1. Add an image field to a content entity type. In the new image field's Field Settings, set its Upload destination to Private files. In its settings, upload a default image.
  2. Add an image field to a content entity type. In the new image field's Field Settings, set its Upload destination to Public files. In its settings, upload a default image.
  3. Create a new instance of that content entity. Leave both image fields empty.
    • Expected behavior: The default image for both fields are shown,
    • Actual behavior: The default image for the field whose Upload Destination is set to "Public files" is shown. The default image for the field whose Upload Destination is set to "Private files" is broken.

Inspection in the browser's Network Console shows Drupal responds to the browser's request with an HTTP/403 Access Denied response. Further inspection of the cause of the 403 shows that a \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException is thrown from \Drupal\image\Controller\ImageStyleDownloadController::deliver() because $this->moduleHandler()->invokeAll('file_download', [$image_uri]) fails to return any headers (i.e.: indicating access denied). In particular, the image module's implementation of hook_file_download() (i.e.: image_file_download()) does not handle a case for default images.

As of 2020-05-29, this happens on 9.1.x and 8.8.x.

Proposed resolution

Modify image_file_download() to handle the case for default images, by granting access if the image URI that is being requested happens to be the default image for at least one field that the current user has 'view' access to.

Remaining tasks

  1. Review and feedback - in particular, is the access check that we are making sound and complete?
  2. RTBC
  3. Maintainer review, feedback
  4. Commit
  5. Backport?

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

To be determined.

Original report by claudiu.cristea

On an image field where the uploaded destination is set to private:// stream, the default image is returning 403.

If you encounter a WSOD while manually testing, see #2799837: WSOD when changing uri_scheme and setting a new default image at the same time for an image field. β†’ .

πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
Image moduleΒ  β†’

Last updated 4 days ago

Created by

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Live updates comments and jobs are added and updated live.
  • Needs subsystem maintainer review

    It is used to alert the maintainer(s) of a particular core subsystem that an issue significantly impacts their subsystem, and their signoff is needed (see the governance policy draft for more information). Also, if you use this tag, make sure the issue component is set to the correct subsystem. If an issue significantly impacts more than one subsystem, use needs framework manager review instead.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates : some issue and comment data are missing.

  • Status changed to RTBC about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States xjm

    This is quite a lot of procedural code, plus the &drupal_static() in there. This looks like D7 code. Is there a reason it has to be added to image.module rather than being better encapsulated?

    Tagging for framework manager feedback on the architecture. This could also use @claudiu.cristea's feedback as the subsystem maintainer -- their patch was the original prototype back in 2013, but the codebase has changed quite a lot since then.

  • Status changed to Needs work about 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
    1. +++ b/core/modules/image/image.module
      @@ -167,6 +168,115 @@ function image_file_download($uri) {
      +  if (strpos($path, 'default_images/') === 0) {
      

      Can use str_starts_with() here.

    2. +++ b/core/modules/image/image.module
      @@ -167,6 +168,115 @@ function image_file_download($uri) {
      +/**
      + * Map default values for image fields, and those fields' configuration IDs.
      + *
      + * This is used in image_file_download() to determine whether to grant access to
      + * an image stored in the private file storage.
      + *
      + * @return array
      + *   An associative array, where the keys are image file URIs, and the values
      + *   are arrays of field configuration IDs which use that image file as their
      + *   default image. For example,
      + *
      + * @code [
      + *     'private://default_images/astronaut.jpg' => [
      + *       'node.article.field_image',
      + *       'user.user.field_portrait',
      + *     ],
      + *   ]
      + * @code
      + */
      +function image_get_default_image_fields() {
      +  $defaults = &drupal_static(__FUNCTION__);
      +  $cid = 'image:default_images';
      +
      +  if (!isset($defaults)) {
      +    if ($cache = \Drupal::cache()->get($cid)) {
      +      $defaults = $cache->data;
      +    }
      +    else {
      +      // Save a map of all default image UUIDs and their corresponding field
      +      // configuration IDs for quick lookup.
      +      $defaults = [];
      +      $fields = \Drupal::entityTypeManager()
      +        ->getStorage('field_config')
      +        ->loadMultiple();
      +
      +      foreach ($fields as $field) {
      +        if ($field->getType() == 'image') {
      +          // Check if there is a default image in the field config.
      +          if ($field_uuid = $field->getSetting('default_image')['uuid']) {
      +            $file = \Drupal::service('entity.repository')
      +              ->loadEntityByUuid('file', $field_uuid);
      +
      +            // A default image could be used by multiple field configs.
      +            $defaults[$file->getFileUri()][] = $field->get('id');
      +          }
      +
      +          // Field storage config can also have a default image.
      +          if ($storage_uuid = $field->getFieldStorageDefinition()->getSetting('default_image')['uuid']) {
      +            $file = \Drupal::service('entity.repository')
      +              ->loadEntityByUuid('file', $storage_uuid);
      +            if ($file) {
      +              // Use the field config id since that is what we'll be using to
      +              // check access in image_file_download().
      +              $defaults[$file->getFileUri()][] = $field->get('id');
      +            }
      +          }
      +        }
      +      }
      +
      +      // Cache the default image list.
      +      \Drupal::cache()
      +        ->set($cid, $defaults, CacheBackendInterface::CACHE_PERMANENT, ['image_default_images']);
      +    }
      +  }
      +
      +  return $defaults;
       }
      

      I agree with @xjm - this code should be a service and the if we do have a cache then it'll need to be cleared when a field is changed.

    3. When we add the service we can also move the code from image_file_download there too
  • πŸ‡©πŸ‡ͺGermany jan kellermann

    Reworks patch #74 for Drupal 9.4.13

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    Hiding the D9.4 patch; we don't need that. We need #82 and #83 addressed. Thanks!

  • πŸ‡ΊπŸ‡ΈUnited States xjm
  • πŸ‡ΊπŸ‡ΈUnited States bas123

    I just updated to Social (social-11.8.12) and the ugly Default Profile image wouldn't display once again and I couldn't get any of the patches to work until I tried #84 πŸ› Image field default value not shown when upload destination set to private file storage Needs work ( 2107455-84.patch β†’ )

    At least for now that one solved it in four Drupal Open Social websites! so Kudos to Jan Kellermann β†’ !

    I just wish this problem which has persisted for several years and incarnations would ge committed to core...

  • 17:03
    13:48
    Running
  • πŸ‡©πŸ‡ͺGermany metalbote Aachen

    Just reroll for 10.1.6

  • last update 3 months ago
    Patch Failed to Apply
  • First commit to issue fork.
  • last update 3 months ago
    29,717 pass, 2 fail
  • πŸ‡ΊπŸ‡ΈUnited States yospyn

    Upgrading to core 10.2.1 caused #79 (and #89) to fail to apply.

    I was using #79 successfully with 10.1.7.

  • First commit to issue fork.
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update about 1 month ago
    Patch Failed to Apply
  • πŸ‡³πŸ‡΄Norway eiriksm Norway

    eiriksm β†’ changed the visibility of the branch 11.x to hidden.

  • πŸ‡³πŸ‡΄Norway eiriksm Norway

    Here is a patch for 10.2

    Going to try to take a swing at the feedback in #82 and #83

  • πŸ‡³πŸ‡΄Norway eiriksm Norway

    eiriksm β†’ changed the visibility of the branch 11.x to active.

  • Status changed to Needs review about 1 month ago
  • πŸ‡³πŸ‡΄Norway eiriksm Norway

    Opened a MR (against 11.x) where i added back the test module needed to actually run the tests in the patch.

    Also refactored the procedural style code into a service.

    I'm pretty sure we are missing a bit of test coverage on this one though (for example the code branch concerning "A default image could be used by multiple field configs."), but since it's a bug it might not be the main priority. The test certainly demonstrates the bug at least, so that's the most important part for me.

    Anyway. the feedback in #82 and #83 should be addressed in this MR

  • Status changed to Needs work about 1 month ago
  • The Needs Review Queue Bot β†’ tested this issue.

    While you are making the above changes, we recommend that you convert this patch to a merge request β†’ . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

  • πŸ‡ΊπŸ‡ΈUnited States yospyn

    Patch #94 via eiriksm worked for me on 10.2.3, many thanks.

  • Status changed to Needs review 13 days ago
  • πŸ‡³πŸ‡΄Norway eiriksm Norway

    Guess it should have status needs review?

  • Status changed to Needs work 8 days ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Assuming the MR, appears to have test failures.

Production build https://api.contrib.social 0.61.6-2-g546bc20