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

Support 'withCredentials' option which will include credentials with CORS requests #1233

Merged
merged 3 commits into from
Oct 5, 2020

Conversation

luiscamachopt
Copy link
Contributor

addresses #881

@gavinr
Copy link
Contributor

gavinr commented Oct 2, 2020

@luiscamachopt thank you for the PR! A few small requests:

  1. It looks like the unit tests are failing, mostly linting issues. Please run "npm test" locally and fix any linting issues.
  2. Issue 881 is quite long, could you please add a summary/description here of the code changes you're proposing here, why they're needed, and how to see the changes in action (replication case)?
  3. Could you please add some unit tests that will test this behavior to ensure it works going into the future.

Thanks!

@luiscamachopt
Copy link
Contributor Author

When sending cross origin requests to servers that are secured with Active Directory (issue refers to Integrated 'Windows Authentication' IWA) it is necessary to send http requests with 'withCredentials' set to true and image elements should property 'crossOrigin' set to 'use-credentials'
With this PR, If the layer is created with the option 'withCredentials' set to true the requests will be adjusted as described so that the remote server receives the credentials and accepts the requests.

when sending cross origin requests to servers that are secured with IWA/AD it is necessary to send request with 'withCredentials' set to true
If the layer is created using this option the requests will be adjusted accordingly
@luiscamachopt
Copy link
Contributor Author

Hi @gavinr
I've fixed the linting issues, pushed tests and added a more complete description of the issue.
Thanks

Copy link
Contributor

@patrickarlt patrickarlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a code perspective this looks good. However this change is difficult for us to test given that:

  1. The original issue is old.
  2. The original group debugging the issue has generally moved on.
  3. We don't have servers readily available to test this setup.

I'm OK with merging but @gavinr can you do some extensive testing against our existing examples to ensure their are no regressions.

src/Request.js Outdated Show resolved Hide resolved
src/Request.js Outdated Show resolved Hide resolved
adjust styling for consistency

Co-authored-by: Patrick Arlt <[email protected]>
@gavinr
Copy link
Contributor

gavinr commented Oct 5, 2020

Tested against all existing samples, looks ok. Merging.

@gavinr gavinr merged commit a3a4d74 into Esri:master Oct 5, 2020
@gavinr
Copy link
Contributor

gavinr commented Oct 5, 2020

This was released in v2.5.1 - thanks!

jgravois pushed a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
…CORS requests (Esri#1233)

* Add support for 'withCredentials' option

when sending cross origin requests to servers that are secured with IWA/AD it is necessary to send request with 'withCredentials' set to true
If the layer is created using this option the requests will be adjusted accordingly

* added tests for 'withCredentials' option

* Apply suggestions from code review

adjust styling for consistency

Co-authored-by: Patrick Arlt <[email protected]>

Co-authored-by: Patrick Arlt <[email protected]>
jgravois pushed a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
…CORS requests (Esri#1233)

* Add support for 'withCredentials' option

when sending cross origin requests to servers that are secured with IWA/AD it is necessary to send request with 'withCredentials' set to true
If the layer is created using this option the requests will be adjusted accordingly

* added tests for 'withCredentials' option

* Apply suggestions from code review

adjust styling for consistency

Co-authored-by: Patrick Arlt <[email protected]>

Co-authored-by: Patrick Arlt <[email protected]>
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.

3 participants