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

CORS workaround to enable downloading of content #1032

Merged
merged 22 commits into from
Oct 12, 2021
Merged

Conversation

millicentachieng
Copy link
Contributor

@millicentachieng millicentachieng commented Jul 19, 2021

Overview

Some queries resulting in a file download have been failing due to CORS policy. We have a few issues that have been raised by some of our users and we have been advising them to use workaround outlined in the GitHub issues, microsoftgraph/msgraph-sdk-javascript#388 and https://github.com/microsoftgraph/microsoft-graph-docs-contrib/issues/8394.

image

The initial plan was to prevent the automatic redirect to the download link which was throwing the CORS error, extract the Location header which holds the download link and finally present the link to the user to download the file. This could not work because the Microsoft Graph JavaScript SDK does not support setting redirects to manual.

The CORS workaround implemented in this PR fetches the file download URL by running a separate query and presenting the link to the user which they can use to download the file. The challenge with this approach is that the file will only be downloadable in it's original format and any formatting instructions in the query parameters are ignored.

Demo

image

The user is also notified that the download URL is only for the original file format.
image

The user is also informed that headers are missing since it's a workaround and not the expected result of the query.
image

We also have other queries that return a file response successfully but the result is not displayed to the user leading to the user thinking that there's no response. Here's an example of an issue raised #459.

The implementation now presents the user with information about the response and a link to download the file.
image

The only challenge here is that the downloaded file will not have the same name as the original file. Had Content-Disposition header been exposed, it would have been possible to download the file with it's original name.

Testing Instructions

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

@ddyett
Copy link

ddyett commented Aug 11, 2021

anything blocking this moving forward?

@ghost
Copy link

ghost commented Sep 13, 2021

anything blocking this moving forward?

@ddyett @adhiambovivian @millicentachieng - we still have some work to do on the copyediting of the error messages and there is also a spacing issue in one of the error messages.

@millicentachieng
Copy link
Contributor Author

anything blocking this moving forward?

@ddyett Sorry, I missed this comment but please review the PR when you get a chance.

@millicentachieng
Copy link
Contributor Author

anything blocking this moving forward?

@ddyett @adhiambovivian @millicentachieng - we still have some work to do on the copyediting of the error messages and there is also a spacing issue in one of the error messages.

@kristen-msft, could you please point me to these error messages? The screenshots in the description of the PR have all the expected messages.

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

src/app/services/actions/query-action-creator-util.ts Outdated Show resolved Hide resolved
src/app/views/query-response/headers/ResponseHeaders.tsx Outdated Show resolved Hide resolved
src/messages/GE.json Outdated Show resolved Hide resolved
src/messages/GE.json Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

@jasonjoh
Copy link
Member

Should we present this as a limitation of the JavaScript SDK? CORS is CORS right, no SDK can bypass it by design. I think this is more accurately described as an issue with the File API (good summary here). Basically, the Files API doing a 302 redirect is fine, but the server it redirects to does NOT support CORS, so the redirect fails in JavaScript. This would fail even without the SDK.

The OneDrive team patched it up to allow simple GET requests on the downloadUrl (these don't trigger CORS), hence the workaround that's required.

I think your A workaround has been used messages should give a little more info on the problem:

We may want to add this to https://docs.microsoft.com/graph/api/driveitem-get-content?view=graph-rest-1.0&tabs=http and just have GE link to that doc for more info.

I also think your file response is only available...due to limitations of the JS SDK message should instead say something like due to limitations of the workaround, the file is only available....

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

1 similar comment
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

@millicentachieng
Copy link
Contributor Author

Thanks @jasonjoh, for these insights. I have updated the contents of the messages to reflect what you've highlighted. I've also talked to Jeremy Kelly about updating the API documentation to include details on the workaround.

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

1 similar comment
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-1032.centralus.azurestaticapps.net

@thewahome thewahome merged commit 078242c into dev Oct 12, 2021
@thewahome thewahome deleted the task/cors-workaround branch October 12, 2021 10:28
ElinorW added a commit that referenced this pull request Nov 18, 2021
* HB of localized GE.jsons (#1094)

* Capture telemetry for Graph Proxy API calls (#1098)

* Capture GE mode for all telemetry items (#1105)

* Update how we are sanitizing track trace to include more info (#1122)

* Capture telemetry for graph toolkit example link (#1125)

* chore: handover-translations (#1107)

* Feature: input hints (#1104)

* Enhancement: Make pivots headers responsive (#1121)

* Chore: add linter workflow (#1132)

* Fix: linting errors (#1133)

* Fix: Request headers textfield bug (#1137)

* Fix: Update packages (#1138)

* Fix: makes query color in the text area black (#1152)

* Fix: CORS workaround to enable downloading of content (#1032)

* Fix: Accessibility error for 'button-name' (#1164)

* Fix: move documentation links to the first column (#1158)

* Fix: Add focus on button (#1153)

* Fix: Create user when emailAddress is unavailable (#1165)

* Chore: add dependabot configuration (#1188)

* Fix: try it interaction (#1185)

* Fix: failing nextlink call (#1184)

* Fix: request and response height (#1201)

* Fix: Remove hover functions on history tab (#1204)

* Feature: Add feedback button and pop-up (#1183)

* Fix: Update GitHub workflow (#962)

* Fix: Concept try-it button (#966)

* Fix: prevent running empty requests (#965)

* Task: add description to documentation link (#1203)

* Chore: translate to German (#1208)

* validate index of selected query (#1210)

* Translate to portuguese (#1212)

* Chore: Handover translations (#1213)

* Chore(deps): Bump @babel/eslint-parser from 7.12.13 to 7.16.3 (#1207)

* Feature: adds go to the snippet tabs (#1189)

* Chore: (Dependabot) Update packages (#1220)

* Chore(deps): Bump jest-watch-typeahead from 0.2.1 to 0.5.0 (#1199)

* Chore(deps): Bump @axe-core/webdriverjs from 4.2.2 to 4.3.1 (#1192)

* Chore(deps-dev): Bump react-test-renderer from 16.8.3 to 16.13.0 (#1195)

* Chore(deps): Bump eslint-plugin-flowtype from 2.50.1 to 2.50.3 (#1198)

* Chore(deps): Bump re-resizable from 6.9.0 to 6.9.1 (#1197)

* Chore(deps): Bump terser-webpack-plugin from 3.1.0 to 4.2.3 (#1196)

* Chore(deps): Bump file-loader from 2.0.0 to 6.2.0 (#1194)

* Chore(deps-dev): Bump @types/react from 16.8.3 to 17.0.35 (#1219)

* Chore(deps): Bump react from 16.8.2 to 16.14.0 (#1193)

* Fix: Linting errors in snippets (#1233)

* Fix: Failure to load on Safari (Mac) (#1222)

* chore(release): 4.14.0
This was referenced Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants