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

[msal-node] Cache Lookup - 2: Save, lookup and delete entities #1655

Merged
merged 21 commits into from Jun 4, 2020

Conversation

sameerag
Copy link
Member

This PR adds the functionality to save, lookup and delete cache entities. It defines the below helpers:

  • saveAccount()
  • saveCredential()
  • getAccount()
  • getCredential()
  • getAccountsFilteredBy()
  • getCredentialsFilteredBy()
  • removeAccount()
  • removeCredential()

Unit tests added.

Note: These helpers help support silent and refreshToken flows in the msal-node library

This was referenced May 20, 2020
@sameerag sameerag self-assigned this May 20, 2020
Copy link
Contributor

@pkanher617 pkanher617 left a comment

Choose a reason for hiding this comment

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

Approving, but I'm starting to see a pattern of repeated logic, and it is going to bloat our codebase. Please make sure you understand the current codebase before making changes like this.

lib/msal-common/src/response/ResponseHandler.ts Outdated Show resolved Hide resolved
lib/msal-common/src/unifiedCache/UnifiedCacheManager.ts Outdated Show resolved Hide resolved
lib/msal-common/src/unifiedCache/UnifiedCacheManager.ts Outdated Show resolved Hide resolved
lib/msal-common/src/unifiedCache/UnifiedCacheManager.ts Outdated Show resolved Hide resolved
lib/msal-common/src/unifiedCache/UnifiedCacheManager.ts Outdated Show resolved Hide resolved
if (matches) {
matchingCredentials[key] = cacheCredentials[key];
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about using reduce instead of forEach.

lib/msal-common/src/unifiedCache/UnifiedCacheManager.ts Outdated Show resolved Hide resolved
lib/msal-common/src/unifiedCache/UnifiedCacheManager.ts Outdated Show resolved Hide resolved
@sameerag
Copy link
Member Author

Approving, but I'm starting to see a pattern of repeated logic, and it is going to bloat our codebase. Please make sure you understand the current codebase before making changes like this.

In this use case I believe scopes should be compared by content instead of count. As specified above, If a use case looks up access_tokens that match for a set of scopes but for different accounts, we would have to match the scopes content and not the count. We can discuss this further if I am missing anything.

[msal-node] Cache Lookup - 3: Msal node cache response
@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Jun 3, 2020
@github-actions github-actions bot added the msal-node Related to msal-node package label Jun 3, 2020
@sameerag sameerag merged commit 4a4dc16 into msal-node-cache-keys Jun 4, 2020
@sameerag sameerag deleted the msal-node-cache-lookup-interface branch June 8, 2020 05:25
pkanher617 pushed a commit to pkanher617/microsoft-authentication-library-for-js that referenced this pull request Dec 30, 2021
…nterface

[msal-node] Cache Lookup - 2: Save, lookup and delete entities
azure-pipelines bot pushed a commit to pkanher617/microsoft-authentication-library-for-js that referenced this pull request Jan 4, 2022
…nterface

[msal-node] Cache Lookup - 2: Save, lookup and delete entities
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package msal-node Related to msal-node package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants