Potential fix for issue highlighted in parent branch#6016
Closed
Avery-Dunn wants to merge 6 commits into
Closed
Conversation
…de flows When SendCertificateOverMtls=true, MSAL previously only routed AcquireTokenForClient to the mTLS endpoint (mtlsauth.microsoft.com) and suppressed client_assertion from the POST body. User flows (OBO, refresh_token, auth_code) fell through to the regular login endpoint with a client_assertion JWT. This change extends the feature to all three user flows by calling TryInitMtlsPopParametersAsync in each executor path, mirroring the existing AcquireTokenForClient behaviour. Changes: - ConfidentialClientExecutor: add TryInitMtlsPopParametersAsync to OBO and auth_code executor paths - ClientApplicationBaseExecutor: add TryInitMtlsPopParametersAsync to the refresh_token (IByRefreshToken) executor path - RegionAndMtlsDiscoveryProvider: attempt region discovery for mTLS-enabled user flows when the app has configured WithAzureRegion, so regional mTLS endpoints (e.g. eastus.mtlsauth.microsoft.com) are used for OBO/RT - TokenCache: use OriginalAuthority for cache alias resolution so that mTLS-transformed (mtlsauth.*) endpoints do not propagate into cache lookups Tests: - MtlsBearerUserFlowTests.cs: 4 unit tests (OBO global/regional mTLS, RT global mTLS, regression for non-mTLS cert credential) - MtlsTransportUserFlowTests.cs: updated integration tests asserting both mTLS transport conditions (mtlsauth endpoint + no client_assertion) for OBO and RT Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CertHelper.GetOrCreateTestCert() returns a static cached instance. Calling Dispose() in ClassCleanup poisons the cache, causing MtlsPopTests.ClassInitialize to receive a disposed X509Certificate2 (m_safeCertContext is an invalid handle) when it runs alphabetically after MtlsBearerUserFlowTests. CertHelper owns the certificate lifetime; test classes must not dispose certs obtained from it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Clear MSAL_FORCE_REGION in regional unit test for defensive isolation - Clarify assertion messages to specify GetHttpClient(X509Certificate2) overload Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix 'requestwith' typo in XML doc - Clarify that ExpectedPostData checks client_assertion_type (not the client_assertion value itself, which is a dynamically generated JWT) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Flow test, no Console.WriteLine - MtlsTransportUserFlowTests: replace secret-based OBO/RT factory tests with cert+SendCertificateOverMtls=true (OboFlow_WithSendCertificateOverMtls_AcquiresTokenAsync, RefreshTokenFlow_WithSendCertificateOverMtls_AcquiresTokenAsync), making them true mTLS integration tests that assert on both the mTLS endpoint and factory invocation - Remove SilentFlow_WithMtlsTransportFactory_UsesRefreshTokenOverMtlsAsync: it attached IMsalMtlsHttpClientFactory to a PCA (public client), which does not perform cert-based client authentication; the test did not exercise the feature being changed - Remove _oboClientSecret, _keyVault, and secret-based TestInitialize; credentials are now the lab cert via SendCertificateOverMtls across all tests - Remove all Console.WriteLine calls; diagnostic context is embedded in Assert messages - MtlsBearerUserFlowTests: rename regional unit test to UserFlow_WithSendCertificateOverMtls_WithRegion_UsesRegionalMtlsEndpointAsync and clarify in XML doc that it is a general-purpose regional routing test (shared code path across all user flows), not an OBO-specific test
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
2 participants
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 need for merging/review, just using this to show a potential fix for the issue highlighted by this review comment in the parent branch: #6009 (comment)