[msal-node] hone surface for msal extension library

[DarylThayil]

Plan

Begin with developer called cache apis

import { PublicClientApplication } from "msal-node"
import { PersistenceCachePlugin } from "msal-extension"


const publicApplication = new PublicClientApplication({
    auth: {...},
    cache: {
        cachePlugin: PersistenceCachePlugin
    }
});
const cacheManager = publicApplication.getCacheManager();

await cacheManager.deserialize(); // read into memmory

// do a bunch of stuff that results in memmory cache changes

await cacheManager.serialize(); //write to persistence

Eventually we will instrument public apis / storage with these calls but for the sake of end to end, and still enabling customers who may not want cache calls everytime they call an api, this should be a good starting point.

Msal Extension will expose two functions that provideWithPersistence will provide

export interface ICachePlugin {
    readFromStorage: () => Promise<string>;
    writeToStorage: (getMergedState: (oldState: string) => string) => Promise<void>;
}

readFromStorage will implement reading the persistent cache file and providing it as a return to the lib
writetoStorage will do fun stuff, accepting a callback called getMergedState(stateFromDisk) which will return a merged state of in memmory and disk. This mainly entails makingsure removals are respsected and updates and new additions happen.
When the function is envoked a file lock will begin, a read will happen and pass to getMergedState, which will pass back the state to be written, and then the lock will be lifted.

Msal Node

• readFromPersistence / setCache should over write internal memmory
• writeToPersistance should Lock -> read persistance file -> merge updates and removals -> write file -> unlock
• merge logic lives at this layer so that anyone implementing persistence does not need to worry about this logic.

Extension

Implents readFromStorage: () => Promise;, writeToStorage: (getMergedState: (oldState: string) => string) => Promise;

[jasonnutter]

Your pseudo code above doesn't align with teh sample code. Are we providing the implementation to MSAL Node and then calling an API on MSAL, or are we providing MSAL Node to the extension library and then calling an API on the extension library?

I would personally advocate strongly for providing the implementation to MSAL Node and calling an API on MSAL node (which invokes the provided implementations from the extension library).

[sangonzal]

Overall looks good. This still seems essentially different than any of the other libraries cache models in that it requires users to wrap their MSAL code in deserialize and serialize calls, which is not nearly as customer friendly as it requires them understanding what MSAL operations might alter the cache. There's no way of working around that without making Storage async first though.

[DarylThayil]

@jasonnutter originally I thought about doing the passing extension to node model, but after chatting with @sangonzal , It sort of became clear that the write to Storage and readFrom storage were somewhat duplicative of the get cache and set cache functions. Cache Manager is an attempt to say, there is one way to get cache state from the lib (Cache Manager), so if you hand roll your own persistence, you can use that. If you use our persistence model, just use the provider from it so that CacheManager apis write and read from it. Seems like a reasonable way to keep the surface low, but the extensibility high.

it also lets us stay away from altering storage to async

[DarylThayil]

@sangonzal agree that the wrapping is a bit unfortunate, but in async await code it looks much cleaner

await cacheManager.deserialize();
// do stuff
await cacheManager.serialize(); 

[sangonzal]

@sangonzal agree that the wrapping is a bit unfortunate, but in async await code it looks much cleaner

await cacheManager.deserialize();
// do stuff
await cacheManager.serialize(); 

@DarylThayil Understood, my concern is that it leaks out some details that customers don't necessarily have to be aware of. With this model, callers have to always wrap msal code in deserialize() and serialize().

This brings the question, if we are asking people to always do this, why can't we just do this in MSAL and hide it from callers? Essentially the only thing that's keeping us from incorporating this into MSAL is keeping Storage synchronous.

All to say, we are making a tradeoff here that we should be aware of- Keeping storage synchronous (because we want to share cache code with msal- browser) at the expense of a more complicated Node API.

[DarylThayil]

@sangonzal and I spent a ton of time detailing what needs to happen and think we have a good understanding

[jasonnutter]

Nit pick: not a big fan of mutation here, as it doesn't really add much value.

if (this.persistence) {
  const stringCacheFromStorage = await this.persistence.readFromStorage();
  const deserlizedCache = Deserializer.deserializeAllCache(this.overlayDefaults(JSON.parse(stringCacheFromStorage)));
  this.storage.setCache(deserializedCache);
  return;
}

if (cache) {
  const deserializedCache = Deserializer.deserializeAllCache(this.overlayDefaults(JSON.parse(cache)));
  this.storage.setCache(deserializedCache);
  return;
}

throw Error("Cache Must be passed in or configured");

[DarylThayil]

resolving

[sangonzal]

Looks good. As you documented as part of this PR, we should consider separating serialize/deserialize from from persistence storage - that can be done in a follow up.

[sameerag]

lgtm. Please merge only after @sangonzal adds merge code.

[coveralls]

Coverage decreased (-0.01%) to 76.613% when pulling 7c4b27a on node-cache-surface into 504d352 on dev.

[jasonnutter]

What is this? Is this allowing arbitrary nested StrictDict object?

[jasonnutter]

Instead of mutating the passed in parameter (which lint rules will not allow in the future), can you construct a new object?

[sangonzal]

Yep, will update.

[jasonnutter]

Same comment about mutating passed in value.

[sameerag]

Closing as this merged with #1801