Problem/Motivation

  • Private image fields on taxonomy term pages always return a 403.

Steps to reproduce

  1. Log in as user 1.
  2. Enable a private files directory.
  3. Create a vocabulary.
  4. Add an image field to the vocabulary that uses the private files directory.
  5. Add a term to the vocabulary and upload an image for the term.
  6. Visit the term page.

Expected result

The image is visible on the term page.

Actual result

The image returns a 403.

Proposed resolution

  • TBD

Remaining tasks

  • TBD

User interface changes

  • TBD

API changes

  • TBD

Original report by johnv

I reported this earlier as a D7.0 error in #846296: file_file_download() only implements access checks for nodes and users. This page now generates a 'Access denied', so i presumed it was fixed as a security issue. However, the error still exists on D7.9.

This is how to create the issue:
- create a vocabulary, and add an image field. The field uses the private file system;
- add terms;
-- under manage fields, add an image using the private file system;
-- under manage display, set to Image and use a Image style;
- create a Views display showing terms. Include an Image field from the term.
- show the Views display: for each image to be displayed, the following warning is generated and the image is not shown:
TYPE access denied
LOCATION http://example.com:8082/system/files/styles/thumbnail/private/tpm/my_ima...
MESSAGE system/files/styles/thumbnail/private/tpm/my_image.jpg
- show the default Term page: idem

Attached patch fixes the issue.

Files: 
CommentFileSizeAuthor
#64 taxonomy-term-file-download-access-1327224-64.patch4.51 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 59,283 pass(es).
[ View ]
#64 taxonomy-term-file-download-access-1327224-64-interdiff.txt1.21 KBBerdir
#61 taxonomy-term-file-download-access-1327224-61.patch4.38 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 59,233 pass(es), 1 fail(s), and 3 exception(s).
[ View ]
#61 taxonomy-term-file-download-access-1327224-61-interdiff.txt1.1 KBBerdir
#57 taxonomy-term-file-download-access-1327224-57.patch4.42 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,538 pass(es).
[ View ]
#57 taxonomy-term-file-download-access-1327224-57-test-only.patch3.51 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,076 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#55 taxonomy-term-file-download-access-1327224-55.patch3.79 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,512 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#49 file-download-access-1327224-49.patch11.13 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file-download-access-1327224-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#49 file-download-access-1327224-49-interdiff.txt2.11 KBBerdir
#47 file-download-access-1327224-47.patch11.07 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,745 pass(es), 0 fail(s), and 6 exception(s).
[ View ]
#45 file-download-access-1327224-45.patch12.96 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,398 pass(es), 4 fail(s), and 10 exception(s).
[ View ]
#45 file-download-access-1327224-45-interdiff.txt3.21 KBBerdir
#42 file-download-access-1327224-42.patch10.02 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,516 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#42 file-download-access-1327224-42-interdiff.txt783 bytesBerdir
#31 file-download-access-1327224-31-interdiff.txt3.47 KBBerdir
#31 file-download-access-1327224-31.patch9.26 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,459 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#31 file-download-access-1327224-31-test-only.patch3.12 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 53,462 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#29 file-download-access-1327224-29.patch8.04 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,076 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
#27 taxonomy_image_test-1327224-27.patch1.9 KBryanissamson
FAILED: [[SimpleTest]]: [MySQL] 49,093 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#25 file-download-access-default-1327224-25.patch3.4 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,273 pass(es), 18 fail(s), and 0 exception(s).
[ View ]
#19 1327224-access_denied_taxonomy_term-19.patch552 bytesryanissamson
FAILED: [[SimpleTest]]: [MySQL] 49,375 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 1327224-17-access_denied_taxonomy_term_image.patch533 bytesnyirocsaba
PASSED: [[SimpleTest]]: [MySQL] 40,572 pass(es).
[ View ]
#16 1327224-16-access_denied_taxonomy_term_image.patch566 bytesedb
PASSED: [[SimpleTest]]: [MySQL] 40,249 pass(es).
[ View ]
#7 1327224-access_denied_taxonomy_term_image.patch703 byteschris.leversuch
PASSED: [[SimpleTest]]: [MySQL] 34,322 pass(es).
[ View ]
#3 1327224_3_taxonomy_Access_denied_to_term_image.patch626 bytesjohnv
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1327224_3_taxonomy_Access_denied_to_term_image.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
846296_taxonomy_Access denied to taxonomy term image.patch631 bytesjohnv
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [846296_taxonomy_Access denied to taxonomy term image.patch] from [drupal.org].
[ View ]

Comments

johnv’s picture

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, 846296_taxonomy_Access denied to taxonomy term image.patch, failed testing.

johnv’s picture

StatusFileSize
new626 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1327224_3_taxonomy_Access_denied_to_term_image.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

patch without spaces in name?

johnv’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1327224_3_taxonomy_Access_denied_to_term_image.patch, failed testing.

xjm’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+Needs tests, +Novice

Thanks for the patch! In order for testbot to test it, it should be created with git. See: http://drupal.org/patch/create

Tagging as novice for creating a new patch.

We should also add tests for this. Should be simple enough: Try to access a term, and if it can be accessed, assert that a file attached to it can also be accessed.

chris.leversuch’s picture

Status:Needs work» Needs review
StatusFileSize
new703 bytes
PASSED: [[SimpleTest]]: [MySQL] 34,322 pass(es).
[ View ]

Here's a git version of the patch. I wasn't sure if it needed to go in a specific place in the file so I've just put the code at the end.

I'll need to read up on writing tests if they need to be in the same patch.

Damien Tournoud’s picture

<?php
+    // Drupal core doesn't restrict access to taxonomy terms, so access to any
+    // files in the private filesystem attached to them shouldn't be restricted
+    // either.
?>

This explanation doesn't make any sense. This type of logic should already be handled by the default access control on files.

xjm’s picture

As I recall, this was related to SA-CORE-2011-001.

johnv’s picture

The patch is over a year old. And now that I have doubled my Drupal-experience :-) , I am not sure if granting such a permission is the best idea. The file is now public for everyone....
Anyway, the behaviour for Term image is different from that of Node images.

chris.leversuch’s picture

I just picked up the issue from the novice queue. Is the issue still valid or should it just be closed?

johnv’s picture

@chris, I didn't realize you just popped in, thanks for the new patch.
The issue is still valid (at least for me - I would expect more people having this problem), but i doubt this is the best solution.
I have also edited the original post for more clarity.

johnv’s picture

Issue summary:View changes

Better explanation of the case.

xjm’s picture

Status:Needs review» Needs work

I agree that blithely returning TRUE is not the correct solution.

The steps to reproduce in the summary make it sound like you need views to reproduce this issue, but it is reproducible without views.

  1. Log in as user 1.
  2. Enable a private files directory.
  3. Create a vocabulary.
  4. Add an image field to the vocabulary that uses the private files directory.
  5. Add a term to the vocabulary and upload an image for the term.
  6. Visit the term page.

Expected result

The image is visible on the term page.

Actual result

The image returns a 403.

xjm’s picture

Issue summary:View changes

better explanation.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes

Correction.

ZenDoodles’s picture

Assigned:Unassigned» ZenDoodles

Zgear will write tests. :)

xjm’s picture

Assigned:ZenDoodles» Unassigned
edb’s picture

Status:Needs work» Needs review
StatusFileSize
new566 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,249 pass(es).
[ View ]

I think the only access control we can use for this that makes sense is field_access. Patch is attached, thoughts?

nyirocsaba’s picture

StatusFileSize
new533 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,572 pass(es).
[ View ]

Looks like the entity_type isn't a string, it's the entity object instead.

ryanissamson’s picture

Assigned:Unassigned» ryanissamson
ryanissamson’s picture

StatusFileSize
new552 bytes
FAILED: [[SimpleTest]]: [MySQL] 49,375 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Rerolling patch

working on tests.

Status:Needs review» Needs work

The last submitted patch, 1327224-access_denied_taxonomy_term-19.patch, failed testing.

ryanissamson’s picture

Actually, while dags and I were working on the tests, we realized that the patch actually doesn't work anymore.

Before and after applying the patch I get the same results. I get a 403 when trying to view the term's image.

Good news is the test is pretty much done when this gets fixed again! :)

Berdir’s picture

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1554,3 +1554,12 @@ function taxonomy_library_info() {
+function taxonomy_file_download_access($field, $entity_type, $entity) {
+  if ($entity_type->entityType() == 'taxonomy_term') {
+    return field_access('view', $field, $entity_type, $entity);
+  }

This hook has changed, the arguments passed in are now $field, $entity, $file, see file.api.php for an example.

It's then $entity->entityType() to get the entity type and that's also what you need to pass to field_access().

Berdir’s picture

Also, as mentioned in IRC, we have an entity access system now, so file.module by default could check $entity->access('view') once the issues mentioned in #1696660-117: Add an entity access API for single entity access are fixed. That would remove the need for custom hook implementations for every single entity type (user might have this problem too?)

Tests are still useful, though.

ryanissamson’s picture

Assigned:ryanissamson» Unassigned

Alright, keeping this as needs work and unassigning myself. Dags and I will keep working on the tests.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new3.4 KB
FAILED: [[SimpleTest]]: [MySQL] 49,273 pass(es), 18 fail(s), and 0 exception(s).
[ View ]

This patch shows what I've been talking about. It should fail for the moment, but pass once the mentioned issues are fixed.

We can simplify it even further maybe, not sure if we still need two hooks there.

Status:Needs review» Needs work

The last submitted patch, file-download-access-default-1327224-25.patch, failed testing.

ryanissamson’s picture

StatusFileSize
new1.9 KB
FAILED: [[SimpleTest]]: [MySQL] 49,093 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Alright, so I am not finished but wanted to post this before I forget. We've started the test but are def not finished. Here's what we've got so far.

Berdir’s picture

+++ b/core/modules/file/file.moduleundefined
@@ -666,9 +666,9 @@ function file_file_download($uri, $field_type = 'file') {
+        $grants = array('system' => $entity->access('vew'));

typo...

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] 49,076 pass(es), 7 fail(s), and 2 exception(s).
[ View ]

Ok, I think we can actually go considerably further here.

Given that we have a unified entity and field access system, I do not see a use case for even needing these hooks at all anymore.

If someone has access to the entity and access to the field, he has access to the file.

Also merged in the test code from #27.

Status:Needs review» Needs work

The last submitted patch, file-download-access-1327224-29.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests, -Novice
StatusFileSize
new3.12 KB
FAILED: [[SimpleTest]]: [MySQL] 53,462 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new9.26 KB
FAILED: [[SimpleTest]]: [MySQL] 54,459 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
new3.47 KB

Ok, here we go.

Worked on the test that now actually passes because the term access issue went in. Removing the needs test tag and the novice tag as writing that test wasn't that easy :)

Attaching the full patch, test only patch and interdiff.

This *should* pass once node access issue is in, we'll see.

johnv’s picture

Wow, a patch that adds functionality by removing code!

Status:Needs review» Needs work

The last submitted patch, file-download-access-1327224-31-test-only.patch, failed testing.

Berdir’s picture

Something else to consider is that file entities should have an access controller too. And I think we should move the entity/field check into an viewAccess() implementation for the file entity. What I'm not yet sure is how the field type argument will be handled.

@davereid also mentioned a download access check for files instead of view but I'm not sure if I see a need to differentiate between view and download. Comes back to my main argument in this issue, that we don't need a separate download check if we can check view for the entity/field combination.

Berdir’s picture

Status:Needs work» Needs review
Berdir’s picture

Berdir’s picture

Issue tags:+Entity Field API

Tagging.

Status:Needs review» Needs work

The last submitted patch, file-download-access-1327224-31-test-only.patch, failed testing.

Berdir’s picture

One test fail remaining because comments are not yet converted: #1862754: Implement entity access API for comments

Berdir’s picture

Issue tags:+Entity Access

Tagging.

Berdir’s picture

Status:Needs work» Needs review
Berdir’s picture

StatusFileSize
new783 bytes
new10.02 KB
FAILED: [[SimpleTest]]: [MySQL] 54,516 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Fixed the node related test, taking over the fix from #1947880: Replace node_access() by $entity->access().

Comments are mean, not sure why I didn't notice that before.

viewAccess() in CommentAccessController just does user_access('access comments'), the removed comment hook here does a lot more. it checks comment status/admin permission, which viewAccess() should absolutely do as well.

The problematic part is that it also checks access to the node, which makes sense as well, but would be a considerable overhead to do in viewAccess(). So.. I'm not sure what to do. Have a special operation for download that defaults to view but entities can override it? That will actually mean that we're back at allowing entities to easily customize download access but making it optional. Which sounds like win-win to me :)

Status:Needs review» Needs work

The last submitted patch, file-download-access-1327224-42.patch, failed testing.

andypost’s picture

This needs update the summary. Not sure that mix of field-level settings and entity_view is good default without alter.
Why the file entity access not involved here?

+++ b/core/modules/file/file.api.phpundefined
@@ -199,58 +199,3 @@ function hook_file_delete(Drupal\file\File $file) {
-function hook_file_download_access($field, Drupal\Core\Entity\EntityInterface $entity, Drupal\file\File $file) {
...
-function hook_file_download_access_alter(&$grants, $context) {

Seems very questionable!

+++ b/core/modules/file/file.moduleundefined
@@ -665,38 +665,15 @@ function file_file_download($uri, $field_type = 'file') {
-        if (!field_access('view', $field, $entity_type, $entity)) {
+        if (!$entity->access('view') || !field_access('view', $field, $entity_type, $entity)) {

But what's about file entity access?

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new3.21 KB
new12.96 KB
FAILED: [[SimpleTest]]: [MySQL] 55,398 pass(es), 4 fail(s), and 10 exception(s).
[ View ]

file entity access is not involved because this is the file entity access *implementation*, it's just not yet within an access controller. For example because there's contextual information (field_type) that is not accessible from within an access controller without doing some ugly tricks.

Here's a re-roll that uses the new flexibility of the access controllers to introduce a new download operation that entities can implement if they want to. I do have to change the default implementation to make NULL a valid return value, so that I can separate between "not implemented" and "denied.

@fubhy, what do you think about that change?

Dave Reid’s picture

Having entities aside from files that have a download operation, just doesn't smell right. Having the comment view access operation not check node access by default doesn't smell right.

Berdir’s picture

StatusFileSize
new11.07 KB
FAILED: [[SimpleTest]]: [MySQL] 55,745 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

This is an implementation that improved the comment view access operation to the point where all this stuff isn't necessary.

If that's not a performance issue then that's fine with me as well. It shouldn't be, because for the typical case, we are only checking comments of a single node so we are hitting the static caches for both the entity load and access checks. A different case would be comments across multiple nodes on a single page.

Status:Needs review» Needs work

The last submitted patch, file-download-access-1327224-47.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new2.11 KB
new11.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file-download-access-1327224-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Refactored the taxonomy image test to avoid #1957888: Exception when a field is empty on programmatic creation.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -Entity Access, -Entity Field API

The last submitted patch, file-download-access-1327224-49.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+Needs issue summary update, +Entity Access, +Entity Field API
andypost’s picture

Suppose this change out of scope, let's fix it after #731724: Convert comment settings into a field to make them work with CMI and non-node entities

+++ b/core/modules/comment/lib/Drupal/comment/CommentAccessController.phpundefined
@@ -24,7 +24,10 @@ class CommentAccessController extends EntityAccessController {
   protected function checkAccess(EntityInterface $entity, $operation, $langcode, User $account) {
     switch ($operation) {
       case 'view':
-        return user_access('access comments', $account);
+        if (user_access('access comments') && $entity->status->value == COMMENT_PUBLISHED || user_access('administer comments')) {
+          return $entity->nid->entity->access('view');
+        }
+        return FALSE;

It's not clear why user with rights to admin comments should depend on access to node?

Berdir’s picture

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +Entity Access, +Entity Field API

The last submitted patch, file-download-access-1327224-49.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new3.79 KB
FAILED: [[SimpleTest]]: [MySQL] 58,512 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Yeah, that didn't work out :(

Here's a limited patch that just adds the test and a hook implementation for terms. Opened #2078473: Use entity access API for checking access to private files to look into improving the API.

Status:Needs review» Needs work

The last submitted patch, taxonomy-term-file-download-access-1327224-55.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:-Needs issue summary update
StatusFileSize
new3.51 KB
FAILED: [[SimpleTest]]: [MySQL] 58,076 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new4.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,538 pass(es).
[ View ]

Ok, fixed the test, was written a long time ago ;)

Status:Needs review» Needs work

The last submitted patch, taxonomy-term-file-download-access-1327224-57-test-only.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

Failed/Passed as expected, test-only behavior is really broken :(

andypost’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Just a small nitpicks

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyImageTest.php
    @@ -0,0 +1,97 @@
    +    $edit['files[field_test_' . Language::LANGCODE_NOT_SPECIFIED . '_0]'] = drupal_realpath($image->uri);

    no langcode now

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyImageTest.php
    @@ -0,0 +1,97 @@
    +    $terms = entity_load_multiple_by_properties('taxonomy_term', array('name' => $edit ['name']));

    space after $edit

Berdir’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.1 KB
new4.38 KB
FAILED: [[SimpleTest]]: [MySQL] 59,233 pass(es), 1 fail(s), and 3 exception(s).
[ View ]

Thanks, fixed.

Status:Needs review» Needs work

The last submitted patch, taxonomy-term-file-download-access-1327224-61.patch, failed testing.

andypost’s picture

A new set of nitpicks

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyImageTest.php
    @@ -0,0 +1,97 @@
    +    $this->vocabulary = $this->createVocabulary();

    Is this need @var?

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyImageTest.php
    @@ -0,0 +1,97 @@
    +    $this->drupalPost('admin/structure/taxonomy/manage/' . $this->vocabulary->vid  . '/add', $edit, t('Save'));

    drupalPostForm() & vocabulary->id()

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
new4.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,283 pass(es).
[ View ]

Yeah, the drupalPost isn't really a nitpick but other than that, I hope you're running out of nitpicks soon :)

andypost’s picture

Status:Needs review» Reviewed & tested by the community

It's now ;) no more nitpicks

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 820205f and pushed to 8.x. Thanks!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyImageTest.php
@@ -0,0 +1,104 @@
+use Drupal\Core\Language\Language;
+

Unnecessary use statement removed during commit.

johnv’s picture

As OP I'd like to thank you for your work!

Status:Fixed» Closed (fixed)
Issue tags:-Entity Access, -Entity Field API

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.

Berdir’s picture

Version:8.x-dev» 7.x-dev
Issue summary:View changes
Status:Closed (fixed)» Patch (to be ported)
Issue tags:+Needs backport to 7.x

I think we forgot to backport this.

bnjmnm’s picture

Assigned:Unassigned» bnjmnm

Working on 7.x backport

David_Rothstein’s picture

Issue tags:-

This issue is closely related to #2305017: Regression: Files or images attached to certain core and non-core entities are lost when the entity is edited and saved, which is a critical regression. Looking for any feedback there on how to fix it...