Fix Entra ID tenant parsing for multi-segment STSURL authorities - #4521
Fix Entra ID tenant parsing for multi-segment STSURL authorities#4521cheenamalhotra wants to merge 4 commits into
Conversation
Fixes dotnet#4496 The Dataverse/Dynamics 365 TDS endpoint returns an ADAL v1 style STSURL ("https://login.microsoftonline.com/{tenantId}/oauth2/authorize") in the FEDAUTHINFO token. AcquireTokenAsync split the authority at the last '/', so the tenant was parsed as the literal "authorize" and the authority host became ".../oauth2/", causing authentication to fail. The tenant is now taken from the first path segment of the authority URL, ignoring trailing endpoint suffixes, and the normalized authority (host + tenant) is used for the MSAL public client application. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4e906c9-daf3-46f8-83e7-ef805d81fdfb
Entra ID authorities (and therefore the STSURL in FEDAUTHINFO) are always absolute HTTPS URLs, and both MSAL's WithAuthority and Azure.Identity's AuthorityHost require an absolute URI, so the legacy last-separator split could never produce a working credential for anything else. Replace the fallback with TryParseAuthority, which rejects such authorities up front with a clear AuthenticationException instead of failing obscurely later. Also stop re-wrapping AuthenticationException in the generic catch block. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4e906c9-daf3-46f8-83e7-ef805d81fdfb
There was a problem hiding this comment.
Pull request overview
This PR fixes Entra ID authority parsing when SQL Server (notably Dataverse/Dynamics 365 TDS) returns multi-segment STSURL values (e.g., /oauth2/authorize) in the FEDAUTHINFO token, ensuring the tenant is parsed correctly and the MSAL authority is normalized.
Changes:
- Add
TryParseAuthorityto split an absolute HTTPS STSURL intoauthorityHost,tenant, and a normalizedmsalAuthority(host + tenant). - Use normalized
msalAuthorityfor MSAL-based flows (viaPublicClientAppKey/WithAuthority) and fail fast with a clearerAuthenticationExceptionwhen the STSURL is malformed. - Add unit tests covering documented authority URL shapes, including v1/v2 endpoints and national cloud hosts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs | Normalizes STSURL parsing (first path segment tenant) and ensures MSAL/Azure.Identity receive correct authority/tenant; improves exception pass-through. |
| src/Microsoft.Data.SqlClient.Extensions/Azure/test/AuthorityParsingTests.cs | Adds test coverage for supported STSURL shapes and rejection cases for missing tenant/empty authority. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The test below connects to a Power Apps Developer Dataverse instance; this test fails on main and passes with this PR. [Fact]
public void ActiveDirectoryDefaultAuthenticationToDataverse_Succeeds()
{
const string ConnectionString = "Data Source=<org name>.crm11.dynamics.com;User ID=<user id>;Encrypt=True;TrustServerCertificate=True;Authentication=ActiveDirectoryDefault;";
using SqlConnection conn = new(ConnectionString);
conn.Open();
using SqlCommand cmd = new("select * from sys.databases", conn);
using SqlDataReader rd = cmd.ExecuteReader();
while (rd.Read())
{ }
} |
Address review feedback: - The 'audience' local no longer held the last path segment after the parsing fix; it holds the tenant that is passed to Azure.Identity as TenantId. Rename the locals and the TokenCredentialKey fields to authorityHost/tenant so the names match what they carry, and refresh the surrounding comment accordingly. - Add a 'consumers' placeholder case to AuthorityParsingTests, which the TryParseAuthority documentation already calls out. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4e906c9-daf3-46f8-83e7-ef805d81fdfb
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs:268
- Typo in comment: "DefaultAzureCredenial" should be "DefaultAzureCredential" to match the type name and avoid confusion during future maintenance/searching.
// Cache DefaultAzureCredenial based on scope, authority host, tenant, and clientId
benrr101
left a comment
There was a problem hiding this comment.
Overall, looks good - I'd just prefer it if we use the Uri class to handle Uri manipulation rather than direct string manipulation.
| if (Uri.TryCreate(authorityUrl, UriKind.Absolute, out Uri? uri) && | ||
| uri.Scheme == Uri.UriSchemeHttps) | ||
| { | ||
| string path = uri.AbsolutePath.Trim('/'); |
There was a problem hiding this comment.
Rather than doing direct string manipulation, Uri offers a handy-dandy Segments array. That way you could just do:
if(uri.Segments.length > 1)
{
tenant = uri.Segments[1];
authorityHost = uri.GetLeftPart(UriPartial.Authority) + "/";
msalAuthority = authorityHost + tenant;
return true;
}There was a problem hiding this comment.
Agreed, done. Two small nuances I kept on top of your snippet:
Segmentskeeps the trailing separator on a segment when further segments follow, soSegments[1]for/{tenant}/oauth2/authorizeis"{tenant}/". Added aTrimEnd('/').- An empty leading segment (
https://host//oauth2/authorize) givesSegments[1] == "/", which trims to empty, so I kept the non-empty check to reject it rather than build an authority with no tenant. Added a test case for that.
Also fixed the DefaultAzureCredenial typo the bot flagged on a comment line I had touched.
Address review feedback: let the Uri class handle URI decomposition instead of doing string manipulation on AbsolutePath. Segments[0] is always the leading "/", so the tenant is Segments[1]. Segments retain their trailing separator when further segments follow, so the value is trimmed. The non-empty check is kept to reject an empty leading segment (e.g. "https://host//oauth2/authorize"), which would otherwise yield an authority with no tenant; a test covers this. Also fix a "DefaultAzureCredenial" typo in a comment touched by the previous commit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4e906c9-daf3-46f8-83e7-ef805d81fdfb
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Microsoft.Data.SqlClient.Extensions/Azure/test/AuthorityParsingTests.cs:114
- This new test method doesn’t include an XML
comment. Adding a brief summary (consistent with the rest of the Azure test suite) helps communicate why these rejection cases matter and makes failures easier to interpret.
[Theory]
// A tenant is required; an authority without one cannot yield a usable credential.
[InlineData("https://login.microsoftonline.com")]
[InlineData("https://login.microsoftonline.com/")]
// An empty leading path segment leaves no tenant to authenticate against.
src/Microsoft.Data.SqlClient.Extensions/Azure/src/ActiveDirectoryAuthenticationProvider.cs:244
- The comment claims “We always have a tenant here, because the server supplies one in the STSURL.” but the provider explicitly supports (and the new tests cover) cases where the server may omit STSURL or provide an authority without a tenant segment. This comment is therefore misleading and should be updated to reflect the actual behavior (fail fast with an AuthenticationException when no tenant is present).
// If no tenant is specified, the app targets Entra ID and personal
// Microsoft accounts as an audience. (That is, it behaves as though
// `common` were specified.) We always have a tenant here, because the
// server supplies one in the STSURL.
src/Microsoft.Data.SqlClient.Extensions/Azure/test/AuthorityParsingTests.cs:93
- This new test method doesn’t include an XML
comment. Other tests in this project consistently document test intent with XML docs, and adding a summary here keeps the test suite consistent and easier to maintain.
This issue also appears on line 110 of the same file.
[Theory]
[MemberData(nameof(AuthorityData))]
public void TryParseAuthority_SplitsHostAndTenant(
Description
Connecting to the Dataverse / Dynamics 365 TDS endpoint with
Authentication=Active Directory Service Principalfails on 7.0.0+ withClientSecretCredential authentication failed.Dataverse returns an OAuth v1 style STSURL in the FEDAUTHINFO TDS token:
ActiveDirectoryAuthenticationProvider.AcquireTokenAsyncsplit the authority at the last/, soClientSecretCredentialreceived the literal string"authorize"as its tenant id, andAuthorityHostbecamehttps://login.microsoftonline.com/{tenantId}/oauth2/. Azure SQL and Fabric return the barehttps://login.microsoftonline.com/{tenantId}form, so only multi-segment STSURLs were affected. The same split existed in 6.x, where the older Azure.Identity / MSAL combination happened to tolerate the malformed authority.Approach
The tenant is now taken from the first path segment of the authority URL rather than the last, so trailing endpoint suffixes (
/oauth2/authorize,/oauth2/v2.0/token, ...) are ignored:TryParseAuthorityhelper splits the STSURL into an authority host (https://login.microsoftonline.com/), a tenant, and a normalized MSAL authority (host + tenant).PublicClientAppKey/WithAuthority, so the interactive, password, integrated, and device-code flows are fixed too, not just the Azure.Identity based ones.WithAuthorityand Azure.Identity'sAuthorityHost(new Uri(...)) require an absolute URI, so the fallback could never have produced a working credential. An unparseable authority now raises a clearAuthenticationExceptionnaming the offending value and the expected shape instead of failing obscurely deeper in the stack.catch (AuthenticationException) { throw; }guard inAcquireTokenAsyncso provider-raised authentication errors are no longer re-wrapped by the generic catch-all as "Unexpected error". This also improves the pre-existing "authentication method not supported" path.Backwards compatibility
No public API changes. Behavior is unchanged for the bare
https://login.microsoftonline.com/{tenantId}authority that Azure SQL, Fabric, and Synapse send. The only behavior difference for existing working scenarios is the improved error message when an authority is malformed.Issues
Fixes #4496
Testing
Added
AuthorityParsingTestsinsrc/Microsoft.Data.SqlClient.Extensions/Azure/test/, covering only authority shapes that Entra ID actually documents:/oauth2/authorizeendpoint (the Dataverse repro)/oauth2/v2.0/tokenendpointlogin.microsoftonline.usandlogin.partner.microsoftonline.cncommon/organizationsplaceholdersResults: 12/12 new tests pass; 34 passed / 1 skipped across the non-integration Azure test suite on net9.0. The remaining
AADConnectionTestfailure in the full run is a pre-existing integration test that requires a live Azure SQL server and network access.Manual verification against a real Dataverse TDS endpoint has not been performed and would be valuable before merge, since that environment is not available in this workspace.
Guidelines
Please review the contribution guidelines before submitting a pull request: