[7.0.3 Cherry-pick] Fix AccessTokenCallback TNIR behavior and make SspiContextProvider mutually exclusive with token auth - #4561
Open
github-actions[bot] wants to merge 2 commits into
Conversation
To resolve, run: git cherry-pick 194b3cb
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
…tually exclusive with token auth (#4520) * Treat AccessTokenCallback like AccessToken for TNIR and pool keys On .NET Framework the driver disables Transparent Network IP Resolution by default whenever federated authentication is in use, unless the caller explicitly specified the TransparentNetworkIPResolution keyword. However, ShouldDisableTnir only tested _accessTokenInBytes (SqlConnection.AccessToken) and ignored _accessTokenCallback (SqlConnection.AccessTokenCallback), so the two token-supplying APIs behaved differently. Raised in review discussion on #4493. Changes: * Add SqlConnectionInternal.IsAccessTokenProvided, a single source of truth for "the caller supplied a token, either literally or via a callback", and use it in all three places that previously inlined the field checks (ShouldDisableTnir plus two spots in TdsParser.ConsumePreLoginHandshake). The duplicated, hand-written expression is what allowed the two paths to drift apart. * Fix the AccessToken, AccessTokenCallback and SspiContextProvider setters, which each rebuilt the ConnectionPoolKey with the sibling authentication values hard-coded to null. Setting SspiContextProvider silently dropped a previously assigned access token or callback from the pool key, so it never reached the internal connection even though the public property still reported it as set. These now preserve sibling state, matching the ConnectionString setter. (AccessToken and AccessTokenCallback are already mutually exclusive, so that pairing was benign; SspiContextProvider is not.) * Expose ShouldDisableTnir as internal static so it can be unit tested, and add coverage for the TNIR decision matrix and for pool-key preservation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5368f578-219b-40a6-92a9-4742b56edbe6 * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> * Address review feedback on TNIR/pool-key fix * Fully qualify the SqlConnection crefs on ShouldDisableTnir so they match the form already used on IsAccessTokenProvided. * Rename TestShouldDisableTnirWithAccessToken to TestShouldDisableTnirWithCallerSuppliedToken, since the parameter is isAccessTokenProvided and the case covers both AccessToken and AccessTokenCallback. * Assign a real non-null SspiContextProvider in the pool-key test instead of null, via a minimal TestSspiContextProvider stub, so the test matches its name and exercises the setter the way callers actually do. * Add SspiContextProviderIsPreservedInPoolKeyWhenAccessTokenStateIsSet, the reciprocal case: assigning a token must not drop a configured SspiContextProvider from the pool key. Both pool-key tests were verified to fail when the SqlConnection.cs fix is reverted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5368f578-219b-40a6-92a9-4742b56edbe6 * Make SspiContextProvider mutually exclusive with token authentication SSPI is an alternative to token-based authentication, not a complement to it. Previously the SspiContextProvider setter silently coexisted with AccessToken/AccessTokenCallback, leaving the connection in an ambiguous authentication state. Both token setters now throw when a context provider is already set, and the SspiContextProvider setter throws when either token property is already set. The setters continue to pass the sibling values through when building the ConnectionPoolKey: with the new validation those values are guaranteed null when a non-null value is assigned, so it only matters on the clearing path, where the remaining authentication state must survive. Also scope the IsAccessTokenProvided remark to the call sites where the token and callback paths are genuinely equivalent. Login feature-extension negotiation must keep testing the fields individually because they select different federated authentication library types (MSAL vs SecurityToken), and the previous wording invited a cleanup that would break fedauth login. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5368f578-219b-40a6-92a9-4742b56edbe6 * Close SspiContextProvider clone/Credential gaps and cover token detection through open - Copy _sspiContextProvider in the SqlConnection copy constructor. Clone retains the source PoolGroup, so a clone previously reported no provider while its pool key still carried one, and would accept an AccessToken that the new mutual-exclusivity validation should have rejected. - Preserve _sspiContextProvider in the Credential setter's pool key rebuild, removing the last property/pool-key divergence. - Add an end-to-end regression test that opens with AccessTokenCallback against a simulated server signalling FEDAUTHREQUIRED, through both Open and OpenAsync. The server rejects a mismatched Login7 echo, so the test fails if IsAccessTokenProvided stops accounting for the callback. - Record the TNIR decision LoginNoFailover actually applied so the .NET Framework test can assert the wiring, not just the pure ShouldDisableTnir helper. - Extend the ShouldDisableTnir theory to cover an explicit TransparentNetworkIPResolution=false. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Document SspiContextProvider / access token mutual exclusivity Setting SspiContextProvider alongside AccessToken or AccessTokenCallback used to succeed silently and now throws InvalidOperationException, so the public docs need to say so. - Add an <exception> element to SspiContextProvider, which previously had none, covering both the new mutual-exclusivity throw and the pre-existing throw when the connection is already open. - Add a remarks paragraph to SspiContextProvider explaining that SSPI is an alternative to token-based authentication, that the throw fires in either assignment order, and that assigning null clears without throwing. - Name the new SspiContextProvider conflict in the AccessToken and AccessTokenCallback remarks, which previously said only "among others". Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: 5368f578-219b-40a6-92a9-4742b56edbe6
cheenamalhotra
changed the title
cheenamalhotra
marked this pull request as ready for review
August 20, 2026 21:06
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This cherry-pick aligns AccessTokenCallback with AccessToken for .NET Framework Transparent Network IP Resolution (TNIR) defaults, and fixes authentication-state/pool-key inconsistencies by making SspiContextProvider mutually exclusive with token-based authentication while preserving remaining auth state when clearing properties.
Changes:
- Treat
AccessTokenCallbackas “token provided” for pre-login FEDAUTHREQUIRED handling, server certificate validation, and .NET Framework TNIR defaulting. - Enforce mutual exclusivity between
SspiContextProviderandAccessToken/AccessTokenCallback, and preserveSspiContextProvideracross clone + credential pool-key rebuilds. - Add/extend regression tests and update docs/resources to describe the newly invalid combinations.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionTests.cs | Adds simulated-server and pool-key regression coverage for FEDAUTHREQUIRED echo, TNIR wiring, cloning, and SSPI/token exclusivity. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStringTest.cs | Adds direct unit coverage for the netfx TNIR-disable decision logic when a caller-supplied token is present. |
| src/Microsoft.Data.SqlClient/src/Resources/Strings.resx | Adds new localized strings for SSPI/token invalid-combination exceptions. |
| src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs | Adds strongly-typed accessors for the new resource strings. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs | Uses a single “token provided” check for FEDAUTHREQUIRED consumption and certificate validation. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs | Preserves auth state in pool keys on setters/clears; adds SSPI/token mutual-exclusivity validation and copies provider on clone. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs | Introduces IsAccessTokenProvided, exposes netfx TNIR decision for tests, and makes TNIR decision logic testable via internal static method. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs | Adds ADP exception helpers for the new invalid combinations. |
| doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml | Documents the SSPI/token mutual exclusivity and the thrown exception behavior. |
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (1)
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStringTest.cs:104
SqlConnectionOptionsdoes not appear to be a valid type in this repo (andShouldDisableTnirexpects aSqlConnectionString). This looks like a typo that will break NETFRAMEWORK compilation of the unit tests.
SqlConnectionOptions connectionOptions = new(builder.ConnectionString);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cheenamalhotra
approved these changes
Aug 20, 2026
mdaigle
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-pick of #4520 into
release/7.0.Summary
AccessTokenCallbacklikeAccessTokenwhen deciding the .NET Framework TNIR default.SspiContextProvidermutually exclusive with token authentication while preserving remaining authentication state when properties are cleared.SspiContextProviderthrough clone and credential pool-key rebuilds.Conflict resolution
Adapted the merged change to the 7.0 branch type names (
SqlConnectionStringandSqlConnectionPoolKey) and cast the base-typed pool key in backported tests.Validation
The .NET Framework-only TNIR matrix remains covered by CI.