The entity:file resource needs an access controller in order to be able to view file entities through the REST module.

Example:

1. On a clean Drupal installation, Enable rest module.
2. Add a file filed to the page content type and create a page adding a file to it.
3. Enable node and file REST resources and give permissions to anonymous users to access GET method on these two.
4. Request http://d8.local/entity/node/1 >> you will get the page node, which contains the file entity.
5. Request http://d8.local/entity/file/1 >> you will get a 403 error.

This happens because the file entity uses EntityAccessController->checkAccess(), which simply looks for the permission 'administer files'.

Should we add more fine grained permissions for entities in the rest module?

Files: 
CommentFileSizeAuthor
#35 2128791-35-should-fail.patch3.95 KBmarthinal
FAILED: [[SimpleTest]]: [MySQL] 63,334 pass(es), 1 fail(s), and 0 exception(s). View
#32 2128791-32.patch4.21 KBmarthinal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2128791-32.patch. Unable to apply patch. See the log in the details link for more information. View
#15 interdiff.txt3.46 KBXano
#15 drupal_2128791_15.patch3.54 KBXano
PASSED: [[SimpleTest]]: [MySQL] 60,043 pass(es). View
#8 drupal-file-access-controller-only-test-2128791-8.patch1.77 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 59,898 pass(es), 1 fail(s), and 0 exception(s). View
#5 intediff-2128791-1-5.txt649 bytesmarthinal
#1 drupal-file-access-controller-2128791-1.patch1.4 KBjuampynr
FAILED: [[SimpleTest]]: [MySQL] 59,741 pass(es), 16 fail(s), and 214 exception(s). View

Comments

juampynr’s picture

Status: Active » Needs review
FileSize
1.4 KB
FAILED: [[SimpleTest]]: [MySQL] 59,741 pass(es), 16 fail(s), and 214 exception(s). View

Here is a patch that adds a very simple access controller. After aplying it, http://d8.local/entity/file/1 will return the requested file if permission 'access get on entity:file' resource is active for anonymous users.

juampynr’s picture

Status: Needs review » Needs work

The last submitted patch, 1: drupal-file-access-controller-2128791-1.patch, failed testing.

dawehner’s picture

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

marthinal’s picture

Status: Needs work » Needs review
FileSize
649 bytes
1.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,897 pass(es). View

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.

Berdir’s picture

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.

juampynr’s picture

Priority: Normal » Critical
FileSize
672 bytes
1.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,834 pass(es). View

Public files are just accessible in the browser without any extra access check. Therefore, their REST resource should not rely on EntityAccessController->checkAccess().

Here is an updated version where the FileAccessController only allows access to public files. Other file streams go through EntityAccessController->checkAccess().

At the moment the REST File resource simply does not work without this patch.

juampynr’s picture

FileSize
3.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,911 pass(es). View
1.77 KB
FAILED: [[SimpleTest]]: [MySQL] 59,898 pass(es), 1 fail(s), and 0 exception(s). View

Adding a test to prove this. One patch contains just the test. The other one contains the test and the AccessController.

Status: Needs review » Needs work

The last submitted patch, 8: drupal-file-access-controller-only-test-2128791-8.patch, failed testing.

juampynr’s picture

Status: Needs work » Needs review

Verified. Patch with just the test fails.

amateescu’s picture

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

juampynr’s picture

@amateescu, can you give me a hint on how to tackle the problem described at the top of this issue?

Berdir’s picture

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?

larowlan’s picture

Xano’s picture

FileSize
3.54 KB
PASSED: [[SimpleTest]]: [MySQL] 60,043 pass(es). View
3.46 KB

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

amateescu’s picture

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

Berdir’s picture

@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 :)

Xano’s picture

Minimal implementation of what?

Berdir’s picture

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

juampynr’s picture

+++ b/core/modules/file/lib/Drupal/file/FileAccessController.php
@@ -0,0 +1,34 @@
+    /** @var \Drupal\file\FileInterface $entity */

Do we need this? @inheritdoc should take care of it.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/FileTest.php
@@ -0,0 +1,62 @@
+    /** @var \Drupal\file\FileInterface $file */

Same here. Do we need this?

Xano’s picture

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

juampynr’s picture

Shouldn't we remove the @inheritdoc and write the list of arguments so it matches reality?

tim.plunkett’s picture

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

juampynr’s picture

Oh, I see now. Thanks.

Are we done here?

klausi’s picture

Component: rest.module » file.module
Priority: Critical » Major
Status: Needs review » Needs work
+++ 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.

Dave Reid’s picture

Issue tags: +Media Initiative
marthinal’s picture

Assigned: Unassigned » marthinal
Issue tags: +#D8SVQ, +#SprintWeekend2014
marthinal’s picture

Issue tags: -#D8SVQ, -#SprintWeekend2014 +D8SVQ, +SprintWeekend2014
marthinal’s picture

Status: Needs work » Needs review
FileSize
2.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2128791-29-only-test.patch. Unable to apply patch. See the log in the details link for more information. View
4.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2128791-29.patch. Unable to apply patch. See the log in the details link for more information. View

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.

The last submitted patch, 29: 2128791-29-only-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 29: 2128791-29.patch, failed testing.

marthinal’s picture

FileSize
4.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2128791-32.patch. Unable to apply patch. See the log in the details link for more information. View

let's try again

marthinal’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: 2128791-32.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
FileSize
3.95 KB
FAILED: [[SimpleTest]]: [MySQL] 63,334 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 35: 2128791-35-should-fail.patch, failed testing.

The last submitted patch, 35: 2128791-35-should-fail.patch, failed testing.

marthinal’s picture

Status: Needs work » Needs review
xjm’s picture

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

The last submitted patch, 32: 2128791-32.patch, failed testing.

juampynr’s picture

Status: Needs review » Closed (won't fix)

Since [#2241059] this bug does not make sense anymore. Paths have changed and now I am seeing a different bug that I will describe in a new issue.

Closing.

juampynr’s picture

blueminds’s picture

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix media tag.