Skip to content

add distributed token cache to ExpressTestApp sample #3986

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

Merged
merged 9 commits into from
Oct 11, 2021

Conversation

derisen
Copy link
Contributor

@derisen derisen commented Aug 19, 2021

This adds distributed caching support to the ExpressTestApp sample using Redis as persistence client (via node-redis).

The custom cache plugin implements the ICachePlugin. The persistent cache here is a simple key-value store. For each user, there are 2 records:

Key Value
sessionId session variable, which contains homeAccountId
homeAccountId msal's cache blob for that user

The way this works is roughly as follows:

  • A custom middleware initializeTokenCachePlugin is used pass the session variable to the custom cache plugin.
  • When a user attempts to sign or acquire a token, cache access is triggered:
    * Before cache access, the plugin checks the persistence store for the given sessionId. If a record is found, it grabs the homeAccountId from the session record, and then gets the relevant cache blob using homeAccountId as key. The blob then is deserialized into msal's in-memory cache.
    * After cache access, the plugin checks if the cache has changed (in-memory vs persistence). If so, it serializes the in-memory cache back to persistence store.
  • When the user signs-out, the session is destroyed, removing the session data but not token cache for that user.

Couple of observations:

  • My intention was to implement this in TypeScript on the wrapper side, as I think this is best abstracted away (ideally, devs should only need supply the persistence client, using whatever persistence solution they like, and the session variable). However, because the initial cachePlugin needs to be re-initialized later on with session variable, and because tokenCache is a private member of ClientApplication, this leads to compile errors.
  • During the instantiation of msal client, if cachePlugin field is left as null in the configuration object, it cannot be initialized later on i.e. it effectively sets the msal client to use the in-memory cache only.
  • I was expecting acquireTokenSilent to trigger a cache access, (so that beforeCacheAccess/afterCacheAccess runs). This wasn't the case, and I had to call getAccountByHomeId to do the job. This seems off, the dev might be storing the account object in a session or etc. so might not necessarily call this API.

@derisen derisen marked this pull request as draft August 19, 2021 16:50
@github-actions github-actions bot added the samples Related to the samples apps for the library. label Aug 19, 2021
@github-actions github-actions bot added the documentation Related to documentation. label Aug 20, 2021
@derisen derisen marked this pull request as ready for review August 25, 2021 15:16
Base automatically changed from refactor-express-sample to dev August 27, 2021 18:44
Copy link
Contributor

@samuelkubai samuelkubai 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, sample works as expected and a great starting point for distributed caching.

@ghost
Copy link

ghost commented Sep 27, 2021

Reminder: The next release is scheduled for next week and this PR appears to be stale :(

If changes have been requested please address feedback.
If this PR is still a work in progress please mark as draft.

@ghost ghost added the Needs: Attention 👋 Awaiting response from the MSAL.js team label Sep 27, 2021
Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Just a couple nits but otherwise looks good

}

if (sessionData) {
persistenceHelper.get(JSON.parse(sessionData).account.homeAccountId, (err, cacheData) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should wrap the JSON.parse in a try/catch

const kvStore = cacheContext.tokenCache.getKVStore();

// getting homeAccountId from account entity in kvStore
const homeAccountId = Object.values(kvStore)[1]["homeAccountId"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should validate that kvStore even has any values and then validate that homeAccountId is defined before moving on to the next part

@@ -1,2260 +0,0 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add an .npmrc file with package-lock=false to this directory to prevent package-locks from being generated in the future.

@@ -21,7 +21,7 @@
"typescript": "^4.0.5"
},
"dependencies": {
"@azure/msal-node": "^1.1.0",
"@azure/msal-node": "file:../../../lib/msal-node",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave this versioned. You can add scripts to help going back and forth between local and published versions. Some of our other samples demonstrate this.

@ghost ghost removed the Needs: Attention 👋 Awaiting response from the MSAL.js team label Oct 1, 2021
@derisen derisen requested a review from tnorling October 3, 2021 01:15
@jasonnutter jasonnutter linked an issue Oct 6, 2021 that may be closed by this pull request
2 tasks
@derisen derisen merged commit 81a38fd into dev Oct 11, 2021
@derisen derisen deleted the add-distributed-cache branch October 11, 2021 13:47
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.

Redis token cache for msal-node?
5 participants