fix: add record files restriction option to enable cache-control header#3324
fix: add record files restriction option to enable cache-control header#3324tmorrell wants to merge 1 commit intoinveniosoftware:masterfrom
Conversation
| emitter(current_app, record=file_item._record, obj=obj, via_api=False) | ||
|
|
||
| return file_item.send_file(as_attachment=download) | ||
| return file_item.send_file(as_attachment=download, restricted=False) |
There was a problem hiding this comment.
shouldt that be assigned to the record resrtiction?
There was a problem hiding this comment.
🤔 By setting restricted=False, we are making send_stream to set the Cache-Control also to public here.
According to MDM
You should add the private directive for user-personalized content, especially for responses received after login and for sessions managed via cookies.
Maybe a good alternative would be to set restricted to the actual record/file restrictions, and then update send_stream/redirect_stream to set Cache-Control to public or private, respectively.
What do y'all think?
132fa2f to
359a29b
Compare
359a29b to
209843f
Compare
egabancho
left a comment
There was a problem hiding this comment.
About the changes required on Invenio-Files-Rest and Invenio-S3, we should check inveniosoftware/invenio-files-rest#283. There was some discussion there about using no-cache instead of private.
❤️ Thank you for your contribution!
Description
InvenioRDM doesn't currently add cache-control headers to file links, which can result in weird behavior if a cache is put in front of the repo. This is because the cache-control headers are only added by default for non-restricted files (which makes sense) https://github.com/inveniosoftware/invenio-files-rest/blob/616dc34c38006433653211feb5a9e523b0d399e7/invenio_files_rest/helpers.py#L182. But because InvenioRDM doesn't pass this option, the cache-control headers are never added.
This should be reviewed carefully to make sure it doesn't have security implications.
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: