Skip to content
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.

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

Added E2E Test: Client Credentials with Certificate from Key Vault #7367

Open
wants to merge 37 commits into
base: dev
Choose a base branch
from

Conversation

Robbie-Microsoft
Copy link
Collaborator

@Robbie-Microsoft Robbie-Microsoft commented Oct 8, 2024

This PR adds a sample with a corresponding e2e test.

A follow up PR that converts the sample to TypeScript is located here.

Additionally, some potential follow up work would be to re-use some of the certificate transforming functionality introduced here and change the way developers pass in certificates.

@github-actions github-actions bot added documentation Related to documentation. samples Related to the samples apps for the library. labels Oct 8, 2024
@github-actions github-actions bot removed the msal-node Related to msal-node package label Oct 18, 2024
Copy link
Member

@trwalke trwalke left a comment

Choose a reason for hiding this comment

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

See if you can also test the cached tokens. Maybe even expire them and try to acquire new ones if that is possible.

@Robbie-Microsoft Robbie-Microsoft self-assigned this Oct 21, 2024
@Robbie-Microsoft
Copy link
Collaborator Author

Robbie-Microsoft commented Oct 21, 2024

See if you can also test the cached tokens. Maybe even expire them and try to acquire new ones if that is possible.

This is all handled in the unit tests. We just want to verify the token is received from the network request (via cert auth) here.

@Robbie-Microsoft Robbie-Microsoft requested review from trwalke and a team and removed request for a team October 21, 2024 19:57
Copy link
Member

@trwalke trwalke left a comment

Choose a reason for hiding this comment

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

I would strongly recommend also testing caching operations in integration tests. But it is not mandatory

@Robbie-Microsoft
Copy link
Collaborator Author

I would strongly recommend also testing caching operations in integration tests. But it is not mandatory

I'll make this improvement in the follow-up PR which converts this sample to TypeScript

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation. samples Related to the samples apps for the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants