5.1 | APIScan | MSAL WithClientName
#3358
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR backports the MSAL application builder refactoring and adds a comprehensive set of CI pipeline templates for building, testing, and validating signed packages, while also updating documentation snippets, configs, and build scripts.
- Add new YAML templates for .NET Framework/.NET Core build-and-test and signed-package validation
- Update documentation samples to mask passwords and fix minor build command quoting
- Introduce audit sources and policy exclusions in NuGet.config and related .config files
Reviewed Changes
Copilot reviewed 156 out of 156 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/pipelines/common/templates/steps/build-and-run-tests-netfx-step.yml | New .NET Framework build & test step template |
| eng/pipelines/common/templates/steps/build-and-run-tests-netcore-step.yml | New .NET Core build & test step template |
| eng/pipelines/common/templates/steps/build-all-configurations-signed-dlls-step.yml | New step for building signed DLLs across configurations |
| eng/pipelines/common/templates/jobs/validate-signed-package-job.yml | New job for verifying NuGet signature, strong names, and file versions |
| eng/pipelines/common/templates/jobs/run-tests-package-reference-job.yml | New job for running tests against the signed package |
| eng/pipelines/common/templates/jobs/build-signed-package-job.yml | Updated signed-package build job integration |
| eng/pipelines/common/templates/jobs/build-signed-akv-package-job.yml | Updated AKV-provider signed-package build job |
| doc/snippets/Microsoft.Data.SqlClient/SqlConnectionStringBuilder.xml | Masked shown password in snippet |
| doc/snippets/Microsoft.Data.SqlClient/SqlColumnEncryptionEnclaveProvider.xml | Fixed spelling and added isRetry parameter documentation |
| doc/samples/SqlConnectionStringBuilder_Remove.cs | Masked password in sample |
| doc/samples/SqlConnectionStringBuilder_IntegratedSecurity.cs | Masked password in sample |
| doc/samples/SqlConnectionStringBuilder3.cs | Masked password in sample |
| doc/samples/SqlConnectionStringBuilder.cs | Masked password and removed dangling quote |
| build.proj | Removed stray quotation mark in DotnetBuildCmd |
| NuGet.config | Added <auditSources> section |
| BUILDGUIDE.md | Removed outdated AzureKeyVault entries |
| .config/tsaoptions.json | New TSA options config |
| .config/PolicheckExclusions.xml | New PolicyCheck exclusions |
| .config/CredScanSuppressions.json | New CredScan suppressions |
Comments suppressed due to low confidence (4)
eng/pipelines/common/templates/jobs/run-tests-package-reference-job.yml:18
- The
dependsOndefault value is set to the literalempty, which differs from other jobs using an empty string (''). Consider aligning defaults to avoid conditional mismatches.
- - name: dependsOn
- type: string
- default: empty
eng/pipelines/common/templates/jobs/build-signed-package-job.yml:27
- Parameter is declared as
publishSymbolsbut later referenced asPublishSymbols. YAML parameter names are case-sensitive; unify casing to ensure correct value propagation.
- - name: publishSymbols
+ type: boolean
eng/pipelines/common/templates/jobs/build-signed-akv-package-job.yml:27
- The
publishSymbolsparameter is later accessed asPublishSymbols. Adjust the parameter name or references to match casing so it's recognized.
- - name: publishSymbols
+ type: boolean
eng/pipelines/common/templates/jobs/validate-signed-package-job.yml:149
- The variable
$(extractedNugetPath)is never defined; it should likely use$(pathToDownloadedNuget)or be set prior to use.
Get-ChildItem -Path $(extractedNugetPath) -Directory | select Name | foreach {
There was a problem hiding this comment.
Pull Request Overview
Backport MSAL application builder changes to use the documented CreateWithApplicationOptions API and simplify framework-specific branching.
- Switch to
PublicClientApplicationBuilder.CreateWithApplicationOptionswithPublicClientApplicationOptions - Consolidate and conditionally apply parent-window/activity delegates for .NET Framework and .NET Standard
- Remove outdated, undocumented API calls
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs:553
- [nitpick] You could combine the
#if NETFRAMEWORKand#if NETSTANDARDblocks into an#if/elifstructure to reduce repeated directive lines and improve readability.
#if NETFRAMEWORK
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs:559
- Add a unit test to verify that the
CreateClientAppInstancemethod correctly applies the parent-activity delegate in the NETSTANDARD target and the Win32 window delegate in NETFRAMEWORK.
#if NETSTANDARD
| // Optionally set clientId when available | ||
| if (tokenCredentialKey._clientId is not null) | ||
| PublicClientApplicationBuilder builder = PublicClientApplicationBuilder | ||
| .CreateWithApplicationOptions(new PublicClientApplicationOptions |
There was a problem hiding this comment.
Consider moving the authority into PublicClientApplicationOptions.Authority instead of calling .WithAuthority(...) afterward to keep all application options in one place.
There was a problem hiding this comment.
There was a problem hiding this comment.
@mdaigle Sadly it's not that easy. 1) Copilot hallucinated an Authority property on PublicClientApplicationOptions, 2) it takes an enum value while _audience is a string. Although internally WithAuthority uses a method to convert a string into an AadAuthorityAudience enum value, that method is internal and inaccessible to us.
| return publicClientApplication; | ||
| } | ||
|
|
||
| private static TokenCredentialData CreateTokenCredentialInstance(TokenCredentialKey tokenCredentialKey, string secret) |
There was a problem hiding this comment.
Nope, it was a mistake 🤦♂️
There was a problem hiding this comment.
Pull Request Overview
Backport MSAL client construction to use CreateWithApplicationOptions, replacing previous undocumented API usage and consolidating helper methods.
- Refactored
CreateClientAppInstanceto leveragePublicClientApplicationBuilder.CreateWithApplicationOptions - Removed old builder overloads and duplicated helper methods, then reintroduced them below for consistency
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs:596
- Add unit tests for the new
CreateClientAppInstancepath to verify options (ClientName, ClientVersion, RedirectUri, parent window) are correctly applied.
PublicClientApplicationBuilder builder = PublicClientApplicationBuilder
...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
Show resolved
Hide resolved
...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
Show resolved
Hide resolved
...crosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/5.1 #3358 +/- ##
===============================================
- Coverage 71.86% 71.78% -0.09%
===============================================
Files 293 293
Lines 61650 61647 -3
===============================================
- Hits 44307 44251 -56
- Misses 17343 17396 +53
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:
|
|
Man I really hosed this PR up. How the heck did I make so many mistakes here??? |
|
This PR has become a mess since diff doesn't look right anymore. Closing and replacing with #3367 |
Description: Backporting rewriting MSAL application building code to use CreateWithApplicationOptions and avoid (formerly) undocumented APIs. See #3354 for full details of change.