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.

Sign up for GitHub

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

Update Azure SQL Authentication to use Azure.Identity #14

Merged
merged 7 commits into from
Sep 30, 2024
Merged

Update Azure SQL Authentication to use Azure.Identity #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6b18efc
Replace deprecated package Microsoft.Azure.Services.AppAuthentication…
evochriso Feb 12, 2024
aca47da
Merge branch 'main' into main
droyad Jul 26, 2024
b6d4b73
Merge branch 'main' of https://github.com/evochriso/dbup-sqlserver in…
droyad Jul 26, 2024
a78bf85
Passed `TokenCredential` in for Azure connections
droyad Jul 26, 2024
0cbdb1f
Undid accidental downgrade
droyad Jul 26, 2024
373de57
Removed unneeded reference
droyad Jul 27, 2024
b88e44a
Upgraded to SqlClient 5.2.2
droyad Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/dbup-sqlserver/AzureSqlConnectionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
using DbUp.Support;

#if SUPPORTS_AZURE_AD
using Microsoft.Azure.Services.AppAuthentication;
using Azure.Identity;
using Azure.Core;

namespace DbUp.SqlServer
{
Expand All @@ -28,12 +29,11 @@ public AzureSqlConnectionManager(string connectionString, string resource)
public AzureSqlConnectionManager(string connectionString, string resource, string tenantId, string azureAdInstance = "https://login.microsoftonline.com/")
: base(new DelegateConnectionFactory((log, dbManager) =>
{
var tokenProvider = new DefaultAzureCredential();
Copy link

Choose a reason for hiding this comment

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

Having the caller pass an instance of TokenCredential would be optimal as DefaultAzureCredential iterates over all possible credential types until it fines one that works.
If this is going to be released on the next major, perhaps add this as a parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Swampen my goal with this PR was to resolve the usage of a deprecated Nuget package, without introducing a breaking change. In my opinion, using the DefaultAzureCredential here makes sense so we can be as flexible as possible, and overhead of checking all possible types of credentials seems negligible for a database upgrader that will just make one connection to perform the upgrade. Changing or adding the constructors for the AzureSqlConnectionManager class to accept other parameters such as TokenCredential would be a breaking change.
I don't know what @droyad has planned for the next major release, but I agree we should change constructors in that release.

var tokenContext = new TokenRequestContext(scopes: new string[] { resource + "/.default" }, tenantId: tenantId);
var conn = new SqlConnection(connectionString)
{
AccessToken = new AzureServiceTokenProvider(azureAdInstance: azureAdInstance).GetAccessTokenAsync(resource, tenantId)
.ConfigureAwait(false)
.GetAwaiter()
.GetResult()
AccessToken = tokenProvider.GetToken(tokenContext).Token
};

if (dbManager.IsScriptOutputLogged)
Expand Down
10 changes: 5 additions & 5 deletions src/dbup-sqlserver/dbup-sqlserver.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,21 @@
<ItemGroup Condition="'$(TargetFramework)' == 'net462'">
<Reference Include="System.Data" />
<Reference Include="System" />
<PackageReference Include="Microsoft.Azure.Services.AppAuthentication" Version="1.6.2" />
<PackageReference Include="Azure.Identity" Version="1.10.4" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' ">
<PackageReference Include="System.Data.SqlClient" Version="4.8.6" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
<PackageReference Include="System.Data.SqlClient" Version="4.8.6" />
<PackageReference Include="Microsoft.Azure.Services.AppAuthentication" Version="1.6.2" />
<PackageReference Include="Azure.Identity" Version="1.10.4" />
</ItemGroup>

<ItemGroup Condition=" '$(TargetFramework)' == 'net6.0' ">
<PackageReference Include="Microsoft.Data.SqlClient" Version="5.1.4" />
<PackageReference Include="Microsoft.Azure.Services.AppAuthentication" Version="1.6.2" />
<PackageReference Include="Microsoft.Data.SqlClient" Version="5.1.5" />
<PackageReference Include="Azure.Identity" Version="1.10.4" />
</ItemGroup>

<PropertyGroup Condition="'$(TargetFramework)' == 'net462'">
Expand Down
Loading