Skip to content

Conversation

@trwalke
Copy link
Member

@trwalke trwalke commented Feb 20, 2025

Fixes #
#5150

Changes proposed in this request
The new WithCertificate(X509Certificate2 certificate, bool sendX5C, bool associateTokensWithCertificateSerialNumber) api enables MSAL to add the certificate serial number to the cache key using the new cache key extension.

This is certificate serial number is added at the application level and is intended to enable app tokens to be associated with the certificate serial number for every request.

This api internally calls WithAdditionalCacheKeyComponents(Dict<string, string>) to further partition the cache key by cert serial number.

Testing
Unit tests
Manual testing

Performance impact
N/A

Documentation

  • All relevant documentation is updated.

@trwalke trwalke requested a review from a team as a code owner February 20, 2025 23:55
Copy link
Contributor

@gladjohn gladjohn left a comment

Choose a reason for hiding this comment

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

Please add the following tests,

  • Validate Cache Partitioning with Different Certificates
  • Verify Token Retrieval from Cache
  • Validate Behavior When associateTokensWithCertificateSerialNumber is False
  • Ensure Cache Key is Formed Correctly

Would adding an integration test add any value here?

@trwalke
Copy link
Member Author

trwalke commented Feb 25, 2025

Please add the following tests,

  • Validate Cache Partitioning with Different Certificates
  • Verify Token Retrieval from Cache
  • Validate Behavior When associateTokensWithCertificateSerialNumber is False
  • Ensure Cache Key is Formed Correctly

Would adding an integration test add any value here?

The underlying api has tests that are already validating cache partitioning/cache key format/token retrieval from cache.

This is one of those scenarios where I am trying to decide how much testing should be done if the underlying api (WithAdditionalCacheKeyComponents(Dict<string, string>)) already has tests for a lot of this functionality. the new api is simply calling an already existing api without much additional logic so my tests here are focusing on new logic.

Seems like an interesting discussion @gladjohn @bgavrilMS

However, I agree that I can test the false case for the api.

trwalke and others added 2 commits February 24, 2025 21:36
Refactoring
@gladjohn
Copy link
Contributor

Please add the following tests,

  • Validate Cache Partitioning with Different Certificates
  • Verify Token Retrieval from Cache
  • Validate Behavior When associateTokensWithCertificateSerialNumber is False
  • Ensure Cache Key is Formed Correctly

Would adding an integration test add any value here?

The underlying api has tests that are already validating cache partitioning/cache key format/token retrieval from cache.

This is one of those scenarios where I am trying to decide how much testing should be done if the underlying api (WithAdditionalCacheKeyComponents(Dict<string, string>)) already has tests for a lot of this functionality. the new api is simply calling an already existing api without much additional logic so my tests here are focusing on new logic.

Seems like an interesting discussion @gladjohn @bgavrilMS

However, I agree that I can test the false case for the api.

added negative testing looks good.

Copy link
Contributor

@gladjohn gladjohn left a comment

Choose a reason for hiding this comment

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

🚢

@trwalke trwalke merged commit 68dc10f into main Feb 25, 2025
7 checks passed
@trwalke trwalke deleted the trwalke/CertSerialNum branch February 25, 2025 22:12
<AdditionalFiles Include="PublicAPI/$(TargetFramework)/PublicAPI.Unshipped.txt" />
</ItemGroup>

<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

@trwalke - please remove this.

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] (RP only) Provide an option to associate tokens with the certificate serial number

4 participants