Esri / esri-leaflet

A lightweight set of tools for working with ArcGIS services in Leaflet. :rocket:
https://developers.arcgis.com/esri-leaflet/
Apache License 2.0
1.61k stars 799 forks source link

Possible to set withCredentials to true? #881

Closed bdk0172 closed 7 years ago

bdk0172 commented 8 years ago

We have a client that has ArcGIS 10.2.2 for Server set up within a secured Active Directory. We are attempting to add layers from this server to a Mapbox map, but are getting errors.

Here is the JavaScript code to declare the layer:

var layer = L.esri.dynamicMapLayer({
  url: url,
  opacity: 1,
  layers: layers,
  token: token
});

The customer used Fiddler to compare the XHR requests that are being sent from our site against a 3rd party software that is able to access their map data and they stated that the difference was withCredentials was set to true coming from the 3rd party software.

Is it possible to set this by passing another option?

jgravois commented 8 years ago

i'm pretty sure this'll have to be fixed way down in the guts of L.esri.request.

if (params.token) {
  httpRequest.withCredentials = true;
}
// immediately prior to calling httpRequest.send()

i'll track down a test server with integrated windows authentication and confirm that this is an appropriate patch soon.

bdk0172 commented 8 years ago

Thanks for working on this. I look forward to hearing back from you.

jgravois commented 7 years ago

i found a test server today and i wasn't able to reproduce the error you described. that is to say, that even without including withCredentials in requests to an Integrated Windows Authentication secured service, a hardcoded token was sufficient to retrieve the features.

would you be willing to test the suggestion i sketched above and let me know whether it resolves the problem on your end?

jgravois commented 7 years ago

okay, i finally figured out how to reproduce the problem you reported. here are a few things i learned.

  1. its only appropriate to set withCredentials on CORS requests
  2. the point of the header is to ensure that cookies automatically generated by the browser to confirm the windows identity of the user are passed through. this seems to indicate that any hardcoded token that is supplied is ignored entirely.

unfortunately the test machine i'm pointing at is pretty flaky so i'm not 100% sure that the fix i proposed in #884 is bulletproof. are you willing/able to test the change independently?

bdk0172 commented 7 years ago

Not easily. We bundle our JavaScript files so we would have to swap in the new file, build it and then deploy the build to production and have our customer test it.

jgravois commented 7 years ago

no problem. i'll cut a release soon. afterward it would be extremely helpful if you could confirm whether or not the fix thats in master now resolves your issue.

bdk0172 commented 7 years ago

I did a build with this fix and am getting an error that "Evented" is undefined.

var Service = L.Evented.extend({

  options: {
    proxy: false,
    useCors: cors,
    timeout: 0
  },
jgravois commented 7 years ago

not sure what to tell you about that one. L.Evented is definitely still a Leaflet object.

bdk0172 commented 7 years ago

OK, let me know when you get out a release and I'll try testing with that.

jgravois commented 7 years ago

i tracked down a dependable test machine today and by setting useCors: false in my dynamicMapLayer constructor I was able to display images from an Integrated Windows Authentication enabled map service.

This approach ensures that appropriate cookies are passed because crossOrigin: false is set downstream when we instantiate L.imageOverlays internally. the only drawback to this approach is that it is not be possible to access individual pixel data.

I noticed a serious unrelated error as a result of my first patch, so we reverted that change in #890.

bdk0172 commented 7 years ago

Just to clarify, just setting userCors: false should fix the issue, it doesn't require a change on your end to set withCredentials to true?

jgravois commented 7 years ago

it doesn't require a change on your end to set withCredentials to true?

correct. I tested after reverting my original change and by setting useCors (not userCors) was able to view IWA secured dynamicMapLayer images

jgravois commented 7 years ago

i'll wait to hear from @bdk0172 with his own test results before closing this issue.

jgravois commented 7 years ago

@bdk0172 can you confirm whether the approach I described previously is a workable solution for you when you have a chance please?

bdk0172 commented 7 years ago

A support team member is reaching out to our customer today to see if they can test the change. Hopefully I can let you know more by the end of day.

bdk0172 commented 7 years ago

We were finally able to get out customer to test this today and it is not working. They stated that Fiddler is not showing a credential cookie being passed in the request.

bdk0172 commented 7 years ago

Have you had an opportunity to look into this to set withCredentials?

jgravois commented 7 years ago

sorry for the delayed reply.

additional information from you and your user about exactly what happened when you tried not using CORS would be pretty helpful.

an additional avenue to pursue would be try adding a special object constructor flag for IWA services to avoid setting the header unless it'd be appropriate.

jgravois commented 7 years ago

in order to support CORS dynamicMapLayer requests to integrated windows authentication enabled services, two things are necessary.

  1. withCredentials needs to be selectively set (as discussed previously) in both xmlHttpGet and request.
if (context && context.options.iwa) {
  httpRequest.withCredentials = true;
}

and

  1. Leaflet.ImageOverlay needs to be extended to pass crossOrigin=use-credentials when it creates an actual <img> DOM node.
// imageOverlay._initImage
img.crossOrigin = 'use-credentials'; // instead of an empty string

because its not solely an esri leaflet change, my instinct is to leave it a one off customization.

bdk0172 commented 7 years ago

Was out of the office for a couple of weeks over the holidays and just got back. Is there an updated file available?

jgravois commented 7 years ago

Is there an updated file available?

no. you'll need to create and host custom builds of both esri leaflet and leaflet yourself with the changes i mentioned above.

jgravois commented 7 years ago

to recap this issue: getting integrated windows authentication functioning correctly with dynamicMapLayer involves customizing Leaflet and Esri Leaflet.

we haven't identified any changes that would be appropriate in the core project here so i'm going to go ahead and add the FAQ label and close now. if anyone at any point has more questions about this topic in the future, i'd be 😀 to discuss it.

zachatrocity commented 5 years ago

@jgravois is this the implementation you were picturing?

L.ImageOverlay.include({
  _initImage: function () {
    var img = this._image = L.DomUtil.create('img',
      'leaflet-image-layer ' + (this._zoomAnimated ? 'leaflet-zoom-animated' : ''));

    img.onselectstart = L.Util.falseFn;
    img.onmousemove = L.Util.falseFn;

    img.onload = L.bind(this.fire, this, 'load');

    if (this.options.crossOrigin) {
      img.crossOrigin = 'use-credentials';
    }

    img.src = this._url;
    img.alt = this.options.alt;
  }
});

Seems to be working for me. Would this be of benefit to the docs?

jgravois commented 5 years ago

is this the implementation you were picturing?

i dunno. its been a couple years. 😉

Seems to be working for me.

when i wrote in January 2017 i laid out instructions for making changes to the source of both Leaflet and esri leaflet in order to support IWA.

are you saying that it's really only necessary to customize Leaflet itself?

Would this be of benefit to the docs?

i dunno about that one either. you'd have to explain in a little more detail what exactly we'd add (and where).

zachatrocity commented 5 years ago

@jgravois Yeah sorry haha, I know this issue is old.

Yeah your recommendation above seemed to suggest to modify the both libraries and host them yourself. That wasn't ideal for my situation, and I was able to accomplish the same effect just by extending the leaflet ImageOverlay class using leaflets built in utilities.

If this is the case and it's really this simple then there might be a case for a PR. Seems like this would be an appropriate place: https://github.com/Esri/esri-leaflet/blob/master/src/Layers/RasterLayer.js#L5-L17

Let me know if this would be useful and I can open a PR.

jgravois commented 5 years ago

thanks for clarifying. PR's are always welcome.

if you find the time to put one together, keep in mind the following:

zachatrocity commented 5 years ago

Are you opposed to adding an iwa attribute to the layer options?

In my scenario I did not have to modify request() at all. I suppose it could depend on the ESRI server configuration.

Unless I'm missing something I think we should just add a crossOrigin option alongside useCors. We shouldn't need to take a stance on IWA vs non-IWA

jgravois commented 5 years ago

all that sounds good to me.