5.1 | APIScan | MSAL WithClientName (3rd attempt)
#3367
Conversation
(second attempt with less mistakes)
There was a problem hiding this comment.
Pull Request Overview
This PR backports changes to the MSAL application building code to use CreateWithApplicationOptions and remove the usage of formerly undocumented APIs.
- Unifies client application creation using a builder pattern.
- Adjusts configuration for different frameworks (NETSTANDARD vs NETFRAMEWORK).
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs:543
- The new implementation no longer applies WithParentActivityOrWindow for NETSTANDARD (_parentActivityOrWindowFunc), which was present in the previous version. Please verify this change is intentional and that no functionality is lost compared to the original branch.
PublicClientApplicationBuilder builder = PublicClientApplicationBuilder.CreateWithApplicationOptions(new PublicClientApplicationOptions
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs:560
- Automated tests appear to be lacking for the new MSAL client configuration paths. Consider adding tests to validate the builder setup for both NETSTANDARD and NETFRAMEWORK scenarios.
return builder.Build();
| .WithClientName(Common.DbConnectionStringDefaults.ApplicationName) | ||
| .WithClientVersion(Common.ADP.GetAssemblyVersion().ToString()) | ||
| .WithRedirectUri(publicClientAppKey._redirectUri) | ||
| .WithParentActivityOrWindow(_parentActivityOrWindowFunc) |
There was a problem hiding this comment.
Don't we need this specific .NET Standard handling like on 5.2?
There was a problem hiding this comment.
Yep, man this PR is cursed.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/5.1 #3367 +/- ##
===============================================
- Coverage 71.86% 71.74% -0.13%
===============================================
Files 293 293
Lines 61650 61647 -3
===============================================
- Hits 44307 44228 -79
- Misses 17343 17419 +76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Backporting rewriting MSAL application building code to use CreateWithApplicationOptions and avoid (formerly) undocumented APIs. See #3354 for full details of change.
Issues
An attempt to resolve ADO APIScan issue, but technically no longer necessary.
Testing
Local build succeeds, far too much work to setup to validate locally.