From 95ab795ff0f0a650f2f6fbbb0af1deec4ca552ed Mon Sep 17 00:00:00 2001 From: Tyler James Leonhardt <2644648+TylerLeonhardt@users.noreply.github.com> Date: Wed, 5 Mar 2025 10:55:10 -0800 Subject: [PATCH] Detach authority/tenant from the PublicClientApp (#242719) everything --- .../src/common/accountAccess.ts | 54 +++++-- .../src/common/publicClientCache.ts | 6 +- .../src/common/scopeData.ts | 10 +- .../src/node/authProvider.ts | 86 +++++++++--- .../src/node/cachedPublicClientApplication.ts | 67 +++++---- .../src/node/flows.ts | 7 +- .../src/node/publicClientCache.ts | 132 ++++++++++++------ 7 files changed, 251 insertions(+), 111 deletions(-) diff --git a/extensions/microsoft-authentication/src/common/accountAccess.ts b/extensions/microsoft-authentication/src/common/accountAccess.ts index a8fdeefef98..98d998c47b7 100644 --- a/extensions/microsoft-authentication/src/common/accountAccess.ts +++ b/extensions/microsoft-authentication/src/common/accountAccess.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { Disposable, Event, EventEmitter, SecretStorage } from 'vscode'; +import { Disposable, Event, EventEmitter, LogOutputChannel, SecretStorage } from 'vscode'; import { AccountInfo } from '@azure/msal-node'; interface IAccountAccess { @@ -21,17 +21,19 @@ export class ScopedAccountAccess implements IAccountAccess { private value = new Array(); constructor( - private readonly _secretStorage: SecretStorage, - private readonly _cloudName: string, - private readonly _clientId: string, - private readonly _authority: string + secretStorage: SecretStorage, + cloudName: string, + clientId: string, + logger: LogOutputChannel, + authoritiesToMigrate?: string[], ) { - this._accountAccessSecretStorage = new AccountAccessSecretStorage(this._secretStorage, this._cloudName, this._clientId, this._authority); + this._accountAccessSecretStorage = new AccountAccessSecretStorage(secretStorage, cloudName, clientId, logger, authoritiesToMigrate); this._accountAccessSecretStorage.onDidChange(() => this.update()); } - initialize() { - return this.update(); + async initialize() { + await this._accountAccessSecretStorage.initialize(); + await this.update(); } isAllowedAccess(account: AccountInfo): boolean { @@ -66,13 +68,14 @@ export class AccountAccessSecretStorage { private readonly _onDidChangeEmitter = new EventEmitter; readonly onDidChange: Event = this._onDidChangeEmitter.event; - private readonly _key = `accounts-${this._cloudName}-${this._clientId}-${this._authority}`; + private readonly _key = `accounts-${this._cloudName}`; constructor( private readonly _secretStorage: SecretStorage, private readonly _cloudName: string, private readonly _clientId: string, - private readonly _authority: string + private readonly _logger: LogOutputChannel, + private readonly _authoritiesToMigrate?: string[] ) { this._disposable = Disposable.from( this._onDidChangeEmitter, @@ -84,6 +87,37 @@ export class AccountAccessSecretStorage { ); } + /** + * TODO: Remove this method after a release with the migration + */ + async initialize(): Promise { + if (!this._authoritiesToMigrate) { + return; + } + const current = await this.get(); + // If the secret storage already has the new key, we have already run the migration + if (current) { + return; + } + try { + const allValues = new Set(); + for (const authority of this._authoritiesToMigrate) { + const oldKey = `accounts-${this._cloudName}-${this._clientId}-${authority}`; + const value = await this._secretStorage.get(oldKey); + if (value) { + const parsed = JSON.parse(value) as string[]; + parsed.forEach(v => allValues.add(v)); + } + } + if (allValues.size > 0) { + await this.store(Array.from(allValues)); + } + } catch (e) { + // Migration is best effort + this._logger.error(`Failed to migrate account access secret storage: ${e}`); + } + } + async get(): Promise { const value = await this._secretStorage.get(this._key); if (!value) { diff --git a/extensions/microsoft-authentication/src/common/publicClientCache.ts b/extensions/microsoft-authentication/src/common/publicClientCache.ts index 925a4d1a88c..9e54e0a2aad 100644 --- a/extensions/microsoft-authentication/src/common/publicClientCache.ts +++ b/extensions/microsoft-authentication/src/common/publicClientCache.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import type { AccountInfo, AuthenticationResult, InteractiveRequest, SilentFlowRequest } from '@azure/msal-node'; +import type { AccountInfo, AuthenticationResult, InteractiveRequest, RefreshTokenRequest, SilentFlowRequest } from '@azure/msal-node'; import type { Disposable, Event } from 'vscode'; export interface ICachedPublicClientApplication extends Disposable { @@ -11,14 +11,14 @@ export interface ICachedPublicClientApplication extends Disposable { onDidRemoveLastAccount: Event; acquireTokenSilent(request: SilentFlowRequest): Promise; acquireTokenInteractive(request: InteractiveRequest): Promise; + acquireTokenByRefreshToken(request: RefreshTokenRequest): Promise; removeAccount(account: AccountInfo): Promise; accounts: AccountInfo[]; clientId: string; - authority: string; } export interface ICachedPublicClientApplicationManager { onDidAccountsChange: Event<{ added: AccountInfo[]; changed: AccountInfo[]; deleted: AccountInfo[] }>; - getOrCreate(clientId: string, authority: string): Promise; + getOrCreate(clientId: string, refreshTokensToMigrate?: string[]): Promise; getAll(): ICachedPublicClientApplication[]; } diff --git a/extensions/microsoft-authentication/src/common/scopeData.ts b/extensions/microsoft-authentication/src/common/scopeData.ts index a43f2c431dd..f6baf20d7ac 100644 --- a/extensions/microsoft-authentication/src/common/scopeData.ts +++ b/extensions/microsoft-authentication/src/common/scopeData.ts @@ -46,6 +46,11 @@ export class ScopeData { */ readonly tenant: string; + /** + * The tenant ID to use for the token request. This is the value of the `VSCODE_TENANT:...` scope if present, otherwise undefined. + */ + readonly tenantId: string | undefined; + constructor(readonly originalScopes: readonly string[] = []) { if (workspace.getConfiguration('microsoft-authentication').get<'v1' | 'v2'>('clientIdVersion') === 'v2') { this._defaultClientId = DEFAULT_CLIENT_ID_V2; @@ -61,7 +66,8 @@ export class ScopeData { this.scopeStr = modifiedScopes.join(' '); this.scopesToSend = this.getScopesToSend(modifiedScopes); this.clientId = this.getClientId(this.allScopes); - this.tenant = this.getTenantId(this.allScopes); + this.tenantId = this.getTenantId(this.allScopes); + this.tenant = this.tenantId ?? this._defaultTenant; } private getClientId(scopes: string[]) { @@ -79,7 +85,7 @@ export class ScopeData { return current.split('VSCODE_TENANT:')[1]; } return prev; - }, undefined) ?? this._defaultTenant; + }, undefined); } private getScopesToSend(scopes: string[]) { diff --git a/extensions/microsoft-authentication/src/node/authProvider.ts b/extensions/microsoft-authentication/src/node/authProvider.ts index 40000e08620..1956def085c 100644 --- a/extensions/microsoft-authentication/src/node/authProvider.ts +++ b/extensions/microsoft-authentication/src/node/authProvider.ts @@ -51,12 +51,7 @@ 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, - this._env.name - ); + this._publicClientManager = new CachedPublicClientApplicationManager(_context.secrets, this._logger, this._env.name); const accountChangeEvent = this._eventBufferer.wrapEvent( this._publicClientManager.onDidAccountsChange, (last, newEvent) => { @@ -109,8 +104,8 @@ export class MsalAuthProvider implements AuthenticationProvider { clientTenantMap.get(key)!.refreshTokens.push(session.refreshToken); } - for (const { clientId, tenant, refreshTokens } of clientTenantMap.values()) { - await this.getOrCreatePublicClientApplication(clientId, tenant, refreshTokens); + for (const { clientId, refreshTokens } of clientTenantMap.values()) { + await this._publicClientManager.getOrCreate(clientId, refreshTokens); } } @@ -173,8 +168,8 @@ export class MsalAuthProvider implements AuthenticationProvider { return allSessions; } - const cachedPca = await this.getOrCreatePublicClientApplication(scopeData.clientId, scopeData.tenant); - const sessions = await this.getAllSessionsForPca(cachedPca, scopeData.originalScopes, scopeData.scopesToSend, options?.account); + const cachedPca = await this._publicClientManager.getOrCreate(scopeData.clientId); + const sessions = await this.getAllSessionsForPca(cachedPca, scopeData, options?.account); this._logger.info(`[getSessions] [${scopeData.scopeStr}] returned ${sessions.length} session(s)`); return sessions; @@ -185,7 +180,7 @@ export class MsalAuthProvider implements AuthenticationProvider { // Do NOT use `scopes` beyond this place in the code. Use `scopeData` instead. this._logger.info('[createSession]', `[${scopeData.scopeStr}]`, 'starting'); - const cachedPca = await this.getOrCreatePublicClientApplication(scopeData.clientId, scopeData.tenant); + const cachedPca = await this._publicClientManager.getOrCreate(scopeData.clientId); // Used for showing a friendlier message to the user when the explicitly cancel a flow. let userCancelled: boolean | undefined; @@ -211,6 +206,7 @@ export class MsalAuthProvider implements AuthenticationProvider { : ExtensionHost.WebWorker, }); + const authority = new URL(scopeData.tenant, this._env.activeDirectoryEndpointUrl).toString(); let lastError: Error | undefined; for (const flow of flows) { if (flow !== flows[0]) { @@ -223,6 +219,7 @@ export class MsalAuthProvider implements AuthenticationProvider { try { const result = await flow.trigger({ cachedPca, + authority, scopes: scopeData.scopesToSend, loginHint: options.account?.label, windowHandle: window.nativeHandle ? Buffer.from(window.nativeHandle) : undefined, @@ -260,7 +257,7 @@ export class MsalAuthProvider implements AuthenticationProvider { if (account.homeAccountId === sessionId) { this._telemetryReporter.sendLogoutEvent(); promises.push(cachedPca.removeAccount(account)); - this._logger.info(`[removeSession] [${sessionId}] [${cachedPca.clientId}] [${cachedPca.authority}] removing session...`); + this._logger.info(`[removeSession] [${sessionId}] [${cachedPca.clientId}] removing session...`); } } } @@ -281,26 +278,69 @@ export class MsalAuthProvider implements AuthenticationProvider { //#endregion - private async getOrCreatePublicClientApplication(clientId: string, tenant: string, refreshTokensToMigrate?: string[]): Promise { - const authority = new URL(tenant, this._env.activeDirectoryEndpointUrl).toString(); - return await this._publicClientManager.getOrCreate(clientId, authority, refreshTokensToMigrate); - } - private async getAllSessionsForPca( cachedPca: ICachedPublicClientApplication, - originalScopes: readonly string[], - scopesToSend: string[], + scopeData: ScopeData, accountFilter?: AuthenticationSessionAccountInformation ): Promise { - const accounts = accountFilter + let filteredAccounts = accountFilter ? cachedPca.accounts.filter(a => a.homeAccountId === accountFilter.id) : cachedPca.accounts; + + // Group accounts by homeAccountId + const accountGroups = new Map(); + for (const account of filteredAccounts) { + const existing = accountGroups.get(account.homeAccountId) || []; + existing.push(account); + accountGroups.set(account.homeAccountId, existing); + } + + // Filter to one account per homeAccountId + filteredAccounts = Array.from(accountGroups.values()).map(accounts => { + if (accounts.length === 1) { + return accounts[0]; + } + + // If we have a specific tenant to target, prefer that one + if (scopeData.tenantId) { + const matchingTenant = accounts.find(a => a.tenantId === scopeData.tenantId); + if (matchingTenant) { + return matchingTenant; + } + } + + // Otherwise prefer the home tenant + return accounts.find(a => a.tenantId === a.idTokenClaims?.tid) || accounts[0]; + }); + + const authority = new URL(scopeData.tenant, this._env.activeDirectoryEndpointUrl).toString(); const sessions: AuthenticationSession[] = []; return this._eventBufferer.bufferEventsAsync(async () => { - for (const account of accounts) { + for (const account of filteredAccounts) { try { - const result = await cachedPca.acquireTokenSilent({ account, scopes: scopesToSend, redirectUri }); - sessions.push(this.sessionFromAuthenticationResult(result, originalScopes)); + let forceRefresh: true | undefined; + if (scopeData.tenantId) { + // If the tenants do not match, then we need to skip the cache + // to get a new token for the new tenant + if (account.tenantId !== scopeData.tenantId) { + forceRefresh = true; + } + } else { + // If we are requesting the home tenant and we don't yet have + // a token for the home tenant, we need to skip the cache + // to get a new token for the home tenant + if (account.tenantId !== account.idTokenClaims?.tid) { + forceRefresh = true; + } + } + const result = await cachedPca.acquireTokenSilent({ + account, + authority, + scopes: scopeData.scopesToSend, + redirectUri, + forceRefresh + }); + sessions.push(this.sessionFromAuthenticationResult(result, scopeData.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 diff --git a/extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts b/extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts index 8d081f1a825..556c8a1b4e4 100644 --- a/extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts +++ b/extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts @@ -3,9 +3,9 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { PublicClientApplication, AccountInfo, Configuration, SilentFlowRequest, AuthenticationResult, InteractiveRequest, LogLevel, RefreshTokenRequest } from '@azure/msal-node'; +import { PublicClientApplication, AccountInfo, SilentFlowRequest, AuthenticationResult, InteractiveRequest, LogLevel, RefreshTokenRequest } from '@azure/msal-node'; import { NativeBrokerPlugin } from '@azure/msal-node-extensions'; -import { Disposable, Memento, SecretStorage, LogOutputChannel, window, ProgressLocation, l10n, EventEmitter } from 'vscode'; +import { Disposable, SecretStorage, LogOutputChannel, window, ProgressLocation, l10n, EventEmitter } from 'vscode'; import { raceCancellationAndTimeoutError } from '../common/async'; import { SecretStorageCachePlugin } from '../common/cachePlugin'; import { MsalLoggerOptions } from '../common/loggerOptions'; @@ -23,11 +23,11 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica private readonly _secretStorageCachePlugin = new SecretStorageCachePlugin( this._secretStorage, // Include the prefix as a differentiator to other secrets - `pca:${JSON.stringify({ clientId: this._clientId, authority: this._authority })}` + `pca:${this._clientId}` ); // Broker properties - private readonly _accountAccess = new ScopedAccountAccess(this._secretStorage, this._cloudName, this._clientId, this._authority); + private readonly _accountAccess = new ScopedAccountAccess(this._secretStorage, this._cloudName, this.clientId, this._logger, this._authoritiesToMigrate); private readonly _isBrokerAvailable: boolean; //#region Events @@ -42,22 +42,19 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica constructor( private readonly _clientId: string, - private readonly _authority: string, private readonly _cloudName: string, - private readonly _globalMemento: Memento, private readonly _secretStorage: SecretStorage, - private readonly _logger: LogOutputChannel + private readonly _logger: LogOutputChannel, + private readonly _authoritiesToMigrate?: string[], ) { - // TODO:@TylerLeonhardt clean up old use of memento. Remove this in an iteration - this._globalMemento.update(`lastRemoval:${this._clientId}:${this._authority}`, undefined); const loggerOptions = new MsalLoggerOptions(_logger); const nativeBrokerPlugin = new NativeBrokerPlugin(); this._isBrokerAvailable = nativeBrokerPlugin.isBrokerAvailable ?? false; this._pca = new PublicClientApplication({ - auth: { clientId: _clientId, authority: _authority }, + auth: { clientId: _clientId }, system: { loggerOptions: { - correlationId: `${_clientId}] [${_authority}`, + correlationId: _clientId, loggerCallback: (level, message, containsPii) => loggerOptions.loggerCallback(level, message, containsPii), logLevel: LogLevel.Trace } @@ -74,7 +71,6 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica get accounts(): AccountInfo[] { return this._accounts; } get clientId(): string { return this._clientId; } - get authority(): string { return this._authority; } async initialize(): Promise { if (this._isBrokerAvailable) { @@ -88,9 +84,9 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica } async acquireTokenSilent(request: SilentFlowRequest): Promise { - this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] starting...`); + this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] starting...`); let result = await this._sequencer.queue(() => this._pca.acquireTokenSilent(request)); - this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] got result`); + this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] got result`); // Check expiration of id token and if it's 5min before expiration, force a refresh. // this is what MSAL does for access tokens already so we're just adding it for id tokens since we care about those. // NOTE: Once we stop depending on id tokens for some things we can remove all of this. @@ -101,13 +97,13 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica * 1000 // convert to milliseconds ); if (fiveMinutesBefore < new Date()) { - this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] id token is expired or about to expire. Forcing refresh...`); + this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] id token is expired or about to expire. Forcing refresh...`); const newRequest = this._isBrokerAvailable // HACK: Broker doesn't support forceRefresh so we need to pass in claims which will force a refresh ? { ...request, claims: '{ "id_token": {}}' } : { ...request, forceRefresh: true }; result = await this._sequencer.queue(() => this._pca.acquireTokenSilent(newRequest)); - this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] got forced result`); + this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] got forced result`); } const newIdTokenExpirationInSecs = (result.idTokenClaims as { exp?: number }).exp; if (newIdTokenExpirationInSecs) { @@ -116,15 +112,15 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica * 1000 // convert to milliseconds ); if (fiveMinutesBefore < new Date()) { - this._logger.error(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] id token is still expired.`); + this._logger.error(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] id token is still expired.`); // HACK: Only for the Broker we try one more time with different claims to force a refresh. Why? We've seen the Broker caching tokens by the claims requested, thus // there has been a situation where both tokens are expired. if (this._isBrokerAvailable) { - this._logger.error(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] forcing refresh with different claims...`); + this._logger.error(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] forcing refresh with different claims...`); const newRequest = { ...request, claims: '{ "access_token": {}}' }; result = await this._sequencer.queue(() => this._pca.acquireTokenSilent(newRequest)); - this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] got forced result with different claims`); + this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] got forced result with different claims`); const newIdTokenExpirationInSecs = (result.idTokenClaims as { exp?: number }).exp; if (newIdTokenExpirationInSecs) { const fiveMinutesBefore = new Date( @@ -132,7 +128,7 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica * 1000 // convert to milliseconds ); if (fiveMinutesBefore < new Date()) { - this._logger.error(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] id token is still expired.`); + this._logger.error(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] id token is still expired.`); } } } @@ -140,15 +136,17 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica } } - if (result.account && !result.fromCache && this._verifyIfUsingBroker(result)) { - this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}] [${request.account.username}] firing event due to change`); + if (!result.account) { + this._logger.error(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] no account found in result`); + } else if (!result.fromCache && this._verifyIfUsingBroker(result)) { + this._logger.debug(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] firing event due to change`); this._onDidAccountsChangeEmitter.fire({ added: [], changed: [result.account], deleted: [] }); } return result; } async acquireTokenInteractive(request: InteractiveRequest): Promise { - this._logger.debug(`[acquireTokenInteractive] [${this._clientId}] [${this._authority}] [${request.scopes?.join(' ')}] loopbackClientOverride: ${request.loopbackClient ? 'true' : 'false'}`); + this._logger.debug(`[acquireTokenInteractive] [${this._clientId}] [${request.authority}] [${request.scopes?.join(' ')}] loopbackClientOverride: ${request.loopbackClient ? 'true' : 'false'}`); return await window.withProgress( { location: ProgressLocation.Notification, @@ -180,8 +178,8 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica * @param request a {@link RefreshTokenRequest} object that contains the refresh token and other parameters. * @returns an {@link AuthenticationResult} object that contains the result of the token acquisition operation. */ - async acquireTokenByRefreshToken(request: RefreshTokenRequest) { - this._logger.debug(`[acquireTokenByRefreshToken] [${this._clientId}] [${this._authority}] [${request.scopes.join(' ')}]`); + async acquireTokenByRefreshToken(request: RefreshTokenRequest): Promise { + this._logger.debug(`[acquireTokenByRefreshToken] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}]`); const result = await this._sequencer.queue(() => this._pca.acquireTokenByRefreshToken(request)); if (result) { // this._setupRefresh(result); @@ -213,7 +211,14 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica if (!result.fromNativeBroker) { return true; } - const key = result.account!.homeAccountId; + // The nativeAccountId is what the broker uses to differenciate all + // types of accounts. Even if the "account" is a duplicate of another because + // it's actaully a guest account in another tenant. + let key = result.account!.nativeAccountId; + if (!key) { + this._logger.error(`[verifyIfUsingBroker] [${this._clientId}] [${result.account!.username}] no nativeAccountId found. Using homeAccountId instead.`); + key = result.account!.homeAccountId; + } const lastSeen = this._lastSeen.get(key); const lastTimeAuthed = result.account!.idTokenClaims!.iat!; if (!lastSeen) { @@ -229,7 +234,7 @@ 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}`); + this._logger.debug(`[update] [${this._clientId}] CachedPublicClientApplication update before: ${before.length}`); // Clear in-memory cache so we know we're getting account data from the SecretStorage this._pca.clearCache(); let after = await this._pca.getAllAccounts(); @@ -237,7 +242,7 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica after = after.filter(a => this._accountAccess.isAllowedAccess(a)); } this._accounts = after; - this._logger.debug(`[update] [${this._clientId}] [${this._authority}] CachedPublicClientApplication update after: ${after.length}`); + this._logger.debug(`[update] [${this._clientId}] CachedPublicClientApplication update after: ${after.length}`); const beforeSet = new Set(before.map(b => b.homeAccountId)); const afterSet = new Set(after.map(a => a.homeAccountId)); @@ -246,13 +251,13 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica const deleted = before.filter(b => !afterSet.has(b.homeAccountId)); if (added.length > 0 || deleted.length > 0) { this._onDidAccountsChangeEmitter.fire({ added, changed: [], deleted }); - this._logger.debug(`[update] [${this._clientId}] [${this._authority}] CachedPublicClientApplication accounts changed. added: ${added.length}, deleted: ${deleted.length}`); + this._logger.debug(`[update] [${this._clientId}] CachedPublicClientApplication accounts changed. added: ${added.length}, deleted: ${deleted.length}`); if (!after.length) { - this._logger.debug(`[update] [${this._clientId}] [${this._authority}] CachedPublicClientApplication final account deleted. Firing event.`); + this._logger.debug(`[update] [${this._clientId}] CachedPublicClientApplication final account deleted. Firing event.`); this._onDidRemoveLastAccountEmitter.fire(); } } - this._logger.debug(`[update] [${this._clientId}] [${this._authority}] CachedPublicClientApplication update complete`); + this._logger.debug(`[update] [${this._clientId}] CachedPublicClientApplication update complete`); } } diff --git a/extensions/microsoft-authentication/src/node/flows.ts b/extensions/microsoft-authentication/src/node/flows.ts index c6f40a943f6..ac678d8313d 100644 --- a/extensions/microsoft-authentication/src/node/flows.ts +++ b/extensions/microsoft-authentication/src/node/flows.ts @@ -25,6 +25,7 @@ interface IMsalFlowOptions { interface IMsalFlowTriggerOptions { cachedPca: ICachedPublicClientApplication; + authority: string; scopes: string[]; loginHint?: string; windowHandle?: Buffer; @@ -45,11 +46,12 @@ class DefaultLoopbackFlow implements IMsalFlow { supportsWebWorkerExtensionHost: false }; - async trigger({ cachedPca, scopes, loginHint, windowHandle, logger }: IMsalFlowTriggerOptions): Promise { + async trigger({ cachedPca, authority, scopes, loginHint, windowHandle, logger }: IMsalFlowTriggerOptions): Promise { logger.info('Trying default msal flow...'); return await cachedPca.acquireTokenInteractive({ openBrowser: async (url: string) => { await env.openExternal(Uri.parse(url)); }, scopes, + authority, successTemplate: loopbackTemplate, errorTemplate: loopbackTemplate, loginHint, @@ -66,12 +68,13 @@ class UrlHandlerFlow implements IMsalFlow { supportsWebWorkerExtensionHost: false }; - async trigger({ cachedPca, scopes, loginHint, windowHandle, logger, uriHandler }: IMsalFlowTriggerOptions): Promise { + async trigger({ cachedPca, authority, scopes, loginHint, windowHandle, logger, uriHandler }: IMsalFlowTriggerOptions): Promise { logger.info('Trying protocol handler flow...'); const loopbackClient = new UriHandlerLoopbackClient(uriHandler, redirectUri, logger); return await cachedPca.acquireTokenInteractive({ openBrowser: (url: string) => loopbackClient.openBrowser(url), scopes, + authority, loopbackClient, loginHint, prompt: loginHint ? undefined : 'select_account', diff --git a/extensions/microsoft-authentication/src/node/publicClientCache.ts b/extensions/microsoft-authentication/src/node/publicClientCache.ts index f4f19ff8df5..d8b5ebc610b 100644 --- a/extensions/microsoft-authentication/src/node/publicClientCache.ts +++ b/extensions/microsoft-authentication/src/node/publicClientCache.ts @@ -14,8 +14,8 @@ export interface IPublicClientApplicationInfo { } export class CachedPublicClientApplicationManager implements ICachedPublicClientApplicationManager { - // The key is the clientId and authority JSON stringified - private readonly _pcas = new Map(); + // The key is the clientId + private readonly _pcas = new Map(); private readonly _pcaDisposables = new Map(); private _disposable: Disposable; @@ -25,7 +25,6 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient readonly onDidAccountsChange = this._onDidAccountsChangeEmitter.event; constructor( - private readonly _globalMemento: Memento, private readonly _secretStorage: SecretStorage, private readonly _logger: LogOutputChannel, private readonly _cloudName: string @@ -44,26 +43,27 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient async initialize() { this._logger.debug('[initialize] Initializing PublicClientApplicationManager'); - let keys: string[] | undefined; + let clientIds: string[] | undefined; + let migrations: Map | undefined; try { - keys = await this._pcasSecretStorage.get(); + migrations = await this._getMigrationsPerClientId(); + clientIds = await this._pcasSecretStorage.get(); } catch (e) { // data is corrupted this._logger.error('[initialize] Error initializing PublicClientApplicationManager:', e); await this._pcasSecretStorage.delete(); } - if (!keys) { + if (!clientIds) { return; } const promises = new Array>(); - for (const key of keys) { + for (const clientId of clientIds) { try { - const { clientId, authority } = JSON.parse(key) as IPublicClientApplicationInfo; // Load the PCA in memory - promises.push(this._doCreatePublicClientApplication(clientId, authority, key)); + promises.push(this._doCreatePublicClientApplication(clientId)); } catch (e) { - this._logger.error('[initialize] Error intitializing PCA:', key); + this._logger.error('[initialize] Error intitializing PCA:', clientId); } } @@ -75,11 +75,11 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient } else { if (!result.value.accounts.length) { pcasChanged = true; - const pcaKey = JSON.stringify({ clientId: result.value.clientId, authority: result.value.authority }); - this._pcaDisposables.get(pcaKey)?.dispose(); - this._pcaDisposables.delete(pcaKey); - this._pcas.delete(pcaKey); - this._logger.debug(`[initialize] [${result.value.clientId}] [${result.value.authority}] PCA disposed because it's empty.`); + const clientId = result.value.clientId; + this._pcaDisposables.get(clientId)?.dispose(); + this._pcaDisposables.delete(clientId); + this._pcas.delete(clientId); + this._logger.debug(`[initialize] [${clientId}] PCA disposed because it's empty.`); } } } @@ -89,37 +89,52 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient this._logger.debug('[initialize] PublicClientApplicationManager initialized'); } + private async _getMigrationsPerClientId(): Promise | undefined> { + await this._pcasSecretStorage.initialize(); + const oldValue = await this._pcasSecretStorage.getOldValue(); + // returns a map of clientIds to the authorities found in the old value + if (!oldValue) { + return undefined; + } + const result = new Map(); + for (const { clientId, authority } of oldValue) { + if (!result.has(clientId)) { + result.set(clientId, []); + } + result.get(clientId)?.push(authority); + } + return result; + } + dispose() { this._disposable.dispose(); Disposable.from(...this._pcaDisposables.values()).dispose(); } - async getOrCreate(clientId: string, authority: string, refreshTokensToMigrate?: string[]): Promise { - // Use the clientId and authority as the key - const pcasKey = JSON.stringify({ clientId, authority }); - let pca = this._pcas.get(pcasKey); + async getOrCreate(clientId: string, refreshTokensToMigrate?: string[]): Promise { + let pca = this._pcas.get(clientId); if (pca) { - this._logger.debug(`[getOrCreate] [${clientId}] [${authority}] PublicClientApplicationManager cache hit`); + this._logger.debug(`[getOrCreate] [${clientId}] PublicClientApplicationManager cache hit`); } else { - this._logger.debug(`[getOrCreate] [${clientId}] [${authority}] PublicClientApplicationManager cache miss, creating new PCA...`); - pca = await this._doCreatePublicClientApplication(clientId, authority, pcasKey); + this._logger.debug(`[getOrCreate] [${clientId}] PublicClientApplicationManager cache miss, creating new PCA...`); + pca = await this._doCreatePublicClientApplication(clientId, refreshTokensToMigrate); await this._storePublicClientApplications(); - this._logger.debug(`[getOrCreate] [${clientId}] [${authority}] PCA created.`); + this._logger.debug(`[getOrCreate] [${clientId}] PCA created.`); } // TODO: MSAL Migration. Remove this when we remove the old flow. if (refreshTokensToMigrate?.length) { - this._logger.debug(`[getOrCreate] [${clientId}] [${authority}] Migrating refresh tokens to PCA...`); + this._logger.debug(`[getOrCreate] [${clientId}] Migrating refresh tokens to PCA...`); for (const refreshToken of refreshTokensToMigrate) { try { // Use the refresh token to acquire a result. This will cache the refresh token for future operations. // The scopes don't matter here since we can create any token from the refresh token. const result = await pca.acquireTokenByRefreshToken({ refreshToken, forceCache: true, scopes: [] }); if (result?.account) { - this._logger.debug(`[getOrCreate] [${clientId}] [${authority}] Refresh token migrated to PCA.`); + this._logger.debug(`[getOrCreate] [${clientId}] Refresh token migrated to PCA.`); } } catch (e) { - this._logger.error(`[getOrCreate] [${clientId}] [${authority}] Error migrating refresh token:`, e); + this._logger.error(`[getOrCreate] [${clientId}] Error migrating refresh token:`, e); } } // reinitialize the PCA so the account is properly cached @@ -128,9 +143,9 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient return pca; } - private async _doCreatePublicClientApplication(clientId: string, authority: string, pcasKey: string) { - const pca = new CachedPublicClientApplication(clientId, authority, this._cloudName, this._globalMemento, this._secretStorage, this._logger); - this._pcas.set(pcasKey, pca); + private async _doCreatePublicClientApplication(clientId: string, authoritiesToMigrate?: string[]): Promise { + const pca = new CachedPublicClientApplication(clientId, this._cloudName, this._secretStorage, this._logger, authoritiesToMigrate); + this._pcas.set(clientId, pca); const disposable = Disposable.from( pca, pca.onDidAccountsChange(e => this._onDidAccountsChangeEmitter.fire(e)), @@ -138,13 +153,13 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient // The PCA has no more accounts, so we can dispose it so we're not keeping it // around forever. disposable.dispose(); - this._pcaDisposables.delete(pcasKey); - this._pcas.delete(pcasKey); - this._logger.debug(`[_doCreatePublicClientApplication] [${clientId}] [${authority}] PCA disposed. Firing off storing of PCAs...`); + this._pcaDisposables.delete(clientId); + this._pcas.delete(clientId); + this._logger.debug(`[_doCreatePublicClientApplication] [${clientId}] PCA disposed. Firing off storing of PCAs...`); void this._storePublicClientApplications(); }) ); - this._pcaDisposables.set(pcasKey, disposable); + this._pcaDisposables.set(clientId, disposable); // Intialize the PCA after the `onDidAccountsChange` is set so we get initial state. await pca.initialize(); return pca; @@ -183,15 +198,14 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient } // Handle the new ones - for (const newPca of pcaKeysFromStorage) { + for (const clientId of pcaKeysFromStorage) { try { - const { clientId, authority } = JSON.parse(newPca); - this._logger.debug(`[_handleSecretStorageChange] [${clientId}] [${authority}] Creating new PCA that was created in another window...`); - await this._doCreatePublicClientApplication(clientId, authority, newPca); - this._logger.debug(`[_handleSecretStorageChange] [${clientId}] [${authority}] PCA created.`); + this._logger.debug(`[_handleSecretStorageChange] [${clientId}] Creating new PCA that was created in another window...`); + await this._doCreatePublicClientApplication(clientId); + this._logger.debug(`[_handleSecretStorageChange] [${clientId}] PCA created.`); } catch (_e) { // This really shouldn't happen, but should we do something about this? - this._logger.error(`Failed to parse new PublicClientApplication: ${newPca}`); + this._logger.error(`Failed to create new PublicClientApplication: ${clientId}`); continue; } } @@ -210,7 +224,8 @@ class PublicClientApplicationsSecretStorage { private readonly _onDidChangeEmitter = new EventEmitter; readonly onDidChange: Event = this._onDidChangeEmitter.event; - private readonly _key = `publicClientApplications-${this._cloudName}`; + private readonly _oldKey = `publicClientApplications-${this._cloudName}`; + private readonly _key = `publicClients-${this._cloudName}`; constructor(private readonly _secretStorage: SecretStorage, private readonly _cloudName: string) { this._disposable = Disposable.from( @@ -223,6 +238,24 @@ class PublicClientApplicationsSecretStorage { ); } + /** + * Runs the migration. + * TODO: Remove this after a version. + */ + async initialize() { + const oldValue = await this.getOldValue(); + if (!oldValue) { + return; + } + const newValue = await this.get() ?? []; + for (const { clientId } of oldValue) { + if (!newValue.includes(clientId)) { + newValue.push(clientId); + } + } + await this.store(newValue); + } + async get(): Promise { const value = await this._secretStorage.get(this._key); if (!value) { @@ -231,6 +264,25 @@ class PublicClientApplicationsSecretStorage { return JSON.parse(value); } + /** + * Old representation of data that included the authority. This should be removed in a version or 2. + * @returns An array of objects with clientId and authority + */ + async getOldValue(): Promise<{ clientId: string; authority: string }[] | undefined> { + const value = await this._secretStorage.get(this._oldKey); + if (!value) { + return undefined; + } + const result: { clientId: string; authority: string }[] = []; + for (const stringifiedObj of JSON.parse(value)) { + const obj = JSON.parse(stringifiedObj); + if (obj.clientId && obj.authority) { + result.push(obj); + } + } + return result; + } + store(value: string[]): Thenable { return this._secretStorage.store(this._key, JSON.stringify(value)); }