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

Add support for sending all credentials to trusted domains #831

Merged
merged 9 commits into from
May 7, 2021

Conversation

patrickarlt
Copy link
Contributor

@patrickarlt patrickarlt commented May 3, 2021

In the process of chasing down a very specific bug in Vector Tile Style Editor for a very specific customer with a very specific security setup we found out that ArcGIS REST JS needs to support sending all credentials to servers defined in the authorizedCrossOriginDomains property on portals/self.

The reason for this is that in some high security scenarios organizations ALSO authenticate with BOTH some sort of cookie based auth (PKI/IWA) AND token based auth. In this case the domains that should also get the cookie based auth are defined in the authorizedCrossOriginDomains property on portals/self so we have to request that before trying to federate otherwise getting the server info will fail.

@dasa we discussed that a protocol is not strictly required by the UI for this. In this case we treat secure.esri.com as https://secure.esri.com to always force HTTPS in these kinds of high security setups. I think you told us the JS API will treat this as https://secure.esri.com OR http://secure.esri.com for purposes of matching. I think we should support HTTPS only in this case. Let me know if you think this is a problem.

@noahmulfinger @tomwayson @dbouwman @MikeTschudi this makes a few internal chagnes to UserSession:

  • Renames trustedServers to federatedServers so we can have the new trustedDomains property and not have it be confusing.
  • Adds a new getPortal like getUser however this doesn't have any type information because I didn't want to move IPortal into common types and make this PR even larger.
  • There is a new optional methods on IAuthenticationManager.getDomainCredentials which will return the proper value for Fetch credentials based on a URL this only needs to be implemented for UserSession and is used internally by request.
  • Most tests have been updated to also mock the new portal/self call which is required.

@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #831 (14c6c9b) into master (96798fe) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #831   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          133       133           
  Lines         2312      2352   +40     
  Branches       403       415   +12     
=========================================
+ Hits          2312      2352   +40     
Impacted Files Coverage Δ
packages/arcgis-rest-auth/src/generate-token.ts 100.00% <ø> (ø)
packages/arcgis-rest-auth/src/UserSession.ts 100.00% <100.00%> (ø)
packages/arcgis-rest-request/src/request.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96798fe...14c6c9b. Read the comment docs.

@dasa
Copy link
Member

dasa commented May 4, 2021

@dasa we discussed that a protocol is not strictly required by the UI for this. In this case we treat secure.esri.com as https://secure.esri.com to always force HTTPS in these kinds of high security setups. I think you told us the JS API will treat this as https://secure.esri.com OR http://secure.esri.com for purposes of matching. I think we should support HTTPS only in this case. Let me know if you think this is a problem.

The doc isn't specific about this either, other than saying that the protocol is optional, but it does seem reasonable. It'd only be an issue if the user expected it to work over http and the app was using http and the URL to the service was also http. I'm not sure how common this is.

Copy link
Member

@dasa dasa left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

Copy link
Contributor

@noahmulfinger noahmulfinger left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I have a few minor comments.

Copy link
Member

@dbouwman dbouwman left a comment

Choose a reason for hiding this comment

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

Nothing to add beyond @noahmulfinger's suggestions. Thanks for digging deep into the weeds and resolving this.

@tomwayson
Copy link
Member

tomwayson commented May 4, 2021

I haven't had a chance to look in depth, but my main concern is whether renaming trustedServers is a breaking change. It is marked as private:

I think we could put a getter like:

get trustedServers() {
  console.log('DEPRECATED: use federatedServers instead');
  return this.federatedServers;
}

But I don't think we have too b/c it's private and doesn't appear in the docs. If our intent is to keep federatedServers private, we might want to rename it to _federatedServers like the other private members.

Copy link
Member

@MikeTschudi MikeTschudi left a comment

Choose a reason for hiding this comment

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

Thanks, Patrick; I don't think that it will break our use of the library.

@patrickarlt patrickarlt merged commit e59e499 into master May 7, 2021
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.

6 participants