Skip to content

Tests | Cleanup AAD Password auth from tests + code cleanup #4390

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

Open
cheenamalhotra wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Tests | Cleanup AAD Password auth from tests + code cleanup #4390

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bcbac46
Tests | Cleanup AAD Password auth from tests + code cleanup
cheenamalhotra Jun 23, 2026
62e88a8
Fix build
cheenamalhotra Jun 23, 2026
18f879c
Update references
cheenamalhotra Jun 23, 2026
8153957
More updates
cheenamalhotra Jun 23, 2026
29d4617
Fix compiler errors for net462
cheenamalhotra Jun 23, 2026
60262cd
Fix error
cheenamalhotra Jun 23, 2026
7119636
Fix failing test
cheenamalhotra Jun 24, 2026
4185ce8
Fix tests
cheenamalhotra Jun 24, 2026
f8f1c89
Fix TestCustomProviderAuthentication
cheenamalhotra Jun 24, 2026
4b40fcc
Address comments
cheenamalhotra Jun 24, 2026
853ffb6
Remove unused AADAuthorityURL config as well
cheenamalhotra Jun 25, 2026
eb45a36
Changes from feedback
cheenamalhotra Jun 26, 2026
a535d7b
Update Azure config, cleanup
cheenamalhotra Jun 26, 2026
f8e3829
CI changes
cheenamalhotra Jun 26, 2026
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
6 changes: 3 additions & 3 deletions .github/instructions/testing.instructions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ Copy `config.default.jsonc` to `config.jsonc` and configure:

| Property | Description |
|----------|-------------|
| `TCPConnectionString` | Primary TCP connection |
| `NPConnectionString` | Named Pipes connection |
| `AADPasswordConnectionString` | Entra ID password auth |
| `TCPConnectionString` | Primary TCP connection to on-premises SQL Server |
| `NPConnectionString` | Named Pipes connection to on-premises SQL Server |
| `AzureSqlConnectionString` | Entra ID connection string to Azure SQL Database |

@benrr101 benrr101 Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned with making this change.

The PR pipeline uses two different test runs, one for localhost, one for azure. The differentiation is made by running it once with an azure connection string for TCPConnectionString/NPConnectionString and then using a localhost connection string. This allows us to run the same set of tests on both environments, since the tests just specify if it can't run on Azure (or I suppose only run on Azure) based on the TCP/NP connection string that was provided.

Changing this "AADPasswordConnectionString" to "AzureSqlConnectionString" and changing the comments for TCP and NP mixes up the ideas. The new guidance suggests that tests are either azure-specific or localhost-specific. But, aside from the handful being changed in this PR, they are not azure/localhost specific. And, worse, in order to enable azure/localhost specific tests, we would need to duplicate all the existing tests that use TCP/NP.

eg, we'd have to change from

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnectionStringsSetUp))]
public void MyTest() { /* ... test that uses TCPConnectionString ... */ }

to

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsLocalhostConnectionStringsSetUp)]
public void MyTest_Localhost() { /* ... test that uses TCPConnectionString ... */ }

[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.IsAzureConnectionStringsSetUp)]
public void MyTest_Azure() { /* ... test that uses AzureConnectionString ... */ }

That seems like a really bad idea to me.

I've been percolating on a better way for us to handle multiple connection strings for our test scenarios. I'd really like a bit of runway to put together a prototype for how it could be used to improve our manual tests.

But anyhow, my general feedback for this would be ... maybe we shouldn't change the name and guidance on these connection strings right now. Maybe we should just make sure that we eliminate the AAD password auth from the tests, but not go further than that.

@mdaigle mdaigle Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 can we just remove the 'AADPasswordConnectionString'? why do we need a replacement?

@cheenamalhotra cheenamalhotra Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done, I think that's a fair change.

| `AzureKeyVaultURL` | AKV for encryption tests |
| `EnclaveEnabled` | Enable enclave tests |
| `FileStreamDirectory` | FileStream test path |
Expand Down
4 changes: 2 additions & 2 deletions TESTGUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,8 @@ dotnet build -t:TestSqlClientManual -p:TestSet=2
| `TCPConnectionStringAASSGX` | Optional connection string for SQL Server with SGX enclave and Microsoft Azure Attestation. | Include `Attestation Protocol=AAS` and `Enclave Attestation Url`. |
| `EnclaveEnabled` | Enables tests that require an enclave-configured server. | `true` or `false`. |
| `TracingEnabled` | Enables tracing-related tests. | `true` or `false`. |
| `AADAuthorityURL` | Optional OAuth authority for `AADPasswordConnectionString`. | `https://login.windows.net/<tenant>` |
| `AADPasswordConnectionString` | Optional connection string for Microsoft Entra ID password authentication tests. | Uses `Authentication=Active Directory Password`. |
| `AADAuthorityURL` | Optional OAuth authority for `AzureSqlConnectionString`. | `https://login.windows.net/<tenant>` |
| `AzureSqlConnectionString` | Optional connection string for Microsoft Entra ID authentication tests, contains Server and Initial Catalog properties only. | Authentication information and credentials are configured by the test code. |
Comment thread
cheenamalhotra marked this conversation as resolved.
Outdated
| `AADServicePrincipalId` | Optional application ID for service-principal authentication tests. | Former docs may refer to this as a secure principal ID. |
| `AADServicePrincipalSecret` | Optional application secret for service-principal authentication tests. | Keep this only in local, ignored config files or secure pipeline variables. |
| `AzureKeyVaultURL` | Optional Azure Key Vault URL for Always Encrypted tests. | `https://<keyvaultname>.vault.azure.net/` |
Expand Down
4 changes: 2 additions & 2 deletions eng/pipelines/common/templates/jobs/ci-run-tests-job.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,8 @@ jobs:
EnclaveEnabled: ${{ eq(parameters.configProperties.EnclaveEnabled, 'true') }}
${{ if parameters.configProperties.TracingEnabled }}:
TracingEnabled: ${{ eq(parameters.configProperties.TracingEnabled, 'true') }}
${{ if parameters.configProperties.AADPasswordConnectionString }}:
AADPasswordConnectionString: ${{ parameters.configProperties.AADPasswordConnectionString }}
${{ if parameters.configProperties.AzureSqlConnectionString }}:
AzureSqlConnectionString: ${{ parameters.configProperties.AzureSqlConnectionString }}
${{ if parameters.configProperties.AADServicePrincipalId }}:
AADServicePrincipalId: ${{ parameters.configProperties.AADServicePrincipalId }}
${{ if parameters.configProperties.AADServicePrincipalSecret }}:
Expand Down
4 changes: 2 additions & 2 deletions eng/pipelines/common/templates/steps/update-config-file-step.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ parameters:
type: string
default: ''

- name: AADPasswordConnectionString
- name: AzureSqlConnectionString
type: string
default: ''

Expand Down Expand Up @@ -154,7 +154,7 @@ steps:

$p.AADAuthorityURL="${{parameters.AADAuthorityURL }}"

$p.AADPasswordConnectionString="${{parameters.AADPasswordConnectionString }}"
$p.AzureSqlConnectionString="${{parameters.AzureSqlConnectionString }}"

$p.AADServicePrincipalId="${{parameters.AADServicePrincipalId }}"

Expand Down
12 changes: 6 additions & 6 deletions eng/pipelines/dotnet-sqlclient-ci-core.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,14 +504,14 @@ stages:
configProperties:
TCPConnectionString: $(AZURE_DB_TCP_CONN_STRING)
NPConnectionString: $(AZURE_DB_NP_CONN_STRING)
AzureSqlConnectionString: $(AZURE_SQL_CONN_STRING_WestUS2)
AADAuthorityURL: $(AADAuthorityURL)
# Pipeline runs against forks of the repo don't have access to Library secrets, so we
# omit them entirely from the configProperties, which causes the dependent tests to be
# skipped.
${{ if eq(variables['System.PullRequest.IsFork'], 'False') }}:
AADPasswordConnectionString: $(AAD_PASSWORD_CONN_STR)
AADServicePrincipalSecret: $(AADServicePrincipalSecret)
AADServicePrincipalId: $(AADServicePrincipalId)
AADServicePrincipalId: $(AADServicePrincipalId)
AzureKeyVaultUrl: $(AzureKeyVaultUrl)
AzureKeyVaultTenantId: $(AzureKeyVaultTenantId)
SupportsIntegratedSecurity: false
Expand All @@ -535,11 +535,11 @@ stages:
configProperties:
TCPConnectionString: $(AZURE_DB_TCP_CONN_STRING_eastus)
NPConnectionString: $(AZURE_DB_NP_CONN_STRING_eastus)
AzureSqlConnectionString: $(AZURE_SQL_CONN_STRING_EastUS)
AADAuthorityURL: $(AADAuthorityURL)
${{ if eq(variables['System.PullRequest.IsFork'], 'False') }}:
AADPasswordConnectionString: $(AAD_PASSWORD_CONN_STR_eastus)
AADServicePrincipalSecret: $(AADServicePrincipalSecret)
AADServicePrincipalId: $(AADServicePrincipalId)
AADServicePrincipalId: $(AADServicePrincipalId)
AzureKeyVaultUrl: $(AzureKeyVaultUrl)
AzureKeyVaultTenantId: $(AzureKeyVaultTenantId)
SupportsIntegratedSecurity: false
Expand Down Expand Up @@ -586,11 +586,11 @@ stages:
configProperties:
TCPConnectionString: $(AZURE_DB_TCP_CONN_STRING)
NPConnectionString: $(AZURE_DB_NP_CONN_STRING)
AzureSqlConnectionString: $(AZURE_SQL_CONN_STRING_WestUS2)
AADAuthorityURL: $(AADAuthorityURL)
${{ if eq(variables['System.PullRequest.IsFork'], 'False') }}:
AADPasswordConnectionString: $(AAD_PASSWORD_CONN_STR)
AADServicePrincipalSecret: $(AADServicePrincipalSecret)
AADServicePrincipalId: $(AADServicePrincipalId)
AADServicePrincipalId: $(AADServicePrincipalId)
AzureKeyVaultUrl: $(AzureKeyVaultUrl)
AzureKeyVaultTenantId: $(AzureKeyVaultTenantId)
SupportsIntegratedSecurity: false
Expand Down
5 changes: 2 additions & 3 deletions eng/pipelines/jobs/test-azure-package-ci-job.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,20 +264,19 @@ jobs:
# The config.jsonc file has many options, but only some of them are
# used by the Azure package tests. We only specify the ones that are
# necessary here.

AADServicePrincipalId: $(AADServicePrincipalId)
AzureKeyVaultTenantId: $(AzureKeyVaultTenantId)
# macOS doesn't support managed identities.
ManagedIdentitySupported: ${{ not(eq(parameters.vmImage, 'macos-latest')) }}
SupportsIntegratedSecurity: ${{ parameters.supportsIntegratedSecurity }}
TCPConnectionString: $(AZURE_DB_TCP_CONN_STRING)
AzureSqlConnectionString: $(AZURE_SQL_CONN_STRING_WestUS2)
UserManagedIdentityClientId: $(UserManagedIdentityClientId)
WorkloadIdentityFederationServiceConnectionId: $(WorkloadIdentityFederationServiceConnectionId)
# Avoid exposing secrets to pipeline jobs triggered via forks. This
# prevents external contributors from creating PRs and running
# pipelines that could expose these secrets.
${{ if eq(variables['System.PullRequest.IsFork'], 'False') }}:
AADPasswordConnectionString: $(AAD_PASSWORD_CONN_STR)
AADServicePrincipalId: $(AADServicePrincipalId)
AADServicePrincipalSecret: $(AADServicePrincipalSecret)

# Perform any local SQL Server setup.
Expand Down
125 changes: 50 additions & 75 deletions src/Microsoft.Data.SqlClient.Extensions/Azure/test/AADConnectionTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// new unit and/or integration tests in the future.

using System.Text.RegularExpressions;
using Microsoft.Data.SqlClient.Tests.Common;

namespace Microsoft.Data.SqlClient.Extensions.Azure.Test;

Expand All @@ -28,43 +29,49 @@ public static void KustoDatabaseTest()

[ConditionalFact(
typeof(Config),
nameof(Config.HasPasswordConnectionString),
nameof(Config.HasAzureSqlConnectionString),
nameof(Config.HasServicePrincipal))]
public static void NoCredentialsActiveDirectoryServicePrincipal()
{
// test Passes with correct connection string.
string[] removeKeys = { "Authentication", "User ID", "Password", "UID", "PWD" };
string connStr = RemoveKeysInConnStr(Config.PasswordConnectionString, removeKeys) +
$"Authentication=Active Directory Service Principal; User ID={Config.ServicePrincipalId}; PWD={Config.ServicePrincipalSecret};";
ConnectAndDisconnect(connStr);
string connString = Config.AzureSqlConnString

@mdaigle mdaigle Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can just modify the TCP connection string as needed in each test.

@cheenamalhotra cheenamalhotra Jun 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

.AddServicePrincipalAuthenticationToConnString()
.AddUserToConnString(Config.ServicePrincipalId)
.AddPasswordToConnString(Config.ServicePrincipalSecret);

ConnectAndDisconnect(connString);

// connection fails with expected error message.
string[] credKeys = { "Authentication", "User ID", "Password", "UID", "PWD" };
string connStrWithNoCred = RemoveKeysInConnStr(Config.PasswordConnectionString, credKeys) +
"Authentication=Active Directory Service Principal;";
InvalidOperationException e = Assert.Throws<InvalidOperationException>(() => ConnectAndDisconnect(connStrWithNoCred));
string connStrWithNoCred = Config.AzureSqlConnString
.AddServicePrincipalAuthenticationToConnString();

InvalidOperationException e = Assert.Throws<InvalidOperationException>
(() => ConnectAndDisconnect(connStrWithNoCred));

string expectedMessage = "Either Credential or both 'User ID' and 'Password' (or 'UID' and 'PWD') connection string keywords must be specified, if 'Authentication=Active Directory Service Principal'.";
Assert.Contains(expectedMessage, e.Message);
}

[ConditionalTheory(
typeof(Config),
nameof(Config.HasPasswordConnectionString),
nameof(Config.HasAzureSqlConnectionString),
nameof(Config.HasUserManagedIdentityClientId))]
[InlineData("2445343 2343253")]
[InlineData("2445343$#^@@%2343253")]
public static void ActiveDirectoryManagedIdentityWithInvalidUserIdMustFail(string userId)
{
// connection fails with expected error message.
string[] credKeys = { "Authentication", "User ID", "Password", "UID", "PWD" };
string connStrWithNoCred = RemoveKeysInConnStr(Config.PasswordConnectionString, credKeys) +
$"Authentication=Active Directory Managed Identity; User Id={userId}";
string connStrWithNoCred = Config.AzureSqlConnString
.AddManagedIdentityAuthenticationToConnString()
.AddUserToConnString(userId);

SqlException e = Assert.Throws<SqlException>(() => ConnectAndDisconnect(connStrWithNoCred));

// The Azure.Identity surface message can vary across SDK versions and
// platforms, so assert on the stable driver-emitted error that proves
// the managed-identity auth path was taken and failed.
Regex expected = new(
@"(\[Managed Identity\]|ManagedIdentityCredential) Authentication unavailable",
@"Failed to authenticate the user.*Authentication=ActiveDirectoryManagedIdentity",
RegexOptions.IgnoreCase);

Assert.Matches(expected, e.GetBaseException().Message);
Expand All @@ -73,13 +80,13 @@ public static void ActiveDirectoryManagedIdentityWithInvalidUserIdMustFail(strin
[ConditionalFact(
typeof(Config),
nameof(Config.OnAdoPool),
nameof(Config.HasPasswordConnectionString),
nameof(Config.HasAzureSqlConnectionString),
nameof(Config.HasUserManagedIdentityClientId))]
public static void ActiveDirectoryDefaultMustPass()
{
string[] credKeys = { "Authentication", "User ID", "Password", "UID", "PWD" };
string connStr = RemoveKeysInConnStr(Config.PasswordConnectionString, credKeys) +
$"Authentication=ActiveDirectoryDefault;User ID={Config.UserManagedIdentityClientId};";
string connStr = Config.AzureSqlConnString
.AddAADDefaultAuthenticationToConnString()
.AddUserToConnString(Config.UserManagedIdentityClientId);

// Connection should be established using Managed Identity by default.
ConnectAndDisconnect(connStr);
Expand All @@ -105,35 +112,36 @@ public static void ActiveDirectoryDefaultMustPass()
public static void ADIntegratedUsingSSPI()
{
// test Passes with correct connection string.
string[] removeKeys = { "Authentication", "User ID", "Password", "UID", "PWD", "Trusted_Connection", "Integrated Security" };
string connStr = RemoveKeysInConnStr(Config.TcpConnectionString, removeKeys) +
$"Authentication=Active Directory Integrated;";
string connStr = Config.TcpConnectionString
.RemoveAuthAndCredsProperties()
.AddAADIntegratedAuthenticationToConnString();
ConnectAndDisconnect(connStr);
}

[ConditionalFact(
typeof(Config),
nameof(Config.SupportsManagedIdentity),
nameof(Config.SupportsSystemAssignedManagedIdentity),
nameof(Config.HasPasswordConnectionString))]
nameof(Config.HasAzureSqlConnectionString))]
public static void SystemAssigned_ManagedIdentityTest()
{
string[] removeKeys = { "Authentication", "User ID", "Password", "UID", "PWD" };
string connStr = RemoveKeysInConnStr(Config.PasswordConnectionString, removeKeys) +
$"Authentication=Active Directory Managed Identity;";
string connStr = Config.AzureSqlConnString
.AddManagedIdentityAuthenticationToConnString();

ConnectAndDisconnect(connStr);
}

[ConditionalFact(
typeof(Config),
nameof(Config.OnAdoPool),
nameof(Config.HasPasswordConnectionString),
nameof(Config.HasAzureSqlConnectionString),
nameof(Config.HasUserManagedIdentityClientId))]
public static void UserAssigned_ManagedIdentityTest()
{
string[] removeKeys = { "Authentication", "User ID", "Password", "UID", "PWD" };
string connStr = RemoveKeysInConnStr(Config.PasswordConnectionString, removeKeys) +
$"Authentication=Active Directory Managed Identity; User Id={Config.UserManagedIdentityClientId};";
string connStr = Config.AzureSqlConnString
.AddManagedIdentityAuthenticationToConnString()
.AddUserToConnString(Config.UserManagedIdentityClientId);

ConnectAndDisconnect(connStr);
}

Expand All @@ -145,16 +153,14 @@ public static void UserAssigned_ManagedIdentityTest()
nameof(Config.IsAzureSqlServer))]
public static void Azure_SystemManagedIdentityTest()
{
string[] removeKeys = { "Authentication", "User ID", "Password", "UID", "PWD", "Trusted_Connection", "Integrated Security" };
string connectionString = RemoveKeysInConnStr(Config.TcpConnectionString, removeKeys)
+ $"Authentication=Active Directory Managed Identity;";
string connectionString = Config.TcpConnectionString
.RemoveAuthAndCredsProperties()
.AddManagedIdentityAuthenticationToConnString();

using (SqlConnection conn = new SqlConnection(connectionString))
{
conn.Open();
using SqlConnection conn = new(connectionString);
conn.Open();

Assert.Equal(System.Data.ConnectionState.Open, conn.State);
}
Assert.Equal(System.Data.ConnectionState.Open, conn.State);
}

[ConditionalFact(
Expand All @@ -166,16 +172,15 @@ public static void Azure_SystemManagedIdentityTest()
nameof(Config.IsAzureSqlServer))]
public static void Azure_UserManagedIdentityTest()
{
string[] removeKeys = { "Authentication", "User ID", "Password", "UID", "PWD", "Trusted_Connection", "Integrated Security" };
string connectionString = RemoveKeysInConnStr(Config.TcpConnectionString, removeKeys)
+ $"Authentication=Active Directory Managed Identity; User Id={Config.UserManagedIdentityClientId}";
string connectionString = Config.TcpConnectionString
.RemoveAuthAndCredsProperties()
.AddManagedIdentityAuthenticationToConnString()
.AddUserToConnString(Config.UserManagedIdentityClientId);

using (SqlConnection conn = new SqlConnection(connectionString))
{
conn.Open();
using SqlConnection conn = new(connectionString);
conn.Open();

Assert.Equal(System.Data.ConnectionState.Open, conn.State);
}
Assert.Equal(System.Data.ConnectionState.Open, conn.State);
}

// The helpers below were copied verbatim from AADConnectionTest.cs and ManualTests
Expand Down Expand Up @@ -203,36 +208,6 @@ private static void ConnectAndDisconnect(

#region Helpers from ManualTests DataTestUtility.cs

public static string RemoveKeysInConnStr(string connStr, string[] keysToRemove)
{
// tokenize connection string and remove input keys.
string res = "";
if (connStr != null && keysToRemove != null)
{
string[] keys = connStr.Split(';');
foreach (var key in keys)
{
if (!string.IsNullOrEmpty(key.Trim()))
{
bool removeKey = false;
foreach (var keyToRemove in keysToRemove)
{
if (key.Trim().ToLower().StartsWith(keyToRemove.Trim().ToLower(), StringComparison.Ordinal))
{
removeKey = true;
break;
}
}
if (!removeKey)
{
res += key + ";";
}
}
}
}
return res;
}

public static string? FetchKeyInConnStr(string connStr, string[] keys)
{
// tokenize connection string and find matching key
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.Data.SqlClient.Extensions/Azure/test/Azure.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@
Condition="'$(ReferenceType)' == 'Package'"/>

<ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient.Extensions/Azure/src/Azure.csproj" />

<!-- Shared test helpers (StringExtensions for composing AAD connection strings, etc.) -->
<ProjectReference Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/Common/Microsoft.Data.SqlClient.TestCommon.csproj" />
</ItemGroup>

<!-- References for netfx -->
Expand Down
Loading
Loading