Dash-Industry-Forum / dash.js

A reference client implementation for the playback of MPEG DASH via Javascript and compliant browsers.
http://reference.dashif.org/dash.js/nightly/samples/dash-if-reference-player/index.html
Other
5.11k stars 1.67k forks source link

Player is not sending cookies if we attachSource #1006

Closed kamranhameedpl closed 8 years ago

kamranhameedpl commented 8 years ago

Hi,

I am implementing dash js in our web application. The content is protected with cookies. When i attach the source to the player and it goes to request the mpd from server it is not including cookies in the request. here is my code how can i tell the player to take cookies with it when it requests the mpd from server. var newSource = "url to mpd" var videoPlayer = document.getElementById("videoPlayer"); var context = new Dash.di.DashContext(); var player = new MediaPlayer(context); player.startup(); player.attachView(videoPlayer); player.attachSource(newSource)

boushley commented 8 years ago

@kamranhameedpl sending cookies isn't something that is generally managed by JS. Cookies are sent automatically by the browser.

Does the mpd file reside on the same domain as the cookies are set on? If so do you have any more specifics about the use case? What is the URL where the page is hosted, what is the URL where the mpd is hosted?

kamranhameedpl commented 8 years ago

Hi,

Thanks for your reply. when i set the src in the video tag it sends cookies but in that way it does not understands the mpd format because the call is not generated with DASH. The mpd is hosted on cloudfront. this is the file hosted on cloud front the url is configured with cname we have configured our content with amazon cloud front distribution using signed cookies method.

http://dev-cdn.dadoof.com/videos/dash/5fae5734-770e-4006-b4be-bea816b6d5be.mpd when we attach the player with this src url it does not send cookies along with the call. there is an attribute of video tag crossorigin="use-credentials" then it sends the cookies with its domain. cookies are already in the browser and is being sent with the images. but DASH is not sending cookies.

Is there any attribute something like useCredentials so we can set it true to make it send cookies when it start fetching the mpd file from the server.

boushley commented 8 years ago

Thanks for the additional details! Knowing that this is a CORS call is super valuable!

I'm not at my computer right now so I can't check if we support the send credentials option for CORS, but I'll double check later. If we do I'll let you know how to set it. If we don't we'll update this ticket with information so we can add support for it.

kamranhameedpl commented 8 years ago

if we use jquery ajax we can send withCredientials like this from the offical jquery documentation

xhrFields Type: PlainObject An object of fieldName-fieldValue pairs to set on the native XHR object. For example, you can use it to set withCredentials to true for cross-domain requests if needed.

$.ajax({ url: a_cross_domain_url, xhrFields: { withCredentials: true } }); In jQuery 1.5, the withCredentials property was not propagated to the native XHR and thus CORS requests requiring it would ignore this flag. For this reason, we recommend using jQuery 1.5.1+ should you require the use of it.

boushley commented 8 years ago

We don't use jQuery for XHR requests.

We use the native XMLHttpRequests object. We simply need to add a line that says request.withCredentials = true. I think for most people this would be reasonable, but I'm not sure we can turn it on by default. We'll probably need to add this as an option.

@AkamaiDASH any input on whether we want to add withCredentials = true to our loaders as default or behind an option?

@kamranhameedpl thanks for filing this bug! Supporting withCredentials for CORS requests is definitely a feature we should have. If you're in a rush to get this out and won't be able to wait for a release you could pretty easily use a modified version of the library. You would just need to search for the 7 or 8 places we use XMLHttpRequest and add a line that sets withCredentials on each one.

boushley commented 8 years ago

Also, it looks like we add withCredentials in one place when we're making calls from the ProtectionController but not for loading manifests or segments.

kamranhameedpl commented 8 years ago

I am using the cdn version of your library. which is a minified version where can i get the full version of the library ? which i can download and replace.

boushley commented 8 years ago

Right here on GitHub! You can checkout the source modify it and build yourself a new minified version: https://github.com/Dash-Industry-Forum/dash.js

dsparacio commented 8 years ago

Not sure I have an educated opinion yet on this. Seems reasonable and something to explore.

boushley commented 8 years ago

The only downsides the adding withCredentials would be that we're sending cookies on domains we didn't previously which can increase the size of requests. Of course we're downloading video... so we're probably not concerned about a few cookies.

Also if the CORS headers from the other domain doesn't allow credentials I'm not sure if withCredentials = true silently does nothing or if it will throw an error. Something we would want to check on before enabling this.

dsparacio commented 8 years ago

also does this work in all browsers I think safari will have issues.

boushley commented 8 years ago

MDN seems to think this will work in Safari, but we would definitely need to do some testing.

dsparacio commented 8 years ago

Solution : add api to turn on withCredential on or off. reference in all loaders on XHR request. Default will be false.

boushley commented 8 years ago

In looking at the fix for this one I really want to move creation of XHR objects into a central factory of some kind so I don't have to check the config setting for this and set it in every location that this is used. To that end it would probably be nice for us to build some abstraction around XHR entirely, but is likely out of scope for this issue. Any ideas or input before I dive into doing that?

dsparacio commented 8 years ago

We have an issue already around refactor all loaders to what you ar saying so for 2.0 not in scope but certainly is for 2.1 ! On phone so I'll add issue number later

boushley commented 8 years ago

Found it, #1048

I figured this was out of scope for 2.0

dsparacio commented 8 years ago

Yeah that is the one. I think this is a great project for next release. I did not want to add the withCredentials to this release because of the pending issue change. I figured @kamranhameedpl can easily add this to the XHR manually for now.

dsparacio commented 8 years ago

Moving to 2.1 release should be done in conjunction with #1048

ShanePhelan commented 8 years ago

Is there any update on when this feature will be added?

davemevans commented 8 years ago

Still awaiting feedback from interested parties before merging.