[7.0.3 Cherry-pick] Fix Entra ID tenant parsing for multi-segment STSURL authorities - #4572
Open
github-actions[bot] wants to merge 1 commit into
Open
[7.0.3 Cherry-pick] Fix Entra ID tenant parsing for multi-segment STSURL authorities#4572github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
* Fix tenant parsing for multi-segment STSURL authorities Fixes #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 * Require an absolute HTTPS authority with a tenant 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 * Rename authority locals and TokenCredentialKey fields for clarity 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 * Use Uri.Segments to extract the tenant 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 * Normalize the password cache key and cover the remaining gaps Address review feedback: - GetAccountPwCacheKey keyed on the raw parameters.Authority, so two STSURL spellings of the same tenant produced separate password-cache entries. It now takes the normalized authority, consistent with the rest of this change. The userId parameter is nullable to preserve the previous concatenation behavior. - Add a test asserting AcquireTokenAsync surfaces the authority AuthenticationException unwrapped, so reordering the catch blocks can't silently regress it back to "Unexpected error". Verified the test fails when the pass-through catch is removed. - Add http:// cases to the rejection theory so the HTTPS requirement is pinned by a test, and rename the theory accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4e906c9-daf3-46f8-83e7-ef805d81fdfb --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b4e906c9-daf3-46f8-83e7-ef805d81fdfb
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
cheenamalhotra
approved these changes
Aug 21, 2026
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. |
paulmedynski
requested changes
Aug 24, 2026
| { | ||
| throw new Extensions.Azure.AuthenticationException( | ||
| parameters.AuthenticationMethod, | ||
| $"The authority '{parameters.Authority}' is not a valid Entra ID authority. " + |
Contributor
There was a problem hiding this comment.
Should this message be localized?
| /// <c>https://login.microsoftonline.com/{tenantId}/oauth2/authorize</c>. | ||
| /// </param> | ||
| /// <param name="authorityHost"> | ||
| /// Receives the authority host with a trailing slash, e.g. <c>https://login.microsoftonline.com/</c>. |
Contributor
There was a problem hiding this comment.
The host would be login.microsoft.com. The example string is a URL. Should this parameter be called something like authorityUrl, and the input parameter stsUrl, since that's what it seems to actually be?
| /// a domain name, or one of the <c>common</c>/<c>organizations</c>/<c>consumers</c> placeholders. | ||
| /// </param> | ||
| /// <param name="msalAuthority"> | ||
| /// Receives the normalized authority (host + tenant) suitable for MSAL's <c>WithAuthority</c>. |
Contributor
There was a problem hiding this comment.
Is this a URL, something else?
| /// <para> | ||
| /// Entra ID authorities are always absolute HTTPS URLs, so anything else is rejected rather | ||
| /// than guessed at. Both MSAL (<c>WithAuthority</c>) and Azure.Identity | ||
| /// (<c>TokenCredentialOptions.AuthorityHost</c>) require an absolute URI as well, so an |
Contributor
There was a problem hiding this comment.
URI or URL? This method seems to deal with URLs only.
| // ("https://login.microsoftonline.com/{tenantId}/oauth2/authorize"), so the tenant is | ||
| // taken from the first path segment rather than the last. | ||
|
|
||
| if (!TryParseAuthority(parameters.Authority, out string authorityHost, out string tenant, out string msalAuthority)) |
Contributor
There was a problem hiding this comment.
msalAuthority is only used in the MSAL path below, so perhaps it would be better to confine it (and thus encapsulate how to build it) within that path, rather than having all callers of TryParseAuthority() define a variable that may not be used. It's trivial to construct, and building it in the context of the MSAL path keeps MSAL concerns close together.
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
paulmedynski
enabled auto-merge (squash)
August 24, 2026 17:10
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-pick of #4521 (89ceebc) into
release/7.0.