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

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.

The last submitted patch, 124: 1452100-124.patch, failed testing. View results →

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

The last submitted patch, 124: 1452100-124.patch, failed testing. View results →

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 📌 [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service Postponed and #2081513: 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 🐛 $entity->isDefaultTranslation() behaves incorrectly when changing default translation, causing file/image field usage to be set to zero, causing files to be deleted Fixed 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 📌 [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service Postponed . 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 📌 [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service Postponed 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 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 for access checking pass 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.

acbramley → changed the visibility of the branch 1452100-private-file-download to hidden.

acbramley → changed the visibility of the branch 11.x to hidden.

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