Add cors headers for file show method #1068
Conversation
|
@edsu this would add the CORs header to any request for a PDF (or any other file download for that matter) Perhaps would allow the current PDF.js based viewer to work if the user is authed? Still doesn't deal with any messaging if they are not authed of course (as that is a different set of work). |
|
I think we only want to set Admittedly there may not be many places that do this. I know that @peterchanws has put web archive links into items that point to replayweb.page that then fetch data from stacks. For example https://purl.stanford.edu/ww180gg9763 which links to: Stanford University Press is doing something similar for some of its items, which use JavaScript to replay web archives stored in stacks: https://archive.supdigital.org/ |
Possibly. Adjusted PR for this situation. Did not test anything. |
|
The new test passes locally for me...not sure why it fails in CI. |
| allow(Purl).to receive(:public_json).and_return(stanford_json) | ||
| end | ||
|
|
||
| it 'sends CORs header' do |
| @@ -7,7 +7,7 @@ class FileController < ApplicationController | |||
| render plain: 'File not found', status: :not_found | |||
| end | |||
|
|
|||
| before_action :set_cors_headers, only: [:show] | |||
| before_action :set_cors_headers, only: [:show], if: proc { current_file.stacks_rights.stanford_restricted? } | |||
| 'Access-Control-Allow-Credentials' => 'true' | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Is it worth adding a test for when the file is not Stanford Only to make sure the header is *?
There was a problem hiding this comment.
yup, can just add that check to an existing spec (basically ensure the header is not there).
are you interested in taking that on and seeing if you can sort why CI is failing?
There was a problem hiding this comment.
I see that CI is running the tests like this, which fails for me locally (unlike a regular rspec run):
SETTINGS__FEATURES__COCINA=true bin/rake spec
There was a problem hiding this comment.
Ahh, yes. good catch. likely an issue with the mocking in the test not accounting for cocina rights computation
There was a problem hiding this comment.
Fixed. Had a problem in my mocked cocina json (it wasn't internally consistent for stanford only).
edsu
changed the title
|
I don't think any of this is needed, because it turns out we set the CORs header globally in stacks and allow any domain, see https://github.com/sul-dlss/stacks/blob/main/app/controllers/application_controller.rb#L11-L19 As to why it isn't respected in some cases, it may have been due to this recent change: #1096 Though that may not fully explain. Regardless, this code is likely duplicative of what already exists. |
|
The reason why it is needed is that when JavaScript does a cross-origin fetch with credentials the wildcard CORS header will not work. |
|
Should this PR be reopened then? |
|
I guess it could be when/if we want to push sul-dlss/sul-embed#1944 forward. |
peetucket
force-pushed
the
cors-headers-show
branch
from
7006043
to
e19daac
Compare
peetucket
force-pushed
the
cors-headers-show
branch
from
e19daac
to
62a36a5
Compare
Basically adds the CORs header to any request to show a file (not just media auth requests, as is current).
Fixes #1066