Problem/Motivation

In #1452100: Private file download returns access denied, when file attached to revision other than current the solution involved limiting to an arbitrary amount the number of entities that Drupal searched to determine if a user is allowed to access a file to avoid performance impacts.

An alternative to limiting search locations would be to have hook_file_download include context regarding the source of the source display (such as entity:type:id) to allow validation checks to narrow searching.

Steps to reproduce

N/A.

Proposed resolution

Generate links with context for display.
Modify FileDownloadController to accept/require context
Modify hook_file_download to accept a source context

Remaining tasks

User interface changes

Introduced terminology

API changes

hook_file_download and FileDownloadController will accept/require context regarding file access approval

Data model changes

Release notes snippet

Comments

cmlara created an issue.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I'm not exactly sure how you'd expect this context to be provided, by whom and to whom? you mean that the URl would include ?context=node:17, and then we only check node 17?

That seems quite challenging, there would be several other places that would need to pass this along, we'd need to pass it to the resolver (although you could argue that we can skip the resolver completely with this or have a new method for it), we'd need to pass it through to the access checks, which has no concept for this either. And only _some_ cases could actually be able to provide that context, link field formatters on an entity, but not something like a list of files.

cmlara’s picture

cmlara
he/him
commented

I'm not exactly sure how you'd expect this context to be provided, by whom and to whom? you mean that the URl would include ?context=node:17

Indeed, as either a query parameter or named argument of the route.

and then we only check node 17?

For the entity permission checker, yes that would be exactly the case, you only need to look at Node 17 and see if it (or its revisions) grant access, if not the context is invalid and you either neutral response (if you want to allow a chance to lookup more) or fail response (treat as a malformed request for trying to fake context approval).

Shared media on a node may be "entity:media:id+entity:node:id" in that if the user has access to the media item or node either could grant access.
(note: these are more description examples, an exact context format can be determined later)

Some hook implementations (especially for un-managed files) may not care on the additional context, an example could be some modules may check export module may do "is this in ./modulename-exports/, and is the user allowed to access exports" they don't need the context of where the link was displayed.

The concept is a middle ground between "Load every single location access may be granted" and "maintain a mapping table for every file on where it is used" (Avoiding a DoS may require signing the context). The other middle ground sometimes used in other systems is 'generate a time limited per user signature" (image styles uses an unlimited time signature for example)

pass it through to the access checks

I do not believe this would need anything special go past the hook_file_download (in the context of the revision check) as the loading to validate would be something along the lines of "does user have access to entity X at revision Y and is file Z attached to the revision" which entity revision access already cover loading the entity itself.

And only _some_ cases could actually be able to provide that context, link field formatters on an entity, but not something like a list of files.

I believe for core this would be most of the cases would it not? I do agree raw random list of files this is indeed one of the areas this idea would struggle. A views list of all managed files and the IMCE contrib module being mos impact. At that point you have to go to either parse every entity, or have already maintained a mapping table of file usage to avoid a false 'access denied' error.

Slipping this in is certainly not the smallest architecture change for core, and I do believe it needs to be questioned on its design to be sure I"m not missing anything, however if implemented it should it allows majority of access checks to shortcut full table lookup trying to find where a user was informed they may try and load the file.