Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalizing assets_proxy for all assets #87

Closed
timmc-edx opened this issue Sep 30, 2024 · 1 comment · Fixed by #91
Closed

Generalizing assets_proxy for all assets #87

timmc-edx opened this issue Sep 30, 2024 · 1 comment · Fixed by #91
Assignees

Comments

@timmc-edx
Copy link
Contributor

While reading through the code in v18.0.2, I notice that the assets_proxy method currently generates a URL and then reads from it. It looks like this was added to support private S3 buckets. I'm wondering if it could be changed to instead just call the open() method on the storage (and by always generating asset-proxy URLs for all storages, not just S3.) I think it would have the following benefits:

  • Remove code from the S3 backend (remove the url method override entirely) and make all storages work the same way (lower cognitive overhead).
  • Possibly, make it possible to use any Django storage without any modification at all.
  • Allow serving local files via the asset proxy, which would help avoid the issue where the default storage's URLs (/media/...) are not covered by the X-Frame-Options exemption that the XBlock handler currently enjoys. (This currently makes SCORM xblocks break in devstack.)

Is this a change that would make sense? Am I missing something about the APIs and constraints?

Danyal-Faheem added a commit to edly-io/openedx-scorm-xblock that referenced this issue Nov 6, 2024
We do this so that custom storage does not need to be defined for Scorm
The default storage configured for Open edX should work out of the box with scorm now
This also fixes an issue of X-Frame-Options on the LMS when running in development mode
closes overhangio#87
Danyal-Faheem added a commit to edly-io/openedx-scorm-xblock that referenced this issue Nov 6, 2024
We do this so that custom storage does not need to be defined for Scorm
The default storage configured for Open edX should work out of the box with scorm now
This also fixes an issue of X-Frame-Options on the LMS when running in development mode
closes overhangio#87
@Danyal-Faheem
Copy link
Contributor

Hey @timmc-edx, thanks for this enhancement request. I've created a PR in #91 for this. Can you take a look and let me know what you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

2 participants