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

changes to the cachekey to use tokenNotificationArgs #279

Merged
merged 10 commits into from
Jul 7, 2020

Conversation

jennyf19
Copy link
Collaborator

@jennyf19 jennyf19 commented Jul 1, 2020

Next release of MSAL .NET will surface the cache key to use in the tokenNotificationArgs, so we'll use that instead.

Making this draft PR for ease of testing. cc: @jmprieur

#235

@jennyf19
Copy link
Collaborator Author

jennyf19 commented Jul 2, 2020

@pmaytak you could probably review this now, i don't expect much to change. If you want to build it and test it, although it's probably easier to wait until MSAL .NET releases.

Copy link
Contributor

@pmaytak pmaytak 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 to me, thanks. Caching API looks cleaner now. Just a few minor suggestions. I'll take another glance once this PR is finalized.

jmprieur and others added 2 commits July 7, 2020 08:17
#294)

* This is an improvement leveraging the fact that MSAL.NET now exposes TokenCacheNotificationArgs.HasTokens which enable applications (and Microsoft.Identity.Web) to remove cache entries when the cache is empty

Also OnBeforeAccessAsync tests if the SuggestedCacheKey is null (can happen in B2C cases, as we still call GetAccountsAsync()

* Removing the un-necessary condition on suggested cache key (which should not be null in a Write to cache)
It can be null in a read to cache if for instance, calling GetAccountsAsync()

* Removing an extra line

* remove secret

Co-authored-by: jennyf19 <jeferrie@microsoft.com>
@jennyf19 jennyf19 marked this pull request as ready for review July 7, 2020 15:34
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks @jennyf19 !

Nice improvements.

@jennyf19 jennyf19 merged commit 2162d6f into master Jul 7, 2020
@jennyf19 jennyf19 deleted the jennyf/cacheKey branch July 7, 2020 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants