Skip to content

Commit

Permalink
No need for memento hack
Browse files Browse the repository at this point in the history
MSAL node made `clearCache` synchronous 🎉 so we can safely depend on it for clearing the cache.

> Context: The default behavior of MSAL's internal cache is that it is a union with what's in the persistant cache (secret storage) but what _we_ want is that secret storage is the source of truth, so every time we receive an update to secret storage, we clear the in-memory cache to get the data from the persistant cache.

Also bumps msal-node-extensions while we're at it.
  • Loading branch information
TylerLeonhardt committed Nov 22, 2024
1 parent 3b7e1c6 commit dd28b32
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 45 deletions.
40 changes: 15 additions & 25 deletions extensions/microsoft-authentication/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions extensions/microsoft-authentication/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@
},
"dependencies": {
"@azure/ms-rest-azure-env": "^2.0.0",
"@azure/msal-node": "^2.13.1",
"@azure/msal-node-extensions": "^1.3.0",
"@azure/msal-node": "^2.16.2",
"@azure/msal-node-extensions": "^1.5.0",
"@vscode/extension-telemetry": "^0.9.0",
"keytar": "file:./packageMocks/keytar",
"vscode-tas-client": "^0.1.84"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,6 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
};
private readonly _isBrokerAvailable = this._config.broker?.nativeBrokerPlugin?.isBrokerAvailable ?? false;

/**
* We keep track of the last time an account was removed so we can recreate the PCA if we detect that an account was removed.
* This is due to MSAL-node not providing a way to detect when an account is removed from the cache. An internal issue has been
* filed to track this. If MSAL-node ever provides a way to detect this or handle this better in the Persistant Cache Plugin,
* we can remove this logic.
*/
private _lastCreated: Date;

//#region Events

private readonly _onDidAccountsChangeEmitter = new EventEmitter<{ added: AccountInfo[]; changed: AccountInfo[]; deleted: AccountInfo[] }>;
Expand All @@ -71,8 +63,9 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
private readonly _secretStorage: SecretStorage,
private readonly _logger: LogOutputChannel
) {
// TODO:@TylerLeonhardt clean up old use of memento. Remove this in an iteration
this._globalMemento.update(`lastRemoval:${this._clientId}:${this._authority}`, undefined);
this._pca = new PublicClientApplication(this._config);
this._lastCreated = new Date();
this._disposable = Disposable.from(
this._registerOnSecretStorageChanged(),
this._onDidAccountsChangeEmitter,
Expand Down Expand Up @@ -147,7 +140,6 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
}

removeAccount(account: AccountInfo): Promise<void> {
this._globalMemento.update(`lastRemoval:${this._clientId}:${this._authority}`, new Date());
if (this._isBrokerAvailable) {
return this._accountAccess.setAllowedAccess(account, false);
}
Expand Down Expand Up @@ -185,14 +177,8 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
private async _update() {
const before = this._accounts;
this._logger.debug(`[update] [${this._clientId}] [${this._authority}] CachedPublicClientApplication update before: ${before.length}`);
// Dates are stored as strings in the memento
const lastRemovalDate = this._globalMemento.get<string>(`lastRemoval:${this._clientId}:${this._authority}`);
if (lastRemovalDate && this._lastCreated && Date.parse(lastRemovalDate) > this._lastCreated.getTime()) {
this._logger.debug(`[update] [${this._clientId}] [${this._authority}] CachedPublicClientApplication removal detected... recreating PCA...`);
this._pca = new PublicClientApplication(this._config);
this._lastCreated = new Date();
}

// Clear in-memory cache so we know we're getting account data from the SecretStorage
this._pca.clearCache();
let after = await this._pca.getAllAccounts();
if (this._isBrokerAvailable) {
after = after.filter(a => this._accountAccess.isAllowedAccess(a));
Expand Down

0 comments on commit dd28b32

Please sign in to comment.