Fix SetProvider to return immediately if user-defined provider found #3620
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where SetProvider method would continue execution after finding a user-defined provider instead of returning immediately, potentially allowing the replacement of existing user-defined authentication providers.
- Replaces
breakstatement withreturn falseto prevent overriding user-defined providers - Ensures proper early exit when a user-defined provider already exists for the authentication method
|
Should we add a test that confirms the broken behaviour when using the config-file based auth provider registration? And then see it pass with your fix. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3620 +/- ##
==========================================
+ Coverage 69.61% 77.31% +7.69%
==========================================
Files 276 273 -3
Lines 61714 46058 -15656
==========================================
- Hits 42964 35611 -7353
+ Misses 18750 10447 -8303
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:
|
|
Wonder if it would be possible to add a test to prevent regression? |
src/Microsoft.Data.SqlClient/tests/FunctionalTests/AADAuthenticationTests.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/FunctionalTests/AADAuthenticationTests.cs
Show resolved
Hide resolved
| [InlineData(SqlAuthenticationMethod.ActiveDirectoryInteractive)] | ||
| public void DefaultAuthenticationProviders_Interactive(SqlAuthenticationMethod method) | ||
| { | ||
| Assert.IsType<DummySqlAuthenticationProvider>(SqlAuthenticationProvider.GetProvider(method)); |
There was a problem hiding this comment.
Does this test add anything over the other one? It seems we could remove this.
There was a problem hiding this comment.
I added this for future reference, to capture all the expected providers wherever default providers are tested. Doesn't hurt to test more than once.
| [Fact] | ||
| public async Task IsDummySqlAuthenticationProviderSetByDefault() | ||
| { | ||
| var provider = SqlAuthenticationProvider.GetProvider(SqlAuthenticationMethod.ActiveDirectoryInteractive); |
There was a problem hiding this comment.
It would be nice to get the config injected into the SqlAuthenticationProviderManager (maybe via an internal method). I see people doing facade patterns over ConfigurationManager which lets you set config without having to modify a global app.config that is visible to all tests.
We have the same basic issue with AppContext switches. We need to move toward more dependency injection so that we can test our flags/config/switches without impacting test parallelization.
There was a problem hiding this comment.
That is possible, but that scenario doesn't help us with reproducing the issue. Since we are setting the default providers in constructors (which is what we want to test here), the issue is reproduced by loading custom providers via external config file.
Fixes #3618
The issue is reproducible when config file is configured with a custom authentication provider.
Using SetProvider (as in tests) to override default providers does not reproduce issue, hence missed.
The fix will be back-ported to other affected branches (release/6.0 and 6.1).