File access (hook_file_download) does not use an EntityAccessController

One thing that doesn't quite fit neatly into any of the tasks above:
To deny access completely, an implementation would return -1, and then all three invokers call throw new AccessDeniedHttpException();.

Should the hook just throw that themselves? would that remain part of new hook, or become part of the entity access?

I misunderstood how the headers were defined. In all core implementations, the headers for an accessible file are always the same, it is not tied to any of the access logic. +1 to the proposal.

Any objections if I take a run at this?

Added #2128791: File resource needs an access controller to related

Duplicate of #2078473: Use entity access API for checking access to private files
See also #1327224: Access denied to taxonomy term image

The main reason I haven't been successful with this is that file exists has the special-flower $field_type context and is only called and checked for files referenced via a given field_type. If we can drop that, great, I have no idea why we need it exactly.

Not sure with download vs view, why do we need two different operations for that? The way core works is that it respects field access view if explicitly provided in the file download access hook. I can see a case where you want to display the field but then not actually allow download access. However, core has no way to do that, as we would need a way to disable the link or redirect to another page to do that in a way that makes sense.

Also, the two issues *are* different. This is about moving the logic here into a access controller, while the other is about changing the logic and instead of using custom hook, replace it by using the existing entity access API. Can be combined into one issue, though.

+1 for dropping $field_type parameter

download vs view: i think core shouldn't be bothered by it and treat it the same..ie view. i can think of some use cases, but those could easily just be in contrib.

Addressed in #2078473: Use entity access API for checking access to private files

tbh, i like this issue's summary more

#438414: hook_file_download() should pass $file objects rather than $filepath

4 years later ;)

Should we postpone this until #2078473: Use entity access API for checking access to private files goes in?

That was *just* fixed, so postponing this issue is no longer necessary :)

i did a start on this, tests will fail, but maybe someone can drive this till i have some time

Will take a look at those tests.

Following things have been done:

- File entity now has getContentHeaders() method that replaced file_get_content_headers()
- all the access logic moved to the FileAccessController, therefore file_file_download(_headers) was removed
- image_file_download(_headers) was removed as the logic is now in the ImageStyleDownloadController::deliver()
- hook_unmanaged_file_download_headers() was introduced to be able to add headers to unmanaged files.

So currently there is no hook that would deal with headers for a managed download file. But consulted this with @slashsrm and @Berdir and they confirmed that there has never been a usecase that needed to alter/add headers. However if that is the case the KernelEvents::RESPONSE can be used to do so.

Let's see what the testbot has and others have to say about this, then the issue summary probably will need to be updated and also a change record will be needed.

Fixing tests

+++ b/core/modules/file/src/Tests/DownloadTest.php
@@ -86,9 +86,9 @@ protected function doPrivateFileTransferTest() {
-    file_test_set_return('download', -1);
+    file_test_set_return('download', NULL);

We should rename the key here I think, this matches the hook name for everything else.

Also, this is an API change to before, do we want to keep support for returning -1 ? Should be easy to support.

That said, we do need a new test that is using managed files and hook_file_access() to deny/grant access I think...

+++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
@@ -112,6 +112,7 @@ public function _testResponsiveImageFieldFormatters($scheme) {
+    debug($test_image);

:)

Also, this is an API change to before, do we want to keep support for returning -1 ? Should be easy to support.

No, -1 has nothing to do with headers, its access related, we are trying to completely split them

@ParisLiakos:

No we are not splitting access and headers (anymore). We are splitting managed files (which handle headers automatically) and unmanaged files, for which we still have the mix of headers and access. We can't use file access for those unless we upcast them to file entities on the fly, which doesn't make sense to me.

ah, i see..i clearly didnt pay much attention :)
in that case, i can certainly see usecases on that

Re-roll the patch

Reroll.

Thank you for the reroll. Code changes are fine and tests are green.
But I think we need an file.api.php change for the hook hook_unmanaged_file_download_headers we added here.

+++ b/core/modules/file/src/FileInterface.php
@@ -119,4 +119,13 @@ public function setTemporary();
+   * @return array
+   *   An associative array of headers, as expected by
+   *   \Symfony\Component\HttpFoundation\StreamedResponse.

We should add some docs that this method can return an empty array.

Thanks. #31 addressed.

Thanks. #31 addressed.

Thanks for the fix. I think we need a change notice as well but this is ready.

Improved hook documentation a bit. Change record draft created.

Nice improvements and thanks for creating the change notice.

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -10,6 +10,7 @@
+use Drupal\file\Entity\File;

@@ -18,6 +19,7 @@
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

Not used

I'm not sure I can see exactly what bug is being fixed here. Access control still seems to be being controlled by whether or not we return headers

+++ b/core/modules/system/src/FileDownloadController.php
@@ -68,4 +59,36 @@ public function download(Request $request, $scheme = 'private') {
+  protected function getContentHeaders($file_uri) {
+    if ($this->moduleHandler()->moduleExists('file')) {
+      $files = $this->entityManager()
+        ->getStorage('file')
+        ->loadByProperties(array('uri' => $file_uri));
+      /** @var \Drupal\file\FileInterface $file */
+      $file = reset($files);
+
+      if (!empty($file) && $file->access('download')) {
+        return $file->getContentHeaders();
+      }
+    }
+
+    if (empty($headers)) {
+      return $this->moduleHandler()->invokeAll('unmanaged_file_download_headers', array($file_uri));
+    }

This looks like $file->access('download') returns FALSE then it is possible for a unmanaged_file_download_headers implementation to override it. Does that mkae sense?

The fallback is there for things that are not managed files.. we have a few uses of that, config export is AFAIK one of them.

Maybe we can refactor it so that when we can match it to a managed file then we respect only the access controller and return FALSE if download access is denied?

That said, I think the only bug here is that we make life hard for file_entity that is trying to have more complex access logic than core is providing for files. So agreed on needing an issue summary update and possibly we need to re-classify this to task and/or not major?

@Berdir what i was trying to say is that if $file->access('download') returns FALSE then we shouldn't be calling the hook.

#2310307: File needs CRUD permissions to make REST work on entity/file/{id} landed, that should make things easier here.

Rerolled. As suggested by @alexpott if $file->access('download') returns FALSE then we don't need to call *unmanaged_file_download_headers*.

  protected function getContentHeaders($file_uri) {
    $headers = NULL;

    if ($this->moduleHandler()->moduleExists('file')) {
      $files = $this->entityManager()
          ->getStorage('file')
        ->loadByProperties(array('uri' => $file_uri));
      /** @var \Drupal\file\FileInterface $file */
      $file = reset($files);

      if (!empty($file) && $file->access('download')) {
        $headers = $file->getContentHeaders();
        if (empty($headers)) {
          $headers = $this->moduleHandler()->invokeAll('unmanaged_file_download_headers', array($file_uri));
        }
      }
    }

    return $headers;
  }

As suggested in #2148353-38: File access (hook_file_download) does not use an EntityAccessController, moving this to a task.

This was a daily bugsmash triage issue, adding tag.

I also made a reroll.

Fixed custom commands against #59