Throw on Authority vs Instance/TenantId conflict (OIDC + MSAL parity)#3873
Merged
Conversation
5f54ccf to
51149d0
Compare
bgavrilMS
reviewed
Jun 18, 2026
bgavrilMS
approved these changes
Jun 18, 2026
bgavrilMS
approved these changes
Jun 22, 2026
…th MSAL conflict tests)
Add WebAppExtensionsAuthorityConflictTests, a complete counterpart to
MergedOptionsAuthorityConflictTests covering the OIDC sign-in code path
in MicrosoftIdentityWebAppAuthenticationBuilderExtensions when Authority
is configured alongside Instance, TenantId, Domain, or
SignUpSignInPolicyId.
The OIDC sign-in path and the MSAL token-acquisition path currently
exhibit different precedence and logging semantics:
Token-acquisition (MergedOptions.ParseAuthorityIfNecessary):
- Instance + TenantId WIN over Authority when both are configured.
- Logs MergedOptionsLogging.AuthorityIgnored (EventId 500) on conflict.
- Logs MergedOptionsLogging.AuthorityUsedConsiderInstanceTenantId
(EventId 501) when Authority alone is used.
OIDC sign-in (this class):
- Authority WINS verbatim when set; Instance + TenantId only compose
a fallback Authority via AuthorityHelpers.BuildAuthority when no
Authority is configured.
- Logs neither EventId 500 nor EventId 501.
The 17 tests added here pin each of these behaviors for AAD, B2C, and
CIAM scenarios, plus a robustness test for the no-logger-registered case:
AAD precedence (5):
OidcSignIn_AuthorityAndInstance_PinsAuthorityWins
OidcSignIn_AuthorityAndTenantId_PinsAuthorityWins
OidcSignIn_AuthorityAndInstanceAndTenantId_PinsAuthorityWins
OidcSignIn_AuthorityOnly_PropagatesAuthorityAsIs (control)
OidcSignIn_InstanceAndTenantIdOnly_ComposesAuthorityFromInstanceTenantId (control)
B2C precedence (2):
OidcSignIn_B2CAuthorityAndInstance_PinsAuthorityWins
OidcSignIn_B2CInstanceAndDomain_ComposesAuthorityFromInstanceDomainUserFlow (control)
CIAM precedence (2):
OidcSignIn_CiamAuthorityAndInstance_PinsAuthorityWins
OidcSignIn_CiamAuthorityOnly_PropagatesAuthorityThroughCiamHelper (control)
Log assertions / bug evidence (7):
OidcSignIn_AuthorityAndInstance_DoesNotLogAuthorityIgnoredWarning
OidcSignIn_AuthorityAndTenantId_DoesNotLogAuthorityIgnoredWarning
OidcSignIn_AuthorityAndInstanceAndTenantId_DoesNotLogAuthorityIgnoredWarning
OidcSignIn_AuthorityOnly_DoesNotLogAuthorityUsedHint
OidcSignIn_InstanceAndTenantIdOnly_DoesNotLogAnyAuthorityWarning
OidcSignIn_B2CAuthorityAndInstance_DoesNotLogAuthorityIgnoredWarning
OidcSignIn_CiamAuthorityAndInstance_DoesNotLogAuthorityIgnoredWarning
Robustness (1):
OidcSignIn_AuthorityAndInstance_DoesNotThrowWhenNoLoggerRegistered
These are PINNING tests: they document what the code does today, not
what it should do. They will fail intentionally if the OIDC sign-in
path is changed to mirror the token-acquisition path -- that failure is
the signal a behavior fix has landed. Update assertions deliberately
when that happens and preserve the rationale in test names and comments.
Test infrastructure:
- BuildAndGetOidcOptions: per-test helper that wires up
AddMicrosoftIdentityWebApp with a synthetic IConfigurationSection
containing only the keys specified by the caller, builds the
ServiceProvider, and resolves OpenIdConnectOptions.
- TestLogger / TestLoggerProvider: ILoggerProvider that aggregates
every category-specific ILogger call into a single Entries list,
enabling DoesNotContain assertions over (LogLevel, EventId.Id, Message).
Verified: dotnet test on net10.0 -- 17/17 pass.
Regression check across WebAppExtensionsTests, MergedOptionsAuthorityConflictTests,
PopulateOpenIdOptionsFromMergedOptionsTests, AuthorityHelpersTests,
MergedOptionsAuthorityParsingTests, CiamAuthorityHelperTest, and this
new class: 173/173 pass.
Local repro notes (unrelated to this commit):
- Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.cs:77
has a pre-existing CS8604 caught by the .NET 10 SDK that
Directory.Build.props' TreatWarningsAsErrors=true promotes to an error.
Workaround: -p:TreatWarningsAsErrors=false until that nullability
bug is fixed separately.
- The repo's UI / TokenAcquisition projects need -p:TargetNetNext=True
to include the net10.0 TFM
(see Microsoft.Identity.Web.UI.csproj line 9 and similar).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the silent EventId 500 warning with InvalidOperationException when the user explicitly configures both Authority and Instance/TenantId. Implementation: - Add AuthorityExplicitlyConfigured latch to MergedOptions (set when Authority is written from MicrosoftIdentityOptions or JwtBearerOptions, NOT set when backfilled by MicrosoftEntraApplicationOptions getter) - Add _authorityParsed flag to make ParseAuthorityIfNecessary idempotent (avoids false-positive on second call after Authority is parsed into Instance+TenantId) - Throw in ParseAuthorityIfNecessary (MSAL path) when latch is set and both Authority + Instance/TenantId are present - Throw in OIDC sign-in path (MicrosoftIdentityWebAppAuthenticationBuilder Extensions) with same check, before Authority is consumed Test updates: - Convert conflict pinning tests to Assert.Throws<InvalidOperationException> - Add SyntheticAuthority_NoThrow test for computed-getter scenario - Fix existing tests that incidentally created conflicts (redirect URI, regions, metadata address tests) Fixes #3869 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Deduplicates the conflict check + throw from ParseAuthorityIfNecessary and the OIDC extensions into a single instance method. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ty+Instance Same pattern as redirect URI and regions tests: remove Instance from ConfidentialClientApplicationOptions since these tests verify cache-key isolation, not authority resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…itive throws When only Authority is configured, the library internally derives Instance/TenantId (via ParseAuthorityIfNecessary and computed getters). The single-latch design incorrectly threw on this valid scenario. The dual-latch approach requires both Authority AND Instance/TenantId to come from user config before throwing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Exercise the UpdateMergedOptions* pipeline (MicrosoftIdentityOptions and ConfidentialClientApplicationOptions) so the AuthorityExplicitlyConfigured / InstanceOrTenantIdExplicitlyConfigured latches are set through the real merge code path, not manually. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3a0c7aa to
db0f75e
Compare
bgavrilMS
approved these changes
Jun 22, 2026
iarekk
added a commit
that referenced
this pull request
Jun 22, 2026
Reflect PR #3873 behavior change -- mixing Authority with Instance/TenantId now throws InvalidOperationException at startup instead of silently ignoring Authority with a warning. Updated across all doc files: - Decision tree, precedence table, error section (authority-configuration.md) - Common mistakes sections (b2c, ciam examples) - FAQ questions about conflicts and upgrades - Migration guide: warning -> exception language - README: removed PreserveAuthority from link text - Changelog: updated entry Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
iarekk
added a commit
that referenced
this pull request
Jun 22, 2026
Reflect PR #3873 behavior change -- mixing Authority with Instance/TenantId now throws InvalidOperationException at startup instead of silently ignoring Authority with a warning. Updated across all doc files: - Decision tree, precedence table, error section (authority-configuration.md) - Common mistakes sections (b2c, ciam examples) - FAQ questions about conflicts and upgrades - Migration guide: warning -> exception language - README: removed PreserveAuthority from link text - Changelog: updated entry Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jun 22, 2026
iarekk
added a commit
that referenced
this pull request
Jun 23, 2026
This was referenced Jun 24, 2026
Merged
This was referenced Jun 24, 2026
Open
Open
Merged
iarekk
added a commit
that referenced
this pull request
Jun 24, 2026
This was referenced Jun 24, 2026
Merged
Closed
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
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.
Summary
Replaces the silent EventId 500 warning with
InvalidOperationExceptionwhen bothAuthorityandInstance/TenantIdare explicitly configured. Both the OIDC sign-in path and the MSAL token-acquisition path now throw the same exception, resolving the behavioral divergence documented in the pinning tests (#3855).Problem
Previously, configuring both
AuthorityandInstance/TenantIdproduced different outcomes depending on the code path:Neither path surfaced the conflict to the developer at startup time.
Solution
Dual-latch provenance tracking on
MergedOptionsTwo latch properties track whether Authority and Instance/TenantId came from user config vs internal derivation:
AuthorityExplicitlyConfigured-- set when Authority is written from user config (MicrosoftIdentityOptionsorJwtBearerOptions), NOT when backfilled byMicrosoftEntraApplicationOptionscomputed getterInstanceOrTenantIdExplicitlyConfigured-- set when Instance or TenantId is written from user config (MicrosoftIdentityOptionsorConfidentialClientApplicationOptions), NOT when derived byParseAuthorityIfNecessaryor the computedAuthoritygetterBoth latches use a set-only pattern (once true, never reverts to false).
Conflict detection
ThrowIfAuthorityConflict()helper onMergedOptions-- throwsInvalidOperationExceptiononly when both latches are set and both sides are populated. Shared by MSAL and OIDC paths._authorityParsedflag -- makesParseAuthorityIfNecessaryidempotent (avoids false-positive on repeated calls after Authority is decomposed into Instance+TenantId)Merge point coverage
The
InstanceOrTenantIdExplicitlyConfiguredlatch is set at 4 user-config merge points:UpdateMergedOptionsFromMicrosoftIdentityOptions-- InstanceUpdateMergedOptionsFromMicrosoftIdentityOptions-- TenantIdUpdateMergedOptionsFromConfidentialClientApplicationOptions-- InstanceUpdateMergedOptionsFromConfidentialClientApplicationOptions-- TenantIdThree synthetic merge points intentionally do NOT set the latch:
ParseAuthorityIfNecessary(decomposes Authority into Instance+TenantId)UpdateMergedOptionsFromMicrosoftIdentityApplicationOptions-- Instance (computed getter)UpdateMergedOptionsFromMicrosoftIdentityApplicationOptions-- TenantId (computed getter)Test changes
Assert.Throws<InvalidOperationException>SyntheticAuthority_NoThrowtest (computed getter scenario = no conflict)AuthorityExplicitlyConfigured+InstanceOrTenantIdExplicitlyConfigured)E2E validation
Manually tested with a learning app across all config scenarios:
InvalidOperationExceptionat startup with clear messageFixes #3869
Builds on #3855
Once this is merged, will improve the docs in #3617 and merge that one