Enable WAM Broker support for Entra ID Auth modes#4288
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds WAM broker support to the Azure extension authentication provider for Entra ID public-client authentication flows on Windows.
Changes:
- Adds
Microsoft.Identity.Client.Brokerdependency and broker configuration during PCA creation. - Adds parent-window callback API and Windows interop helpers for WAM prompt parenting.
- Adds basic tests and a draft feature spec for WAM broker support.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Directory.Packages.props |
Adds centralized MSAL broker package version. |
src/Microsoft.Data.SqlClient.Extensions/Azure/src/Azure.csproj |
References the broker package from the Azure extension. |
src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs |
Makes the provider partial, adds parent-window callback API, and configures WAM broker on Windows. |
src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.Windows.cs |
Adds Windows-specific parent-window discovery for broker UI. |
src/Microsoft.Data.SqlClient.Extensions/Azure/src/Interop/Interop.GetAncestor.cs |
Adds user32.GetAncestor interop helper. |
src/Microsoft.Data.SqlClient.Extensions/Azure/src/Interop/Interop.GetConsoleWindow.cs |
Adds kernel32.GetConsoleWindow interop helper. |
src/Microsoft.Data.SqlClient.Extensions/Azure/test/WamBrokerTests.cs |
Adds basic tests for the new parent-window setter. |
specs/002-wam-broker/spec.md |
Adds the draft WAM broker feature specification. |
Comments suppressed due to low confidence (5)
src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs:766
- The parent-window delegate is captured on the cached PublicClientApplication, but the static PCA cache key does not include the new SetParentActivityOrWindow callback. A later provider instance with the same authority/client/redirect can reuse an app built with an earlier provider's delegate, causing WAM prompts to be parented to the wrong window (or to ignore the later callback). Include the callback identity in the cache key or avoid capturing provider instance state in the cached app.
builder.WithParentActivityOrWindow(GetBrokerParentWindow);
}
#else
builder.WithParentActivityOrWindow(GetBrokerParentWindow);
src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs:752
- The tests added for this feature only exercise the setter and do not verify that Windows PCA construction actually enables the broker or wires the parent-window callback. Please add unit coverage around CreateClientAppInstance (or an observable wrapper) so regressions in the WAM configuration are caught without relying solely on manual integration testing.
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
builder.WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows));
specs/002-wam-broker/spec.md:70
- This API description says Func can return IWin32Window, but the implementation only accepts IntPtr from this callback and silently ignores any other object type before falling back to console detection. Please either document only the supported return type or handle IWin32Window here as described.
// Cross-platform API to set the parent window/activity for WAM dialog // On Windows: accepts IntPtr (window handle) or IWin32Window via Func<object> // On Unix: no-op (WAM not available) public void SetParentActivityOrWindow(Func<object> parentActivityOrWindowFunc);specs/002-wam-broker/spec.md:131
- This rollout note is inaccurate: SetIWin32WindowFunc is not changed to delegate to SetParentActivityOrWindow; it still stores a separate _iWin32WindowFunc and the new callback is independent. Please update the spec or implementation so consumers and maintainers do not rely on a delegation behavior that is not present.
- WAM broker is **always enabled** on Windows when using PCA flows - No opt-in connection string keyword needed (aligns with MSAL PCA compliance requirements) - Existing `SetIWin32WindowFunc` remains as a backward-compatible API on .NET Framework, delegating to `SetParentActivityOrWindow`src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs:749
- This comment overstates the behavior: the code still uses AcquireTokenByIntegratedWindowsAuth, AcquireTokenByUsernamePassword, and AcquireTokenWithDeviceCode for those modes, so broker configuration does not make every supported authentication mode a WAM flow. Please narrow the comment to the modes MSAL will actually broker or update the acquisition paths accordingly.
// Enable WAM broker on Windows for all supported authentication modes. // The broker provides enhanced security by enabling device-based Conditional Access // policies through the Windows Account Manager (WAM).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- doc/apps/AzureSqlConnector/MainForm.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
doc/apps/AzureSqlConnector/README.md:110
- The last two
dotnet runlines appear duplicated and are not formatted as part of the Notes list or a code block. This looks like leftover copy/paste noise and should be removed (or moved into the earlier Build & Run section as a single example).
developer overrides).
- This is a sample / diagnostic tool, **not** a product. It does not persist credentials.
dotnet run --project .\doc\apps\AzureSqlConnector\AzureSqlConnector.csproj
dotnet run --project .\doc\apps\AzureSqlConnector\AzureSqlConnector.csproj
…et/SqlClient into dev/cheena/entra-wam-broker
cheenamalhotra
added
the
Hotfix 7.0.2
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- doc/apps/AzureSqlConnector/MainForm.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
doc/apps/AzureSqlConnector/README.md:109
- These two trailing
dotnet runlines are duplicated and not formatted as a code block; the build/run instructions already appear earlier in the README.
dotnet run --project .\doc\apps\AzureSqlConnector\AzureSqlConnector.csproj
specs/002-wam-broker/spec.md:132
- The spec claims
SetIWin32WindowFuncdelegates toSetParentActivityOrWindow, but the current code keepsSetIWin32WindowFuncas a separate setting. Either implement that delegation or update this statement to avoid promising behavior that doesn't exist.
- WAM broker is **always enabled** on Windows when using PCA flows
- No opt-in connection string keyword needed (aligns with MSAL PCA compliance requirements)
- Existing `SetIWin32WindowFunc` remains as a backward-compatible API on .NET Framework, delegating to `SetParentActivityOrWindow`
Co-authored-by: cheenamalhotra <13396919+cheenamalhotra@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4288 +/- ##
==========================================
- Coverage 66.50% 64.16% -2.35%
==========================================
Files 285 280 -5
Lines 43311 66162 +22851
==========================================
+ Hits 28806 42454 +13648
- Misses 14505 23708 +9203
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| /// <include file='../doc/ActiveDirectoryAuthenticationProvider.xml' path='docs/members[@name="ActiveDirectoryAuthenticationProvider"]/AcquireTokenAsync/*'/> | ||
| public override async Task<SqlAuthenticationToken> AcquireTokenAsync(SqlAuthenticationParameters parameters) |
There was a problem hiding this comment.
What stance have you taken on synchronizing this action with the main UI thread? Will we leave it broken for UI apps?
We can pop a non-system browser window without being on the main thread, but the system browser and WAM picker I believe do require the main UI thread. We should test with winforms / VS extension / others. I previously had to expose an API to pass SynchronizationContext: https://github.com/dotnet/SqlClient/pull/3874/changes#diff-7b20b6d8d7c505871ab9d3762e51086d6848bae49bf6864f8e5b5adfc3070846R318
We should also test calling SqlConnection.Open() and OpenAsync() directly from the main thread to ensure we don't introduce any deadlock.
There was a problem hiding this comment.
The sample WinForms app I added uses system browser and doesn't seem to go in a deadlock. Have you tried that?
| string serverVersion; | ||
| using (SqlConnection connection = new SqlConnection(builder.ConnectionString)) | ||
| { | ||
| await connection.OpenAsync().ConfigureAwait(true); |
There was a problem hiding this comment.
Good to see that this works. Have you tested the sync API?
Also looking for a test that covers opening on a worker thread.
There was a problem hiding this comment.
Sync API works too in my sample, tested.
There was a problem hiding this comment.
it'd be good for this app to have 2 buttons, one for sync .Open and one for async. SSMS is still only using synchronous calls on the UI thread, so evidence it will work there is helpful.
There was a problem hiding this comment.
what changed in the core driver that unblocks using the synchronous SqlConnection.Open call on the UI thread? How does MSAL invoke the dialog open back to the UI thread while the driver is blocking, waiting for the token acquisition? Can you capture a call stack at the time the MSAL window is displayed during the synchronous call? I'm very curious what it looks like.
There was a problem hiding this comment.
MSAL calls WithBroker(...) and routes through Microsoft.Identity.Client.NativeInterop → WAM (Microsoft.AAD.BrokerPlugin). WAM runs in a separate process. We pass a window handle via WithParentActivityOrWindow(GetParentWindow()) (resolved in ActiveDirectoryAuthenticationProvider.Windows.cs) purely so WAM can parent its dialog to our app for modality / Z-order / taskbar grouping. The dialog itself is drawn entirely by WAM in its own process; our UI thread just blocks on the WAM IPC call. No in-process message pump is required, so sync Open() on the UI thread is safe in this mode.
The stacktrace goes like this:
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Overall the changes look good. The test app is a great idea. My comments are mostly to tighten up the API and docs and add some missing test coverage.
There was a problem hiding this comment.
Hmm... why do we need this file at all? Who consumes this? Just curious - it's an existing problem, so not necessary to fix in this PR.
There was a problem hiding this comment.
I was curious too, needs a cleanup I suppose.
This should be removed IMO - I wanted to verify before I hit the delete button.
Likewise, there are many other xml files left overs in the root doc folder that need to be cleaned up. This should be done separately.
| </example> | ||
| </SetDeviceCodeFlowCallback> | ||
| <SetParentActivityOrWindowFunc> | ||
| <SetParentActivityOrWindow> |
There was a problem hiding this comment.
Interesting - the SetParentActivityOrWindowsFunc method didn't actually exist on the class. This PR is effectively removing that imagined method, and adding the new SetParentActivityOrWindow method.
There was a problem hiding this comment.
This API was introduced for .NET Standard only, but was removed when .NET Standard support was dropped I think. It is added back now for both .NET Standard and .NET Framework this time. I've changed to name to SetParentActivityOrWindowFunc again to retain Func suffix.
|
|
||
| namespace Microsoft.Data.SqlClient; | ||
|
|
||
| internal static partial class Interop |
There was a problem hiding this comment.
Missing class and field docs, explaining why this exists, how/when to use it, etc.
|
|
||
| namespace Microsoft.Data.SqlClient; | ||
|
|
||
| internal static partial class Interop |
There was a problem hiding this comment.
Missing class docs, explaining why this exists, how/when to use it, etc.
|
|
||
| internal static partial class Interop | ||
| { | ||
| internal static partial class User32 |
There was a problem hiding this comment.
Do we typically diverge from file name == class name for these Interop things? I do like that the class name implies which Windows DLL it imports, but I'm not sure if that is worth the divergence.
| { | ||
| // A custom application id without explicitly opting in must NOT enable WAM broker. | ||
| string customAppId = Guid.NewGuid().ToString(); | ||
| var provider = new ActiveDirectoryAuthenticationProvider(customAppId); |
There was a problem hiding this comment.
What if the caller supplies the SqlClient first-party app id here?
Note that this is calling a different constructor than the test on line 80, so it needs a separate test.
| private static bool GetUseWamBrokerField(ActiveDirectoryAuthenticationProvider provider) | ||
| { | ||
| FieldInfo? field = typeof(ActiveDirectoryAuthenticationProvider) | ||
| .GetField("_useWamBroker", BindingFlags.Instance | BindingFlags.NonPublic); |
There was a problem hiding this comment.
Maybe we want internal bool UseWamBroker { get; } on the provider, and then we can access it directly in these tests.
| var provider = new ActiveDirectoryAuthenticationProvider( | ||
| deviceCodeFlowCallbackMethod: static _ => Task.CompletedTask, | ||
| applicationClientId: customAppId, | ||
| useWamBroker: true); |
There was a problem hiding this comment.
Missing the case where useWamBroker is false.
| // A provider created with useWamBroker=true and registered via SqlAuthenticationProvider | ||
| // must retain its WAM broker setting after registration (i.e. SetProvider must not wrap | ||
| // or replace the instance). | ||
| string customAppId = Guid.NewGuid().ToString(); |
There was a problem hiding this comment.
We should probably assert that the guid isn't the SqlClient app id. Here and elsewhere.
| { | ||
| if (original is not null) | ||
| { | ||
| SqlAuthenticationProvider.SetProvider(SqlAuthenticationMethod.ActiveDirectoryInteractive, original); |
There was a problem hiding this comment.
I have wondered why we don't support un-setting a provider. This test has now polluted the global state if ActiveDirectoryInteractive didn't have a provider registered before.
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
|
Adding @shueybubbles for visibility on the PR. |
| /// connection string via <see cref="SqlConnectionStringBuilder"/>, and optionally tests | ||
| /// connectivity using <see cref="SqlConnection"/>. | ||
| /// </summary> | ||
| public partial class MainForm : Form |
| <PackageReference Include="Microsoft.Extensions.Caching.Memory" /> | ||
| <!-- Explicitly depend on the same version of Microsoft.Identity.Client as SqlClient. --> | ||
| <PackageReference Include="Microsoft.Identity.Client" /> | ||
| <PackageReference Include="Microsoft.Identity.Client.Broker" /> |
| // Maintain a flag to disable WAM broker mode for backwards compatibility | ||
| // in applications using ActiveDirectoryAuthenticationProvider with custom Application Client ID. | ||
| private readonly bool _useWamBroker = false; | ||
| private const string s_wamBrokerRedirectUriPrefix = "ms-appx-web://microsoft.aad.brokerplugin/"; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 4 comments.
Files not reviewed (2)
- doc/apps/AzureSqlConnector/MainForm.Designer.cs: Generated file
- doc/apps/AzureSqlConnector/MainFormWorker.Designer.cs: Generated file
| <PropertyGroup> | ||
| <TargetFrameworks Condition="'$(OS)' == 'Windows_NT'">net481;net10.0-windows</TargetFrameworks> | ||
| <TargetFramework Condition="'$(OS)' != 'Windows_NT'">net10.0-windows</TargetFramework> | ||
| <OutputType>WinExe</OutputType> | ||
| <RootNamespace>Microsoft.Data.SqlClient.Samples.AzureSqlConnector</RootNamespace> | ||
| <AssemblyName>AzureSqlConnector</AssemblyName> | ||
| <UseWindowsForms>true</UseWindowsForms> | ||
| <LangVersion>latest</LangVersion> | ||
| <Nullable>disable</Nullable> | ||
| <Platforms>AnyCPU</Platforms> |
| /// <include file='../doc/ActiveDirectoryAuthenticationProvider.xml' path='docs/members[@name="ActiveDirectoryAuthenticationProvider"]/ctorOptions/*'/> | ||
| public ActiveDirectoryAuthenticationProvider(ProviderOptions options) | ||
| { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- doc/apps/AzureSqlConnector/MainForm.Designer.cs: Generated file
- doc/apps/AzureSqlConnector/MainFormWorker.Designer.cs: Generated file
| } | ||
| #if NETFRAMEWORK | ||
| if (publicClientAppKey.IWin32WindowFunc is not null) | ||
| else if (publicClientAppKey.IWin32WindowFunc is not null) | ||
| { | ||
| // Not on Windows (shouldn't happen for NETFRAMEWORK, but be defensive). |
| var provider = new ActiveDirectoryAuthenticationProvider(); | ||
|
|
||
| Assert.True(provider.UseWamBroker, | ||
| "Default ctor must enable WAM broker (uses SqlClient first-party application id)."); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- doc/apps/AzureSqlConnector/MainForm.Designer.cs: Generated file
- doc/apps/AzureSqlConnector/MainFormWorker.Designer.cs: Generated file
| /// <include file='../doc/ActiveDirectoryAuthenticationProvider.xml' path='docs/members[@name="ActiveDirectoryAuthenticationProvider"]/ctorOptions/*'/> | ||
| public ActiveDirectoryAuthenticationProvider(ProviderOptions options) | ||
| { | ||
| if (options is null) | ||
| { | ||
| _applicationClientId = applicationClientId; | ||
| throw new ArgumentNullException(nameof(options)); | ||
| } | ||
|
|
||
| _deviceCodeFlowCallback = options.DeviceCodeFlowCallback ?? DefaultDeviceFlowCallback; | ||
| if (options.ApplicationClientId is not null) | ||
| { | ||
| _applicationClientId = options.ApplicationClientId; | ||
| } | ||
| // WAM broker mode is always enabled for the SqlClient first-party application id (its | ||
| // app registration is configured for the WAM broker redirect URI). For a caller-supplied | ||
| // application id, WAM is opt-in via ProviderOptions.UseWamBroker. | ||
| _useWamBroker = _applicationClientId == s_sqlClientApplicationId || options.UseWamBroker; | ||
| } |
| // The SqlClient first-party application client id that is hard-coded in the provider. | ||
| private const string SqlClientApplicationId = "2fd908ad-0664-4344-b9be-cd3e8b574c38"; | ||
|
|
||
| /// <summary> | ||
| /// Defensive guard: every test that uses <see cref="Guid.NewGuid"/> as a stand-in for a | ||
| /// caller-supplied application id assumes the GUID will never collide with the SqlClient | ||
| /// first-party id. This test makes that assumption explicit. | ||
| /// </summary> | ||
| [Fact] | ||
| public void Ctor_CustomAppId_GuidIsNotSqlClientAppId() | ||
| { | ||
| for (int i = 0; i < 16; i++) | ||
| { | ||
| Assert.NotEqual(SqlClientApplicationId, Guid.NewGuid().ToString()); | ||
| } | ||
| } |
Enable WAM Broker support for Entra ID Auth modes
Summary
Enables Windows Account Manager (WAM) broker support for interactive Entra ID authentication in
Microsoft.Data.SqlClient.Extensions.Azure. WAM provides a more secure, integrated sign-in experience on Windows (single sign-on, Windows Hello, conditional access) for the following Entra ID auth modes:Behavior
useWamBrokerparameter (defaults to disabled for backward compatibility).New Public APIs
Two new constructor overloads are introduced on
Microsoft.Data.SqlClient.ActiveDirectoryAuthenticationProvider(inMicrosoft.Data.SqlClient.Extensions.Azure). These are added as overloads — rather than adding optional parameters to the existing constructors — to preserve binary compatibility for already-compiled consumers:Existing constructors are unchanged and remain fully source- and binary-compatible:
Usage
Changes
ActiveDirectoryAuthenticationProvider.Windows.cs, newInterop.GetAncestor/Interop.GetConsoleWindow).WamBrokerTests.cscovering built-in vs. custom client id behavior withuseWamBrokertrue/false.Testing
WamBrokerTests.cs)