Skip to content

Refactor client credential material resolution#5835

Open
gladjohn wants to merge 6 commits intomainfrom
gladjohn/refactor
Open

Refactor client credential material resolution#5835
gladjohn wants to merge 6 commits intomainfrom
gladjohn/refactor

Conversation

@gladjohn
Copy link
Copy Markdown
Contributor

@gladjohn gladjohn commented Mar 10, 2026

What does this PR do?

This PR refactors client credential handling to separate credential material generation from token request application, while also addressing review feedback from the earlier iteration.

Main changes

Credential material refactor

  • Replaced the old ClientCredentialApplicationResult flow with:
    • CredentialMaterial
    • CredentialContext
    • CredentialMaterialResolver
    • CredentialSource
    • ClientAuthMode
  • Updated IClientCredential to return normalized credential material through GetCredentialMaterialAsync(...).

TokenClient update

  • Updated TokenClient to resolve the token endpoint once and pass that exact endpoint through credential resolution.
  • This avoids re-resolving the endpoint during credential material generation and ensures assertion generation uses the same endpoint as the outgoing token request.

Credential implementations

Updated credential implementations to follow the new model:

  • certificate credential
  • certificate + claims credential
  • secret credential
  • static signed assertion credential
  • string delegate assertion credential
  • delegate assertion credential with optional token-binding certificate

mTLS / PoP handling

  • Preserved the distinction between regular and mTLS-bound flows.
  • Enforced invalid combinations such as:
    • secret + mTLS
    • static signed assertion + mTLS
    • string-returning assertion delegate + mTLS
  • Supported delegate-returned ClientSignedAssertion with TokenBindingCertificate for JWT-PoP scenarios.

Logging

  • Added minimal credential-resolution diagnostics through CredentialContext.Logger.
  • Restored useful flow/error logging in credential implementations without logging sensitive certificate details such as thumbprint or subject.

Public surface / errors

  • Added:
    • MsalError.InvalidCredentialMaterial

Tests

  • Added CredentialMatrixTests covering the canonical credential matrix and edge cases.
  • Updated test contexts to provide a logger now that credential classes emit diagnostic logs.

Why

This change makes credential resolution more explicit and easier to reason about by:

  • separating “what the credential produces” from “how TokenClient applies it”
  • centralizing validation of returned credential material
  • making regular vs mTLS behavior clearer
  • addressing review feedback on endpoint selection and logging

Validation

  • Ran updated unit tests for credential matrix coverage

@gladjohn gladjohn requested a review from a team as a code owner March 10, 2026 21:41
Copilot AI review requested due to automatic review settings March 10, 2026 21:42
Copilot AI reviewed Mar 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors confidential client credential handling by introducing a normalized “credential material” model (context in → material out) and updating the token request pipeline to resolve/apply that material consistently, including mTLS/PoP constraints and diagnostics.

Changes:

  • Introduces CredentialContext, CredentialMaterial, CredentialMaterialResolver, CredentialSource, and ClientAuthMode; updates IClientCredential to return CredentialMaterial.
  • Updates TokenClient to resolve the token endpoint once and pass it through credential material resolution, then apply returned body parameters and resolved certificate.
  • Adds MsalError.InvalidCredentialMaterial and adds unit coverage via CredentialMatrixTests.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/Microsoft.Identity.Test.Unit/RequestsTests/CredentialMatrixTests.cs Adds matrix/edge-case tests for credential material resolution across Regular vs mTLS modes.
src/client/Microsoft.Identity.Client/PublicApi/netstandard2.0/PublicAPI.Unshipped.txt Public API addition for MsalError.InvalidCredentialMaterial.
src/client/Microsoft.Identity.Client/PublicApi/net8.0/PublicAPI.Unshipped.txt Public API addition for MsalError.InvalidCredentialMaterial.
src/client/Microsoft.Identity.Client/PublicApi/net8.0-ios/PublicAPI.Unshipped.txt Public API addition for MsalError.InvalidCredentialMaterial.
src/client/Microsoft.Identity.Client/PublicApi/net8.0-android/PublicAPI.Unshipped.txt Public API addition for MsalError.InvalidCredentialMaterial.
src/client/Microsoft.Identity.Client/PublicApi/net472/PublicAPI.Unshipped.txt Public API addition for MsalError.InvalidCredentialMaterial.
src/client/Microsoft.Identity.Client/PublicApi/net462/PublicAPI.Unshipped.txt Public API addition for MsalError.InvalidCredentialMaterial.
src/client/Microsoft.Identity.Client/OAuth2/TokenClient.cs Uses CredentialMaterialResolver and applies returned request parameters + resolved certificate.
src/client/Microsoft.Identity.Client/MsalError.cs Adds InvalidCredentialMaterial error code with guidance.
src/client/Microsoft.Identity.Client/Microsoft.Identity.Client.csproj Adds item entries for new credential files (currently introduces duplicate compile item risk).
src/client/Microsoft.Identity.Client/Internal/ClientCredential/IClientCredential.cs Replaces “apply to OAuth2Client” API with GetCredentialMaterialAsync(context, ct).
src/client/Microsoft.Identity.Client/Internal/ClientCredential/CredentialSource.cs New enum indicating static vs callback origin.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/CredentialMaterialResolver.cs New centralized resolver building context + validating returned material.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/CredentialMaterial.cs New normalized output type for credential resolution.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/CredentialContext.cs New immutable input context type for credential resolution.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/ClientCredentialApplicationResult.cs Removes legacy result type.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/ClientAuthMode.cs New enum for Regular vs MtlsMode.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/ClientAssertionStringDelegateCredential.cs Updates to return CredentialMaterial and enforce mTLS incompatibility.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/ClientAssertionDelegateCredential.cs Updates to return CredentialMaterial (JWT bearer vs JWT-PoP + optional cert).
src/client/Microsoft.Identity.Client/Internal/ClientCredential/SignedAssertionClientCredential.cs Updates to return CredentialMaterial and reject mTLS mode.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/SecretStringClientCredential.cs Updates to return CredentialMaterial and reject mTLS mode.
src/client/Microsoft.Identity.Client/Internal/ClientCredential/CertificateAndClaimsClientCredential.cs Updates to return empty params + cert in mTLS mode, assertion params + cert in regular.

Copilot AI review requested due to automatic review settings March 10, 2026 22:11
Copilot AI reviewed Mar 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.

Copilot AI mentioned this pull request Mar 12, 2026
Open
1 task
bgavrilMS
bgavrilMS reviewed Mar 16, 2026
View reviewed changes
src/client/Microsoft.Identity.Client/Internal/ClientCredential/CredentialMaterial.cs Outdated
public IReadOnlyDictionary<string, string> TokenRequestParameters { get; }

/// <summary>Whether the credential was resolved statically or via a runtime callback.</summary>
public CredentialSource Source { get; }
Copy link
Copy Markdown
Member

@bgavrilMS bgavrilMS Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used except for logging. Pls remove it.

Copy link
Copy Markdown
Contributor Author

@gladjohn gladjohn Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

bgavrilMS
bgavrilMS reviewed Mar 16, 2026
View reviewed changes
...ent/Microsoft.Identity.Client/Internal/ClientCredential/ClientAssertionDelegateCredential.cs
/// Handles client assertions supplied via a delegate that returns a
/// <see cref="ClientSignedAssertion"/> (JWT + optional certificate bound for mTLS-PoP).
/// </summary>
internal sealed class ClientAssertionDelegateCredential : IClientCredential, IClientSignedAssertionProvider
Copy link
Copy Markdown
Member

@bgavrilMS bgavrilMS Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests or a debug assertion to ensure that each credential, and especially this one, is called once and only once per token request?

Copy link
Copy Markdown
Contributor Author

@gladjohn gladjohn Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test coverage to verify the callback-backed credential path is invoked exactly once per token request.

bgavrilMS
bgavrilMS reviewed Mar 16, 2026
View reviewed changes
...ent/Microsoft.Identity.Client/Internal/ClientCredential/ClientAssertionDelegateCredential.cs Outdated
{
{
OAuth2Parameter.ClientAssertionType,
useJwtPop ? OAuth2AssertionType.JwtPop : OAuth2AssertionType.JwtBearer
Copy link
Copy Markdown
Member

@bgavrilMS bgavrilMS Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls compute the assertion type only once. You seem to do it twice, once here and once at line 85 for logging.

Copy link
Copy Markdown
Contributor Author

@gladjohn gladjohn Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Computed the assertion type once and reused it for both logging and request parameters

bgavrilMS
bgavrilMS reviewed Mar 16, 2026
View reviewed changes
src/client/Microsoft.Identity.Client/Internal/ClientCredential/CredentialMaterial.cs
/// </list>
/// <see langword="null"/> when no certificate is involved (secret, plain JWT assertion).
/// </summary>
public X509Certificate2 ResolvedCertificate { get; }
Copy link
Copy Markdown
Member

@bgavrilMS bgavrilMS Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be MtlsCertificate ?

bgavrilMS
bgavrilMS reviewed Mar 16, 2026
View reviewed changes
Copilot AI review requested due to automatic review settings March 24, 2026 15:45
Copilot AI reviewed Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 22 changed files in this pull request and generated 2 comments.

@gladjohn gladjohn force-pushed the gladjohn/refactor branch from 0cb95ce to 302a6df Compare March 24, 2026 23:43
@gladjohn gladjohn force-pushed the gladjohn/refactor branch from 302a6df to 1503f8e Compare March 24, 2026 23:48
@gladjohn gladjohn requested a review from bgavrilMS March 24, 2026 23:58
@AzureAD AzureAD deleted a comment from Copilot AI Mar 25, 2026
@AzureAD AzureAD deleted a comment from Copilot AI Mar 25, 2026
@gladjohn gladjohn mentioned this pull request Mar 25, 2026
Closed
1 task
@gladjohn
Copy link
Copy Markdown
Contributor Author

gladjohn commented Mar 25, 2026

@neha-bhargava @bgavrilMS

FYI: #5835 is the follow-up to (and supersedes) #5748. I went through every open thread/comment on #5748 and carried them forward here:

  • Consolidated the “extra context” objects into CredentialContext (no separate MTLS-only context).
  • Replaced string/flag-style mode handling with a single OAuthMode enum (and resolution-time validation).
  • Canonical credential/mode matrix is now enforced via CredentialMatrixTests + per-credential validation (doc-only matrix removed).
  • Endpoint is resolved once in TokenClient and flowed through resolution via context.TokenEndpoint.
  • Output material is reduced to TokenRequestParameters + ResolvedCertificate; old metadata/hash-prefix/source fields were removed.
  • Exception taxonomy updated: internal invariants → InvalidOperationException, unsupported combinations → MsalClientException.
  • CancellationToken is passed explicitly to async methods (not stored on the context).

Copilot AI review requested due to automatic review settings March 25, 2026 16:12
Copilot AI reviewed Mar 25, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.

Copilot AI review requested due to automatic review settings March 31, 2026 15:20
@gladjohn
Copy link
Copy Markdown
Contributor Author

gladjohn commented Mar 31, 2026

@bgavrilMS @neha-bhargava can you please review this

Copilot AI reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings April 2, 2026 19:12
@gladjohn gladjohn force-pushed the gladjohn/refactor branch from a431912 to 127e7df Compare April 2, 2026 19:15
Copilot AI reviewed Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 21 out of 22 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tests/Microsoft.Identity.Test.Unit/RequestsTests/CredentialMatrixTests.cs:418

  • CredentialMaterial_NullTokenRequestParameters_ThrowsInvalidOperationException asserts an ArgumentNullException is thrown. The test name should be updated to match the asserted exception type to avoid confusion when diagnosing failures.

@gladjohn gladjohn force-pushed the gladjohn/refactor branch from bd2f1ce to 1503f8e Compare April 2, 2026 19:26
Copilot AI review requested due to automatic review settings April 2, 2026 19:27
@gladjohn gladjohn force-pushed the gladjohn/refactor branch from a431912 to 1503f8e Compare April 2, 2026 19:32
Copilot AI reviewed Apr 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 1 comment.

tests/Microsoft.Identity.Test.Unit/RequestsTests/CredentialMatrixTests.cs
Comment on lines +411 to +419
public void CredentialMaterial_NullTokenRequestParameters_ThrowsInvalidOperationException()
{
// CredentialMaterial rejects null TokenRequestParameters at construction time.
Assert.ThrowsExactly<InvalidOperationException>(
() => new CredentialMaterial(null));
}

[TestMethod]
public void CredentialMaterial_EmptyTokenRequestParameters_IsValid()
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test name says it throws InvalidOperationException, but the assertion verifies an ArgumentNullException (and the comment also describes an ArgumentNullException scenario). Rename the test (or change the asserted exception) so the name matches the behavior being validated.

Copilot uses AI. Check for mistakes.
@gladjohn
Copy link
Copy Markdown
Contributor Author

gladjohn commented Apr 9, 2026

@bgavrilMS @neha-bhargava can you please review this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@bgavrilMS bgavrilMS Awaiting requested review from bgavrilMS

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gladjohn @bgavrilMS