File resource needs an access controller

Maybe we could just introduce an "admin_permission" on the File entity.

Let's try this patch. @juampy, should be the access controller into the file core module? If we can access to the node and to the file attached to this node too... we don't need to alter the entity, I mean the file should be accessible.

Files do have their own access API that is important for e.g. private files, you can't just allow anyone to see them. See file_file_download(), I tried to deprecate that in favor of relying on an access controller but the problem is that based on how this currently works (the API is limited to a specific field type), there's not enough context in the entity access controller/hooks.

I've also opened #2078473: Use entity access API for checking access to private files to deprecate the custom API in favor of relying more on the entity access API, I originally tried to do that in #1327224: Access denied to taxonomy term image.

+++ b/core/modules/rest/rest.module
@@ -34,3 +34,14 @@ function rest_help($path, $arg) {
+function rest_entity_info_alter(&$entity_info) {
+  if (isset($entity_info['file'])) {
+    $controllers = $entity_info['file']->get('controllers');
+    $controllers['access'] = 'Drupal\\rest\\FileAccessController';
+    $entity_info['file']->set('controllers', $controllers);
+  }
+}

So.. this means that when the rest module is enabled, every part of drupal that wants to check access to a public file will go through Drupal\rest\FileAccessController, which returns TRUE for that scheme. That doesn't sound right at all...

For a start, move the access controller to file.module. rest.module should rely on the entity access API, not define access rules itself.

Then we need to decide what to do about private files.

file_permission() defines 'access files overview', you could require that for private files?

Added #2148353: File access (hook_file_download) does not use an EntityAccessController to related issues

@Berdir is right in #6: hook_file_download_access() (only called by file_file_download(), which is an implementation of what seems to be an undocumented hook) takes too much context to be easily replaced with hook_entity_access(). What I'm not sure of is 1) should file entities always be field values, or can they exist stand-alone? and 2) Can a single file entity be a value for multiple file fields? Let's discuss this in #2078473: Use entity access API for checking access to private files, however.

This patch moves the access controller to the File module and adds some type hinting to make IDEs happy.

@juampy, sorry for not being more detailed in my review, #13 / #15 is what I meant :)

@Xano: Both hooks are documented, hook_file_download() is in system.api.php. I'm fine with getting a minimal implementation in here and continue in the referenced issue, but I'm not sure what that minimal implementation needs to do exactly :)

Minimal implementation of what?

Of the file access controller? If we need to care about private files, or not, ...

We need it in the access controller at least, because the method expects EntityInterface and not FileInterface, but we are calling FileInterface::getFileUri().

@juampy It doesn't matter, the actual typehint in the method params will prevent an IDE from caring about the @param. It needs to be inline.

+++ b/core/modules/file/lib/Drupal/file/FileAccessController.php
--- /dev/null
+++ b/core/modules/rest/lib/Drupal/rest/Tests/FileTest.php

the test in REST module looks wrong. This should have a unit test for file entity access() in file.module, since there is no bug in REST module, right?

And I don't think this is critical, Drupal does not stop working, this is not a security issue and this could be fixed from contrib.

This is a first attempt. I don't know if we need to attach first the test for unit test. So the only test patch will crash.

I was talking with @juampy about the possibility to use the functional test for the rest module to reproduce this bug too... I understood that he agrees about that.

let's try again

When I run this unit test from phpunit directly it pass. When I run the test using the run-test.sh script it fails because file_uri_scheme() is used from core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php and not from the current test.

32: 2128791-32.patch queued for re-testing.

Related to and addressed in #2078473: Use entity access API for checking access to private files

Fix media tag.