Microsoft/vscode
1
0
Fork 0
mirror of https://github.com/microsoft/vscode.git synced 2026-05-04 15:25:47 +01:00
Code Issues Packages Projects Releases Wiki Activity

Rework eventing for PCAs and fix a few bugs along the way (#227854)

Browse Source 
A big change, but a good one... This addresses some core issues around how we manage multiple PublicClientApplications (which are an object that should be created for each set of clientId,authority). Previously, we were doing some pretty nasty things to detect when a new PCA was created/deleted and as a result it would cause infinite loops and the likes...

Now we've focused on managing that in SecretStorage by looking for a `publicClientApplications` key. This is all encapsulated in the new `PublicClientApplicationsSecretStorage`.

Since we no longer relied on that hack, we still needed some way to have a PCA inform that:
* accounts have changed
* the last account was removed (signaling that this PCA could be disposed of in `PublicClientApplicationsSecretStorage`)

Both of these events have been added to `CachedPublicClientApplication` (now in its own file) and are being used. (replacing the old `_accountChangeHandler` which was hacky... true events are cleaner).

Last thing in the eventing space is that I try to minimize calls to `_storePublicClientApplications` so to not spam events across SecretStorage. You can see this in my usage of `_doCreatePublicClientApplication` over `getOrCreate`.

Couple random other things:
* `changed` accounts are properly bubbled up in `_onDidChangeSessionsEmitter` which is needed when a token is refreshed
* `getSessions` when no scopes are passed in no longer causes new tokens to be minted
* we use to only remove the first account we found but in some cases there may be the same account across different PCAs, so there's a `return` that's removed in `authProvider.ts` that fixes this bug
* Logging is clearer and more verbose (in a good way)
This commit is contained in:
Tyler James Leonhardt
2024-09-07 06:03:25 +02:00
committed by GitHub
parent bc0764dcb1
commit 533d8ec6a5
4 changed files with 355 additions and 216 deletions
Download Patch File Download Diff File Expand all files Collapse all files
extensions/microsoft-authentication/src/node/authProvider.ts
+65 -41
View File
@@ -48,15 +48,12 @@ export class MsalAuthProvider implements AuthenticationProvider {
private readonly _env: Environment = Environment.AzureCloud
) {
this._disposables = context.subscriptions;
this._publicClientManager = new CachedPublicClientApplicationManager(
context.globalState,
context.secrets,
this._logger,
(e) => this._handleAccountChange(e)
this._publicClientManager = new CachedPublicClientApplicationManager(context.globalState, context.secrets, this._logger);
this._disposables.push(
this._onDidChangeSessionsEmitter,
this._publicClientManager,
this._publicClientManager.onDidAccountsChange(e => this._handleAccountChange(e))
);
this._disposables.push(this._publicClientManager);
this._disposables.push(this._onDidChangeSessionsEmitter);
}
async initialize(): Promise<void> {
@@ -79,40 +76,43 @@ export class MsalAuthProvider implements AuthenticationProvider {
* See {@link onDidChangeSessions} for more information on how this is used.
* @param param0 Event that contains the added and removed accounts
*/
private _handleAccountChange({ added, deleted }: { added: AccountInfo[]; deleted: AccountInfo[] }) {
const process = (a: AccountInfo) => ({
// This shouldn't be needed
accessToken: '1234',
id: a.homeAccountId,
scopes: [],
account: {
id: a.homeAccountId,
label: a.username
},
idToken: a.idToken,
private _handleAccountChange({ added, changed, deleted }: { added: AccountInfo[]; changed: AccountInfo[]; deleted: AccountInfo[] }) {
this._onDidChangeSessionsEmitter.fire({
added: added.map(this.sessionFromAccountInfo),
changed: changed.map(this.sessionFromAccountInfo),
removed: deleted.map(this.sessionFromAccountInfo)
});
this._onDidChangeSessionsEmitter.fire({ added: added.map(process), changed: [], removed: deleted.map(process) });
}
//#region AuthenticationProvider methods
async getSessions(scopes: string[] | undefined, options?: AuthenticationGetSessionOptions): Promise<AuthenticationSession[]> {
const askingForAll = scopes === undefined;
const scopeData = new ScopeData(scopes);
this._logger.info('[getSessions]', scopes ? scopeData.scopeStr : 'all', 'starting');
if (!scopes) {
// Do NOT use `scopes` beyond this place in the code. Use `scopeData` instead.
// Do NOT use `scopes` beyond this place in the code. Use `scopeData` instead.
this._logger.info('[getSessions]', askingForAll ? '[all]' : `[${scopeData.scopeStr}]`, 'starting');
const allSessions: AuthenticationSession[] = [];
// This branch only gets called by Core for sign out purposes and initial population of the account menu. Since we are
// living in a world where a "session" from Core's perspective is an account, we return 1 session per account.
// See the large comment on `onDidChangeSessions` for more information.
if (askingForAll) {
const allSessionsForAccounts = new Map<string, AuthenticationSession>();
for (const cachedPca of this._publicClientManager.getAll()) {
const sessions = await this.getAllSessionsForPca(cachedPca, scopeData.originalScopes, scopeData.scopesToSend, options?.account);
allSessions.push(...sessions);
for (const account of cachedPca.accounts) {
if (allSessionsForAccounts.has(account.homeAccountId)) {
continue;
}
allSessionsForAccounts.set(account.homeAccountId, this.sessionFromAccountInfo(account));
}
}
const allSessions = Array.from(allSessionsForAccounts.values());
this._logger.info('[getSessions] [all]', `returned ${allSessions.length} session(s)`);
return allSessions;
}
const cachedPca = await this.getOrCreatePublicClientApplication(scopeData.clientId, scopeData.tenant);
const sessions = await this.getAllSessionsForPca(cachedPca, scopeData.originalScopes, scopeData.scopesToSend, options?.account);
this._logger.info(`[getSessions] returned ${sessions.length} sessions`);
this._logger.info(`[getSessions] [${scopeData.scopeStr}] returned ${sessions.length} session(s)`);
return sessions;
}
@@ -121,7 +121,7 @@ export class MsalAuthProvider implements AuthenticationProvider {
const scopeData = new ScopeData(scopes);
// Do NOT use `scopes` beyond this place in the code. Use `scopeData` instead.
this._logger.info('[createSession]', scopeData.scopeStr, 'starting');
this._logger.info('[createSession]', `[${scopeData.scopeStr}]`, 'starting');
const cachedPca = await this.getOrCreatePublicClientApplication(scopeData.clientId, scopeData.tenant);
let result: AuthenticationResult;
try {
@@ -169,32 +169,43 @@ export class MsalAuthProvider implements AuthenticationProvider {
}
}
const session = this.toAuthenticationSession(result, scopeData.originalScopes);
const session = this.sessionFromAuthenticationResult(result, scopeData.originalScopes);
this._telemetryReporter.sendLoginEvent(session.scopes);
this._logger.info('[createSession]', scopeData.scopeStr, 'returned session');
this._logger.info('[createSession]', `[${scopeData.scopeStr}]`, 'returned session');
// This is the only scenario in which we need to fire the _onDidChangeSessionsEmitter out of band...
// the badge flow (when the client passes no options in to getSession) will only remove a badge if a session
// was created that _matches the scopes_ that that badge requests. See `onDidChangeSessions` for more info.
// TODO: This should really be fixed in Core.
this._onDidChangeSessionsEmitter.fire({ added: [session], changed: [], removed: [] });
return session;
}
async removeSession(sessionId: string): Promise<void> {
this._logger.info('[removeSession]', sessionId, 'starting');
const promises = new Array<Promise<void>>();
for (const cachedPca of this._publicClientManager.getAll()) {
const accounts = cachedPca.accounts;
for (const account of accounts) {
if (account.homeAccountId === sessionId) {
this._telemetryReporter.sendLogoutEvent();
try {
await cachedPca.removeAccount(account);
} catch (e) {
this._telemetryReporter.sendLogoutFailedEvent();
throw e;
}
this._logger.info('[removeSession]', sessionId, 'removed session');
return;
promises.push(cachedPca.removeAccount(account));
this._logger.info(`[removeSession] [${sessionId}] [${cachedPca.clientId}] [${cachedPca.authority}] removing session...`);
}
}
}
this._logger.info('[removeSession]', sessionId, 'session not found');
if (!promises.length) {
this._logger.info('[removeSession]', sessionId, 'session not found');
return;
}
const results = await Promise.allSettled(promises);
for (const result of results) {
if (result.status === 'rejected') {
this._telemetryReporter.sendLogoutFailedEvent();
this._logger.error('[removeSession]', sessionId, 'error removing session', result.reason);
}
}
this._logger.info('[removeSession]', sessionId, `attempted to remove ${promises.length} sessions`);
}
//#endregion
@@ -217,7 +228,7 @@ export class MsalAuthProvider implements AuthenticationProvider {
for (const account of accounts) {
try {
const result = await cachedPca.acquireTokenSilent({ account, scopes: scopesToSend, redirectUri });
sessions.push(this.toAuthenticationSession(result, originalScopes));
sessions.push(this.sessionFromAuthenticationResult(result, originalScopes));
} catch (e) {
// If we can't get a token silently, the account is probably in a bad state so we should skip it
// MSAL will log this already, so we don't need to log it again
@@ -227,7 +238,7 @@ export class MsalAuthProvider implements AuthenticationProvider {
return sessions;
}
private toAuthenticationSession(result: AuthenticationResult, scopes: readonly string[]): AuthenticationSession & { idToken: string } {
private sessionFromAuthenticationResult(result: AuthenticationResult, scopes: readonly string[]): AuthenticationSession & { idToken: string } {
return {
accessToken: result.accessToken,
idToken: result.idToken,
@@ -239,4 +250,17 @@ export class MsalAuthProvider implements AuthenticationProvider {
scopes
};
}
private sessionFromAccountInfo(account: AccountInfo): AuthenticationSession {
return {
accessToken: '1234',
id: account.homeAccountId,
scopes: [],
account: {
id: account.homeAccountId,
label: account.username
},
idToken: account.idToken,
};
}
}