-
Notifications
You must be signed in to change notification settings - Fork 286
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
Feature | Add support for user-defined ApplicationClientId #740
Conversation
03a36c4
to
dca5f8f
Compare
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 am curious to know whether you've found the current code structure, with separate .cs files for the various target platforms, and separate skeleton DLLs under /ref, to be truly sustainable.
I perceive it as requiring a lot of duplicate coding with a high risk of bugs showing up in one platform and not the other.
For SMO I took the approach of using inline '#if' directives to fork code based on the target platform, and the DLLs under /lib in the nuget package are what clients use for reference as well.
What advantages did you get with the separate CS files that I am missing?
doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml
Outdated
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml
Outdated
Show resolved
Hide resolved
...qlClient/netcore/src/Microsoft/Data/SqlClient/SqlAuthenticationProviderManager.NetCoreApp.cs
Outdated
Show resolved
Hide resolved
The reason we have separate code base is because they came from different sources when the code was imported and there were a lot of new features coming in .NET Core codebase, which pushed merge activity to lower priority... We're trying to integrate it to common location, but it is a big activity and does require a dedicated time. Also our NetFx project file (csproj) is not both based on "Microsoft.Net.Sdk" right now, it's based on old MsBuild architecture. It may be possible to resolve all these differences, but effort is not prioritized yet. Regarding cs file differences for different platforms, I think we have a mix and match. Most of the times for large files, we've been using separate files and including them for specific platform in csproj. |
doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml
Outdated
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml
Outdated
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml
Outdated
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml
Outdated
Show resolved
Hide resolved
doc/snippets/Microsoft.Data.SqlClient/ActiveDirectoryAuthenticationProvider.xml
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlUtil.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
f5b05a9
to
c8d7a65
Compare
LGTM, the only thing is about the static constructor. Can we have a more specific message stating the static constructor has caught the exception? Since the static members are made before all other members it is sometimes difficult debugging/troubleshooting the source of the issues they cause. |
The PR enables support for passing "Application Client Id" to the MSAL library via driver for fetching access tokens in order to perform Active Directory authentication. In order to support the same, below new APIs are introduced:
ActiveDirectoryAuthenticationProvider
[Applies to all .NET Platforms (.NET Framework, .NET Core and .NET Standard)]
Usage:
SqlAuthenticationProviderConfigurationSection
andSqlClientAuthenticationProviderConfigurationSection
[Applies to .NET Framework and .NET Core]
Usage:
cc @David-Engel @shueybubbles