Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Drupal should not check to see if a user has permission to view published nodes when they are uploading a managed file. That file could be on a term, a user, or anywhere else in the Drupal system - having nothing at all to do with viewing content that has been published.
In the file module, the AJAX callbacks look like this.
file.ajax_progress:
path: '/file/progress/{key}'
defaults:
_controller: '\Drupal\file\Controller\FileWidgetAjaxController::progress'
requirements:
_permission: 'access content'
Steps to reproduce
Proposed resolution
To create a new permission for this specific use case
Remaining tasks
Issue Summary update
Decide on a solution, see #2
possible Reroll
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #38 | 1960784-38.patch | 2.53 KB | _utsavsharma |
| #38 | interdiff_36-38.txt | 1.16 KB | _utsavsharma |
| #36 | 1960784-36.patch | 2.53 KB | ameymudras |
| #16 | interdiff.txt | 1.1 KB | swentel |
| #16 | 1960784-16.patch | 3.86 KB | swentel |
Comments
Comment #1
neRok CreditAttribution: neRok commentedI manually applied the patch to 7.22 (dont have a git setup atm), and everything seems to work as expected, so the code looks good.
However, is it a good idea to have no access checks for those menu paths at all? Maybe a new permission needs adding, "Can upload files to fields" or something of the sort.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedThis would need to go in Drupal 8 first.
And yeah, I think it needs a bit more review - just looks a bit scary to remove permissions entirely from this callback (even if the current permission being used there doesn't make sense)! I think the callbacks themselves might do enough checking of their own that it's safe, but would be good to see more reviews.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedOh, I missed that part of the comment. Well, I'm going by the fact that file/ajax and file/progress both exist in Drupal 8 and have the same permission check, and (currently at least) appear to be in use.
Comment #4
slashrsm CreditAttribution: slashrsm commentedI agree with @David_Rothstein here. Removing all access checks from a callback is generally not a good idea. However, file_ajax_upload() do check for form_id and fails if not present or invalid. file_ajax_progress() on the other hand performs no special checks and I am not sure if it is safe to open it to the entire world.
Comment #5
garphy CreditAttribution: garphy commentedThis patch adds a new 'upload files' permission.
Added a quick hook_update_N() to 'copy' the 'access content' permission.
NB: This is a D7 patch, for now.
Comment #7
swentel CreditAttribution: swentel commentedD8 version - will probably need some more 'upload files' permission in tests I guess.
Comment #9
swentel CreditAttribution: swentel commentedDifferent approach for D8
Comment #11
swentel CreditAttribution: swentel commentedRight, duh.
Comment #11.0
swentel CreditAttribution: swentel commentedbug
Comment #12
slashrsm CreditAttribution: slashrsm commentedThis keeps the same configuration for sites that are upgraded from D7 to D8. It seems a bit strange to give all users permissions to upload files, but I understand the rationale behind that.
Do we want to add this permission to authenticated users only for freshly installed sites?
Comment #13
slashrsm CreditAttribution: slashrsm commentedComment #14
swentel CreditAttribution: swentel commentedRerolling - the update is not needed anymore.
AFAICT, there's no anonymous form in standard with a file upload, so I guess default installation is currently fine ?
Comment #16
swentel CreditAttribution: swentel commentedComment #17
dawehnerCan we somehow ensure that the file widgets aren't shown in that case?
On top you might be able to upload files via REST. this would have to check access as well.
Comment #18
swentel CreditAttribution: swentel commentedInteresting remark re: the widget - haven't even thought about that.
Comment #19
jhedstromI think this is at needs work based on #17.
Comment #20
BerdirSee also #2310307: File needs CRUD permissions to make REST work on entity/file/{id} .
Comment #21
BerdirComment #23
dawehnerAnother related issue: #1927648: Allow creation of file entities from binary data via REST requests
Comment #35
quietone CreditAttribution: quietone at PreviousNext commentedThis was a bugsmash triage target. It was discussed with mstrelan and lendude. They concluded that this is still an issue, that it needs a discussion of the proposed fix because the solution in #2 'sounds bad', an issue summary update and a possible reroll.
Comment #36
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedSubmitting a patch for 9.5.x with a custom permission
Comment #37
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedComment #38
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedTried to fix CCF in #36.
Comment #40
brad.bulger CreditAttribution: brad.bulger commentedre #14
The default user registration form has a user picture field. If anonymous users don't have access content, they can't register.
That's about more than the Ajax callback - see FileAccessControlHandler::checkAccess - but seems relevant here.
#2988255: user_picture upload fails during registration without published content access