Extracting what I tried to do in https://drupal.org/node/1327224 into separate issue, so that we can get a simple bugfix in.

This might not be possible for 8.x, sadly.

Will provide a detailed issue summary, the basic idea is to replace hooks, which every entity type has to implement to work with private files, with relying on entity access and implying that view access to an entity also allows access to attached files.

Comments

Xano’s picture

I have two questions:

  1. Should file entities always be field values, or can they exist stand-alone? What if someone wants file management without using fields?
  2. Can a single file entity be a value for multiple file fields?
larowlan’s picture

1) no (see file entity project)
2) yes (see media project)

Berdir’s picture

1) Core handles file access for files attached to entities and makes it easier for entities to control access to their files using hook_file_download_access_alter(). Anything else can be done by implementing hook_file_download() yourself. This hook is currently based on the path. I guess this would be replaced with an hook_file_access() implementation that acts on the file entity if we move this into the

2) Yes. file_file_download() implements it so that access is granted if the user is able to view the file if any of those references grant access. The current implementation already checks the field access API, we just need to replace all the hook_file_download_access() with a single $entity->access('view'), as earlier patches of mine did in the referenced issue. If for whatever reason that assumption is *not* true, there's still the possibly to interfere by implementing hook_file_download($uri) (now) or hook_file_access($file) (if we replace it using an access controller), just requires a bit more code.

Fact is that most entities are *not* implementing this hook, which means that private files attached to them are *not* visible. This still includes terms in 7.x as the related issue has not yet been backported. The suggested default assumption is going to solve 99% of the use cases without additional code for entity types.

The main problem to move this into the access controller is the weird $field_type stuff, that limits all this logic to a single field type (defaults to file and then image calls this hook again using it's own field type). I'm not sure why we need this nor if is actually working, if you look at file_get_file_references(), you can see that there is a loop that overrides the $field_type argument.

Dave Reid’s picture

Issue summary: View changes
Issue tags: +Media Initiative
blueminds’s picture

Assigned: Unassigned » blueminds
Status: Active » Needs review
FileSize
13.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2078473-file-access-controller-5.patch. Unable to apply patch. See the log in the details link for more information. View
12.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2078473-file-access-controller-NO-COMMENT-UPDATE5.patch. Unable to apply patch. See the log in the details link for more information. View

Here is a patch that:
- Provides the FileAccessController
- Removes the hook_file_download_access
- Updates the comment access controller to consider the comment status and the access to the parent entity

The last submitted patch, 5: 2078473-file-access-controller-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2078473-file-access-controller-NO-COMMENT-UPDATE5.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
18.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,992 pass(es), 14 fail(s), and 0 exception(s). View
18.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,306 pass(es), 8 fail(s), and 0 exception(s). View

rerolled

The last submitted patch, 8: 2078473-file-access-controller-NO-COMMENT-UPDATE-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2078473-file-access-controller-7.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
947 bytes
18.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,964 pass(es), 9 fail(s), and 0 exception(s). View
19.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,183 pass(es), 8 fail(s), and 0 exception(s). View

another try

Status: Needs review » Needs work

The last submitted patch, 11: 2078473-file-access-controller-12.patch, failed testing.

The last submitted patch, 11: 2078473-file-access-controller-NO-COMMENT-CHANGE-12.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
12.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,013 pass(es), 2 fail(s), and 0 exception(s). View
14.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,002 pass(es), 1 fail(s), and 0 exception(s). View

hopefully this time...

The last submitted patch, 14: 2078473-file-access-controller-NO-COMMENT-CHANGE-14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2078473-file-access-controller-14.patch, failed testing.

blueminds’s picture

Status: Needs work » Needs review
FileSize
15.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,309 pass(es). View
1.83 KB

- Fixed the private file test
- The fail without the comment controller update shows that the actual update of the controller fixes the issue introduced by removing the file_download_access hook and removing further access logic inside file_file_download.

slashrsm’s picture

Patch looks good to me. Agree with potential problems with different return behaviour of hook_file_download and access controller, but I'm not sure if this is something that should be addressed in this issue.

andypost’s picture

Issue tags: +API change, +Needs issue summary update

Summary should be updated for current patch that looks great!

+++ b/core/modules/file/file.api.php
@@ -78,60 +78,5 @@ function hook_file_move(Drupal\file\FileInterface $file, Drupal\file\FileInterfa
-function hook_file_download_access($field, Drupal\Core\Entity\EntityInterface $entity, Drupal\file\FileInterface $file) {

hook removal is api change and seems need approval

blueminds’s picture

FileSize
15.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,367 pass(es). View
1.84 KB

Added the download operation to check the access.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

OK. Looks good and it seems that we pretty much agree on it. Let's see what committers have to say about it (API change, etc.).

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Yep this is much better then having to implement a hook. Nice work.

Committed 64387e5 and pushed to 8.x. Thanks!

  • alexpott committed 64387e5 on 8.x
    Issue #2078473 by blueminds | Berdir: Added Use entity access API for...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix media tag.