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

Player is not sending cookies if we attachSource #1006

Closed
kamranhameedpl opened this issue Jan 13, 2016 · 20 comments
Closed

Player is not sending cookies if we attachSource #1006

kamranhameedpl opened this issue Jan 13, 2016 · 20 comments
Assignees
Milestone

Comments

@kamranhameedpl
Copy link

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
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Contributor

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

@boushley
Copy link
Member

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
Copy link
Contributor

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

@boushley
Copy link
Member

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

@dsparacio dsparacio added this to the 2.0.0 milestone Jan 25, 2016
@dsparacio dsparacio self-assigned this Jan 25, 2016
@dsparacio
Copy link
Contributor

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

@boushley
Copy link
Member

boushley commented Feb 5, 2016

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
Copy link
Contributor

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
Copy link
Member

boushley commented Feb 5, 2016

Found it, #1048

I figured this was out of scope for 2.0

@dsparacio
Copy link
Contributor

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 dsparacio modified the milestones: 2.1.0, 2.0.0 Feb 5, 2016
@dsparacio
Copy link
Contributor

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

@ShanePhelan
Copy link
Contributor

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

@davemevans davemevans added this to the 2.2.0 milestone May 11, 2016
@davemevans
Copy link
Contributor

Still awaiting feedback from interested parties before merging.

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

No branches or pull requests

6 participants