Background

This is the follow-up issue of Private file download returns access denied, as was suggested by @Berdir here.

Once upon a time a patch was committed to D7 core to make private files accessible, when there are nodes with revisions on your site (see "Private file download returns access denied" issue mentioned above). To achieve this, they changed some code at modules/file/file.module, namely file_get_file_references function parameter to FIELD_LOAD_CURRENT from FIELD_LOAD_REVISION.

But FIELD_LOAD_REVISION parameter was there for a good reason, with FIELD_LOAD_CURRENT we are not able to open files attached to all entity revisions except current. It's fatal in case you're trying to build, say, intranet with library for documents.

Problem

When only a non-default revision is referencing an uploaded private file, users of the site will get an access denied response when trying to view the file, even if they have access to view the revision where the file was uploaded.

There are several use-cases that could be affected by this issue with non-default revisions, one specific case is editorial workflow using content_moderation, if a user attaches a file to a new non-default revision of a published node, that document is no longer accessible to the user who uploaded it (or anyone else) to review. Steps to reproduce:

  1. Install standard drupal profile
  2. Configure private files path in settings.php
  3. Enable content_moderation module, enable "Article" node type in default editorial workflow.
  4. Add a new file upload field to "Article" node type, with "Private files" selected as the upload destination.
  5. Create a new "Article" with no file attached, set the moderation state to "Published".
  6. Edit the node you just created, attach a file to the file upload field and set the new moderation state to "Draft". Save the new revision.
  7. PROBLEM: Click the link to the document you attached to the new non-default draft revision, you will get an access denied error, even as UID 1.
  8. Change the moderation state to published using the form at the top of the page, once the new revision is published and default, the link to the document will start working.

How the proposed solution works:

  • In file_get_file_references(), after retrieving the file usage mapping, the default revision for each referencing entity is checked first, if it contains the file reference that revision is returned as the referencing entity.
  • If the default revision does not contain a reference to the file, a second query is run to find the most-recent non-default revision of that entity that references the file. If found, it is returned as the referencing entity.
  • FileAccessControlHandler::checkAccess() then checks if the referencing entity is a non-default revision, and if so, adds an additional "view revision" access check against the non-default revision referencing the file.

Once in place step 7 above would work and the user would be able to view/download the attached file as long as they have access to the non-default revision it is attached to.

CommentFileSizeAuthor
#150 1452100-150.patch15.99 KBacbramley
#149 1452100-nr-bot.txt86 bytesneeds-review-queue-bot
#147 1452100-147.patch15.98 KBsmustgrave
#140 1452100-140-interdiff.txt7.93 KBBerdir
#140 1452100-140.patch15.99 KBBerdir
#133 interdiff-1452100-131-133.txt1.05 KBfenstrat
#133 1452100-133.patch14.6 KBfenstrat
#131 interdiff-1452100-124-131.txt1.13 KBfenstrat
#131 1452100-131.patch14.6 KBfenstrat
#124 interdiff-1452100-123-124.txt750 bytesacbramley
#124 1452100-124.patch14.48 KBacbramley
#123 interdiff-1452100-114-123.txt2.16 KBfenstrat
#123 1452100-123.patch14.44 KBfenstrat
#121 1452100-121.patch14.27 KBjoey91133
#119 1452100.patch15.24 KBAmit@94
#118 1452100.diff15.24 KBAmit@94
#114 1452100-114.patch14.95 KBacbramley
#112 1452100-112.patch15.03 KBbbrala
#108 interdiff.txt5.74 KBKapilV
#108 1452100-108.patch12.3 KBKapilV
#103 1452100-103.patch12.31 KBraman.b
#98 file-access-1452100-98.patch12.38 KBThandavaPati
#97 1452100-file-access-97.patch12.33 KBdpi
#97 1452100-file-access-97-rc1.patch12.31 KBdpi
#92 interdiff-1452100-90-92.txt7.96 KBacbramley
#92 1452100-92.patch12.31 KBacbramley
#90 1452100-90.patch12.33 KBacbramley
#87 to_allow_access_to_private_files_which_are_realted_to_revisions.patch30.99 KBshubhangi1995
#81 interdiff_80-81.txt751 bytesdrenton
#81 1452100-81.patch11.98 KBdrenton
#80 interdiff_78-80.txt526 bytesdrenton
#80 1452100-80.patch12.29 KBdrenton
#78 interdiff-78.txt14.34 KBamateescu
#78 1452100-78.patch12.28 KBamateescu
#76 1452100-file-access-D8-76.patch7.5 KBleontin
#73 1452100-file-access-D8-73.patch11.7 KBcristiroma
#71 1452100-file-access-D8-70.patch11.84 KBcristiroma
#67 1452100-file-access-67.patch8.5 KBrakshith.thotada
#66 1452100-file-access-66.patch8.5 KBrakshith.thotada
#65 1452100-file-access-65.patch8.49 KBrakshith.thotada
#63 1452100-file-access-63.patch8.38 KBrakshith.thotada
#60 1452100-file-access-60.patch8.4 KBrakshith.thotada
#58 1452100-58.patch4.93 KBeiriksm
#56 1452100-56.patch4.69 KBeiriksm
#40 cm-files.patch8.95 KBbenjy
#37 test-only-fail-1452100-37.patch2.15 KBaerozeppelin
#37 1452100-37.patch3.44 KBaerozeppelin
#27 drupal-1452100-27-private-download-revisions.patch5.43 KBgapple
#23 drupal-1452100-23-private-download-revisions.patch5.43 KBgapple
#20 drupal-1452100-private-download-revisions.patch663 bytesgapple
#20 drupal-1452100-private-download-revisions-D7.patch1.84 KBgapple
#14 1452100-14-private-access-denied.patch1.84 KBtherainmakor
#7 ckeditor_setting.png6.77 KBjlongbottom
#6 fubar_priv_file.png18.19 KBjlongbottom

Issue fork drupal-1452100

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jlongbottom’s picture

Title: Private file download returns access denied » Private file download returns access denied message
Issue tags: +Security
marcingy’s picture

Priority: Critical » Normal

This is not critical

Berdir’s picture

Priority: Normal » Major
Status: Active » Postponed (maintainer needs more info)

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.

renat’s picture

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

jlongbottom’s picture

I am not using revisions. When I add a new "Document" content type (as defined in my original post), the File attached is inaccessible as soon as its uploaded.

jlongbottom’s picture

FileSize
18.19 KB

The link (circled in red) gives me the access denied message before I even save my node. And its also inaccessible after the node is saved. Again, this is the case whether revisions are being used or not.

jlongbottom’s picture

FileSize
6.77 KB

Ok I got it working after I edited the Global Profile for the CKEditor module found at: admin/config/content/ckeditor

I am not sure why this module effects the direct viewing of private files found else where on the site (as opposed to viewing them through CKEditor itself while adding content) but thats what did it.

jlongbottom’s picture

Status: Postponed (maintainer needs more info) » Fixed

Apparently this issue has been resolved in the latest development release of CKEditor: http://drupal.org/node/999344

Upgrading to it solved the problem for me.

Berdir’s picture

Priority: Major » Normal

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

Status: Fixed » Closed (fixed)

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

shadysamir’s picture

Status: Closed (fixed) » Active

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.

pedrogk’s picture

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?

franz’s picture

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.

therainmakor’s picture

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.

therainmakor’s picture

Status: Active » Needs review

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

Berdir’s picture

Title: Private file download returns access denied message » Private file download returns access denied message if the file is attached to an old revision.
Version: 7.12 » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs backport to D7

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

therainmakor’s picture

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

Berdir’s picture

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.

ayalon’s picture

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

gapple’s picture

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

Status: Needs review » Needs work

The last submitted patch, drupal-1452100-private-download-revisions-D7.patch, failed testing.

gapple’s picture

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

gapple’s picture

Status: Needs work » Needs review
FileSize
5.43 KB

Patch with tests

Status: Needs review » Needs work
Issue tags: -Needs tests, -Security, -Needs backport to D7

The last submitted patch, drupal-1452100-23-private-download-revisions.patch, failed testing.

gapple’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Security, +Needs backport to D7

The last submitted patch, drupal-1452100-23-private-download-revisions.patch, failed testing.

gapple’s picture

Status: Needs work » Needs review
FileSize
5.43 KB

Updated patch for latest 8.x-dev

Status: Needs review » Needs work
Issue tags: -Needs tests, -Security, -Needs backport to D7

The last submitted patch, drupal-1452100-27-private-download-revisions.patch, failed testing.

gapple’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Security, +Needs backport to D7

The last submitted patch, drupal-1452100-27-private-download-revisions.patch, failed testing.

rooby’s picture

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.

rooby’s picture

Issue summary: View changes

Added more detail to problem description.

Paul B’s picture

Title: Private file download returns access denied message if the file is attached to an old revision. » Private file download returns access denied, when file attached to node revision other than current
Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anybody’s picture

Anybody’s picture

Priority: Normal » Major

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

aerozeppelin’s picture

Rerolled patch #27.

benjy’s picture

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.

Anybody’s picture

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

benjy’s picture

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

arlinsandbulte’s picture

Priority: Major » Critical
Status: Needs review » Needs work

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.

cilefen’s picture

Priority: Critical » Major
Issue tags: +Triaged core major

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.

ckaotik’s picture

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.

arlinsandbulte’s picture

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.

rooby’s picture

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.

Anybody’s picture

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.

ckaotik’s picture

Issue summary: View changes

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Ram Prawesh Kumar’s picture

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.

cilefen’s picture

Hi @ram_prawesh_kumar:

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

Ram Prawesh Kumar’s picture

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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DanieleN’s picture

Patch #37 works also for 8.5.2

eiriksm’s picture

Status: Needs work » Needs review
FileSize
4.69 KB

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.

Status: Needs review » Needs work

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

eiriksm’s picture

Status: Needs work » Needs review
FileSize
4.93 KB

Add a check for method to exist for finding revisions.

Status: Needs review » Needs work

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

rakshith.thotada’s picture

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

rakshith.thotada’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 60: 1452100-file-access-60.patch, failed testing. View results

rakshith.thotada’s picture

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

rakshith.thotada’s picture

Status: Needs work » Needs review
rakshith.thotada’s picture

Updated patch to consider only the current Revision being loaded

rakshith.thotada’s picture

rakshith.thotada’s picture

FileSize
8.5 KB

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cristiroma’s picture

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.

cristiroma’s picture

Status: Needs review » Needs work
cristiroma’s picture

Status: Needs work » Needs review
FileSize
11.84 KB

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

Status: Needs review » Needs work

The last submitted patch, 71: 1452100-file-access-D8-70.patch, failed testing. View results

cristiroma’s picture

Status: Needs work » Needs review
FileSize
11.7 KB

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

Status: Needs review » Needs work

The last submitted patch, 73: 1452100-file-access-D8-73.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jonathanshaw’s picture

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.

leontin’s picture

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

jonathanshaw’s picture

@leontin it really helps if you provide an interdiff

amateescu’s picture

Status: Needs work » Needs review
FileSize
12.28 KB
14.34 KB

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.

Status: Needs review » Needs work

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

drenton’s picture

Changed $entity_ids to array_keys($entity_ids).

drenton’s picture

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

maximpodorov’s picture

Status: Needs work » Needs review

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

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

Status: Needs review » Needs work

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

shubhangi1995’s picture

Assigned: Unassigned » shubhangi1995
arlinsandbulte’s picture

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

shubhangi1995’s picture

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

jonathanshaw’s picture

Assigned: shubhangi1995 » Unassigned
Status: Needs work » Needs review
acbramley’s picture

Title: Private file download returns access denied, when file attached to node revision other than current » Private file download returns access denied, when file attached to revision other than current
Status: Needs review » Needs work

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.

acbramley’s picture

Version: 8.6.x-dev » 8.9.x-dev
Status: Needs work » Needs review
FileSize
12.33 KB

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

shubhangi1995’s picture

Assigned: Unassigned » shubhangi1995
acbramley’s picture

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.

lahoosascoots’s picture

Any update on getting this into core?

sonnykt’s picture

Status: Needs review » Reviewed & tested by the community

Confirming patch #92 fixes the issue.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

dpi’s picture

ThandavaPati’s picture

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’s picture

dpi’s picture

@ThandavaPati please always attach an interdiff when making changes.

Status: Needs review » Needs work

The last submitted patch, 98: file-access-1452100-98.patch, failed testing. View results

biblos’s picture

Confirm this issue with D7.

raman.b’s picture

Status: Needs work » Needs review
FileSize
12.31 KB

Required another re-roll for 9.1.x

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Kasey_MK’s picture

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

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kim.pepper’s picture

Issue tags: +#pnx-sprint
KapilV’s picture

Reroll for 9.3.

richard.thomas’s picture

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.

richard.thomas’s picture

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.

bbrala’s picture

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.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

VladimirAus’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Works well.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Amit@94’s picture

FileSize
15.24 KB

updated with custom role

Amit@94’s picture

FileSize
15.24 KB

updated with custom role

acbramley’s picture

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

joey91133’s picture

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

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

fenstrat’s picture

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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

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

acbramley’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

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

acbramley’s picture

Status: Needs work » Reviewed & tested by the community

Another random fail.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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

fenstrat’s picture

Status: Needs work » Needs review
FileSize
14.6 KB
1.13 KB

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

Berdir’s picture

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.

fenstrat’s picture

FileSize
14.6 KB
1.05 KB

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

mxr576’s picture

Issue tags: +Performance
+        $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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs review » Needs work

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

Berdir’s picture

See also #3035352: [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service 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.

fenstrat’s picture

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.

Berdir’s picture

Assigned: Unassigned » Berdir

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
15.99 KB
7.93 KB

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.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bryanmanalo’s picture

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.

bryanmanalo’s picture

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.

richard.thomas’s picture

Issue summary: View changes

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.

acbramley’s picture

Assigned: Berdir » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

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.

smustgrave’s picture

Anybody’s picture

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!

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
86 bytes

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.

acbramley’s picture

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

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

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

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.

kim.pepper’s picture

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.

Boobaa’s picture

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.

mxr576’s picture

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