Access denied to taxonomy term image

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.

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.

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

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

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

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.

better explanation.

Updated issue summary.

Updated issue summary.

Correction.

Zgear will write tests. :)

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

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

Rerolling patch

working on tests.

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

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

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.

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

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.

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.

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

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.

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.

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.

#31: file-download-access-1327224-31.patch queued for re-testing.

#31: file-download-access-1327224-31-test-only.patch queued for re-testing.

Tagging.

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

Tagging.

#31: file-download-access-1327224-31.patch queued for re-testing.

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

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?

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?

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.

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.

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

#49: file-download-access-1327224-49.patch queued for re-testing.

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?

#49: file-download-access-1327224-49.patch queued for re-testing.

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.

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

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

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

Thanks, fixed.

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()

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

It's now ;) no more nitpicks

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.

I think we forgot to backport this.

Working on 7.x backport

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

Those needing a quick fix on Drupal 7 may implement this hook in a custom module:

/**
 * Implements hook_file_download_access_alter().
 */
function MYMODULE_file_download_access_alter(&$grants, $file_item, $entity_type, $entity) {
  // Fix bug: Access denied to taxonomy term image
  // https://www.drupal.org/node/1327224
  if ($entity_type == 'taxonomy_term') {
    $grants['MYMODULE'] = user_access('access content');
  }
}

Sorry, double submit.

Here the patch from D7 with tests (tests were easily ported from D8 thankfully)!

The access control should be handled by 'access content' which is what already controls access to viewing a term. This is also what D8 does.

patch applied successfully after the patch i am able to access image sharing screenshot ...

I think the patch in #79 looks good. Reuploading the same patch with test only version to verify that failures are present. Anyway if the testbot does not show any problems, this should be RTBC (has test, backport, verified with manual testing).

Patch #79 verified, switching to RTBC.

Patch looks good and excellent to have the backported tests.

Is this likely to break existing functionality or just fix the OP's bug?

Trying to decide if it needs a Change Record; I think that would only be necessary if we think sites might be relying on the existing functionality (or lack thereof) in some way.

I think we can add a Change record, because this patch is changing the default behavior from access denied to everyone to allow access to everyone (with access content permission / access to taxonomy terms by default), though it is a bug. Maybe some sites were using this broken behavior, but I do not see any reasonable use for it.

But according to the description of hook_file_download_access() (see: https://api.drupal.org/api/drupal/modules%21file%21file.api.php/function/hook_file_download_access/7.x):

Return value
TRUE is access should be allowed by this entity or FALSE if denied. Note that denial may be overridden by another entity controller, making this grant permissive rather than restrictive.

So we are basically setting TRUE for all (with access content permission / access to taxonomy terms by default), so if there are some contrib/custom modules setting this more precisely now (using this concrete hook), we will override them (but hopefully most of them are using the alter version of this hook correctly). These modules would need to be updated to use the hook_file_download_access_alter(). Taking into account this possibility I think we should add the Change record.

Here is a draft of the proposed change record: https://www.drupal.org/node/3307802

Thanks everyone!