Pin OIDC sign-in Authority precedence (parity with MSAL conflict tests)#3855
Open
iarekk wants to merge 1 commit into
Open
Pin OIDC sign-in Authority precedence (parity with MSAL conflict tests)#3855iarekk wants to merge 1 commit into
iarekk wants to merge 1 commit into
Conversation
9745122 to
0220e3d
Compare
…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>
0220e3d to
173f916
Compare
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.
Add 17 pinning tests for the OIDC sign-in code path with parity to the MSAL conflict suite.
Description
MergedOptionsAuthorityConflictTestscovers the MSAL token-acquisition path whenAuthorityis configured alongsideInstance,TenantId,Domain, orSignUpSignInPolicyId. There is currently no equivalent coverage for the OIDC sign-in path inMicrosoftIdentityWebAppAuthenticationBuilderExtensions, and the two paths exhibit different precedence and logging semantics today:MergedOptions.ParseAuthorityIfNecessary)AddMicrosoftIdentityWebApp->Configure<OpenIdConnectOptions>)Authority+Instance/TenantId/bothInstance+TenantIdwin;Authorityignored; logsAuthorityIgnored(EventId 500)Authoritywins verbatim;Instance/TenantIdare dead config; no warningAuthorityonlyAuthorityintoInstance+TenantId; logsAuthorityUsedConsiderInstanceTenantId(EventId 501)Authorityverbatim (afterEnsureAuthorityIsV2); no parsing, no hintInstance+TenantIdonlyAuthorityHelpers.BuildAuthoritycomposesInstance/TenantId/v2.0This divergence is observable to library users and isn't documented anywhere we can find. The downstream docs (and PR #3617) describe the
Instance+TenantId-wins precedence as if it were universal, but it only holds for the token-acquisition path.When each code path runs in a sample app
A typical ASP.NET Core web app that signs users in and calls a downstream API exercises both paths -- but at different times.
OIDC sign-in path (Authority wins, no warning)
Triggered on every sign-in challenge by the ASP.NET Core authentication middleware.
No call to
ParseAuthorityIfNecessaryanywhere on this path; no EventId 500 / 501 ever fires.MSAL token-acquisition path (Instance+TenantId wins, EventId 500 logged on conflict)
Triggered later, only when the app actually asks for an access token.
ParseAuthorityIfNecessaryis called from three hosts in the same way:TokenAcquisitionAspnetCoreHost.cs:61,DefaultTokenAcquisitionHost.cs:54, andOwinTokenAcquisitionHost.cs:56.Net effect
For an app configured with both
AuthorityandInstance+TenantId:Authority(OIDC path).Instance+TenantId(MSAL path) -- silently, with EventId 500 in the logs only if you have Information-level logging enabled.If those two values disagree, the user signs in against one tenant and gets tokens for another. That mismatch is what these tests pin.
What this PR adds
A single new test class
WebAppExtensionsAuthorityConflictTests(455 lines, 17 tests) covering:Authority+Instance,Authority+TenantId,Authority+both,Authority-only (control),Instance+TenantId-only (control)Authority+Instance+Domain+SignUpSignInPolicyIdconflict, plusInstance+Domain+UserFlowcomposition controlAuthority+Instanceconflict, plusAuthority-only-through-BuildCiamAuthorityIfNeededcontrolDoesNotContain(EventId.Id == 500)/== 501checks for each conflict scenario across AAD/B2C/CIAM -- the strongest single piece of evidence that the divergence is realNoLogger_NoException-- builds aServiceProviderwith noAddLogging()registered and asserts no exceptionEvery test carries a comment of one of three shapes so the file can be skim-reviewed against the MSAL suite:
// MSAL parity: <MethodName>. Divergence: ...(11 tests)// MSAL parity: <MethodName>. Same outcome on both paths.(3 control / robustness tests)// No MSAL counterpart; ...(the 2 OIDC-only tests)