Skip to content

Add ICloudConfiguration for cross-cloud metadata resolution - #6104

Open
Avery-Dunn wants to merge 2 commits into
mainfrom
avdunn/cloud-configuration
Open

Add ICloudConfiguration for cross-cloud metadata resolution#6104
Avery-Dunn wants to merge 2 commits into
mainfrom
avdunn/cloud-configuration

Conversation

@Avery-Dunn

@Avery-Dunn Avery-Dunn commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Introduces a public, extensible mechanism for resolving cloud-specific metadata by authority host. Adds the ICloudConfiguration interface, an immutable string-keyed CloudSettings bag, a built-in KnownCloudConfiguration covering the Azure clouds MSAL knows, and a ready-made InMemoryCloudConfiguration so callers can add or override clouds over the built-in baseline. The first metadata value carried is the FIC (Federated Identity Credential) token-exchange audience.

Related work in ID Web and Abstractions:
AzureAD/microsoft-identity-web#3994
AzureAD/microsoft-identity-abstractions-for-dotnet#266

Motivation

Applications using Federated Identity Credentials in sovereign or government clouds must supply the correct cloud-specific token-exchange audience (e.g. api://AzureADTokenExchangeUSGov for US Government, api://AzureADTokenExchangeChina for China). Today MSAL does not expose this value, and higher-level SDKs (Microsoft.Identity.Web, MISE) hardcode the public-cloud value api://AzureADTokenExchange with no override path. Customers in sovereign clouds hit opaque failures because the wrong audience is used, with no programmatic way to resolve the right one from the authority host.

This PR makes cloud-specific metadata a first-class, publicly resolvable concept in MSAL so that higher-level SDKs and customer code can look up the correct value per authority — and override it when a cloud is new, private, or air-gapped.

Design

Storage: a string-keyed metadata bag

CloudSettings holds one cloud's metadata as an immutable, case-insensitive bag of string values. Values are addressed by well-known key constants rather than fixed typed properties, so new cloud-specific values can be added — and obsolete ones removed — without source- or binary-breaking callers (an unknown/removed key simply misses and the consumer falls back to a documented default).

public sealed class CloudSettings
{
    public CloudSettings(IReadOnlyDictionary<string, string> values);
    public IReadOnlyDictionary<string, string> Values { get; }
    public bool   TryGetValue(string key, out string value);
    public string GetValueOrDefault(string key);
}

public static class MsalCloudKeys
{
    public const string TokenExchangeAudience = "token_exchange_audience"; // stored bare, no "/.default"
}

The instance is immutable: the constructor copies values into a case-insensitive dictionary wrapped in a ReadOnlyDictionary, so callers cannot downcast Values to mutate a shared (e.g. singleton) bag.

Typed accessors are provided as optional extension methods over the bag (never the storage):

public static class CloudSettingsExtensions
{
    public static string TokenExchangeAudience(this CloudSettings settings); // bare (MI / resource context)
    public static string TokenExchangeScope(this CloudSettings settings);    // bare + "/.default" (CC context)
}

TokenExchangeScope() computes the /.default scope form from the bare audience so no call site hand-appends the suffix. It is the single owner of that rule across the stack.

Resolution: ICloudConfiguration

public interface ICloudConfiguration
{
    CloudSettings GetSettingsByAuthorityHost(string authorityHost); // null if the host is not recognized
}

KnownCloudConfiguration — the built-in implementation covering the Azure clouds MSAL knows. Exposed as a Default singleton for lookups without an app instance. Its data is projected from a single internal source of truth (KnownCloudData), from which MSAL's internal KnownMetadataProvider also derives, so the alias/preferred-host magic strings live in exactly one place. A unit test asserts the two projections stay consistent.

Lookup is case-insensitive and alias-aware — querying login.windows.net, login.microsoft.com, or sts.windows.net all return the public-cloud entry.

InMemoryCloudConfiguration — a ready-made mutable provider for callers who need to add a new cloud or override an existing one, with an optional fallback:

// Add/override over the built-in clouds (per-cloud or per-key), keeping every other cloud MSAL ships:
var config = new InMemoryCloudConfiguration(fallback: KnownCloudConfiguration.Default)
    .AddOrUpdate("login.mynewcloud.example", new Dictionary<string, string>
    {
        [MsalCloudKeys.TokenExchangeAudience] = "api://AzureADTokenExchangeMyCloud",
    })
    .AddOrUpdate("login.microsoftonline.us", MsalCloudKeys.TokenExchangeAudience, "api://Custom");

// Complete replacement of MSAL's defaults: construct with no fallback (or implement ICloudConfiguration).
var only = new InMemoryCloudConfiguration().AddOrUpdate("login.mynewcloud.example", /* ... */);

Overrides merge over the fallback per key (own value wins; other keys and clouds are preserved), so a caller can adjust a single field of one cloud while leaving everything else as MSAL ships it.

Key design decisions

  1. MSAL is a resolver and public baseline, not an internal consumer. MSAL exposes the lookup surface but does not transform scopes/resources internally from it. Higher-level SDKs call GetSettingsByAuthorityHost(...) to resolve the audience/scope and hand MSAL a finished string via AcquireTokenForClient(...) / AcquireTokenForManagedIdentity(...). This keeps MSAL a store/baseline; re-introducing a builder attach point later (if MSAL ever needs to self-consume a key) is additive and non-breaking.

  2. Extensible, add/remove-safe surface. Storage is a string-keyed bag; typed getters are extension methods keyed off MsalCloudKeys. Values and accessors can be added or obsoleted independently of the CloudSettings shape, so the metadata list can grow without breaking callers.

  3. Single source of truth. KnownCloudData is the only place the built-in alias/preferred-host/audience magic strings live; both the public KnownCloudConfiguration and the internal KnownMetadataProvider project from it, and a unit test verifies their alias sets stay consistent.

  4. The audience is stored once, bare. MsalCloudKeys.TokenExchangeAudience holds the value without /.default; managed-identity/resource contexts use the bare form (TokenExchangeAudience()), client-credentials contexts use TokenExchangeScope() (bare + /.default). MSAL's managed-identity flow strips /.default anyway via ScopeHelper.RemoveDefaultSuffixIfPresent().

  5. Preferred network/cache hosts are not part of the public bag. MSAL's instance-discovery pipeline resolves those from its own internal table (KnownCloudData via KnownMetadataProvider) and never reads them from CloudSettings, so exposing them here would be an inert override surface. Callers override preferred hosts through WithInstanceDiscoveryMetadata instead. Aliases likewise remain an internal detail of KnownCloudData (they key the built-in lookup); they are not exposed on the public bag.

New Public APIs

Namespace Microsoft.Identity.Client.Instance.Discovery:

API Kind
ICloudConfiguration.GetSettingsByAuthorityHost(string) : CloudSettings interface
CloudSettings(IReadOnlyDictionary<string,string> values), Values, TryGetValue, GetValueOrDefault sealed class
MsalCloudKeys.TokenExchangeAudience = "token_exchange_audience" static const
CloudSettingsExtensions.TokenExchangeAudience() / .TokenExchangeScope() static ext
KnownCloudConfiguration + .Default + GetSettingsByAuthorityHost sealed class
InMemoryCloudConfiguration(fallback = null) + AddOrUpdate(host, values) + AddOrUpdate(host, key, value) + GetSettingsByAuthorityHost sealed class

Other Changes

File Description
Instance/Discovery/ICloudConfiguration.cs New public interface — resolve CloudSettings by authority host
Instance/Discovery/CloudSettings.cs New immutable per-cloud metadata bag (string-keyed values)
Instance/Discovery/MsalCloudKeys.cs New public well-known key constants (TokenExchangeAudience)
Instance/Discovery/CloudSettingsExtensions.cs New typed accessors (TokenExchangeAudience, computed TokenExchangeScope)
Instance/Discovery/KnownCloudData.cs New internal single source of truth for built-in clouds
Instance/Discovery/KnownCloudConfiguration.cs New default implementation + Default singleton, projected from KnownCloudData
Instance/Discovery/InMemoryCloudConfiguration.cs New ready-made mutable provider (AddOrUpdate, optional fallback, per-key merge)
Instance/Discovery/KnownMetadataProvider.cs Refactored to derive its instance-discovery entries from KnownCloudData
PublicApi/*/PublicAPI.Unshipped.txt (×6) New public API surface entries
InstanceProviderTests.cs New unit tests for cloud-configuration behavior
CrossCloudTokenExchangeTests.cs New integration tests: resolve the cloud-specific exchange scope, then assert it on the wire of a real client-credentials request

Test coverage

InstanceProviderTests (unit):

  • All known clouds resolve to the expected TokenExchangeAudience (including null where a cloud has no FIC app).
  • TokenExchangeScope() appends /.default to the bare audience; TokenExchangeAudience() stays bare.
  • Alias lookups return the same CloudSettings; lookups are case-insensitive; unknown/empty hosts return null.
  • CloudSettings accessors: present/missing/null key via TryGetValue/GetValueOrDefault; a null values bag is treated as empty.
  • KnownMetadataProvider's alias sets stay consistent with the KnownCloudData source of truth.
  • Default is a singleton.
  • InMemoryCloudConfiguration — injects a new cloud with fallback to defaults, overrides an existing cloud (per-cloud and per-key), preserves sibling keys/clouds on merge, grows a value for a no-FIC cloud per key, builds a cloud incrementally with no fallback, validates its arguments (a null/empty/whitespace authorityHost or key throws ArgumentException; a null value throws ArgumentNullException — consistent with the Abstractions and MISE twins), and returns null for unregistered hosts when no fallback is supplied.

Copilot AI lite review requested due to automatic review settings July 6, 2026 21:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new public “cloud configuration” abstraction to let callers resolve cloud-specific metadata (authority aliases, preferred hosts, and FIC token-exchange audiences) for cross-cloud/sovereign scenarios, with a built-in KnownCloudConfiguration implementation.

Changes:

  • Added public ICloudConfiguration and CloudSettings types under Microsoft.Identity.Client.Instance.Discovery.
  • Added KnownCloudConfiguration.Default with mappings for known Azure clouds, including FIC token-exchange audiences where applicable.
  • Added WithCloudConfiguration(...) on AbstractApplicationBuilder<T> and surfaced the setting on ApplicationConfiguration; updated PublicAPI + added unit tests.

Reviewed changes

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

Show a summary per file
File Description
tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/InstanceProviderTests.cs Adds unit tests for cloud configuration lookup behavior and builder wiring.
src/client/Microsoft.Identity.Client/Instance/Discovery/ICloudConfiguration.cs New public interface to resolve cloud settings by authority host.
src/client/Microsoft.Identity.Client/Instance/Discovery/CloudSettings.cs New public data container for per-cloud metadata including token exchange audience.
src/client/Microsoft.Identity.Client/Instance/Discovery/KnownCloudConfiguration.cs Default implementation providing known cloud entries and alias-aware lookup.
src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs Adds WithCloudConfiguration(...) builder API.
src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs Adds CloudConfiguration field to app configuration.
src/client/Microsoft.Identity.Client/PublicApi/netstandard2.0/PublicAPI.Unshipped.txt Declares new public API surface for netstandard2.0.
src/client/Microsoft.Identity.Client/PublicApi/net8.0/PublicAPI.Unshipped.txt Declares new public API surface for net8.0.
src/client/Microsoft.Identity.Client/PublicApi/net8.0-ios/PublicAPI.Unshipped.txt Declares new public API surface for net8.0-ios.
src/client/Microsoft.Identity.Client/PublicApi/net8.0-android/PublicAPI.Unshipped.txt Declares new public API surface for net8.0-android.
src/client/Microsoft.Identity.Client/PublicApi/net472/PublicAPI.Unshipped.txt Declares new public API surface for net472.
src/client/Microsoft.Identity.Client/PublicApi/net462/PublicAPI.Unshipped.txt Declares new public API surface for net462.

Comment thread src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@Avery-Dunn
Avery-Dunn marked this pull request as ready for review July 7, 2026 20:43
@Avery-Dunn
Avery-Dunn requested a review from a team as a code owner July 7, 2026 20:43
Copilot AI review requested due to automatic review settings July 8, 2026 18:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI review requested due to automatic review settings July 9, 2026 21:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Copilot AI review requested due to automatic review settings July 10, 2026 12:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

/// via <see cref="AbstractApplicationBuilder{T}.WithCloudConfiguration(ICloudConfiguration)"/>
/// to add entries for private or internal-only clouds.
/// </remarks>
public interface ICloudConfiguration

@bgavrilMS Bogdan Gavril (bgavrilMS) Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question:

This is general purpose interface that is to be used in MSAL, Id.Web, MISE. What is the best location for it?

Thoughts on this Neha Sharma (@Neha), MARCIN Z (@MZOLN) ? Should we just put it in MSAL and longer term get rid of abstractions? Or have an MSAL specific interface and a MISE / Id.Web / Abstractions parallel one?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this one, I think we can completely avoid abstractions and have this in MSAL. The issue comes when you need to expose some object/ interface in MSAL and that need to referenced in abstractions. Then it will require bringing MSAL as a dependency in Abstractions. Which was the case in metadata propagation. In this case, does not seem like. This will resolve completely in Idweb / MISE.

If we plan to consolidate, this is the safest starting point. But if not, for consistency and how the SDKs are designed we can expose this in abstractions.

@Avery-Dunn Avery-Dunn Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After investigating the MISE/ID Web side more for current practices and precedent the current design splits it a bit: MSAL keeps its own self-contained mirror (no Abstractions dependency, matching what we do today), and a neutral provider contract lives in Abstractions (Id.Web and MISE already have an Abstractions dependency). This allows MISE and ID Web to share interfaces without strictly depending on each other, ID Web can translate it into MSAL's style.

I figured if we do decide to deprecate abstractions then whatever solution works for migrating the existing relationships will work for this new one, so it hopefully won't be much of a complication down the line.

/// A <see cref="CloudSettings"/> instance with cloud-specific metadata,
/// or <c>null</c> if the host is not recognized.
/// </returns>
CloudSettings GetSettingsByAuthority(string authorityHost);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also have a string? GetSettingsByAuthority(string authorityHost, string setting) - this would allow a weakly typed to go in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest commit CloudSettings is now a string-keyed bag (GetValueOrDefault(key)/TryGetValue, keys in are in MsalCloudKeys), rather than strongly typed keys that we expose.

/// Callers should append <c>/.default</c> when using this value as a scope
/// in the client credentials flow, and omit it for the managed identity flow.
/// </remarks>
public string TokenExchangeAudience { get; init; }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see cases where we'd not want to expose strongly typed properties in open source.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest commit CloudSettings is now a string-keyed bag (GetValueOrDefault(key)/TryGetValue, keys in are in MsalCloudKeys), rather than strongly typed keys that we expose.

/// </remarks>
public T WithCloudConfiguration(ICloudConfiguration cloudConfiguration)
{
Config.CloudConfiguration = cloudConfiguration ?? throw new ArgumentNullException(nameof(cloudConfiguration));

@gladjohn Gladwin Johnson (gladjohn) Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only stores to the internal ApplicationConfiguration? how do you plan to expose it for IdWeb?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: #6104 (comment)

In short, ID Web (or any other MSAL caller) reads KnownCloudConfiguration.Default if they need cloud-specific metadata, adds any overrides, and passes the Strings into MSAL as needed.

/// A <see cref="CloudSettings"/> instance with cloud-specific metadata,
/// or <c>null</c> if the host is not recognized.
/// </returns>
CloudSettings GetSettingsByAuthority(string authorityHost);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the cross-cloud FIC audience belong to the source managed-identity cloud?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From testing I believe the relevant cloud is where the resource is: ESTS will happily give you a token with a public cloud exchange endpoint audience, but if you actually try to use it in the non-public cloud you'll get an error about an incorrect audience.

/// All known host aliases for this cloud. Tokens issued by any alias
/// are equivalent and share a cache entry.
/// </summary>
public IReadOnlyList<string> Aliases { get; init; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aliases is IReadOnlyList but init'd from a string[] on the shared singleton entry. (string[])settings.Aliases lets any caller mutate global state

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CloudSettings now stores aliases as a ReadOnlyCollection<string> (via new List<string>(aliases).AsReadOnly()) and defensively copies the values dictionary, so callers can't downcast-and-mutate the shared KnownCloudConfiguration.Default instances.

/// MSAL ships a default implementation (<see cref="KnownCloudConfiguration"/>) that
/// covers all publicly known Azure clouds. Callers can provide a custom implementation
/// via <see cref="AbstractApplicationBuilder{T}.WithCloudConfiguration(ICloudConfiguration)"/>
/// to add entries for private or internal-only clouds.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc says custom impls add entries, but the builder stores the provider as a full replacement with no fallback to Default

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithCloudConfiguration is a full replacement. To add/override over the built-ins, you could use CompositeCloudConfiguration(custom, KnownCloudConfiguration.Default) or (as of the latest commit) InMemoryCloudConfiguration(fallback: KnownCloudConfiguration.Default).AddOrUpdate(...)

/// </summary>
/// <remarks>
/// <c>null</c> for clouds that do not have a known token exchange application.
/// Callers should append <c>/.default</c> when using this value as a scope

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a doc-only contract here is error-prone?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest commit we now store the audience bare and expose a computed TokenExchangeScope() that appends /.default. MI/resource contexts use TokenExchangeAudience() (bare), CC contexts use TokenExchangeScope(), so no caller needs to hand-append the suffix.

Copilot AI review requested due to automatic review settings July 27, 2026 23:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread src/client/Microsoft.Identity.Client/Instance/Discovery/CloudSettings.cs Outdated
Copilot AI review requested due to automatic review settings July 28, 2026 00:14
@Avery-Dunn
Avery-Dunn force-pushed the avdunn/cloud-configuration branch from 08c51a4 to 3df2bb0 Compare July 28, 2026 00:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Comment thread src/client/Microsoft.Identity.Client/Instance/Discovery/CloudSettings.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/Instance/Discovery/CloudSettings.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/Instance/Discovery/KnownMetadataProvider.cs Outdated
@Avery-Dunn
Avery-Dunn marked this pull request as draft July 28, 2026 15:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comments suppressed due to low confidence (2)

tests/Microsoft.Identity.Test.Unit/PublicApiTests/CrossCloudTokenExchangeTests.cs:20

  • The class-level doc comment references KnownCloudData, which is an internal type, and describes it as the public store of cloud-specific strings. Since this test is exercising the public surface, it would be clearer (and avoids internal-type references in docs) to point at KnownCloudConfiguration as the public resolver and mention that it projects from internal known-cloud data.
    /// MSAL is the store of publicly-known, cloud-specific FIC token-exchange magic strings
    /// (<see cref="KnownCloudData"/>) and the single owner of the audience→scope ("/.default") rule
    /// (<see cref="CloudSettingsExtensions.TokenExchangeScope(CloudSettings)"/>). A consumer (ID Web, or a
    /// direct MSAL caller) resolves the cloud-specific exchange scope from the cloud configuration and then

tests/Microsoft.Identity.Test.Unit/PublicApiTests/CrossCloudTokenExchangeTests.cs:30

  • The doc comment says these tests are "NOT intended for the final PRs", but they’re being introduced as part of this PR’s test coverage. This can confuse future maintainers about whether the tests are meant to stay. Consider removing that note and keeping the comment focused on what the tests validate.
    /// These mirror the MISE UserFic / ID Web FIC pseudo-E2E tests one layer down and are POC-confidence
    /// probes — NOT intended for the final PRs.

Copilot AI review requested due to automatic review settings July 30, 2026 17:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comments suppressed due to low confidence (2)

src/client/Microsoft.Identity.Client/Instance/Discovery/CloudSettingsExtensions.cs:49

  • TokenExchangeScope uses StringComparison.OrdinalIgnoreCase when checking for the protocol suffix "/.default". Elsewhere in MSAL (e.g., ScopeHelper.RemoveDefaultSuffixIfPresent) the suffix is treated as case-sensitive with StringComparison.Ordinal. Using OrdinalIgnoreCase can incorrectly treat a non-canonical scope like ".../.DEFAULT" as already-suffixed and return it unchanged.
            return audience.EndsWith(DefaultSuffix, StringComparison.OrdinalIgnoreCase)
                ? audience
                : audience + DefaultSuffix;

src/client/Microsoft.Identity.Client/Instance/Discovery/InMemoryCloudConfiguration.cs:79

  • AddOrUpdate(string, IReadOnlyDictionary<string,string>) validates that values is non-null, but it does not validate the individual entries. ConcurrentDictionary throws if a key is null/empty or if a value is null, which would surface as a low-signal runtime exception coming from inside the collection rather than a clear argument error on values. Validate kvp.Key and kvp.Value explicitly and throw an ArgumentException with nameof(values) so callers get actionable feedback.
            ConcurrentDictionary<string, string> bag = GetOrAddBag(authorityHost);
            foreach (KeyValuePair<string, string> kvp in values)
            {
                bag[kvp.Key] = kvp.Value;
            }

@Avery-Dunn
Avery-Dunn marked this pull request as ready for review August 3, 2026 15:14
Copilot AI review requested due to automatic review settings August 3, 2026 15:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

src/client/Microsoft.Identity.Client/Instance/Discovery/InMemoryCloudConfiguration.cs:79

  • The dictionary overload of AddOrUpdate does not validate individual entries. A null/empty key will throw from the dictionary indexer, and a null value can be stored (which then makes CloudSettings.TryGetValue return true with a null value). This is inconsistent with the single-key overload (which rejects null values) and produces less actionable exceptions for callers. Consider validating key/value pairs and throwing an ArgumentException with the offending key/parameter name.
            ConcurrentDictionary<string, string> bag = GetOrAddBag(authorityHost);
            foreach (KeyValuePair<string, string> kvp in values)
            {
                bag[kvp.Key] = kvp.Value;
            }

tests/Microsoft.Identity.Test.Unit/PublicApiTests/CrossCloudTokenExchangeTests.cs:30

  • The class-level XML docs say these tests are "NOT intended for the final PRs", but the file is checked in as part of this PR. That wording is likely to become stale/confusing for future maintainers reading the test suite. Consider removing the “not intended” phrasing and keeping the description focused on what the tests validate.
    /// These mirror the MISE UserFic / ID Web FIC pseudo-E2E tests one layer down and are POC-confidence
    /// probes — NOT intended for the final PRs.

Copilot AI review requested due to automatic review settings August 4, 2026 18:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

src/client/Microsoft.Identity.Client/Instance/Discovery/CloudSettings.cs:13

  • The PR description states that CloudSettings also carries authority-host aliases (and even shows a constructor accepting aliases), but the shipped public API in this PR exposes only the string-keyed Values bag. This is a material mismatch for downstream consumers reading the PR description. Either update the PR description/design notes to match the implemented public surface, or add alias exposure/composition helpers as described (which would require API + test updates). As a minimum, consider clarifying in the public XML docs that alias resolution lives in ICloudConfiguration, not in CloudSettings.
    /// <summary>
    /// Cloud-specific metadata for a single Azure cloud environment, modeled as an immutable
    /// string-keyed bag of values addressed by well-known key constants.
    /// </summary>

@Avery-Dunn
Avery-Dunn force-pushed the avdunn/cloud-configuration branch from 18af15e to 02a333a Compare August 4, 2026 18:34
…onfiguration, InMemoryCloudConfiguration, CloudSettings)

Introduces a public, keyed-bag cloud-metadata surface in MSAL for auto-resolving
cloud-specific FIC token-exchange values by authority host, with caller override.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0f4470fd-e0ff-4d14-b2c7-ece13faf29ff

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (1)

src/client/Microsoft.Identity.Client/Instance/Discovery/InMemoryCloudConfiguration.cs:79

  • AddOrUpdate(authorityHost, values) validates the host and the dictionary itself, but it does not validate individual entries. If a caller passes a custom IReadOnlyDictionary that yields a null/empty key or a null value, this will either throw from inside ConcurrentDictionary with a less actionable exception, or allow null values even though the per-key overload explicitly rejects them. Consider validating each kvp to provide a consistent, clear contract.
            ConcurrentDictionary<string, string> bag = GetOrAddBag(authorityHost);
            foreach (KeyValuePair<string, string> kvp in values)
            {
                bag[kvp.Key] = kvp.Value;
            }

Copilot AI review requested due to automatic review settings August 5, 2026 17:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

src/client/Microsoft.Identity.Client/Instance/Discovery/InMemoryCloudConfiguration.cs:79

  • AddOrUpdate(authorityHost, values) does not validate per-entry keys/values. This is inconsistent with the per-key overload (which rejects whitespace keys and null values) and can result in unclear exceptions or unintended keys being stored when callers pass invalid entries in the dictionary.
            ConcurrentDictionary<string, string> bag = GetOrAddBag(authorityHost);
            foreach (KeyValuePair<string, string> kvp in values)
            {
                bag[kvp.Key] = kvp.Value;
            }

tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/InstanceProviderTests.cs:223

  • This test hardcodes the full set of known hosts/aliases, which duplicates KnownCloudData and can drift when clouds/aliases are added or removed. Deriving the list from KnownCloudData.Entries keeps the test automatically in sync with the single source of truth.
            string[] knownHosts = new[]
            {
                "login.microsoftonline.com",
                "login.windows.net",
                "login.microsoft.com",

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

src/client/Microsoft.Identity.Client/Instance/Discovery/InMemoryCloudConfiguration.cs:79

  • InMemoryCloudConfiguration.AddOrUpdate(authorityHost, values) validates the host and the values bag, but it does not validate individual entries. If a caller supplies a null/whitespace key or a null value, the ConcurrentDictionary indexer will throw (with less actionable param names) and the per-cloud overload will behave inconsistently with the per-key overload (which validates key/value). Consider validating each kvp before writing to the bag and throwing ArgumentException/ArgumentNullException with clear messages.
            ConcurrentDictionary<string, string> bag = GetOrAddBag(authorityHost);
            foreach (KeyValuePair<string, string> kvp in values)
            {
                bag[kvp.Key] = kvp.Value;
            }

src/client/Microsoft.Identity.Client/Instance/Discovery/KnownMetadataProvider.cs:49

  • KnownMetadataProvider projects InstanceDiscoveryMetadataEntry.Aliases directly from KnownCloudData (string[]). Because InstanceDiscoveryMetadataEntry.Aliases is a mutable array with a public setter, any internal code that mutates metadata.Aliases would also mutate the KnownCloudData source array (shared reference), undermining the “single source of truth” immutability expectation. Consider cloning the aliases array when projecting into InstanceDiscoveryMetadataEntry.
                var entry = new InstanceDiscoveryMetadataEntry()
                {
                    Aliases = cloud.Aliases,
                    PreferredNetwork = cloud.PreferredNetwork,
                    PreferredCache = cloud.PreferredCache,

/// </param>
public InMemoryCloudConfiguration(ICloudConfiguration fallback = null)
{
_fallback = fallback;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the logic to decide the value of fallback?

/// taking precedence per key and <paramref name="lower"/> supplying any keys the higher layer does
/// not set. Either argument may be <c>null</c>.
/// </summary>
internal static CloudSettings Merge(CloudSettings lower, CloudSettings higher)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is there a need to merge the cloud settings? Cross cloud scenarios? Maybe rename to fallback and override here as well. I am not clear why is this needed

return higher;
}

var mergedValues = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point both are null. The following is dead code loops

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants