Conversation
added 5 commits
November 8, 2015 04:41
separate extension project
cache and iplatformparameters as property
update docs
kpanwar
pushed a commit
that referenced
this pull request
Dec 4, 2015
Merged
7 tasks
3 tasks
7 tasks
5 tasks
Closed
7 tasks
1 task
Merged
1 task
Robbie-Microsoft
added a commit
that referenced
this pull request
May 21, 2026
Bug #1 (cache crash on 2nd mTLS call): - TokenCache.ITokenCacheInternal.cs: FilterTokensByEnvironmentAsync and FindRefreshTokenAsync used requestParams.AuthorityInfo for alias resolution. After ResolveAuthorityAsync(), AuthorityInfo.Host is 'mtlsauth.microsoft.com', which causes RegionAndMtlsDiscoveryProvider to throw MtlsPopNotSupportedForEnvironment. - Fix: use requestParams.AuthorityManager.OriginalAuthority.AuthorityInfo (same pattern already applied to GetTenantProfilesAsync in this file). - Added regression test: OboFlow_WithSendCertificateOverMtls_SecondCallDoesNotCrashAsync Bug #2 (AcquireTokenSilent does not route RT redemption to mTLS endpoint): - ClientApplicationBaseExecutor.cs: the AcquireTokenSilentParameters overload never called TryInitMtlsPopParametersAsync, so IsMtlsRequested=false and RT redemption went to login.microsoftonline.com instead of mtlsauth.microsoft.com. - Fix: add TryInitMtlsPopParametersAsync call before CreateRequestContextAndLogVersionInfo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gladjohn
added a commit
that referenced
this pull request
Jun 1, 2026
…aims error message - CredentialMaterialResolver / CertificateAndClaimsClientCredential: Document the single-invocation invariant (issue #5943) at both the resolver short-circuit site and inside the credential, so future CertificateAndClaimsClientCredential subclasses understand they must keep mTLS-mode output equal to (empty, cert) — overriding the method alone will be silently bypassed by the resolver at runtime. Addresses Robbie's review comment #1 (Option 2). - MtlsPopParametersInitializer.cs: Correct the Case 1 comment in TryInitImplicitBearerOverMtlsAsync — the polymorphic resolve is reachable only because ConfidentialClientApplicationBuilder.Validate rejects the non-cert pairing at construction time; there is no Bearer-path fallback. Addresses Robbie's review comment #2. - CertificateAndClaimsClientCredential / MsalErrorMessage: When mTLS PoP is combined with WithClientClaims, throw the existing MsalError.MtlsCertificateNotProvided (unchanged code, preserves MtlsPopWithoutCertificateWithClientClaimsAsync) with a new MsalErrorMessage.MtlsPopNotSupportedWithClientClaimsMessage that describes the actual WithClientClaims + mTLS conflict instead of the misleading "callback returned null" string. Addresses Robbie's review comment #3 (Option 2). Full local unit test suite: 2055/2055 passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gladjohn
added a commit
that referenced
this pull request
Jun 1, 2026
…aims error message - CredentialMaterialResolver / CertificateAndClaimsClientCredential: Document the single-invocation invariant (issue #5943) at both the resolver short-circuit site and inside the credential, so future CertificateAndClaimsClientCredential subclasses understand they must keep mTLS-mode output equal to (empty, cert) — overriding the method alone will be silently bypassed by the resolver at runtime. Addresses Robbie's review comment #1 (Option 2). - MtlsPopParametersInitializer.cs: Correct the Case 1 comment in TryInitImplicitBearerOverMtlsAsync — the polymorphic resolve is reachable only because ConfidentialClientApplicationBuilder.Validate rejects the non-cert pairing at construction time; there is no Bearer-path fallback. Addresses Robbie's review comment #2. - CertificateAndClaimsClientCredential / MsalErrorMessage: When mTLS PoP is combined with WithClientClaims, throw the existing MsalError.MtlsCertificateNotProvided (unchanged code, preserves MtlsPopWithoutCertificateWithClientClaimsAsync) with a new MsalErrorMessage.MtlsPopNotSupportedWithClientClaimsMessage that describes the actual WithClientClaims + mTLS conflict instead of the misleading "callback returned null" string. Addresses Robbie's review comment #3 (Option 2). Full local unit test suite: 2055/2055 passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gladjohn
added a commit
that referenced
this pull request
Jun 2, 2026
…ateClientCredential) (#5957) * draft * fix * Resolver consolidation + Path C: single provider invocation per mTLS PoP request Consolidates ResolveCertificateForPreflightAsync into a parameterized ResolveCertificateForMtlsAsync (single resolver, optional nullErrorCode + nullErrorMessage). Adds non-AAD authority guard on TenantId computation. Plumbs preflight-resolved cert through CredentialContext.PreResolvedCertificate so credential material resolution reuses it instead of invoking the provider delegate again. Honors the single-invocation principle from issue #5943 ("a token request should generate at most 1 credential resolution"). Test now asserts providerCallCount == 1 to lock in the invariant. Removes references to #5886 (closed not_planned on 2026-05-11). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Eliminate concrete-credential downcasts in mTLS PoP initializer Addresses review feedback on #5957: replaces the `is X || is Y` downcast chain in MtlsPopParametersInitializer.InitExplicitMtlsPopAsync with a single polymorphic call through the existing IClientCredential.GetCredentialMaterialAsync(Mode=Mtls) abstraction introduced in #5835. - MtlsPopParametersInitializer.cs: InitExplicitMtlsPopAsync no longer downcasts to concrete credential types. All credentials are resolved through one helper, ResolveMtlsMaterialAsync, which builds a preflight CredentialContext with Mode=Mtls. Internal InvalidCredentialMaterial errors are translated to the public MtlsCertificateNotProvided code to preserve the existing public mTLS PoP API contract. InitMtlsPopParametersAsync unchanged, preserving the IAuthenticationOperation3.AfterCredentialEvaluationAsync lifecycle from #5996. - CertificateAndClaimsClientCredential.cs: adds a _claimsToSign != null guard in the Mtls branch (preserves WithClientClaims + WithMtlsProofOfPossession rejection). ResolveCertificateAsync is now mode-aware on null cert. Drops the now-dead ResolveCertificateForMtlsAsync helper. - CredentialMatrixTests.cs: null-cert assertions split per mode; WithClaims test renamed to assert MtlsCertificateNotProvided throw. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * address pr comments * Address PR review (#5957): document mTLS invariant + fix WithClientClaims error message - CredentialMaterialResolver / CertificateAndClaimsClientCredential: Document the single-invocation invariant (issue #5943) at both the resolver short-circuit site and inside the credential, so future CertificateAndClaimsClientCredential subclasses understand they must keep mTLS-mode output equal to (empty, cert) — overriding the method alone will be silently bypassed by the resolver at runtime. Addresses Robbie's review comment #1 (Option 2). - MtlsPopParametersInitializer.cs: Correct the Case 1 comment in TryInitImplicitBearerOverMtlsAsync — the polymorphic resolve is reachable only because ConfidentialClientApplicationBuilder.Validate rejects the non-cert pairing at construction time; there is no Bearer-path fallback. Addresses Robbie's review comment #2. - CertificateAndClaimsClientCredential / MsalErrorMessage: When mTLS PoP is combined with WithClientClaims, throw the existing MsalError.MtlsCertificateNotProvided (unchanged code, preserves MtlsPopWithoutCertificateWithClientClaimsAsync) with a new MsalErrorMessage.MtlsPopNotSupportedWithClientClaimsMessage that describes the actual WithClientClaims + mTLS conflict instead of the misleading "callback returned null" string. Addresses Robbie's review comment #3 (Option 2). Full local unit test suite: 2055/2055 passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Robbie's round-2 review (PR #5957) Fixes 5 follow-up review comments after fdff674 (squashed in rebase as 7af3ea0). One real diagnostic bug + one regression test + doc tightening: * MsalErrorMessage.cs: - Renamed MtlsPopNotSupportedWithClientClaimsMessage to MtlsNotSupportedWithClientClaimsMessage and generalized the wording. The guard that uses this message fires for *every* mTLS-mode resolution path — explicit .WithMtlsProofOfPossession() AND implicit CertificateOptions.SendCertificateOverMtls = true. The old wording falsely blamed Proof-of-Possession even when the user only opted into Bearer-over-mTLS, producing a confusing diagnostic. * CertificateAndClaimsClientCredential.cs: - Updated reference to the renamed constant. - Tightened the in-line comment above the guard to call out that the rejection fires for both explicit PoP and implicit Bearer-over-mTLS, so the message wording is intentional. * MtlsPopParametersInitializer.cs: - Added the 4th sub-bullet to the <remarks> on InitExplicitMtlsPopAsync covering the WithClientClaims direct-throw path through CertificateAndClaimsClientCredential.GetCredentialMaterialAsync. The list now describes all four GetCredentialMaterialAsync(Mtls) outcomes: cert credentials, signed-assertion credentials, client-claims (direct throw), and unsupported credentials (translated via the catch). * MtlsPopTests.cs: - Tightened MtlsPopWithoutCertificateWithClientClaimsAsync to assert StringAssert.Contains(ex.Message, "WithClientClaims"). A future error-message centralisation refactor cannot silently revert the diagnostic without failing this test. - Added SendCertificateOverMtls_WithClientClaims_ThrowsClearMessageAsync as a regression test for the previously-uncovered combo: WithCertificate(cert, CertificateOptions { SendCertificateOverMtls = true }) + WithClientClaims(cert, claims), *no* .WithMtlsProofOfPossession(). The new test asserts the message names both transports and the WithClientClaims API so the diagnostic stays actionable for the Bearer-over-mTLS path. Build: 0 warnings, 0 errors, 4 TFMs. Test (Microsoft.Identity.Test.Unit, net8.0): 2119/2119 passed, 19 skipped (platform-gated). Includes the new regression test and the tightened existing test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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.
No description provided.