Simplify GetTenantedAuthority in CiamAuthority and DstsAuthority (follow-up to #5958)

[Robbie-Microsoft]

Simplifies the GetTenantedAuthority logic in CiamAuthority and DstsAuthority, following up on @gladjohn's feedback on #5958 (#5958 (comment)).

Changes

• CiamAuthority.GetTenantedAuthority — remove dead/unreachable code paths.
• DstsAuthority.GetTenantedAuthority — keep the silent-flow rewrite path that Authority.CreateAuthorityWithTenant relies on (tenantless dstsv2/common / dstsv2/organizations configured authorities must be rewritten to the tenanted form even when forceSpecifiedTenant is false), and document the contract with a comment. Adds two regression tests covering the non-forced rewrite for both common and organizations.
• AuthorityInfo — tighten an XML doc comment.

Scope note

The earlier history of this branch had unintentionally bundled the WithClientClaims(string) work via local merges. That work is owned by #5999 and has been removed from here via a force-push. The original bundled history is preserved at the tag pr6001-backup-pre-rebase for reference.

[Copilot]

Pull request overview

This PR follows up on #5958 by continuing the “consumers/MSA GUID is a real tenant” fix across authority handling, token cache filtering, and regression tests, and by attempting to simplify tenant override behavior in CIAM/DSTS authorities.

Changes:

• Updates AAD logic to treat only common / organizations as tenantless (not consumers / MSA GUID) and aligns request-authority selection, cache filtering, and mTLS PoP validation accordingly.
• Adds/updates unit tests to ensure request-level tenant overrides are honored for consumers / MSA GUID scenarios and that the correct token endpoint is used.
• Refactors GetTenantedAuthority overrides for CIAM and DSTS to return the canonical authority unchanged.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/Microsoft.Identity.Test.Unit/PublicApiTests/ConfidentialClientApplicationTests.cs	Adds regression coverage for request-level tenant override when app authority is MSA GUID; updates a consumers-related assertion.
tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/AadAuthorityTests.cs	Adds regression test ensuring MSA GUID is not treated as tenantless by GetTenantedAuthority.
tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs	Adds request-level authority override regression coverage for MSA GUID/consumers and introduces new static test authorities.
src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs	Updates tenant filtering to treat consumers/MSA GUID as tenanted (filterable).
src/client/Microsoft.Identity.Client/Instance/DstsAuthority.cs	Changes DSTS GetTenantedAuthority behavior to always return canonical authority.
src/client/Microsoft.Identity.Client/Instance/CiamAuthority.cs	Changes CIAM GetTenantedAuthority behavior to always return canonical authority and removes unused usings.
src/client/Microsoft.Identity.Client/Instance/AadAuthority.cs	Removes consumers from the “tenantless” set and drops the IsCommonOrganizationsOrConsumersTenant helper.
src/client/Microsoft.Identity.Client/AppConfig/AuthorityInfo.cs	Updates request-authority selection logic/comments to only treat common/organizations as tenantless.
src/client/Microsoft.Identity.Client/ApiConfig/Parameters/MtlsPopParametersInitializer.cs	Updates mTLS PoP validation to only reject common/organizations tenantless authorities.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/client/Microsoft.Identity.Client/Instance/DstsAuthority.cs:42

• DstsAuthority.GetTenantedAuthority now rewrites the authority only when forceSpecifiedTenant=true. This changes behavior for DSTS authorities configured with tenantless segments like /dstsv2/common/ or /dstsv2/organizations/: calls that tenantize an authority with forceSpecifiedTenant:false (e.g., Authority.CreateAuthorityWithTenant(...) used by token/caching helpers) will no longer replace the tenant and will keep common/organizations, which can lead to incorrect tenant IDs and cache partitioning. Consider restoring the non-forced rewrite when the current TenantId is tenantless (e.g., common/organizations) while still keeping the forced rewrite for request-level .WithTenantId(...).

        // DSTS authorities use their own URL template (dstsv2/{tenantId}/), not the AAD template.
        // Only honor tenant overrides when forceSpecifiedTenant=true (i.e., .WithTenantId() at request level).
        // The old non-forced path (IsCommonOrOrganizationsTenant) was dead because DSTS authorities
        // always carry a real tenant, never "common" or "organizations".
        internal override string GetTenantedAuthority(string tenantId, bool forceSpecifiedTenant = false)
        {
            if (!string.IsNullOrEmpty(tenantId) && forceSpecifiedTenant)
            {
                var authorityUri = AuthorityInfo.CanonicalAuthority;

                return string.Format(
                    CultureInfo.InvariantCulture,
                    DstsCanonicalAuthorityTemplate,
                    authorityUri.Authority,
                    tenantId);
            }

            return AuthorityInfo.CanonicalAuthority.ToString();

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (3)

src/client/Microsoft.Identity.Client/Internal/ClaimsHelper.cs:96

• SortJsonObjectKeys only recurses into JObject children. Object keys nested inside JSON arrays (e.g., {"id_token":{"acr":{"values":[{"z":1,"a":2}]}}} or any complex claim using arrays of objects) will not be sorted, so two semantically identical claims values that differ only in key ordering inside an array element will normalize to different strings and produce separate cache entries — defeating the stated goal of preventing cache key fragmentation. Consider recursing through JsonArray elements as well (sorting object elements while preserving array element order).

        private static JObject SortJsonObjectKeys(JObject obj)
        {
            var sorted = new JObject();
            foreach (var key in obj.Select(kvp => kvp.Key).OrderBy(k => k, StringComparer.Ordinal))
            {
                var value = obj[key];
                if (value is JObject nestedObj)
                {
                    sorted[key] = SortJsonObjectKeys(nestedObj);
                }
                else
                {
                    // JsonNode.DeepClone is .NET 6+; use Parse(ToJsonString()) for portability.
                    sorted[key] = value is null ? null : JsonNode.Parse(value.ToJsonString());
                }
            }
            return sorted;
        }

src/client/Microsoft.Identity.Client/ManagedIdentity/AbstractManagedIdentity.cs:72

• String literal "claims" is used here for the query/body parameter name, but OAuth2Parameter.Claims is the established constant for this name (and is used in the test on line 433). Please use OAuth2Parameter.Claims instead of the bare string literal in both branches to keep the wire-format key in one place and avoid drift.

                    request.QueryParameters["claims"] = Uri.EscapeDataString(parameters.ClientClaims);
                    _requestContext.Logger.Info("[Managed Identity] Adding client claims to IMDS request as query parameter.");
                }
                else
                {
                    request.BodyParameters["claims"] = parameters.ClientClaims;

src/client/Microsoft.Identity.Client/Internal/ClaimsHelper.cs:76

• The MergeClaimsObjects catch block reuses MsalErrorMessage.InvalidJsonClaimsFormat(claims1) which embeds the raw claims1 value into the exception message. This contradicts the comment on the sister method NormalizeClaimsJson (lines 44-45) which deliberately does not include the raw JSON because "it may contain sensitive data". Either both should sanitize, or the comment in NormalizeClaimsJson is misleading. Please align — preferably do not embed the raw claims JSON in the merge error message either.

            catch (Exception ex) when (ex is JsonException || ex is InvalidOperationException)
            {
                // InvalidOperationException is thrown by JsonNode.AsObject() when a value is
                // valid JSON but not an object (e.g. an array or a scalar).
                throw new MsalClientException(
                    MsalError.InvalidJsonClaimsFormat,
                    MsalErrorMessage.InvalidJsonClaimsFormat(claims1),
                    ex);
            }

[trwalke]

Is this the correct description for this PR? the comment seems to suggest this is refactoring but the code looks like you are adding the withClientCliams api?

[Robbie-Microsoft]

Is this the correct description for this PR? the comment seems to suggest this is refactoring but the code looks like you are adding the withClientCliams api?

The title/description was stale. Updated them to cover both the new WithClientClaims(string) API and the GetTenantedAuthority cleanup.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

[Robbie-Microsoft]

@gladjohn — to your earlier question about why so many files were changing here: you were right, the diff had drifted well beyond the CIAM/DSTS cleanup you originally flagged. The branch had picked up the WithClientClaims(string) work (via local merges of my other in-flight branch) and some other already-shipped commits.

I've force-pushed the branch down to just the original scope — a single commit (7d7309b) touching 4 files (+39/−23):

• Instance/CiamAuthority.cs
• Instance/DstsAuthority.cs (+ regression tests in DstsAuthorityTests.cs)
• AppConfig/AuthorityInfo.cs (1-line doc tweak)

The WithClientClaims(string) API work has its own dedicated PR at #5999 — please direct any feedback on that surface there. The pre-rewrite state of this branch is preserved at the tag pr6001-backup-pre-rebase if anyone needs to refer back to it.

Sorry for the noise — ready for another look when you have a moment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

stale