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

Allow xhr.withCredentials to be set per request type #1607

Conversation

davemevans
Copy link
Contributor

As requested by Trevor Hunsaker on Slack.

@davemevans
Copy link
Contributor Author

Trevor's actual use case was for requests to key server. Protection does not use XHRLoader so this won't fix that. Perhaps, then, this PR isn't needed but seems like it might be good to be able to, for instance, get MPD with credentials but not segments or something?

Trevor discovered that if you set "withCredentials": true in protData, withCredentials is set on the XHR.

@vzTrevorHunsaker
Copy link

vzTrevorHunsaker commented Oct 10, 2016

I realized that my initial request overlooked something: during testing I'd manually set a cookie in my browser that was being sent back during the key request. This oversight didn't take into account that at some point I'll need to have the browser set the cookie, and the request for any XHR response that wishes to set a cookie will also need to be made with withCredentials == true. I was planning on sending the Set-Cookie header in the response to the MPD request. I just tested the branch from this pull request and was able to successfully set the cookie dynamically during the MPD request/response and see it sent back in the key requests.

As such, I'd like to still advocate for this pull request.

@dsparacio dsparacio merged commit 998df78 into Dash-Industry-Forum:development Oct 28, 2016
@davemevans davemevans deleted the XHRWithCredentialsPerDownloadType branch October 28, 2016 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants