- Issue created by @anaconda777
- š©šŖGermany jurgenhaas Gottmadingen
To download files, the
hook_file_download
needs to be implemented which then determines who has access to such files to download. This is something we should add to the eca_file submodule. - First commit to issue fork.
- š©šŖGermany jurgenhaas Gottmadingen
A couple of quick notes, although this was set to "Needs review" yet:
- In ECA 3.0 hooks are implemented with the new object-oriented technique, not as hooks in a module file any longer. Please do that here as well, and should we decide to backport the feature to ECA 2 later on, we will have to add the legacy hook handling in a separate MR.
- Please get the test to green before anyone will have to review the code.
- First commit to issue fork.
- šµš¹Portugal lolgm
I created the merge request MR !546 based on MR !535.
The main difference is that all logic related to handling downloads of public files has been removed, since that approach does not work ā access to public files is not managed by Drupal but rather by the web server directly.
In this case, the added event will only react to the download of private files, using
hook_file_download
. - š©šŖGermany jurgenhaas Gottmadingen
Thank you @lolgm, this is a great MR.
There is one thing that could simplify this a bit: the current user doesn't have to be handled by the hook or event. It is known to ECA models already and there is no reason to provide it again as a token. I suggest that this should be removed.
And then, the event needs to allow the "Set access" action plugins to set the access result decision, which should then be collected by the hook which returns that to the caller.
- šµš¹Portugal lolgm
Thank you for the review, @jurgenhaas.
The following changes have been applied:
use
statements were sorted alphabetically.- The logic related to the current user was removed from the event.
- Support for the
SetAccessResult
action has been implemented.
While testing, I encountered an issue related to point 3:
When
SetAccessResult
is set toForbidden
, any access to private files correctly returns a 403 response. However, if the configuration is changed toAllowed
orNeutral
, anonymous users still receive the 403 unless the cache is cleared.It seems that this behavior occurs because
hook_file_download
is not invoked when the 403 response is cached.Any suggestions on how to overcome this issue?
- š©šŖGermany jurgenhaas Gottmadingen
@lolgm it looks like in one commit you removed the current user code, but then there was another commit where all that was brought back again. Can you please verify that?
Also, the
hook_file_download
needs to be extended such that if the ECA model returnsAllowed
then the file download is controlled by this and we need to return an array with the appropriate headers so that the user can actually download that file. As we can't guess what the response header need to be, we probably need a separate action for that where the ECA model declares which headers should be returned.So, if the model allows access, it also requires a second action that defines the headers. As that's hard for users to understand, there should probably be a separate SetAccessResult action for file downloads which allows the selection of the access, and if allowed, then the extra field to define headers should become visible.
Maybe that will then also resolve the caching issue, I'm not sure. But we can check when that's implemented.
- šµš¹Portugal lolgm
@jurgenhaas I believe I may be misunderstanding something, but when implementing
\Drupal\eca\Event\AccessEventInterface
on\Drupal\eca_file\Event\FileDownload
, Iām required to implement thegetAccount()
method, which in turn forces me to declare aprotected AccountInterface $account
property ā since this interface extends\Drupal\eca\Event\AccountEventInterface
, which mandates that method.References to the interfaces:
- AccessEventInterface: https://git.drupalcode.org/project/eca/-/blob/3.0.x/src/Event/AccessEven...
- AccountEventInterface: https://git.drupalcode.org/project/eca/-/blob/3.0.x/src/Event/AccountEve...
So, if the model allows access, it also requires a second action that defines the headers. As that's hard for users to understand, there should probably be a separate SetAccessResult action for file downloads which allows the selection of the access, and if allowed, then the extra field to define headers should become visible.
In that case, should this new action be something like SetFileDownloadAccessResult, extending \Drupal\eca_access\Plugin\Action\SetAccessResult?
- š©šŖGermany jurgenhaas Gottmadingen
Hmm, good point. I didn't realize that
AccessEventInterface
extendsAccountEventInterface
and I don't see why this was done that way. I could only think of it in a way that this event could be used to check access not only for the current user but also for any other. That would not be required for file download, but maybe that's not even true, either. Like, e.g. if you send an email that should contain a link to a private download but only if the recipient (i.e. not the current user) has access permission. So, it's probably correct to keep the account part in.On the second part, i.e. creating an
SetFileDownloadAccessResult
action the way you describe it, seems to be the way to go. I wonder if it should actually be part of the eca_access module, so that this only can be used, when that module is also enabled. - šµš¹Portugal lolgm
@jurgenhaas I agree that the
SetFileDownloadAccessResult
action should be part of theeca_access
module, as its primary responsibility is access control.Moreover, the
FileDownload
event is still useful even without this action ā for example, it can be used to log download events for statistical purposes.I've already included a proposed implementation of the
SetFileDownloadAccessResult
action inMR !546
.