Credential system refactor: unified context, canonical matrix, eliminate dead code by Copilot · Pull Request #5748 · AzureAD/microsoft-authentication-library-for-dotnet

Copilot · 2026-02-10T16:16:39Z

Changes proposed in this request

Comprehensive refactor addressing 32 code review comments. Simplifies architecture, enforces canonical credential×mode matrix, eliminates unused telemetry infrastructure.

Architecture

Single ClientAuthMode enum replaces MtlsRequired/MtlsBearerMode booleans
Merged MtlsValidationContext into CredentialContext (renamed from CredentialRequestContext)
CancellationToken now explicit parameter, not in context struct
Output: MtlsCertificate → ResolvedCertificate (mode-agnostic naming)
CredentialMaterialOrchestrator → CredentialMaterialResolver

Dead code removed (~370 LOC)

CredentialMaterialMetadata, CredentialMaterialHelper, MtlsValidationContext classes
All Stopwatch instrumentation (data never consumed)
Certificate hash prefix computation
Runtime reserved parameter validation (replaced with tests)

Canonical matrix enforcement

Input         Mode      Output            Enforcement
────────────────────────────────────────────────────
X509Cert      Regular   JWT (jwt-bearer)  ✅
X509Cert      MtlsMode  cert only         ✅
secret        Regular   client_secret     ✅
secret        MtlsMode  NOT SUPPORTED     ❌ MsalClientException
jwt           Regular   jwt-bearer        ✅
jwt           MtlsMode  NOT SUPPORTED     ❌ MsalClientException
jwt+cert      Regular   NOT SUPPORTED     ❌ MsalClientException
jwt+cert      MtlsMode  jwt-pop + cert    ✅

Matrix documented in CREDENTIAL_MATRIX.cs. Credentials validate mode compatibility, throw MsalClientException for unsupported combinations. Resolver throws InvalidOperationException for invariant violations (null material/params).

Correctness fixes

jwt-pop logic: use when TokenBindingCertificate present (confirmation claim presence determines type). Fixes SNI assertion flow AADSTS3921994
Exception wrapping: CryptographicException → MsalClientException for disposed certificates
Type safety: CredentialSource string → enum

Output guarantees

TokenRequestParameters never null (empty dict for TLS-only scenarios)
ResolvedCertificate set whenever cert obtained (any mode)

Testing

Added CredentialMatrixTests.cs with 16 tests:

All 10 matrix rows (supported paths + unsupported combinations)
Output guarantees (non-null params, correct assertion types)
Exception taxonomy validation

Existing integration tests pass unchanged.

Performance impact

Removed ~100ns Stopwatch overhead per credential resolution.

Documentation

All relevant documentation is updated.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

…eters Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

gladjohn · 2026-02-10T17:03:33Z

@copilot build error "src\client\Microsoft.Identity.Client\MsalError.cs(1215,29): Error RS0016: Symbol 'InvalidCredentialMaterial' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)"

…er error Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Copilot · 2026-02-10T17:06:13Z

@copilot build error "src\client\Microsoft.Identity.Client\MsalError.cs(1215,29): Error RS0016: Symbol 'InvalidCredentialMaterial' is not part of the declared public API (https://github.com/dotnet/ros...

Fixed in d09d962. Added InvalidCredentialMaterial to all PublicAPI.Unshipped.txt files (net8.0, net472, net462, net8.0-android, net8.0-ios, netstandard2.0). Build now passes with no RS0016 errors.

…CredentialMaterialAsync via orchestrator Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

…credential system Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

…icException Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

bgavrilMS · 2026-02-12T11:50:00Z

+        public long ResolutionTimeMs { get; }
+    }
+
+    internal enum CredentialType


Classes already implement an interface, so there is no need to hold duplicate information. We should at most use this for quick telemetry resolution.

In #5835 this was taken further: CredentialSource is no longer part of CredentialMaterial; the only remaining cleanup there is deleting the unused enum file.

bgavrilMS · 2026-02-12T11:50:53Z

+        /// <summary>
+        /// Whether mTLS binding was requested (MtlsRequired in context).
+        /// </summary>
+        public bool MtlsCertificateRequested { get; }


It is not the credential's concern. It should not be part of the state of the credential.

Carried forward in #5835 by consolidating the credential-resolution inputs into CredentialContext and removing mode state from the credential itself.

bgavrilMS · 2026-02-12T11:51:15Z

+        /// Hash prefix of mTLS certificate (first 8-16 chars of SHA-256 hash of RawData).
+        /// Not full thumbprint; used for correlation only.
+        /// </summary>
+        public string MtlsCertificateIdHashPrefix { get; }


Do we really need to cache this?

In #5835 the token endpoint is resolved once in TokenClient and reused; it is not cached within the credential or context.

bgavrilMS · 2026-02-12T11:52:04Z

+        public string CredentialSource { get; }
+
+        /// <summary>
+        /// Hash prefix of mTLS certificate (first 8-16 chars of SHA-256 hash of RawData).


How can the hash prefix be useful ? it is not a good identifier. You cannot guarantee that the first 8-16 chars of a hash provide reasonable uniquness.

Carried forward in #5835; the hash-prefix field is no longer part of the material model.

bgavrilMS · 2026-02-12T11:55:47Z

+        /// <summary>
+        /// Cancellation token for async operations.
+        /// </summary>
+        public CancellationToken CancellationToken { get; init; }


Keep this separate pls. CancellationToken is always the last param of an async method.

Carried forward in #5835; CancellationToken is no longer part of CredentialContext and is passed explicitly to the async methods.

bgavrilMS · 2026-02-12T11:57:07Z

+        /// <summary>
+        /// FMI path for client assertion (Federated Managed Identity).
+        /// </summary>
+        public string ClientAssertionFmiPath { get; init; }


It is not used?

In #5835 unused fields have been removed as part of the overall cleanup and refactoring. @copilot

bgavrilMS · 2026-02-12T12:12:58Z

+            // Validate structure
+            if (material == null)
+            {
+                throw new MsalClientException(


This is not a good exception. CX can't do anything about this. This is a bug in MSAL, so InvalidOperationException is more appropiate.

Carried forward in #5835; internal invariants now use InvalidOperationException, while invalid credential/mode combinations still throw MsalClientException.

bgavrilMS · 2026-02-12T12:13:10Z

+                    "Credential returned null material.");
+            }
+
+            if (material.TokenRequestParameters == null)


Not a good exception.

Carried forward in #5835; the exception taxonomy has been refined so that internal invariant failures use InvalidOperationException.

bgavrilMS · 2026-02-12T12:13:39Z

+    /// - Validating material structure
+    /// - Enforcing mTLS constraints
+    /// </summary>
+    internal static class CredentialMaterialOrchestrator


CredentialResolver ?

But I don't see the point of this class. ICredential should have ResolveAsync

Carried forward in #5835; the active type is CredentialMaterialResolver.

bgavrilMS · 2026-02-12T14:30:55Z

+    /// Contains only what credentials need to produce material.
+    /// Immutable by design.
+    /// </summary>
+    internal readonly record struct CredentialRequestContext


Don't we already have a public class for this ? AssertionRequestOptions ? Can we add more stuff to that one instead - with internal? Objects (even data objects) are expensive.

carried forward in #5835 with a small internal CredentialContext

bgavrilMS · 2026-02-12T14:32:36Z

+        /// When true, certificate should be used for TLS client auth only (mTLS bearer mode).
+        /// No JWT client_assertion should be sent.
+        /// </summary>
+        public bool MtlsBearerMode { get; init; }


It looks like you went with 2 flags. This is confusing. I would not have any flags in this object. Instead, the IClientCreential can have a method like Task<CredentialResult> ResolveAsync(bool forAcquisitionOverMtls, CredentialContext context, CancellationToken token)

Carried forward in #5835 as OAuthMode; the credential resolution now takes the mode as an explicit parameter rather than storing flags in the context object.

- Replace MtlsRequired/MtlsBearerMode with ClientAuthMode enum - Rename CredentialRequestContext to CredentialContext - Merge MtlsValidationContext into CredentialContext - Rename MtlsCertificate to ResolvedCertificate - Remove CancellationToken from context, add as explicit parameter - Delete CredentialMaterialMetadata entirely - Remove CredentialMaterialHelper entirely - Add CredentialSource enum - Rename CredentialMaterialOrchestrator to CredentialMaterialResolver - Update exception taxonomy (InvalidOperationException for invariants) Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

gladjohn · 2026-03-25T15:51:50Z

@neha-bhargava @bgavrilMS All comments from #5748 are now accounted for with corresponding addressed items in #5835.

Initial plan

ea565a8

Copilot AI assigned Copilot and gladjohn Feb 10, 2026

Copilot started work on behalf of gladjohn February 10, 2026 16:17 View session

Copilot AI and others added 2 commits February 10, 2026 16:26

Implement Phase 1 core types and credential implementations

169aee8

Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Add ResolvedCredentialMaterial property to AuthenticationRequestParam…

406134b

…eters Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Implement complete code for Phase 1 components~~ Phase 1: Complete credential material system with working implementations Feb 10, 2026

Copilot AI requested a review from gladjohn February 10, 2026 16:29

Copilot finished work on behalf of gladjohn February 10, 2026 16:29

Copilot started work on behalf of gladjohn February 10, 2026 17:04 View session

Add InvalidCredentialMaterial to PublicAPI files to fix RS0016 analyz…

d09d962

…er error Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Copilot finished work on behalf of gladjohn February 10, 2026 17:07

Copilot started work on behalf of gladjohn February 10, 2026 20:24 View session

Remove legacy AddConfidentialClientParametersAsync method and use Get…

bd17fea

…CredentialMaterialAsync via orchestrator Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Copilot AI changed the title ~~Phase 1: Complete credential material system with working implementations~~ Remove legacy AddConfidentialClientParametersAsync - use orchestrator path only Feb 10, 2026

Copilot finished work on behalf of gladjohn February 10, 2026 20:32

Copilot started work on behalf of gladjohn February 10, 2026 21:01 View session

Copilot AI and others added 2 commits February 10, 2026 21:06

Add ExtraClientAssertionClaims and ClientAssertionFmiPath support to …

eeec027

…credential system Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Add mTLS bearer mode support to skip JWT assertion when cert is pre-set

4f83a3c

Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Copilot AI changed the title ~~Remove legacy AddConfidentialClientParametersAsync - use orchestrator path only~~ Remove legacy credential method, use orchestrator exclusively Feb 10, 2026

Copilot finished work on behalf of gladjohn February 10, 2026 21:10

Copilot started work on behalf of gladjohn February 10, 2026 21:26 View session

Copilot AI changed the title ~~Remove legacy credential method, use orchestrator exclusively~~ Remove legacy credential method, migrate to orchestrator pattern exclusively Feb 10, 2026

Copilot finished work on behalf of gladjohn February 10, 2026 21:28

Copilot started work on behalf of gladjohn February 10, 2026 21:33 View session

Fix DisposedCert test - wrap HasPrivateKey check to catch Cryptograph…

ddfeee7

…icException Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Copilot AI changed the title ~~Remove legacy credential method, migrate to orchestrator pattern exclusively~~ Remove legacy credential method, migrate to orchestrator pattern Feb 10, 2026

Copilot finished work on behalf of gladjohn February 10, 2026 21:36

bgavrilMS reviewed Feb 12, 2026

View reviewed changes

Copilot started work on behalf of gladjohn February 13, 2026 13:53 View session

Copilot AI and others added 3 commits February 13, 2026 14:01

Add canonical matrix doc and enforce matrix constraints in credentials

0eb9dda

Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Add comprehensive unit tests for credential resolution matrix

cff4860

Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Copilot AI changed the title ~~Credential resolution via orchestrator pattern~~ Refactor credential system: canonical matrix, simplified architecture Feb 13, 2026

Copilot finished work on behalf of gladjohn February 13, 2026 14:17

Copilot started work on behalf of gladjohn February 13, 2026 15:21 View session

Add review response guide for 32 comments with commit references

a008344

Co-authored-by: gladjohn <90415114+gladjohn@users.noreply.github.com>

Copilot AI changed the title ~~Refactor credential system: canonical matrix, simplified architecture~~ Refactor credential system: canonical matrix, unified context, eliminate dead code Feb 13, 2026

Copilot finished work on behalf of gladjohn February 13, 2026 15:32

Copilot started work on behalf of gladjohn February 13, 2026 15:53 View session

Copilot AI changed the title ~~Refactor credential system: canonical matrix, unified context, eliminate dead code~~ Credential system refactor: unified context, canonical matrix, eliminate dead code Feb 13, 2026

Copilot finished work on behalf of gladjohn February 13, 2026 15:55

This was referenced Feb 18, 2026

Add request-level WithClientAssertion API and deprecate app-level methods #5762

Closed

Credential system refactor: unified context, request-level WithClientAssertion APIs, dual credential support #5763

Closed

Copilot AI mentioned this pull request Mar 10, 2026

Credential system refactor: unified context, canonical matrix, eliminate dead code #5831

Closed

1 task

gladjohn closed this Mar 10, 2026

gladjohn mentioned this pull request Mar 25, 2026

Refactor client credential material resolution #5835

Merged

Conversation

Copilot AI commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gladjohn commented Feb 10, 2026

Uh oh!

Copilot AI commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gladjohn commented Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Feb 10, 2026 •

edited

Loading

bgavrilMS Feb 12, 2026 •

edited

Loading