Skip to content
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

API | AccessTokenCallback support #1260

Merged
merged 41 commits into from Jun 27, 2023

Conversation

christothes
Copy link
Contributor

@christothes christothes commented Sep 10, 2021

This feature adds support for TokenCredential authentication in SqlClient without exposing the type in the core package.

This allows developers to use the full fidelity of the TokenCredential abstraction in Azure.Core including all the specific implementations from Azure.Identity.

This approach allows a new AccessTokenCallback property to be set on the SqlConnection which is a delegate that will return an access token. The implementation for the delegate could be anything, but the below example shows usage of the DefaultAzureCredential

Example usage:

var cred = new DefaultAzureCredential();
var myTokenCallback = async (ctx, cancellationToken) =>
{
	string scope = ctx.Resource.EndsWith(s_defaultScopeSuffix) ? ctx.Resource : ctx.Resource + s_defaultScopeSuffix;
	var token = await cred.GetTokenAsync(new TokenRequestContext(new[] { scope }), cancellationToken);
	return new SqlAuthenticationToken(token.Token, token.ExpiresOn);
}
using (SqlConnection sqlConnection = new SqlConnection(
    "Server=name.database.windows.net;Database=db;")
        { AccessTokenCallback = myTokenCallback })
{
	sqlConnection.Open();
	Console.WriteLine("Connected successfully!")
}

Can you cover the advantage of using the AccessTokenCallback over SqlAuthenticationProvider implementation in terms of access token/pool key and why should this be needed over the provider implementation - I recall there was a discussion long time ago, but let's capture that in this PR description?

This new approach is much simpler to implement and allows us to track in a direction that eliminates a static dependency on Azure.Identity for "Authentication=ActiveDirectoryDefault". In addition it allows ultimate flexibility for DefaultAzureCredential (or any TokenCredential implementation) scenarios such as allowing arbitrary options and logging configuration.

@christothes
Copy link
Contributor Author

Discussion update - we'll refine further by implementing the delegate without the need for a SqlAuthenticationProvider using the hash of the delegate as the connection pool key.

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Patch coverage: 79.76% and project coverage change: -0.65 ⚠️

Comparison is base (9ec1f21) 70.59% compared to head (67bfd01) 69.94%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1260      +/-   ##
==========================================
- Coverage   70.59%   69.94%   -0.65%     
==========================================
  Files         305      305              
  Lines       61820    61934     +114     
==========================================
- Hits        43639    43320     -319     
- Misses      18181    18614     +433     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.42% <78.35%> (+0.09%) ⬆️
netfx 68.16% <81.48%> (-1.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t/src/Microsoft/Data/Common/AdapterUtil.Windows.cs 66.66% <ø> (ø)
...SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs 96.25% <ø> (ø)
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 68.42% <62.50%> (+0.38%) ⬆️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 68.33% <70.58%> (+0.18%) ⬆️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 73.78% <75.51%> (+0.08%) ⬆️
...SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs 68.50% <80.00%> (+0.25%) ⬆️
...core/src/Microsoft/Data/SqlClient/SqlConnection.cs 79.45% <85.18%> (+0.22%) ⬆️
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs 60.65% <85.18%> (+0.47%) ⬆️
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs 68.85% <100.00%> (ø)
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs 64.39% <100.00%> (ø)
... and 2 more

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

christothes and others added 3 commits March 10, 2023 17:25
Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can eliminate the need for a new Authentication option by simply keying off whether AccessTokenCallback is null or not. So the connection string/example usage would be simplified:

var cred = new DefaultAzureCredential();
var myTokenCallback = async (ctx, cancellationToken) =>
{
	string scope = ctx.Resource.EndsWith(s_defaultScopeSuffix) ? ctx.Resource : ctx.Resource + s_defaultScopeSuffix;
	var token = await cred.GetTokenAsync(new TokenRequestContext(new[] { scope }), cancellationToken);
	return new SqlAuthenticationToken(token.Token, token.ExpiresOn);
}
using (SqlConnection sqlConnection = new SqlConnection(
    "Server=name.database.windows.net;Database=db;")
        { AccessTokenCallback = myTokenCallback })
{
	sqlConnection.Open();
	Console.WriteLine("Connected successfully!")
}

.gitignore Outdated Show resolved Hide resolved
christothes and others added 7 commits June 6, 2023 16:17
Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>
Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>
Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the current state of this feature. It'll be nice to get this one merged.

@cheenamalhotra in case you want to give it another quick once over.

@DavoudEshtehari DavoudEshtehari changed the title AccessTokenCallback support API | AccessTokenCallback support Jun 22, 2023
SqlClient v5.2 automation moved this from In progress to Reviewer approved Jun 22, 2023
@DavoudEshtehari DavoudEshtehari merged commit 8fad4a4 into dotnet:main Jun 27, 2023
132 checks passed
SqlClient v5.2 automation moved this from Reviewer approved to Done Jun 27, 2023
SqlClient v5.2 automation moved this from Done to Reviewer approved Jun 27, 2023
@aburakab
Copy link

aburakab commented Apr 29, 2024

Currently, I'm using SqlClient to connect to Dynamics 365 using Service Principal Credentials.
After the latest update for the Azure.Identity Nuget package. It's impossible to use SqlClient or maybe an extra step should be added for the Access Token, as I can see here for the Default Azure Credentials.
I added some details in this Stackoverflow question

@JRahnama
Copy link
Member

Currently, I'm using SqlClient to connect to Dynamics 365 using Service Principal Credentials. After the latest update for the Azure.Identity Nuget package. It's impossible to use SqlClient or maybe an extra step should be added for the Access Token, as I can see here for the Default Azure Credentials. I added some details in this Stackoverflow question

Two questions:

  1. Have you tried to use the latest stable release of Azure.Identity (1.11.2)?
  2. Can you provide a sample repro please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 Public API Use this label for new API additions to the driver.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants