Problem/Motivation

One of the biggest challenges of the File entity module is providing a more fine-grained access control API for different actions like viewing, downloading, editing, etc. Currently, Drupal core only provides one access API via hook_file_download() which is not compatible with the File entity access API because it assumes that the file is not an entity. This hook is even more fun because it has two purposes: check access to download a file, and return all the headers that should be sent if the file can be downloaded. In addition, this hook is very limited; you cannot check if another user has access to download a file, you can only check if the current user has access.

Proposed resolution

Extend EntityAccessController for use with file entities.

Remaining tasks

  • Make a new hook to replace the 'Provide download headers for this file' functionality of hook_file_download()

User interface changes

None

API changes

Add hook_file_download_headers()

Will need a change notice for hook_file_download().

Files: 
CommentFileSizeAuthor
#45 2148353-45.patch14.2 KBmarthinal
#35 interdiff.txt663 bytesslashrsm
#35 2148353_35.patch13.32 KBslashrsm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,989 pass(es). View
#32 interdiff.txt1.46 KBslashrsm
#32 2148353_32.patch13.23 KBslashrsm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,993 pass(es). View
#30 2148353_30.patch12.28 KBslashrsm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,924 pass(es). View
#28 drupal-file-2148353-28.interdiff.txt10.17 KBtien.xuan.vo
#28 drupal-file-2148353-28.patch15.6 KBtien.xuan.vo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,432 pass(es), 8 fail(s), and 4 exception(s). View
#21 drupal-file-2148353-21.interdiff.txt869 bytesblueminds
#21 drupal-file-2148353-21.patch16.38 KBblueminds
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-file-2148353-21.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

tim.plunkett’s picture

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?

Dave Reid’s picture

Not quite sure I understand. I would want this system to work exactly like node access does, without the grant system.

tim.plunkett’s picture

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.

larowlan’s picture

Any objections if I take a run at this?

larowlan’s picture

larowlan’s picture

Dave Reid’s picture

Status: Closed (duplicate) » Active

This has way more detailed plans on how to replace private file access. We should have at least transfered that knowledge and plan before closing this.

Berdir’s picture

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.

ParisLiakos’s picture

+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.

blueminds’s picture

ParisLiakos’s picture

tbh, i like this issue's summary more

Dave Reid’s picture

Status: Closed (duplicate) » Active

Please *do not close this again*. #2078473: Use entity access API for checking access to private files is for removing hook_file_download_access(). This issue is for removing hook_file_download().

Berdir’s picture

slashrsm’s picture

Xano’s picture

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

ParisLiakos’s picture

Status: Active » Needs review
FileSize
10.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,153 pass(es), 55 fail(s), and 9 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 16: drupal-file-2148353-16.patch, failed testing.

blueminds’s picture

Assigned: Unassigned » blueminds
Issue summary: View changes

Will take a look at those tests.

blueminds’s picture

Status: Needs work » Needs review
FileSize
15.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,940 pass(es), 2 fail(s), and 0 exception(s). View
14.42 KB

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.

Status: Needs review » Needs work

The last submitted patch, 19: drupal-file-2148353-19.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
16.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-file-2148353-21.patch. Unable to apply patch. See the log in the details link for more information. View
869 bytes

Fixing tests

Berdir’s picture

+++ 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...

ParisLiakos’s picture

+++ 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

Berdir’s picture

@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.

ParisLiakos’s picture

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

Status: Needs review » Needs work

The last submitted patch, 21: drupal-file-2148353-21.patch, failed testing.

tien.xuan.vo’s picture

Status: Needs work » Needs review
FileSize
15.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,432 pass(es), 8 fail(s), and 4 exception(s). View
10.17 KB

Re-roll the patch

Status: Needs review » Needs work

The last submitted patch, 28: drupal-file-2148353-28.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
12.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,924 pass(es). View

Reroll.

jibran’s picture

Status: Needs review » Needs work

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.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
13.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,993 pass(es). View
1.46 KB

Thanks. #31 addressed.

slashrsm’s picture

Thanks. #31 addressed.

jibran’s picture

Assigned: blueminds » Unassigned
Status: Needs review » Reviewed & tested by the community

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

slashrsm’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
13.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,989 pass(es). View
663 bytes

Improved hook documentation a bit. Change record draft created.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Nice improvements and thanks for creating the change notice.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
+++ 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?

Berdir’s picture

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?

alexpott’s picture

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

Dave Reid’s picture

Issue tags: +D8Media

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

marthinal’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
14.2 KB

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;
  }
marthinal’s picture

Issue tags: +drupaldevdays

Status: Needs review » Needs work

The last submitted patch, 45: 2148353-45.patch, failed testing.