Skip to content

Add extensibility APIs #5573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
neha-bhargava wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Add extensibility APIs #5573

neha-bhargava wants to merge 22 commits into from
+1,519 −48

Conversation

@neha-bhargava
Copy link
Contributor

@neha-bhargava neha-bhargava commented Nov 7, 2025

Fixes #5568

Changes proposed in this request

  • Create the extensibility apis for dynamic cert, retry and observer

Testing

Performance impact

Documentation

  • All relevant documentation is updated.

@neha-bhargava neha-bhargava requested a review from a team as a code owner November 7, 2025 23:17
@neha-bhargava neha-bhargava marked this pull request as draft November 7, 2025 23:17
Copilot AI reviewed Nov 14, 2025
View reviewed changes
Copy link
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

This PR introduces extensibility APIs for client credential flows, enabling dynamic certificate provisioning, custom retry logic for service failures, and execution observation. The changes support scenarios like certificate rotation from Azure Key Vault and custom telemetry collection.

  • Dynamic certificate provider callback that's invoked before each token request
  • Retry callback for handling service failures with custom logic
  • Execution observer for tracking token acquisition results (success or failure)

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
src/client/Microsoft.Identity.Client/Extensibility/ClientCredentialExtensionParameters.cs New parameter class providing application context to extensibility callbacks
src/client/Microsoft.Identity.Client/Extensibility/ExecutionResult.cs New result class containing either successful AuthenticationResult or exception
src/client/Microsoft.Identity.Client/Extensibility/ConfidentialClientApplicationBuilderExtensions.cs Extension methods for WithCertificate, OnMsalServiceFailure, and OnSuccess
src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs Adds storage for three new extensibility callback properties
src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs Implements retry loop and callback invocation logic for token acquisition
src/client/Microsoft.Identity.Client/Internal/ClientCredential/CertificateAndClaimsClientCredential.cs Adds dynamic certificate resolution via provider callback
src/client/Microsoft.Identity.Client/ApiConfig/AbstractConfidentialClientAcquireTokenParameterBuilder.cs Updates validation to allow dynamic certificate provider
src/client/Microsoft.Identity.Client/MsalError.cs Adds error constant for invalid client credential configuration
tests/Microsoft.Identity.Test.Unit/PublicApiTests/ConfidentialClientApplicationExtensibilityTests.cs Comprehensive integration tests for all three extensibility points
tests/Microsoft.Identity.Test.Unit/AppConfigTests/ConfidentialClientApplicationExtensibilityApiTests.cs Unit tests for builder API and configuration storage
src/client/Microsoft.Identity.Client/PublicApi/*/PublicAPI.Unshipped.txt Documents new public API surface for all target frameworks
src/client/Microsoft.Identity.Client/Microsoft.Identity.Client.csproj Adds new extensibility files to project
src/client/Microsoft.Identity.Client/ManagedIdentity/AbstractManagedIdentity.cs Modifies exception handling pattern (appears unrelated to extensibility)

@neha-bhargava neha-bhargava marked this pull request as ready for review November 14, 2025 22:34
@neha-bhargava neha-bhargava self-assigned this Nov 14, 2025
@neha-bhargava neha-bhargava marked this pull request as draft November 17, 2025 21:15
@neha-bhargava neha-bhargava marked this pull request as ready for review November 17, 2025 21:45
tlupes
tlupes reviewed Nov 19, 2025
View reviewed changes
...soft.Identity.Test.Unit/AppConfigTests/ConfidentialClientApplicationExtensibilityApiTests.cs
}

[TestMethod]
public void WithCertificate_AllowsMultipleCallbackRegistrations_LastOneWins()
Copy link

@tlupes tlupes Nov 19, 2025

Choose a reason for hiding this comment

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

Is this how the rest of the APIs currently work? If you call WithCertificate multiple times, it just overwrites?

Copy link
Contributor Author

@neha-bhargava neha-bhargava Nov 19, 2025

Choose a reason for hiding this comment

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

Right now that is how it is. The test case was generated by copilot. I am going to propose another PR that will validate and throw if the cert is being set twice. Or if the dynamic cert is used then avoid static cert.

bgavrilMS
bgavrilMS reviewed Nov 26, 2025
View reviewed changes
src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs Outdated
/// <summary>
/// Success callback that receives the result of token acquisition attempts (typically successful, but can include failures after retries are exhausted).
/// </summary>
public Func<AssertionRequestOptions, ExecutionResult, Task> OnSuccessCallback { get; set; }
Copy link
Member

@bgavrilMS bgavrilMS Nov 26, 2025

Choose a reason for hiding this comment

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

Do we need this ExecutionResult object? It seems to me that OnMsalServiceFailure takes care of the error sceanrio. And so this can be Func<AssertionRequestOptions, AuthenticationResult> OnTokenAcquisitionSuccess

Copy link
Contributor Author

@neha-bhargava neha-bhargava Dec 5, 2025

Choose a reason for hiding this comment

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

No, the OnMsalServiceFailure wll only handle the exceptions defined in the callback. There can be other exceptions as well that can be thrown.

Copy link
Contributor Author

@neha-bhargava neha-bhargava Dec 9, 2025

Choose a reason for hiding this comment

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

Keeping the executionResult as is. Also sending back the cert used

bgavrilMS
bgavrilMS reviewed Nov 26, 2025
View reviewed changes
.../Microsoft.Identity.Client/Internal/ClientCredential/CertificateAndClaimsClientCredential.cs
// Log the incoming request parameters for diagnostic purposes
requestParameters.RequestContext.Logger.Verbose(() => $"Building assertion from certificate with clientId: {clientId} at endpoint: {tokenEndpoint}");

if (requestParameters.MtlsCertificate == null)
Copy link
Member

@bgavrilMS bgavrilMS Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is fine for Bearer tokens, but it is going to require a bit of work for mTLS POP tokens, which is an important scenario.

In case of MTLS POP tokens, the certificate is used to talk to Entra via mTLS.

I recommend you use a follow-up PR to address this sceanrio. There are integration tests for mtls POP.

Copy link
Contributor Author

@neha-bhargava neha-bhargava Dec 5, 2025

Choose a reason for hiding this comment

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

Dynamic cert for the MTLS POP as well?

Copy link
Contributor Author

@neha-bhargava neha-bhargava Dec 9, 2025

Choose a reason for hiding this comment

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

Discussed offline. This will go in another iteration.

bgavrilMS
bgavrilMS requested changes Nov 26, 2025
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

TenantId is probably not correctly set, as it can be overridden

@bgavrilMS bgavrilMS mentioned this pull request Dec 1, 2025
Merged
@neha-bhargava neha-bhargava mentioned this pull request Dec 5, 2025
Open
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 bgavrilMS requested changes

+1 more reviewer

@tlupes tlupes tlupes left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

@neha-bhargava neha-bhargava

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature Request] Add 3 new extensiblity APIs

4 participants

@neha-bhargava @tlupes @bgavrilMS