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.

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

Cache key extensibility for MSAL #5107

Merged
merged 16 commits into from
Feb 7, 2025
Merged

Cache key extensibility for MSAL #5107

merged 16 commits into from
Feb 7, 2025

Conversation

trwalke
Copy link
Member

@trwalke trwalke commented Jan 30, 2025

Fixes #
Implementation of #5091

Changes proposed in this request
Enabling MSAL to associate cache key components with cache items (Access tokens only) for later retrival.
Cache components are used when filtering the cache for tokens

Testing
Unit tests

Performance impact

Documentation

  • All relevant documentation is updated.

@trwalke
Copy link
Member Author

trwalke commented Jan 30, 2025

Just need to update tests a little bit

@trwalke trwalke marked this pull request as ready for review February 3, 2025 03:49
@trwalke trwalke requested a review from a team as a code owner February 3, 2025 03:49
bgavrilMS
bgavrilMS requested changes Feb 3, 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.

Cache keys are not correct.

trwalke and others added 3 commits February 4, 2025 20:40
@trwalke @gladjohn
Apply suggestions from code review
82d320d 
Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
@trwalke @gladjohn
Update tests/Microsoft.Identity.Test.Unit/PublicApiTests/CacheKeyExte…
7ac8b7a 
…nsionTests.cs

Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
Addressing feedback.
d22c068 
clean up.
refactoring
bgavrilMS
bgavrilMS approved these changes Feb 6, 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.

LGTM, 1 minor comment.

gladjohn
gladjohn approved these changes Feb 6, 2025
View reviewed changes
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.

looks good @trwalke

I do not see any explicit tests ensuring that app developers cannot add standard keys (e.g. "client_id", "tenant_id") to AdditionalCacheKeyComponents. Can that be added?

@trwalke trwalke merged commit 6ff1aa7 into main Feb 7, 2025
6 checks passed
@trwalke trwalke deleted the trwalke/cacheExtension branch February 7, 2025 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rayluo rayluo

@bgavrilMS bgavrilMS

@gladjohn gladjohn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@trwalke @rayluo @bgavrilMS @gladjohn