-
Notifications
You must be signed in to change notification settings - Fork 378
Update the SF flow to get a http client from the factory #5221
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
Conversation
src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/AppConfig/IMsalSFHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.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.
Copilot reviewed 18 out of 24 changed files in this pull request and generated 1 comment.
Files not reviewed (6)
- src/client/Microsoft.Identity.Client/PublicApi/net462/PublicAPI.Unshipped.txt: Language not supported
- src/client/Microsoft.Identity.Client/PublicApi/net472/PublicAPI.Unshipped.txt: Language not supported
- src/client/Microsoft.Identity.Client/PublicApi/net8.0-android/PublicAPI.Unshipped.txt: Language not supported
- src/client/Microsoft.Identity.Client/PublicApi/net8.0-ios/PublicAPI.Unshipped.txt: Language not supported
- src/client/Microsoft.Identity.Client/PublicApi/net8.0/PublicAPI.Unshipped.txt: Language not supported
- src/client/Microsoft.Identity.Client/PublicApi/netstandard2.0/PublicAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (1)
src/client/Microsoft.Identity.Client/Http/HttpManager.cs:155
- [nitpick] There is an inconsistency in naming the certificate validation callback parameter ('validateServerCert' versus 'validateServerCertificate' in other parts of the code). Standardizing this naming across the codebase would improve clarity.
if (validateServerCert != null)
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.
the PR looks good. well-commented, logically consistent, and with good test coverage. just some minor comments.
src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ServiceFabricManagedIdentitySource.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
| }; | ||
|
|
||
| string key = handler.GetHashCode().ToString(); | ||
| return s_httpClientPool.GetOrAdd(key, new HttpClient(handler)); |
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.
I don't think there's any benefit from caching HttpClient. HttpClient is just a holder of handlers. It's the handlers that need to be cached.
This pull request introduces a new interface
IMsalSFHttpClientFactoryto handle custom server certificate validation for Service Fabric scenarios and refactors the existing HTTP manager code to support this new interface. The most important changes include the addition of the new interface, modification of existing methods to use the new validation callback, and updates to various classes to ensure compatibility.New Interface for Custom Certificate Validation:
src/client/Microsoft.Identity.Client/AppConfig/IMsalSFHttpClientFactory.cs: Introduced theIMsalSFHttpClientFactoryinterface to createHttpClientinstances with custom server certificate validation callbacks.Refactoring HTTP Manager:
src/client/Microsoft.Identity.Client/Http/HttpManager.cs: RefactoredSendRequestAsyncandExecuteAsyncmethods to use the newvalidateServerCertcallback instead ofcustomHttpClient. UpdatedGetHttpClientmethod to handle the new interface and createHttpClientinstances appropriately. [1] [2] [3] [4] [5] [6] [7]Interface Updates:
src/client/Microsoft.Identity.Client/Http/IHttpManager.cs: Updated theIHttpManagerinterface to replace thecustomHttpClientparameter with thevalidateServerCertificatecallback in theSendRequestAsyncmethod. [1] [2] [3]Class Method Updates:
src/client/Microsoft.Identity.Client/Instance/Region/RegionManager.cs: Modified methods to use thevalidateServerCertificatecallback instead ofcustomHttpClient. [1] [2] [3]src/client/Microsoft.Identity.Client/Instance/Validation/AdfsAuthorityValidator.cs: Updated theValidateAuthorityAsyncmethod to use thevalidateServerCertificatecallback. [1] [2]src/client/Microsoft.Identity.Client/ManagedIdentity/AbstractManagedIdentity.cs: Adjusted theAuthenticateAsyncmethod to use thevalidateServerCertificatecallback and added a newValidateServerCertificatemethod for custom validation logic. [1] [2] [3]Additional Cleanups:
Changes proposed in this request
Testing
Performance impact
Documentation