Improve sovereign cloud support and network error handling#1018
Improve sovereign cloud support and network error handling#1018Avery-Dunn wants to merge 4 commits intodevfrom
Conversation
| @@ -45,7 +45,10 @@ class AadInstanceDiscoveryProvider { | |||
| "login.chinacloudapi.cn", | |||
There was a problem hiding this comment.
As long as you are here, pls add login.partner.microsoftonline.cn too. See https://login.microsoftonline.com/common/discovery/instance?api-version=1.1&authorization_endpoint=https://login.microsoftonline.us/cab8a31a-1906-4287-a0d8-4eef66b95f6e/oauth2/v2.0/authorize
There was a problem hiding this comment.
Added in the latest commit, Java's list should now match .NET's
| try { | ||
| aadInstanceDiscoveryResponse = sendInstanceDiscoveryRequest(authorityUrl, msalRequest, serviceBundle); | ||
| } catch (MsalServiceException ex) { | ||
| // MsalServiceException indicates a definitive HTTP-level error from the server |
There was a problem hiding this comment.
We should only throw on "invalid_instance" error code.
| } | ||
|
|
||
| @Test | ||
| void discoveryEndpoint_routesToSovereignHost() throws Exception { |
There was a problem hiding this comment.
These tests mock too much and it's hard to undertand what they do. I don't think the scenario in the work item is captured here.
We need an acceptance test with mocked HTTP request / response.
- Authority is
login.sovcloud-identity.fr - Add HTTP mocks
- Call some acquire token (ideally from CCA)
- Look at HTTP requets and assert that all the requests where made on https://login.sovcloud-identity.fr/* URLs. If any of the requets were made on https://login.microsoftonline.com or any other host, fail the test.
There was a problem hiding this comment.
I've removed these tests and added a new SovereignCloudInstanceDiscoveryTest class: the unit tests there set up a CCA app/make acquireToken() calls and check that no calls get made to the public endpoint.
| @@ -45,7 +45,10 @@ class AadInstanceDiscoveryProvider { | |||
| "login.chinacloudapi.cn", | |||
| "login-us.microsoftonline.com", | |||
There was a problem hiding this comment.
should this be https://login.microsoftonline.us instead? I am not sure if login-us.microsoftonline.com exist?
There was a problem hiding this comment.
Java has both, and so does .NET: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/112f8228349e96c25846883ed3fd95067d7fda59/src/client/Microsoft.Identity.Client/Instance/Discovery/KnownMetadataProvider.cs#L68
It still could be outdated though, we may need to audit our list of well-known hosts at some point to make sure they're still up to date.
3 participants
Resolves issue #1008 and makes similar changes as two MSAL.NET PRs
This PR makes the following changes:
KnownMetadataProviderclass containing known metadata entries/aliases, matching what is done in .NET