Skip to content

Conversation

gladjohn
Copy link
Contributor

@gladjohn gladjohn commented Jul 2, 2025

Fixes #5370

Changes proposed in this request
This pull request introduces several updates to the Microsoft.Identity.Client library, focusing on adding support for binding certificates in mTLS-PoP scenarios, improving the AuthenticationResult API, and enhancing test coverage. Key changes include the addition of a BindingCertificate property, modifications to property access levels, and updates to unit and integration tests to validate the new functionality.

Updates to AuthenticationResult API:

  • Added a new BindingCertificate property to AuthenticationResult for mTLS-PoP token scenarios. This property allows binding an X509 certificate to access tokens. (src/client/Microsoft.Identity.Client/AuthenticationResult.cs - [1] [2]
  • Changed several properties in AuthenticationResult (e.g., UniqueId, ExpiresOn, TenantId, Account, etc.) to internal set to enable modification within the library while maintaining encapsulation. (src/client/Microsoft.Identity.Client/AuthenticationResult.cs - [1] [2] [3]
  • Marked constructors in AuthenticationResult as obsolete to discourage direct use and encourage usage of factory methods or builders. (src/client/Microsoft.Identity.Client/AuthenticationResult.cs - [1] [2]

Integration of mTLS-PoP functionality:

Test enhancements:

  • Added integration tests to validate the BindingCertificate functionality in mTLS-PoP scenarios, ensuring the certificate is correctly set and retrieved from cache during token acquisition. (tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsMtlsPopTests.cs - [1] [2]
  • Updated unit tests to include assertions for the BindingCertificate property and deprecated constructor usage warnings. (tests/Microsoft.Identity.Test.Unit/PublicApiTests/AuthenticationResultTests.cs - [1] [2] [3]
  • Enhanced mTLS-PoP unit tests to verify the presence and correctness of the BindingCertificate in cached and non-cached scenarios. (tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs - [1] [2]

These changes collectively improve the library's support for mTLS-PoP scenarios, enhance API usability, and ensure robust test coverage for the new functionality.

Testing
unit and integration

Performance impact
none

Documentation

  • All relevant documentation is updated.

@gladjohn
Copy link
Contributor Author

gladjohn commented Jul 2, 2025

@neha-bhargava @trwalke @bgavrilMS , need suggestion on how to fix the failing tests - PublicTestConstructorCoversAllProperties

test fails with,

Assert.AreEqual failed. Expected:<15>. Actual:<16>. The constructor should include all properties of AuthenticationObject

but if I add the property to the test ctor, and update the public API, then I get this,

Symbol 'AuthenticationResult' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details.

@gladjohn gladjohn marked this pull request as ready for review July 3, 2025 17:31
@gladjohn gladjohn requested a review from a team as a code owner July 3, 2025 17:31
@gladjohn
Copy link
Contributor Author

gladjohn commented Jul 3, 2025

@neha-bhargava @trwalke @bgavrilMS , need suggestion on how to fix the failing tests - PublicTestConstructorCoversAllProperties

test fails with,

Assert.AreEqual failed. Expected:<15>. Actual:<16>. The constructor should include all properties of AuthenticationObject

but if I add the property to the test ctor, and update the public API, then I get this,

Symbol 'AuthenticationResult' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details.

i have a fix for this, hopefully not breaking any public APIs, please review carefully.

@gladjohn gladjohn changed the title [draft] Expose cert used in mtls flows in the auth result Expose cert used in mtls flows in the auth result Jul 3, 2025
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.

No need for new integration test

@gladjohn gladjohn force-pushed the gladjohn/expose_cert_authrslt branch from 4c9e84d to fe22302 Compare July 14, 2025 13:40
@gladjohn gladjohn requested a review from bgavrilMS July 14, 2025 14:25
@gladjohn gladjohn force-pushed the gladjohn/expose_cert_authrslt branch from d797007 to 958ee16 Compare July 14, 2025 17:20
@gladjohn gladjohn merged commit 31dbff6 into main Jul 14, 2025
11 checks passed
@gladjohn gladjohn deleted the gladjohn/expose_cert_authrslt branch July 14, 2025 18:08
@gladjohn gladjohn added this to the 4.74.0 milestone Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Return mTLS Client certificate as part of the AuthenticationResult

4 participants