feat(datafactory): bridge host auth to DataFactory.MCP.Core via TokenCredential - #2636
Conversation
Register TokenCredential from the host's IAzureTokenCredentialProvider before calling AddDataFactoryMcpServices(). This allows DataFactory commands to automatically use the Fabric MCP Server's authentication instead of requiring a separate login flow. Also bump Microsoft.DataFactory.MCP.Core to 0.21.0-preview which includes the TokenCredentialAuthenticationService adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you for your contribution Ebram-Tawfik! We will review the pull request and get back to you soon. |
There was a problem hiding this comment.
Pull request overview
This PR bridges the Fabric MCP Server’s host authentication into DataFactory.MCP.Core by registering an Azure.Core.TokenCredential in DI derived from the host IAzureTokenCredentialProvider, and updates the DataFactory core package version to pick up the new TokenCredentialAuthenticationService support.
Changes:
- Register
TokenCredentialinDataFactoryAreaSetupso DataFactory services can auto-use host auth. - Bump
Microsoft.DataFactory.MCP.Coredependency from0.20.0-betato0.21.0-preview.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/Fabric.Mcp.Tools.DataFactory/src/DataFactoryAreaSetup.cs | Adds DI bridge from IAzureTokenCredentialProvider to TokenCredential before registering DataFactory MCP services. |
| Directory.Packages.props | Updates Microsoft.DataFactory.MCP.Core to 0.21.0-preview. |
Jon Gallant (jongio)
left a comment
There was a problem hiding this comment.
Two issues with the auth bridging approach that need resolution before this can merge:
-
Singleton caching breaks OBO/remote mode:
IAzureTokenCredentialProvider.GetTokenCredentialAsyncis designed to return per-execution-context credentials (see the interface xmldoc). Caching the first resolvedTokenCredentialin a singleton means all subsequent users share one credential. If the Fabric server ever runs in HTTP mode with OBO, this is a correctness bug. -
Sync-over-async in DI factory:
.GetAwaiter().GetResult()can deadlock on platforms with aSynchronizationContextand is generally an anti-pattern in DI registration.
The fix is to register a delegating TokenCredential subclass that wraps IAzureTokenCredentialProvider and calls through to the provider on each GetToken/GetTokenAsync invocation - no caching, no blocking. Something like:
internal sealed class ProviderDelegatingCredential(IAzureTokenCredentialProvider provider) : TokenCredential
{
public override AccessToken GetToken(TokenRequestContext ctx, CancellationToken ct)
=> GetTokenAsync(ctx, ct).GetAwaiter().GetResult();
public override async ValueTask<AccessToken> GetTokenAsync(TokenRequestContext ctx, CancellationToken ct)
{
var credential = await provider.GetTokenCredentialAsync(tenantId: null, ct);
return await credential.GetTokenAsync(ctx, ct);
}
}Then register: services.TryAddSingleton<TokenCredential, ProviderDelegatingCredential>();
This preserves OBO correctness (provider resolves per-context) and avoids sync-over-async in the factory.
Also: all CI builds are currently failing - worth checking if the 0.21.0-preview package is available on the feed.
8733d21 to
6c42ddf
Compare
Address review feedback from jongio: 1. Replace singleton-cached TokenCredential with ProviderDelegatingCredential that calls IAzureTokenCredentialProvider on each token request, preserving per-execution-context credentials for OBO/multi-user scenarios. 2. Remove sync-over-async (.GetAwaiter().GetResult()) from DI factory by registering the delegating class directly as a singleton. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Verify that: - Each GetTokenAsync call delegates to IAzureTokenCredentialProvider (no credential caching, OBO-safe) - TokenRequestContext is forwarded correctly to the resolved credential Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expose internal ProviderDelegatingCredential to unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Only register ProviderDelegatingCredential when IAzureTokenCredentialProvider is available in the service collection. In CLI contexts like 'tools list', the auth provider isn't registered, causing DI resolution failure. DataFactory.MCP.Core auto-detects TokenCredential in DI and falls back to standalone auth when it's not present, so this is safe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jon Gallant (jongio)
left a comment
There was a problem hiding this comment.
Addresses my previous feedback cleanly. The delegating TokenCredential wrapper is the right pattern - per-call resolution preserves OBO correctness without sync-over-async in the DI factory. Tests are solid (proving no caching + context propagation).
Steven Vukelich (vukelich)
left a comment
There was a problem hiding this comment.
Late suggestions. Main feedback is around the Microsoft.DataFactory.MCP.Core DI extensibility and suggesting a more direct DI contract with the very broad TokenCredential type.
| // avoiding credential caching (which breaks OBO/multi-user). | ||
| if (services.Any(d => d.ServiceType == typeof(IAzureTokenCredentialProvider))) | ||
| { | ||
| services.TryAddSingleton<TokenCredential, ProviderDelegatingCredential>(); |
There was a problem hiding this comment.
Ebram-Tawfik I'd like to provide some suggestions on design improvements to be mindful that TokenCredential can mean different things to different code paths. For example, it's totally reasonable for a remote MCP server to use TokenCredential instances to talk to its own resources (e.g., using its own MI and RBAC to its own Redis cache) versus using TokenCredential instances that reflect accessing the user's intended resource for MCP tool invocations.
That being said, this PR is fine for the state of this repo today because the code only expects downstream authentication to be getting data for the MCP client/user. If/when we add functionality for the MCP server to interact with separate downstream auth credentials, then I'd be a little more worried about this possible pollution of the DI container. Such future functionality could be using a Redis cache to manage long-running or distributed MCP tool work.
(Caveat: technically, the remote MCP server will use the Azure Identity SDK + Microsoft.Identity.Web to authenticate as the Entra app registration for remote MCP server OBO flows. I do not believe those libraries would consume a TokenCredential from DI for that, so I'm not worried on affecting remote scenarios today.)
Does the Microsoft.DataFactory.MCP.Core package have DI configurability to very directly provide TokenCredential, such as through keyed services or named options?
I asked Opus 4.7, and it provided a well-laid out explanation and supports this suggestion for the Microsoft.DataFactory.MCP.Core library. Claude also called out the Azure SDK's similar WithCredential DI extension method to tightly couple an outbound client instance with the credential that client instance should use.
| // in DI — if present, uses it; otherwise falls back to standalone auth. | ||
| // The delegating credential calls through to the provider on each token request, | ||
| // avoiding credential caching (which breaks OBO/multi-user). | ||
| if (services.Any(d => d.ServiceType == typeof(IAzureTokenCredentialProvider))) |
There was a problem hiding this comment.
When did you see cases where IAzureTokenCredentialProvider was not in the DI container? I'd expect that to always be present, but maybe there's a use case I'm not expecting.
There was a problem hiding this comment.
If we expect it to always be there, then I'd recommend a pattern where we throw an exception to fast-fail. Such failures should be caught in any manual or automated testing and never end up in the customer's hands. Catching unexpected state before we complete pull requests is the best time to fix them.
| var context = new TokenRequestContext(["https://api.fabric.microsoft.com/.default"]); | ||
|
|
||
| // Act | ||
| var token1 = await resolved.GetTokenAsync(context, CancellationToken.None); |
There was a problem hiding this comment.
Please do not use CancellationToken.None. Using a meaningful CancellationToken allows for responsive ending of unit tests. As detailed in the CONTRIBUTING.md, use Xunit.TestContext.Current.CancellationToken
…Credential (microsoft#2636) * feat: bridge host auth to DataFactory.MCP.Core via TokenCredential Register TokenCredential from the host's IAzureTokenCredentialProvider before calling AddDataFactoryMcpServices(). This allows DataFactory commands to automatically use the Fabric MCP Server's authentication instead of requiring a separate login flow. Also bump Microsoft.DataFactory.MCP.Core to 0.21.0-preview which includes the TokenCredentialAuthenticationService adapter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use delegating TokenCredential to preserve OBO and avoid deadlocks Address review feedback from jongio: 1. Replace singleton-cached TokenCredential with ProviderDelegatingCredential that calls IAzureTokenCredentialProvider on each token request, preserving per-execution-context credentials for OBO/multi-user scenarios. 2. Remove sync-over-async (.GetAwaiter().GetResult()) from DI factory by registering the delegating class directly as a singleton. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: add ProviderDelegatingCredential unit tests Verify that: - Each GetTokenAsync call delegates to IAzureTokenCredentialProvider (no credential caching, OBO-safe) - TokenRequestContext is forwarded correctly to the resolved credential Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: correct NuGet version to 0.21.0-beta Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: add InternalsVisibleTo for test project Expose internal ProviderDelegatingCredential to unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: guard TokenCredential registration for CLI contexts Only register ProviderDelegatingCredential when IAzureTokenCredentialProvider is available in the service collection. In CLI contexts like 'tools list', the auth provider isn't registered, causing DI resolution failure. DataFactory.MCP.Core auto-detects TokenCredential in DI and falls back to standalone auth when it's not present, so this is safe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Ebram Tawfik <ebramtawfik@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Bridges the Fabric MCP Server's authentication to DataFactory.MCP.Core so that Data Factory commands automatically use the host's IAzureTokenCredentialProvider instead of requiring a separate login flow.
Changes
Auth Flow
Fabric MCP Server (DefaultAzureCredential)
-> IAzureTokenCredentialProvider
-> TokenCredential (registered by DataFactoryAreaSetup)
-> TokenCredentialAuthenticationService (auto-detected)
-> FabricAuthenticationHandler -> Bearer token on API calls
Why
Without this bridge, Data Factory commands fail at auth time because FabricAuthenticationHandler calls IAuthenticationService.GetAccessTokenAsync() which requires an explicit login that never happens in the Fabric MCP Server context. Other Fabric tools (OneLake, Core) accept TokenCredential via constructor — this aligns DataFactory with that pattern.
Related