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

Authenticated Imagery Sources with TileImageLayer #76

Closed
AkeluX opened this issue Jan 13, 2017 · 3 comments
Closed

Authenticated Imagery Sources with TileImageLayer #76

AkeluX opened this issue Jan 13, 2017 · 3 comments
Assignees
Milestone

Comments

@AkeluX
Copy link
Contributor

AkeluX commented Jan 13, 2017

TileImageLayer forces the origin on image requests to "anonymous", see:

https://github.com/NASAWorldWind/WebWorldWind/blob/master/src/layer/TiledImageLayer.js#L480

image.crossOrigin = 'anonymous';

This doesn't work for servers requiring authentication. The appropriate value in this situation is "use-credentials".

As discussed with the development team, there is an upcoming refactoring of the image layers that will decouple the tile retrieval mechanism. This issue will be addressed as part of this refactoring.

@pdavidc pdavidc modified the milestone: v0.1.0 Feb 21, 2017
AkeluX added a commit that referenced this issue Mar 31, 2017
@pdavidc pdavidc removed this from the WebWW v0.9.0 milestone Sep 28, 2017
@ghost ghost assigned AkeluX Nov 1, 2017
@ghost ghost added the needs review label Nov 1, 2017
@pdavidc pdavidc added this to the WebWW v0.9.0 milestone Nov 8, 2017
@pdavidc
Copy link
Contributor

pdavidc commented Nov 8, 2017

@Beak-man I have an update from a conversation with @AkeluX this morning. @AkeluX made a good case for keeping this PR as-is. By exposing the crossOrigin retrieval option as a property, rather than through the constructor as we are considering, eliminates the need to modify every TiledImageLayer subclass constructor.

This made a lot of sense to me. What to you think?

@Beak-man
Copy link
Member

Beak-man commented Nov 8, 2017

@pdavidc, I concur with @AkeluX considering that we want to release 0.9.0 soon. TiledImageLayer has seven subclasses and each one would need to be refactored.

I do like your idea of using a configuration object for the constructor. We wouldn't be redesigning from scratch, since two of TiledImageLayer's subclasses — WmsLayer and RestTiledImageLayer — make use of configuration objects to feed the TiledImageLayer constructor (each one in a different manner, and RestTiledImageLayer's is entirely optional, it's not being currently used). Refactoring the seven subclasses with such a pattern may be considered a big task considering we just want to address a single property.

Furthermore, I just discussed this with @markpet49 and he pointed out that just using a 'plain', user-made object as configuration (as is the case with the two aforementioned subclasses) instead of one controlled through a constructor of its own (i.e. a class), is kind of flaky and prone to errors. To have good control over the configuration parameters, we would need to design a 'LayerConfiguration' class or such. I didn't consider this before talking with Mark, and I concur with him. Again this widens the scope of this task and increases the amount of code to maintain if we implement it.

I think the authors of WmsLayer and RestTiledImageLayer used configuration objects as a way to work around the amount of parameters for TiledImageLayer's constructor, but if we do a refactor to consider a configuration class or even just a plain, arbitrary object, it should be considered as a separate, broader task that has more to do with homogenizing the usage of TiledImageLayer by its subclasses, than considering a single property.

@pdavidc
Copy link
Contributor

pdavidc commented Nov 8, 2017

@Beak-man That's a good summary of the key points to consider.

Let's merge #268 as-is, and revisit this when the need arises.

@ghost ghost removed the needs review label Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants