-
Notifications
You must be signed in to change notification settings - Fork 339
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
[Region] Redesign regional #2509
Conversation
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationOptions.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationOptions.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @bgavrilMS
I've proposed a few suggestions
src/client/Microsoft.Identity.Client/ConfidentialClientApplication.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Instance/Discovery/RegionDiscoveryProvider.cs
Show resolved
Hide resolved
return result; | ||
} | ||
|
||
private async Task<RegionInfo> DiscoverRegionNoCacheAsync(ICoreLogger logger, CancellationToken requestCancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actual discovery logic was just copied, no review required.
src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationOptions.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Instance/Discovery/RegionDiscoveryProvider.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Instance/Discovery/RegionDiscoveryProvider.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments. (Disregard my previous accidental approval).
src/client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForClientParameterBuilder.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForClientParameterBuilder.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationOptions.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationOptions.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Instance/Discovery/RegionDiscoveryProvider.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Instance/Region/RegionManager.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Instance/Region/RegionManager.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs
Show resolved
Hide resolved
lgtm as well. mostly wondering about the tests as those didn't pass (I would also have assumed a few updates to tests would be needed). |
@henrik-me - because I'm adding new tests as I have full understanding of the feature. There were a few missed scenarios, and I am logging bugs for them. I also refactored the existing logic a bit to decouple a few things.... |
Like the refactorings for sure. Before signing off, would like to see the tests. Assuming someone is helping to also do manual verification. In reply to: 808589600 [](ancestors = 808589600) |
Pushed 2 commits.
Tested with the app on my PC and also on a VM. Everything worked except the invalid user-provided region.
|
4cb3c10
to
8a3f51f
Compare
8a3f51f
to
d17db23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If invalidregion is passed in, it's expected that we go to the above authority. The authority exists (it will point to global for now). In reply to: 808673645 [](ancestors = 808673645) |
@henrik-me - the "auto-discovery" code runs more than once because we "resolve the authority" many times during an auth request. It's not a great design, but seemed too much to refactor now. Note that the auto-discovery does all the work the first time, but afteerwards reads from the cached static var. |
Work remaining:
An important improvement can also be made: