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:
- Install standard drupal profile
- Configure private files path in settings.php
- Enable content_moderation module, enable "Article" node type in default editorial workflow.
- Add a new file upload field to "Article" node type, with "Private files" selected as the upload destination.
- Create a new "Article" with no file attached, set the moderation state to "Published".
- 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.
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#150 | 1452100-150.patch | 15.99 KB | acbramley |
#149 | 1452100-nr-bot.txt | 86 bytes | needs-review-queue-bot |
#147 | 1452100-147.patch | 15.98 KB | smustgrave |
#140 | 1452100-140-interdiff.txt | 7.93 KB | Berdir |
#140 | 1452100-140.patch | 15.99 KB | Berdir |
|
Issue fork drupal-1452100
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:
Comments
Comment #1
jlongbottom CreditAttribution: jlongbottom commentedComment #2
marcingy CreditAttribution: marcingy commentedThis is not critical
Comment #3
BerdirYou 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.
Comment #4
renat CreditAttribution: renat commented@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..
Comment #5
jlongbottom CreditAttribution: jlongbottom commentedI 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.
Comment #6
jlongbottom CreditAttribution: jlongbottom commentedThe 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.
Comment #7
jlongbottom CreditAttribution: jlongbottom commentedOk 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.
Comment #8
jlongbottom CreditAttribution: jlongbottom commentedApparently 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.
Comment #9
Berdir@renat: I suggest you create your own issue as your's is a) a different issue b) actually a bug in core :)
Comment #11
shadysamir CreditAttribution: shadysamir commentedExcuse 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.
Comment #12
pedrogk CreditAttribution: pedrogk commentedI 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?
Comment #13
franzI'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.
Comment #14
therainmakor CreditAttribution: therainmakor commentedI 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:
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).
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.
Comment #15
therainmakor CreditAttribution: therainmakor commented#14: 1452100-14-private-access-denied.patch queued for testing.
Comment #16
Berdir7.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
Comment #17
therainmakor CreditAttribution: therainmakor commentedJust 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
Comment #18
BerdirThere 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.
Comment #19
ayalon CreditAttribution: ayalon commented#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.
Comment #20
gapple8.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 additionalentity_load()
is needed.I've also added an updated patch for 7.x that uses the
$conditions
parameter toentity_load()
if needed to load a specific revision for an entity, instead of assuming the parameters to$entity_info['load hook']
.Comment #22
gappleOkay, 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
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
Comment #23
gapplePatch with tests
Comment #25
gapple#23: drupal-1452100-23-private-download-revisions.patch queued for re-testing.
Comment #27
gappleUpdated patch for latest 8.x-dev
Comment #29
gapple#27: drupal-1452100-27-private-download-revisions.patch queued for re-testing.
Comment #31
rooby CreditAttribution: rooby commentedI 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.
Comment #31.0
rooby CreditAttribution: rooby commentedAdded more detail to problem description.
Comment #32
Paul B CreditAttribution: Paul B commentedCopied the issue description from #1453138: [D7] Private file download returns access denied, when file attached to node revision other than current
Comment #34
AnybodyAny 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)?
Comment #35
AnybodyIs 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. :(
Comment #37
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedRerolled patch #27.
Comment #38
benjy CreditAttribution: benjy at PreviousNext commentedThe 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.
Comment #39
Anybody@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.
Comment #40
benjy CreditAttribution: benjy at PreviousNext commentedThe 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.
Comment #42
AnybodyRelated Drupal 7 issue #1453138: [D7] Private file download returns access denied, when file attached to node revision other than current
Comment #43
arlinsandbulte CreditAttribution: arlinsandbulte as a volunteer commentedI'm going to raise the priority of this to critical based on:
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.
Comment #44
cilefen CreditAttribution: cilefen commentedThank 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.
Comment #45
ckaotikI 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.
Comment #46
arlinsandbulte CreditAttribution: arlinsandbulte as a volunteer commentedBy 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.
Comment #47
rooby CreditAttribution: rooby commentedCertain 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.
Comment #48
AnybodyWell 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.
Comment #49
ckaotikThis 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
).Comment #51
Ram Prawesh Kumar CreditAttribution: Ram Prawesh Kumar commentedHi 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.
Comment #52
cilefen CreditAttribution: cilefen commentedHi @ram_prawesh_kumar:
I think you may have forgotten to `git push` because there is no code in the module you posted.
Comment #53
Ram Prawesh Kumar CreditAttribution: Ram Prawesh Kumar commentedHi @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.
Comment #55
DanieleN CreditAttribution: DanieleN commentedPatch #37 works also for 8.5.2
Comment #56
eiriksmHere 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.
Comment #58
eiriksmAdd a check for method to exist for finding revisions.
Comment #60
rakshith.thotada CreditAttribution: rakshith.thotada as a volunteer commentedUploading 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
Comment #61
rakshith.thotada CreditAttribution: rakshith.thotada as a volunteer commentedComment #63
rakshith.thotada CreditAttribution: rakshith.thotada as a volunteer commentedUploading 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
Comment #64
rakshith.thotada CreditAttribution: rakshith.thotada as a volunteer commentedComment #65
rakshith.thotada CreditAttribution: rakshith.thotada as a volunteer commentedUpdated patch to consider only the current Revision being loaded
Comment #66
rakshith.thotada CreditAttribution: rakshith.thotada as a volunteer commentedWorking and tested patch on 8.5.X
Comment #67
rakshith.thotada CreditAttribution: rakshith.thotada as a volunteer commentedComment #69
cristiroma CreditAttribution: cristiroma commentedIMHO 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.Comment #70
cristiroma CreditAttribution: cristiroma commentedComment #71
cristiroma CreditAttribution: cristiroma commentedConsider 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:
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
Comment #73
cristiroma CreditAttribution: cristiroma commentedPrevious patch was against 8.6.3, fixed patch to apply against 8.6.x-dev.
Comment #75
jonathanshawYes. And that is more basic than the content moderation module.
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.
Comment #76
leontin CreditAttribution: leontin commentedI had a look on @cristiroma's patch and changed a little.
Comment #77
jonathanshaw@leontin it really helps if you provide an interdiff
Comment #78
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI 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.Comment #80
drenton CreditAttribution: drenton commentedChanged $entity_ids to array_keys($entity_ids).
Comment #81
drenton CreditAttribution: drenton commentedFix 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).
Comment #82
maximpodorov CreditAttribution: maximpodorov commentedIt's time to get it in the core. Very annoying bug.
Comment #85
shubhangi1995Comment #86
arlinsandbulte CreditAttribution: arlinsandbulte as a volunteer commented@shubhangi1995, thanks for volunteering to push this forward.
Any progress?
Comment #87
shubhangi1995Hi @arlinsandbulte
Please find the patch.
Details:
core: 8.7.7
Comment #88
jonathanshawComment #89
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedThis 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 actualFileAccessControlHandler
uses the FIELD_LOAD_REVISION flag when getting references.Setting to NW since the last patch failed to apply.
Comment #90
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedHere's a reroll of #81 against 8.9.x
Comment #91
shubhangi1995Comment #92
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFixed 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.
Comment #93
lahoosascoots CreditAttribution: lahoosascoots commentedAny update on getting this into core?
Comment #94
sonnyktConfirming patch #92 fixes the issue.
Comment #95
catchLooks 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?
Comment #97
dpiAttached 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.Comment #98
ThandavaPati CreditAttribution: ThandavaPati commentedExisting 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.
Comment #99
ThandavaPati CreditAttribution: ThandavaPati commentedComment #100
dpi@ThandavaPati please always attach an interdiff when making changes.
Comment #102
biblos CreditAttribution: biblos as a volunteer commentedConfirm this issue with D7.
Comment #103
raman.b CreditAttribution: raman.b at OpenSense Labs commentedRequired another re-roll for 9.1.x
Comment #105
Kasey_MK CreditAttribution: Kasey_MK commentedPatch in #103 works for us on Drupal 8.9.7 (with Group 1.3.0). Thank you!
Comment #107
kim.pepperComment #108
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedReroll for 9.3.
Comment #109
richard.thomas CreditAttribution: richard.thomas commentedSo 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.
Comment #111
richard.thomas CreditAttribution: richard.thomas commentedDecided 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.
Comment #112
bbralaSince 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.
Comment #114
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedReroll of #112
Comment #115
VladimirAusThanks! Works well.
Comment #116
alexpottI think this can be written in a way that has less potential to go wrong in the future. I.e.
There's no else ... less code and if there's a change to
Then we only have one place to update.
Comment #118
Amit@94 CreditAttribution: Amit@94 commentedupdated with custom role
Comment #119
Amit@94 CreditAttribution: Amit@94 as a volunteer commentedupdated with custom role
Comment #120
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented@Amit@94 would you be able to attach an interdiff?
Comment #121
joey91133 CreditAttribution: joey91133 as a volunteer commentedI make a change from #114 #116.
permission not only allow by "view all revisions", also can allow by "view {$bundle} permission".
Comment #123
fenstratHere'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.
Comment #124
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedAdded fix for #95 by only loading all revisions if the current revision returns no references.
Comment #125
smustgrave CreditAttribution: smustgrave at Mobomo commentedConfirmed 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.
Comment #127
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedMan these random fails are rampant lately, back to RTBC.
Comment #129
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedAnother random fail.
Comment #130
larowlanAt 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
Comment #131
fenstratGood 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')
.Comment #132
BerdirYou 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.
Comment #133
fenstrat@Berdir hmm interesting, I tried that so:
However then this assert fails (locally, when it was passing before):
So that looks like it contradicts what you've said?
Noticed a line wrap issue in the test, attached fixed that.
Comment #134
mxr576I 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.
Comment #135
BerdirHm. 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.
Comment #136
BerdirCould 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 :)
Comment #137
BerdirSee 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.
Comment #138
fenstratJust 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.
Comment #139
BerdirRe #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.
Comment #140
BerdirThis 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.
Comment #141
smustgrave CreditAttribution: smustgrave at Mobomo commentedHate 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?
Comment #143
bryanmanalo CreditAttribution: bryanmanalo at IOM - UN Migration commentedJust 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.
Comment #144
bryanmanalo CreditAttribution: bryanmanalo at IOM - UN Migration commentedFound 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.
Comment #145
richard.thomas CreditAttribution: richard.thomas commentedUpdated 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.
Comment #146
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedIS 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.
Comment #147
smustgrave CreditAttribution: smustgrave at Mobomo commentedHere's a copy of 140 with the fuzz fixed.
Comment #148
AnybodyLeaving 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!
Comment #149
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #150
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedAnother reroll
Comment #151
smustgrave CreditAttribution: smustgrave at Mobomo commentedReroll looks good. Think this is ready for committer bucket.
Comment #152
BerdirFrom #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.
Comment #153
kim.pepperAgree 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.
Comment #154
BoobaaThe 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.Comment #155
mxr576Based on Drupal core's built-in logic, that should not happen, at least not on nodes... #fixme
Source: (https://github.com/drupal/core/blob/10.1.4/modules/node/src/NodeAccessCo...)
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...