Private file download returns access denied, when file attached to revision other than current

This is not critical

You need to provide more details, e.g. are you using revisions? What kind of private files, how are they attached to content?

Private files in general are working just fine for me, so this doesn't seem to be a general problem and isn't critical.

@Berdir, more detailed report can be found here:
http://drupal.org/node/992674#comment-5622346
Yes, it is true only for documents, attached to node's revisions other than current. This is because file_get_file_references function parameter was changed to FIELD_LOAD_CURRENT from FIELD_LOAD_REVISION as a solution for problem mentioned at this topic..

@renat: I suggest you create your own issue as your's is a) a different issue b) actually a bug in core :)

Excuse me but I am having the same problem with WYSIWYG + TinyMCE + Media uploaded images. Thumbnail does not show after upload, in editor, or in node view. Trying to access the file itself I get drupal Access Denied (while being user 1).

The original file itself gets uploaded properly into location on server which is outside the drupal installation.

I am also getting "Access denied" when trying to download private files, even if I try as admin.

Any ideas on how I can trace it in order to provide you valuable info that you can use?

I'm getting a similar error but with imported content through Feeds, no Wysiwyg related. The database entries have an uri in the format "private:///filename" which basically is causing the error, since when loading it searches for "private://filename". I don't know why it was saved like that yet. Editing the node and saving again doesn't fix.

I have come across this issue just recently and I believe I found a solution with the attached patch. The issue stems from file_file_download() using entity_load() to load the entity and check access on it and entiy_load() only allows accessing an entity by an id (nid in the case of nodes). file_file_download() uses file_get_file_references() to get all the references of the file in question. It returns an array in the form of this example:

$references = array(
  ['field_file'] => array(
    ['node'] => array(
      [29628] => stdClass Object(
        [nid] => 11940,
        [vid] => 29628,
        [type] => 'page',
      ),
      [37561] => stdClass Object(
        [nid] => 11940,
        [vid] => 37561,
        [type] => 'page',
      )
    )
  )
);

As you can see, the array indexes of $references['field_file']['node'] are equal the node's vid. This means that that in line 5 of the code below, $id = $node->vid, which means the node will not load (unless there is a nid that equals $id in the database and that's not good).

foreach ($references as $field_name => $field_references) {
    foreach ($field_references as $entity_type => $type_references) {
      foreach ($type_references as $id => $reference) {
        // Try to load $entity and $field.
        $entity = entity_load($entity_type, array($id));

This patch loads the entity's info from entity_get_info() in order to call it's own loading function, stored in $entity_info['load hook']. It also checks whether or not the entity supports revisions in order to send the correct number of parameters. I'm not sure if this is the route to go since it assumes that all entitys' *_load() function's parameters are either ($id, $revision_id) or ($id).

I've tested this patch with file fields on nodes and on users (users do not have revisions) and it works.

#14: 1452100-14-private-access-denied.patch queued for testing.

7.x core still loads the current revision, which means the code is below is correct. You can't hack core and then report a bug that is caused by your change :)

However, it is probably correct that it should load the revision.

Re-titling and moving to 8.x, it will need to be fixed there first before it can be backported.

Notes:
- 7.x has no generic entity API to load a revision, the code there that does something with load hook is incorrect and just works because the the load hook name matches the load function in case of nodes. This probably needs a node specific solution in 7.x
- 8.x can use entity_revision_load()
- This needs tests

Just some clarification if you do not mind.

I didn't report a bug caused by my change, the patch was submitted to help fix the issue others were reporting and I recently found myself.

The patch changes from loading the current revision to loading all revisions, which is the entire cause of the problem in my opinion.

Also, do you mind elaborating on your first note please? I thought all entities were supposed to have a hook_load() function? Even entity_get_info() provides the default [load hook] as $name . '_load' which can be altered by a module's hook_entity_info_alter() if it need be (bad practice if the module does not define their own load function).

I'm not sure how to proceed with making a fix for 8.x since it doesn't seem to be loading any entities at all.

Thank you

There is no such thing as a hook_load() function. hook load in entity info is the name of the hook (hook_${hook_load}() that is invoked when entities are loaded.

Your code assumes that this is the name of the entity_load() wrapper and that the second argument is the revision id. Those are assumptions that can not be made, because it is not required to create a function named like that and with those arguments. commerce_order_load(), despite orders being revisionable, is such an example. In 8.x and in the backported entity.module version for it, http://api.drupal.org/api/drupal/core%21includes%21entity.inc/function/e... is the function that should be used to load a revision.

Should be as easy as porting your patch to 8.x to get startet (8.x also loads entities, not sure what you were saying), and then add some tests for file attachments for old revisions.

#14 works, thanks a lot! I had private file system an the user was not able so see attachment images of an old revision he created. The patch solved this problem.

8.x seems like a simple fix, as the $revisions array contains the whole entity, instead of just an object with nid/vid/type, so no additional entity_load() is needed.

I've also added an updated patch for 7.x that uses the $conditions parameter to entity_load() if needed to load a specific revision for an entity, instead of assuming the parameters to $entity_info['load hook'].

Okay, maybe not so simple...

The field access permissions test fails because file_get_file_references() skips over checking which fields actually contain the requested file, since it has to assume that the reference could have occurred in an older revision of the node:

File.module : 1601

foreach ($file_fields[$entity_type][$bundle] as $field_name => $field_column) {
  $match = $match_entity_type;
  // If we didn't match yet then iterate over the field items to find
  // the referenced file. This will fail if the usage checked is in a
  // non-current revision because field items are from the current
  // revision.
  if (!$match && ($field_items = field_get_items($entity, $field_name))) {
    foreach ($field_items as $item) {
      if ($file->fid == $item[$field_column]) {
        $match = TRUE;
        break;
      }
    }
  }
  if ($match) {
    $references[$file->fid][$age][$field_name][$entity_type][$entity->id()] = $entity;
  }

As a result, all file fields on the node are indicated to have contained the file.
In file_file_download(), since the unrestricted field was indicated to have referenced the file, access is then incorrectly allowed even though access to the restricted field is denied.

I also found another issue which may be worth noting when addressing this one:
#1997716: file_get_file_references() doesn't load entities correctly

Patch with tests

#23: drupal-1452100-23-private-download-revisions.patch queued for re-testing.

Updated patch for latest 8.x-dev

#27: drupal-1452100-27-private-download-revisions.patch queued for re-testing.

I think this issue could do with a proper issue summary.

At minimum, what is the functionality now and what is the proposed new functionality.
For example, after the patch will anyone be able to download private files from any revision? If so I think that is bad news.
You should have to have a special permission to view a private file from an old revision.

Added more detail to problem description.

Copied the issue description from #1453138: [D7] Private file download returns access denied, when file attached to node revision other than current

Any plans for further progress here or for Drupal 7 (#1453138: [D7] Private file download returns access denied, when file attached to node revision other than current)?

Is there any core developer who could have a look at this and has the relevant knowledge? I'm setting up the priority because this breaks core functionality and doesn't seem to ever have worked for Drupal 7 so this absolutely needs a backport.

Thank you very much, I'm afraid I can't help here sadly. :(

Rerolled patch #27.

The latest patch doesn't work unless your current revision has a file attached to it. I worked around this using a custom version of file_get_file_references() and using loadRevision() instead.

@Benjy: thank you, would you be so kind to share your code? That way we might create a better patch and others might use your workaround. Furthermore you'll perhaps receive helpful feedback.

The approach is pretty much as described in http://drupal.stackexchange.com/questions/101269/access-denied-on-file-a...

I put the code in the content_moderation module but I'm not sure that's correct given revisions are natively supported in core but there isn't any other way to actually create a forward revision which is when I had the issue.

Note that this can present an access bypass I believe if you have access to the published entity, you can access all files in the revision history. That probably needs to check for the CM "access any unpublished entity" and view latest revision permissions provided by CM.

Related Drupal 7 issue #1453138: [D7] Private file download returns access denied, when file attached to node revision other than current

I'm going to raise the priority of this to critical based on:

1. Renders a site unusable and have no workaround. If you need to be able to view node revisions with private files, there is no workaround.
2. Exposes security vulnerabilities. Only 'workaround' for above is to use public files. And that can be a security vulnerability.

Moderators can triage and re-prioritize. Marcingy downgraded from critical to normal early on, but did not provide specific reasons.

Also setting to 'Needs Work', due to the reason benjy gave in #40: There is probably a better place to put this code than content_moderation, which can be disabled, re-introducing this bug.

Thank you, everyone, for the work on this issue.

@xjm, @alexpott, @effulgentsia, @lauriii, @catch and I discussed this issue at a recent meeting and the consensus was to downgrade this to major priority.

We read and discussed comment #43 by @arlinsandbulte. Point 1, that the issue "renders a site unusable and has no workaround" did not persuade. The issue certainly "renders one feature unusable with no workaround", that being private files in revisions. This is part of the definition of a major issue. Point 2 is not moot because using public files is not a workaround providing the same functionality.

I just stumbled across this issue and have to say: Isn't preventing access to old revisions a wanted behavior? I might have understood the issue incorrectly, please correct me if so, but my expectations as a user would be to only grant access to the currently visible content revision. Old revisions are old/outdated, and no longer considered published, as was explained in @benjy's comment (#40). Shouldn't this also apply to the attached files?

Or does the issue apply only when revisions are used as an archive functionality, instead of a moderation tool? That would make sense, I guess, to still grant access to files if the old revisions are publicly visible and/or published. Is this the case?

There's also #2887696: Access denied to published private file if original translation is unpublished which has covers similar but but related to translations. Maybe someone will find that issue useful.

Isn't preventing access to old revisions a wanted behavior? ... Or does the issue apply only when revisions are used as an archive functionality, instead of a moderation tool?

By that logic, revisions and revision system is not needed. Just eliminate that whole system from Drupal because only the current/latest revision should be available anyway.

Accessing old node revisions is possible now, depending on how permissions are configured.
But, accessing files attached to those nodes is NOT possible if using private files, even with permissions correctly configured.

'Normal' users probably would not get access to revisions.
But, admins & editors & site managers may want to see how a piece of content has evolved. In that case, they can get the permission to view revisions, but they cannot load old pictures or files to see what changed.

A simple Example:
Say you have a zoo website.
There is a Giraffe node with a picture of the giraffe.
One day, you notice the Giraffe node's picture has changed to a zebra.... someone edited the node and changed the picture.
Now, how can you figure out who and when that mistake was made? You can view the previous node revisions, but you cannot see the pictures. If there are lots of other edits to the node, it becomes impossible to investigate and place the blame.

Certain users need to be able to access files attached to old revisions but there definitely needs to also be a way to configure permissions so that certain users can't see old revision files.

Otherwise you get problems like search engines indexing outdated versions of files, which is potentially extremely bad depending on what is in those files.

Well things are not that complicated as discussed in the three last comments. The logic is quite simple:
A file is accessible for a user if:
a) It is (still) accessible in the published revision
or
b) The user may access the revision which the file is attached to

All other cases are then solved that way, for example if a file is deleted in a later revision, it may be accessed accordingly to the users revision access in all revisions between its uploads and its deletion.
So if a user has access to a revision he should also be able to access its files. If not, not.
For moderation and other cases modules should be able to hook into that logic of course.

I think there may already be a logic for that comparison because files are shown/hidden properly through revisioning when removing files in a certain revision. At least as far as I can remember.

So please let's now come back to a finding a solution in code.

So if a user has access to a revision he should also be able to access its files. If not, not.

This makes perfect sense and probably best describes the goal. I'll add it to the issue summary.

This seems to be analog to the file access issue on translations, only for revisions . In both cases, only the default/active instance of the entity is checked to determine access.

Changes could be made in one of two places, FileAccessControlHandler::checkAccess, so the logic is applied for general file access checks, or file_get_file_references which the access handler calls, too. The access handler only checks for the active instance via $referencing_entity->access('view', $account, TRUE)
I wonder which place should best be changed - when getting the relevant entities (file_get_file_references) or when checking those entities for access (FileAccessControlHandler::checkAccess).

Hi If you are facing this problem in D7 then you can use this module. I had also faced this problem but, did not get any solution without core modification, So I have created a module to resolve this issue, you can use this module for D7 only.

Hi @ram_prawesh_kumar:

I think you may have forgotten to `git push` because there is no code in the module you posted.

Hi @cilefen,
Thank for for your reminder, Yes I was forgot to push my module, I am new in drupal community and this is my first project. Now I have pushed my module files, please check if still any bugs found then please remind me.

Patch #37 works also for 8.5.2

Here is what i consider to be a cleaner approach.

Check the access in the file module. I mean, this bug can be reproduced without the content_moderation module enabled.

Would probably need some tests, but interested to see if this breaks something first.

Add a check for method to exist for finding revisions.

Uploading the new patch - considering the access check for files within revisions could be handled in content moderation module as suggested by

Comment #40 benjy

The provided patch was failing and I am creating the new patch here for the same

Uploading the new patch - considering the access check for files within revisions could be handled in content moderation module as suggested by

Comment #40 benjy

The provided patch was failing and I am creating the new patch here for the same

Updated patch to consider only the current Revision being loaded

Working and tested patch on 8.5.X

IMHO it has nothing to do with content_moderation. file_get_file_references is broken and we need to fix it. In our case new revisions are created importing data from Excel, while users must see the historical data of products with pictures. content_moderation is disabled.

Consider our case: We show a history of node revisions and wish the user to view files from each revision. According to this bug user cannot access older files which are not in the current revision.

Considerations:

a) Quote comment #48 which IMHO makes perfect sense:

So if a user has access to a revision he should also be able to access its files. If not, not.

Modified the code below so that user must have 'view all revision' permission to be able to view previous files.

b) I think patch from #67 is not addressing properly the issue. The issue still persists if I don't have the content_moderation enabled.

c) Attaching a fresh patch, I cannot do/too complex to provide an inter-diff with previous one since code are on different modules.

d) Not sure this is the proper or intended usage for the permission 'view all revisions'.

I attaching our attempt to fix this issue for Drupal 8 as described above. Here's a summary of the changes:

I. file.module - file_get_file_references

1. It uses the previously unused $age to distinguish whether the caller wants to check file references for the current (FIELD_LOAD_CURRENT) revision file or all revisions (FIELD_LOAD_REVISION - default)
2. If the caller wants to see if a file is attached to previous revisions the system checks all revisions and returns the first (newest) revision found instead of current revision (providing the file was removed from the current revision)

II. In FileAccessControlHandler.php added the following behavior:

- If the file requested is attached to an older revision but not to the current revision, the file can be downloaded only if the current user has permissions view all revisions granted to his account.

III. Attached a test

Previous patch was against 8.6.3, fixed patch to apply against 8.6.x-dev.

So if a user has access to a revision he should also be able to access its files. If not, not.

Yes. And that is more basic than the content moderation module.

If the file requested is attached to an older revision but not to the current revision, the file can be downloaded only if the current user has permissions view all revisions granted to his account.

That seems strangely clumsy. Does Drupal's revision API offer us no access control method to determine whether a user can access a particular revision of a particular entity? I couldn't find one with a brief search.

I had a look on @cristiroma's patch and changed a little.

@leontin it really helps if you provide an interdiff

I looked a bit at this and I think we can do better in file_get_file_references() and not kill performance even more than it is now. Also fixed some coding standards issues. The new test is failing for me, and after a quick read I'm not sure they are testing stuff properly.. but the problem might be from my changes because I didn't have time to dig too much into it.

Changed $entity_ids to array_keys($entity_ids).

Fix for entities that do not have revisions. Was getting the following error :

Drupal\Core\Entity\Query\QueryException: No revision table for ......, invalid query. in Drupal\Core\Entity\Query\Sql\Query->prepare() (line 91 of core/lib/Drupal/Core/Entity/Query/Sql/Query.php).

It's time to get it in the core. Very annoying bug.

@shubhangi1995, thanks for volunteering to push this forward.
Any progress?

Hi @arlinsandbulte
Please find the patch.
Details:
core: 8.7.7

This is not specific to nodes, the same behaviour occurs when attaching files to Media for example.

The interesting thing is that file_file_download returns early after checking if references loaded with the FIELD_LOAD_CURRENT flag are empty, whereas the actual FileAccessControlHandler uses the FIELD_LOAD_REVISION flag when getting references.

Setting to NW since the last patch failed to apply.

Here's a reroll of #81 against 8.9.x

Fixed tests by moving our whole test block to the very bottom of the test method. Before, it was in the middle of other assertions that relied on variables before and after our block.

Any update on getting this into core?

Confirming patch #92 fixes the issue.

+++ b/core/modules/file/file.module
@@ -704,7 +705,7 @@ function file_file_download($uri) {
 
   // Find out which (if any) fields of this type contain the file.
-  $references = file_get_file_references($file, NULL, EntityStorageInterface::FIELD_LOAD_CURRENT, NULL);
+  $references = file_get_file_references($file, NULL, EntityStorageInterface::FIELD_LOAD_REVISION, NULL);
 
   // Stop processing if there are no references in order to avoid returning

Looks like even when a file is referenced by a default revision, we'd still be going back and loading all revisions to check if any grant access. Could we instead only fallback to loading all revisions if nothing comes back from the current revision?

Attached rerolls after #3132964: assertResponse() does not actually support a $message parameter, so stop passing one and #3139218: Replace usages of AssertLegacyTrait::assertResponse(), which is deprecated.

1452100-file-access-97-rc1.patch is for users of Drupal 9.0.0-rc1 which only needs one of these. Ignore this file for future use.

Existing patch is working only if users have access to all revisions (node), so update patch with new permission "edit any media document" which is more specific to media documents.

@ThandavaPati please always attach an interdiff when making changes.

Confirm this issue with D7.

Required another re-roll for 9.1.x

Patch in #103 works for us on Drupal 8.9.7 (with Group 1.3.0). Thank you!

Reroll for 9.3.

So after some manual testing of the patch from #103 there seems to be an issue with file_get_file_references() when asking for revisions. Because the return array is keyed by field_name -> entity_type -> entity_id, if you have multiple revisions of one entity referencing the same file, only the latest revision is returned, and in the case where you have a published revision with a later draft revision (i.e. content_moderation), this causes the file to return access denied to public users after the draft revision is created.

To me, it seems the logical thing to do would be for file_get_file_references() to key the return array by revision ID rather than entity ID if the caller passed in $age = EntityStorageInterface::FIELD_LOAD_REVISION, however that would technically be changing the API. It is only used in two places in core though, and FileAccessControlHandler doesn't appear to rely on the ID key at all.

Decided to have a go at the issue fork process. I've committed the latest patch from #108, added an additional test for the issue I identified with multiple revisions of the same entity referencing the same file, and updated file_get_file_references() to key returned entities by revision ID if $age = EntityStorageInterface::FIELD_LOAD_REVISION and the entity type is revisionable.

Since a GitLab merge request is a moving target i'm adding a patch file of the current state for security reaons since we want to use it in composer-patches.

Reroll of #112

Thanks! Works well.

1. +++ b/core/modules/file/src/FileAccessControlHandler.php
   @@ -34,9 +35,25 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +              if ($referencing_entity instanceof RevisionableInterface && !$referencing_entity->isDefaultRevision()) {
   +                // @todo The 'view all revisions' permission is provided only
   +                //   by the node entity type. Use the generic permission name in
   +                //   https://www.drupal.org/project/drupal/issues/2809177.
   +                $entity_and_field_access = AccessResult::allowedIf($account->hasPermission('view all revisions'))
   +                  ->andIf($referencing_entity->access('view', $account, TRUE))
   +                  ->andIf($referencing_entity->$field_name->access('view', $account, TRUE));
   +
   +                if ($entity_and_field_access->isAllowed()) {
   +                  return $entity_and_field_access;
   +                }
   +              }
   +              else {
   +                $entity_and_field_access = $referencing_entity->access('view', $account, TRUE)
   +                  ->andIf($referencing_entity->$field_name->access('view', $account, TRUE));
   +
   +                if ($entity_and_field_access->isAllowed()) {
   +                  return $entity_and_field_access;
   +                }
   

   I think this can be written in a way that has less potential to go wrong in the future. I.e.

               foreach ($referencing_entities as $referencing_entity) {
                 $entity_and_field_access = $referencing_entity->access('view', $account, TRUE)
                   ->andIf($referencing_entity->$field_name->access('view', $account, TRUE));
                 if ($referencing_entity instanceof RevisionableInterface && !$referencing_entity->isDefaultRevision()) {
                   // @todo The 'view all revisions' permission is provided only
                   //   by the node entity type. Use the generic permission name in
                   //   https://www.drupal.org/project/drupal/issues/2809177.
                   $entity_and_field_access = $entity_and_field_access->andIf(AccessResult::allowedIf($account->hasPermission('view all revisions')));
                 }
                 if ($entity_and_field_access->isAllowed()) {
                   return $entity_and_field_access;
                 }
               }
   

   There's no else ... less code and if there's a change to

                 $entity_and_field_access = $referencing_entity->access('view', $account, TRUE)
                   ->andIf($referencing_entity->$field_name->access('view', $account, TRUE));
   

   Then we only have one place to update.

2. I'm not sure that @catch's point in #95 has been addressed.

updated with custom role

updated with custom role

@Amit@94 would you be able to attach an interdiff?

I make a change from #114 #116.
permission not only allow by "view all revisions", also can allow by "view {$bundle} permission".

Here's a reroll of #114.

Without interdiffs I couldn't easily decipher what had been done in #118, #119, or #121.

This also makes @alexpott's suggestion from #116 1.

Leaving as NW as I'm also not sure @catch's point in #95 has been addressed.

Added fix for #95 by only loading all revisions if the current revision returns no references.

Confirmed this issue on Drupal 10.1 with a standard install
Configured Article image field to upload to private folder
Followed the steps in the issue summary.
When viewing the previous revision the image was broken
Applying patch #124 fixed the issue.

Also seems like remaining points have been addressed.

Man these random fails are rampant lately, back to RTBC.

Another random fail.

+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -34,7 +35,14 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+                // @todo The 'view all revisions' permission is provided only
+                //   by the node entity type. Use the generic permission name in
+                //   https://www.drupal.org/project/drupal/issues/2809177.
+                $entity_and_field_access = $entity_and_field_access->andIf(AccessResult::allowedIf($account->hasPermission('view all revisions')));

At least media and block content in core now support both revisions and permissions to access revisions.

We introduced a generic access approach in #3043321: Use generic access API for node and media revision UI so we should be able to use $entity->access('view all revisions') OR $entity->access('view revision') now

So I think we can do this now rather than later

Good catch with #130. Attached updates that.

However $entity->access('view all revisions') still fails, as it seems that is only provided by node module? So I've kept it as $account->hasPermission('view all revisions') and left the @todo note. I have added the OR with $entity->access('view revision').

You don't need to check for all, you have a specific entity. \Drupal\node\NodeAccessControlHandler::checkAccess() then maps that access operation to the specific permissions, including all.

@Berdir hmm interesting, I tried that so:

diff --git a/core/modules/file/src/FileAccessControlHandler.php b/core/modules/file/src/FileAccessControlHandler.php
index 27b01384be..2b0585ca87 100644
--- a/core/modules/file/src/FileAccessControlHandler.php
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -38,11 +38,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
               $entity_and_field_access = $referencing_entity->access('view', $account, TRUE)
                 ->andIf($referencing_entity->$field_name->access('view', $account, TRUE));
               if ($referencing_entity instanceof RevisionableInterface && !$referencing_entity->isDefaultRevision()) {
-                // @todo The 'view all revisions' permission is provided only
-                //   by the node entity type. Use the generic permission name in
-                //   https://www.drupal.org/project/drupal/issues/2809177.
-                $revision_access = AccessResult::allowedIf($account->hasPermission('view all revisions'))
-                  ->orIf($entity->access('view revision', $account, TRUE));
+                $revision_access = $entity->access('view revision', $account, TRUE);
                 $entity_and_field_access = $entity_and_field_access->andIf($revision_access);
               }
               if ($entity_and_field_access->isAllowed()) {

However then this assert fails (locally, when it was passing before):

$this->assertTrue($access->isAllowed(), 'Confirmed that a file referenced in an old node revision is accessible if user has role view all revisions.');

So that looks like it contradicts what you've said?

Noticed a line wrap issue in the test, attached fixed that.

+        $entities = $storage->loadMultipleRevisions(array_keys($result));
+      }
+      else {
+        $entities = $storage->loadMultiple(array_keys($entity_ids));
+      }
+
+      /** @var \Drupal\Core\Entity\FieldableEntityInterface[] $entities */
       foreach ($entities as $entity) {

I believe potential performance issues that this change can cause on sites with a huge amount of content + revisions could be avoided here by only loading a chunk of entities/revisions (like max 20) at a time.

Hm. the bulk load is complicated indeed. There can be hundreds of revisions for a single entity, so this could be very expensive. load revisions also has no static or persistent cache.

However, loading in chunks only partially solves that. By loading in chunks, you limit the memory usage, but it makes it even slower. the entity_usage contrib module tracks revisions, core does not.

We also seem to check for revisions by default in \Drupal\file\FileAccessControlHandler::getFileReferences and we don't first check the default revision, so this will come at an immediate, possibly massive cost for existing sites.

I fear that's a deal-breaker for this. Without changing the file_usage storage, the only somewhat feasible way is IMHO to rewrite file_get_file_references() completely into an API that uses yield, with the assumption that you'll only proceed as far as you have to until you find something that grants access. Then we could try the default revisions first and only look into revisions if we have to.

Could use this in a project, so I'll try to help bring this forward.

That said, just to be certain, I did test the current patch in a project and confirmed the suspicion I had. In my case, it loaded about 10 or so revisions of my test media entities, but it could be hundreds. And the return then contained 3 past revisions of the same entity that had that specific file.

I also see that file_file_download() checks current revision first and only if that fails, it falls back to revisions. But \Drupal\file\FileAccessControlHandler::getFileReferences() does not, it always goes for revisions. At the very least we should keep those two in sync, load current first and then fall back to revisions. At least then the hit we have is only when the default revision doesn't contain it. But still the same problem, and it also doesn't account for some scenarios like having the file on two different entities, one default revision and one not and you don't have access to the one that has it on the default revision.

As written, an unlimited query + loadMultiple() is a nogo.

One question, did anyone here ever have a use case where users only had access to some entity revisions but not others, do we really need all revisions that ever had the file?

I have an idea that might work without having to completely rewrite everything. Essentially, we'd deprecate the age argument entirely, we first load default revisions, check those. If we don't find it in an entity, we have to assume that specific entity has it in a non-default revision. At this point, we know the file fields of this bundle. We can write an entity query on all revisions *that reference this file*, ordered by revision id, range (0, 1). Then we load that revision and add that to the list. For now, we'd still need to do that for any entity that doesn't have it, to account for the use case above with mixed access. A replacement API of this could then introduce some sort of stream/yield API that doesn't need to load & return all entities but just as many as necessary to find a match that has access. Thoughts?

I'll try to implement this later this week and will assign this to me then. If someone wants to give this a try in the meantime, be my guest :)

See also #3035352: [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service and #2081513: [pp-1] Deprecate FIELD_LOAD_* constants as related issues, the second would then basically be resolved by my idea as we no longer need those constants.

Just confirming that in our use case our media revisions have a couple dozen revisions at most. Most of them have far less. So probably explains why we haven't seen any performance issues here.

The approach of deprecating the age argument makes sense. Happy to test it out in our use case.

Re #133: It fails because you are checking the wrong entity. $entity is the file, you need to do $referencing_entity->access('view revision', $account, TRUE).

Looking into my my idea now.

This switches to view revision access and it implements my idea with the automatic fallback to old revisions. Tests are now passing, I had a fail initially because the static resetting in the test is done at the wrong point. Specifically, we reset the cache, then load the node, and then we update it. That mean that the static cache was not updated by that action. With the old implementation, the static caches were irrelevant anyway as they weren't used for loading revisions, but now it matters. My implement still found the file on the default revision as it loaded that first and had a static cache hit. Is there a reason the test uses file access directly and not just attempt to fetch the file with the current user? Then we don't need to worry about static caches. Either it should do everything in the test (then it would be a kernel test) or all in the browser, mixing i always tricky.

Needs more cleanup in those tests and in general. The $age argument is now unused, but I didn't remove/deprecate it yet. Also due to the extra $field_type arguments that is unused in core and fail to see the use case for. IMHO all 3 extra arguments should be deprecated, if someone really wants to limit to a specific field or type then they could implement that loop at the end or so.

It's worth nothing that this does make a few assumption that might not work in every case, also depending on how it is optimized
* We assume that access does not vary by revision. That if there are 10 revisions with the file, the user either has access to them all or none. I suppose in theory it is possible to implement revision access by something on the entity which might give you only access to some revisions.
* Right now I do a revision query for any entity that doesn't find the file on the default revision. We also don't know if there is any chance that a user will have access to revisions without querying for them and loading them. That means there is a performance regression compared to HEAD, even though less than previous patches. Only idea I had would be a hook or so that allows to opt-out/in of revision checks per entity type/user, node and media could then provide that with permission checks? As mentioned above a streaming/yield-based API could return default revisions of multiple entities first before falling back to revisions, but multiple entities referencing the file is already an edge case in todays world with media entities I think.

IMHO we can document what the method assumes/supports and if it doesn't work for someone, they can always implement their own hook_file_download()/access. It's clearly better than what's in HEAD.

Hate to do it but can the issue summary be updated to include the proposed solution? The steps to reproduce are written for D7. Same for D10?

Just a heads up that we experienced an issue surrounding this if you are using this patch with filefield_paths with replace option enabled on a field.

This is becaue filefield_paths replace will allow 2 different fids with the same uri. There will be an instance where the file reference retreived will be on a older revision.

This patch checks that if the entity is not the latest revision, it will require the 'view all revision' permission.

Found a work around on the above.

Enable 'view all revision' to the roles that need it. And use 'https://www.drupal.org/project/config_perms' contrib to block off access to 'entity.node.revision' and 'entity.node.version_history' . You can specificy other routes here to be able to pick which roles has access to which entity routes.

Updated issue summary and reproducing steps for D9/10 with content moderation.

I've based my description of the proposed solution on the patch in #140, hopefully I understood that correctly.

IS reads nicely now, thanks @richard.thomas!

Also unassigning as the work was done in #140

140 still applies to 11.x (with some fuzz) so triggering a test run against that.

Here's a copy of 140 with the fuzz fixed.

Leaving the reference on #2810355: $entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted here, if someone else runs into this similar issue and thinks it's this one. Also take a look over there.
I also tried #147 and it works as expected (but doesn't fix my bug).

Nice work everyone!

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Another reroll

Reroll looks good. Think this is ready for committer bucket.

From #140:

> Needs more cleanup in those tests and in general. The $age argument is now unused, but I didn't remove/deprecate it yet. Also due to the extra $field_type arguments that is unused in core and fail to see the use case for. IMHO all 3 extra arguments should be deprecated, if someone really wants to limit to a specific field or type then they could implement that loop at the end or so.

This is not ready to be RTBC.

We need to properly deprecate that or better all 3 arguments here, but that's really tricky because we can't change the default behavior of them and the default behavior of them is useless, you must set the field type to NULL or you don't get data on image fields for example. So we can't do deprecation messages if you call it with non-default values. I suppose we could do deprecation messages if you don't provide the exact kind of values that will then result in the new and only supported behavior?

One option is to merge this with #3035352: [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service. That would make deprecation easier, the new API won't have the arguments and any call to the old API triggers a deprecation message. However, it will result in patch that's twice as large and we get into the tricky static reset BC topic that has held up the other issue.

We also need a change record.

Agree that a new File Usage API would make deprecations here much easier. Lets see if we can get #3035352: [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service moving forward.

The idea of dropping the need of the way-too-generic view all revisions permission for having access to those files is a good direction. However, the patch in #150 still mentions this permission (tho only in the tests, so they might have became irrelevant/outdated by now), meaning this definitely Needs work.

Regarding FileAccessControlHandler::checkAccess(), I'm not sure the "view" operation is the one that should be used. Can we please consider the "view revision" operation instead, at least for the $referencing_entity->access() for revisionable entities? There might be cases when one does have "view" access to the entity, but does NOT have "view revision" access to the revision that has the file attached.

There might be cases when one does have "view" access to the entity, but does NOT have "view revision" access to the revision that has the file attached.

Based on Drupal core's built-in logic, that should not happen, at least not on nodes... #fixme

      // First check the access to the default revision and finally, if the
      // node passed in is not the default revision then check access to
      // that, too.
      $node_storage = $this->entityTypeManager->getStorage($node->getEntityTypeId());
      $access = $this->access($node_storage->load($node->id()), $entity_operation, $account, TRUE);
      if (!$node->isDefaultRevision()) {
        $access = $access->andIf($this->access($node, $entity_operation, $account, TRUE));
      }

Source: (https://github.com/drupal/core/blob/10.1.4/modules/node/src/NodeAccessCo...)

The idea of dropping the need of the way-too-generic view of all revisions permission for having access to those files is a good direction. However, the patch in #150 still mentions this permission (tho only in the tests, so they might have become irrelevant/outdated by now), meaning this definitely Needs work.

Indeed, the fix does not depend on that permission directly, it just set up the system in test for a passing access check based on how \Drupal\node\NodeAccessControlHandler::checkAccess() works as of today. I have also run a test case in which I only granted the "view ENTITY BUNDLE permission" (view article revisions in this context) for the test user and everything nicely passed. In the end, I decided that adding that to the latest patch would have no added value, but please let me know if it would and I upload a new patch with that.

This is how my change started...

    // Test that a file only referenced in an old revision is still accessible.
    foreach (['view all revisions', 'view article revisions'] as $permission) {

In the meantime, re-roll of the patch in 150.

Now that we have an MR with the content of patch #156 , let's do a clean up and continue with the MR.

Reroll of patch from #156 for D11.1

Per triage in #44 by @cilefen:

@xjm, @alexpott, @effulgentsia, @lauriii, @catch

Adding credits for the triage.

Reroll of patch in #160 for D11.2.

Patch in #156 applies to drupal/core:10.5.4 and helped me. I was upgrading from 10.2 which was using #150

Trying to push this forward with my suggestion as a combined solution with #3035352: [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service now that drupal_static() has an official replacement.

BC not fully in place yet, also for drupal_static_reset(). field/field type is still in the old function, $age doesn't do anything. There's an option that we could keep the old function in place exactly as it is in HEAD. What's a bugfix, what's a BC break?

Pipeline is likely going to fail, but still possibly in a state that can be reviewed.

Wow this is dense.

First of all great work!
I was able to reproduce this with the steps in the summary and confirmed this issue resolves that access.

Couple of trivial comments on the MR.

I read through the full issue and the new direction makes sense, I also took a high level look at the related issue.

I almost lean towards deprecating and keeping the old function, so people can decide when to stop using age?

Did I say this was dense?
There are literally 8 levels of foreach and if statements. At first glance it does feel necessary, but I wonder if there is any way we can make it a bit more readable.

I think for test coverage we probably want to test some of the language paths, I didn't see that in the tests.

I will definitely need to read through this a couple more times to fully understand it.

It is very dense. I did a first pass to "de-densify" and deduplicatae it, extracted two helper methods, not entirely happy yet with how the information is passed around, but I think it's already much better.

I did some more refactoring, I could do more, but I'm unsure if it really helps.

The main concern I have is still that this possibly creates a considerable performance issue in some extreme cases. It's already not great, but this makes it worse.

If you imagine an extreme scenario of 1000 entities that each reference a given file but only in an old revision, then what it used to do is load 1000 entities, already bad enough. But now we also run 1000 entity revision queries. And then we return those 1000 results and the caller looks at the first, says, ok the user has access to them and returns it.

What I think we should do I think is make this a generator. What use cases like file download access actually needs is a generator that yields results one by-one. Then we can loop over them and abort as soon as we have one result that the user has access to. But caching gets more complex with that. What we'd need is something like https://github.com/lunkkun/php-caching-generator, but then also the memory cache and invalidation of that? Not sure if we should for this case go back to protected properties and an API to clear it.

Of course if the user does _not_ have access to any, then there's no way around this being slow, but at least we can control the memory usage then.

But that's quite a bit of code and complexity and before I go down that path, I'd like some feedback on that idea.

This is actually much better! I think that performance concern is valid.

What happens now? The bug just immediately returns access denied?

Do you think the generator will be less performant in the normal case? In other words would we be sacrificing the normal path performance for an extreme edge case and if we would be, should we just provide a way to swap the logic out for a project that needs it?

Do you mean like this: https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co...

> Do you think the generator will be less performant in the normal case? In other words would we be sacrificing the normal path performance for an extreme edge case and if we would be, should we just provide a way to swap the logic out for a project that needs it?

The "normal" case being that access is granted (or not) based on an active reference in the default revision, then no, it is equally performant. We only fall back to checking revisions if there is a usage for node A but we don't find it, so we assume that it's in a past revision. Then we check one revision for that node and return that if we find it.

It would be less performant if you have both node A and B referencing the file. node A does have it in the default revision, but node B does not. Then for node B, we also fetch the revision, but the usage in node A is *probably* going to be sufficient for the access check. But we don't know that.

> Do you mean like this: https://git.drupalcode.org/project/drupal/-/blob/main/core/lib/Drupal/Co...

It's more complex than that. Just like ImplementationList, this is fully initialized with all referencing nodes (the part in \Drupal\Core\Extension\ModuleHandler::getHookImplementationList*). We'd want to start yielding as we go through it. But we also do not want to redo the work on repeated calls so we want to keep the static cache. That means the yielding logic would need to keep track of a partially-filled cache, first return these, and then start to fetch new ones if we run out of already-prepared values. Multiple calls is a real scenario because FileDownloadHook first calls getReferences() but it just wants to know if there's any references. And then FileAccessControlHandler calls it again and loops over them until it finds the first entity the user has access to. Optimally, FileDownloadHook would only need to go to the first yield (we'd need to change the logic for that or add a public method for it) and then FileAccessControlHandler might or might not grant access for that first node.

I think I'll do the value object instead of this massive array structure and then we'll re-evaluate from there.

* For hooks such an optimization is less useful. Typically, hooks are called for all modules. But if you do invoke($module, $hook) then there is a possible scenario to optimize that a would not prepare all the hook classes if just one is needed.

I pushed a first version of that with a FileReferenceUsage object and I also did convert it to a generator without complex optimizations. This would allow us to optimize the internals here without BC breaks on the return value.

I think this makes it easier to use, the caller just needs 1 foreach instead of three and the available information is documented.

There is one possible additional optimization option that we could consider for cases where we have dozens/hundreds+ of entities referencing a given file beside the yield optimization that would help with cases where a user has no access to any entity. We could, possibly only if there are more than N of a given entity type, run them through an entity query with a condition on the ID with access check on. If a site does use something like node grants or custom query access alters, it would allow us to filter out inaccessible entities. But currently this service does not do access checks, it's not cached by that either. An easier version of that would be to put the entity load part into a protected method so that someone who has extreme use cases could override just that to do something about it.

This is so much clearer.

One minor comment on the MR.

I think it's tricky enough that I definitely need to review it again and pull it down to test and tinker, but this is vastly improved.

Is there a clean way to get the edge case and see memory usage?

I did some testing, essentially based on this script, before running this, make sure that private files are set up and add a field_file field to the page node type on standard install profile (or adjust the script:

use Drupal\Core\File\FileSystemInterface;
use Drupal\file\FileRepositoryInterface;
use Drupal\node\Entity\Node;

echo \Drupal::service(FileSystemInterface::class)->realpath('private://the-file.txt') . "\n";

$file = \Drupal::service(FileRepositoryInterface::class)->writeData('super secret', 'private://the-file.txt');

echo $file->createFileUrl() . "\n";

for ($i = 0; $i < 1000; $i++) {
  $node = Node::create([
    'type' => 'page',
    'title' => 'file usage ' . $i,
    'status' => FALSE,
    'field_file' => $file->id(),
  ]);
  $node->save();

  if (TRUE) {
    $node->setNewRevision(TRUE);
    $node->set('field_file', NULL);
    $node->save();
  }

}

Then access that file either as anonymous user or a user with access to revisions and unpublished notes. You can also compare revisions vs not with the if TRUE flag there. Running it again will give you a new file with a new set of nodes.

I want to stress again that the this is an extreme scenario and very very unlikely. Core provides no way to reuse existing files on multiple entities, if you use private files directly on nodes then in almost all cases, you have one or a handful of usages.
Note that this is with tiny nodes and no node grants.

As always, actually profiling a scenario is always surprising and not what you'd expect. Production-like profiling of the worst-case scenario (all those nodes and revisions are not yet in cache ) is not trivial, but as an anonymous user, gave me this:

https://blackfire.io/profiles/962dac94-b0b1-48c7-9719-b52f77b0306f/graph

memory: 122MB
wall time: 2.8s

Of that, getReferences() is 70% and getReferencingRevision() is 60%, mostly because revision lookup happens one-by-one.

surprise: node access checks make up 25% of the rest (20% of that is hasPermission(), access policy support is really expensive). This is with assertions disabled. I think we can improve that. right now, for non-default revisions, we always check view revision *and* view, I think we only need to do one and we only need to do field access if access to the entity is granted.

With warm entity caches, it goes down to 1.5s and 88MB, getReferences() shrinking to <50%. For users *with* access, it will return on the first entity you have access to, so that part will drop to near-zero.

This scenario can not be compared with HEAD, because it will not find any referenced entities and access for everyone is denied.

I optimized the entity/field access logic now to only check either view or view revision. That reduces the cost of Node access from 25% to 15%.

IMHO, this feels acceptable in regards to performance and this is sufficiently pluggable now that someone who cares about perfomance in a scenario like this could optimize it by adding some assumptions, for example skip revision lookup for users that lack certain permissions, or enable access checks on the entity query to consider node grants, but that's not really feasible in core.

Did you forget to push something up?

This needs a rebase anyway because of the filter issue.

I had a chance to run the test, thank you for that script. I'm seeing much lower memory than you are with SPX, see the screenshot.

I tested logged in and out.
Logged in getReferencingRevision is called 1000 times, but logged out it doesn't show up.

I get access denied logged out, and the file when logged in.

I see the 1000 file usage, but the files are not attached to the actual nodes due to the revision in the test as expected.

Still need the trigger error I think for the deprecation and I want to go through it again, but I think it's very close.

Sounds like you tested with page caching enabled, so the access denied page was cached (which has cacheability bugs as a change to the nodes wouldn't result in invalidating the page, but that's out of scope for this issue), that's why anonymous is so fast.

> I see the 1000 file usage, but the files are not attached to the actual nodes due to the revision in the test as expected.

Not sure I get that. With the MR, the revisions should be returned and it should go through the loop.

Let me run it again without page caches on.

Ok running it again without page caches I see more expected results.

Anonymous 53.69MB Walltime 1.85s
Admin 51.36MB Walltime 1.42s

Edit: added a bunch of fairly minor suggestions we need to take care of and we need the CR to updated and probably the issue summary.

I think we should get a review from the file maintainers too.

I updated the issue summary now.

I also talked about this with alexpott at DevDays. His proposal is to restrict the fix to the first N revisions. Essentially, instead of eating the possible performance hit by fixing this bug, we only load the first N entity revisions per entity type, the others we ignore. that will fix the problem for 99.9% of the sites, and that one extremely special one would need to provide their own implementation that either does live with using more memory or provide their own optimized implementation that limits the entities early based on entity access/groups/...

I think that makes sense, do we just add a setting for that limit for the lower end of extreme implementations?

@alexpott explicitly suggested to not make it a setting but a constant, as I've already implemented. So that sites that have this case are encouraged to implement a better approach for them and not just adjust the limit.

Should we at least log if we hit the limit?

Possibly. It's kind of only an issue if you then also don't have access but should have, meaning, you have per-entity-revision access and the one you'd have access to is not in the first 20. We can't really log in only that case, we could log a warning once if you hit that case, not sure.

I think we just log if it is over the limit, otherwise how would a site owner figure out they need to update this?

I'm also not sure I agree we should add a barrier to increasing the limit. I think most people that will hit this will have higher memory and bumping it can test if it's an issue with memory usage.

To be honest this is already very complex and fixing a long standing bug so I think we could probably do a follow up to discuss both points. This is a huge improvement over the current situation.

I need to read through everything again before I would be ready to mark this though.

Are you able to rerun nightwatch and run test only? I can't run those.

> I think we just log if it is over the limit, otherwise how would a site owner figure out they need to update this?

They'd figure out it by getting unexpected access denieds. The alternative is that they will get a continuous stream of warnings in the logs that they can't fix but also nothing is actually broken. For this to really be an issue, two things need to be true at the same time. 20+ non-default-revision references to a file *and* per-entity access rules so that user does not have access to the first 20 revisions but would have access to later. I agree it's not great, but there's a fairly high chance that nobody will ever run into this problem. I can also raise to 50 or so, that would still be quite manageable I think.

@alexpott suggested this approach and on purpose making it "hard" in person at DevDays. I'm not 100% convinced myself, but I'm happy to stick with his recommendation.

Let's just create a follow up to discuss further.

I created #3587643: [pp-1] Should increasing REVISION_LOOKUP_LIMIT use just a Settings value

I think this is ready, I've been through each iteration many, many times.

This is not perfect, but it's a very complex issue and this is an extremely large improvement to status quo. I've created the follow up for one point I think bears more discussion, and we can open up any other optimizations later if we uncover them.

I don't have permission to run the test only job, but this is one of those issues that I really can't see how it would pass, but a maintainer with permission might want to run that to be sure.

Add a few small MR comments - this is looking nice.

At the request of @catch on Slack, I've changed the value object to store only the id, it's a bit awkward I think but avoids referencing entity objects and I think we'd need to store vast amounts of references to make an impact on memory usage. I agree it would be useful to explore having a proper LRU memory cache bin, but I think that can be done in an follow-up. The only change to the resolver would be updating the autowire to a different bin.

I think this is ready again then. The only remaining question is whether we need a follow up to explore the LRU cache, but as mentioned on the MR I don't think that is viable based on the discussion in slack https://drupal.slack.com/archives/C079NQPQUEN/p1777565082686269

I reviewed the requested changed in 200 and they look good to me!

The cr needs filling in.

Did a first pass of the CR, pretty complex stuff to explain, did my best.

The CR is focused on the API changes. It's important to point out that the default private file download bug around revisions just works and doesn't need any adjustments, the CR is only for the ~13 contrib projects that use the API directly, several of which I believe try to work around this or other bugs and most of those only have a handful of installs. With the exception of focal_point, but the code there is honestly completely bogus, it's checking entity-type-wide permissions that do not exist like that and that code doesn't do anything except for admin role users.