Enable the broker in macOS (#261148)

* Enable the broker in macOS

Fixes https://github.com/microsoft/vscode/issues/260158

* for testing

* better globbing

* guh

* guh

* delete

* log it all

* let's just log everything

* Only do on supported OS/Arches

* Add a console.log

* look at VSCODE_ARCH

* add msal files

* add entitlement maybe here

* actually it's probably here

* build: bundle msal libs for x64 and arm64

* revert that

* try again

* try adding $(AppIdentifierPrefix)

* temp: add debuggee entitlements

* bump msal and pass in redirect uri on macOS

* revert entitlement files

* forgot the .helper

* Allow PII for the output channel only

* use unsigned option

---------

Co-authored-by: deepak1556 <hop2deep@gmail.com>

extensions/microsoft-authentication/src/node/authProvider.ts

@@ -213,6 +213,7 @@ export class MsalAuthProvider implements AuthenticationProvider {
 			extensionHost: isNodeEnvironment
 				? this._context.extension.extensionKind === ExtensionKind.UI ? ExtensionHost.Local : ExtensionHost.Remote
 				: ExtensionHost.WebWorker,
+			isBrokerSupported: cachedPca.isBrokerAvailable
 		});
 
 		const authority = new URL(scopeData.tenant, this._env.activeDirectoryEndpointUrl).toString();
@@ -342,6 +343,7 @@ export class MsalAuthProvider implements AuthenticationProvider {
 			extensionHost: isNodeEnvironment
 				? this._context.extension.extensionKind === ExtensionKind.UI ? ExtensionHost.Local : ExtensionHost.Remote
 				: ExtensionHost.WebWorker,
+			isBrokerSupported: cachedPca.isBrokerAvailable
 		});
 
 		const authority = new URL(scopeData.tenant, this._env.activeDirectoryEndpointUrl).toString();

extensions/microsoft-authentication/src/node/cachedPublicClientApplication.ts

@@ -24,7 +24,7 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
 	private readonly _secretStorageCachePlugin: SecretStorageCachePlugin;
 
 	// Broker properties
-	private readonly _isBrokerAvailable: boolean;
+	readonly isBrokerAvailable: boolean;
 
 	//#region Events
 
@@ -51,14 +51,16 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
 
 		const loggerOptions = new MsalLoggerOptions(_logger, telemetryReporter);
 		const nativeBrokerPlugin = new NativeBrokerPlugin();
-		this._isBrokerAvailable = nativeBrokerPlugin.isBrokerAvailable;
+		this.isBrokerAvailable = nativeBrokerPlugin.isBrokerAvailable;
 		this._pca = new PublicClientApplication({
 			auth: { clientId: _clientId },
 			system: {
 				loggerOptions: {
 					correlationId: _clientId,
 					loggerCallback: (level, message, containsPii) => loggerOptions.loggerCallback(level, message, containsPii),
-					logLevel: LogLevel.Trace
+					logLevel: LogLevel.Trace,
+					// Enable PII logging since it will only go to the output channel
+					piiLoggingEnabled: true
 				}
 			},
 			broker: { nativeBrokerPlugin },
@@ -110,7 +112,7 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
 			);
 			if (fiveMinutesBefore < new Date()) {
 				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
+				const newRequest = this.isBrokerAvailable
 					// HACK: Broker doesn't support forceRefresh so we need to pass in claims which will force a refresh
 					? { ...request, claims: request.claims ?? '{ "id_token": {}}' }
 					: { ...request, forceRefresh: true };
@@ -128,7 +130,7 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
 
 					// 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) {
+					if (this.isBrokerAvailable) {
 						this._logger.error(`[acquireTokenSilent] [${this._clientId}] [${request.authority}] [${request.scopes.join(' ')}] [${request.account.username}] forcing refresh with different claims...`);
 						const newRequest = { ...request, claims: request.claims ?? '{ "access_token": {}}' };
 						result = await this._sequencer.queue(() => this._pca.acquireTokenSilent(newRequest));
@@ -166,20 +168,25 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
 				title: l10n.t('Signing in to Microsoft...')
 			},
 			(_process, token) => this._sequencer.queue(async () => {
-				const result = await raceCancellationAndTimeoutError(
-					this._pca.acquireTokenInteractive(request),
-					token,
-					1000 * 60 * 5
-				);
-				if (this._isBrokerAvailable) {
-					await this._accountAccess.setAllowedAccess(result.account!, true);
+				try {
+					const result = await raceCancellationAndTimeoutError(
+						this._pca.acquireTokenInteractive(request),
+						token,
+						1000 * 60 * 5
+					);
+					if (this.isBrokerAvailable) {
+						await this._accountAccess.setAllowedAccess(result.account!, true);
+					}
+					// Force an update so that the account cache is updated.
+					// TODO:@TylerLeonhardt The problem is, we use the sequencer for
+					// change events but we _don't_ use it for the accounts cache.
+					// We should probably use it for the accounts cache as well.
+					await this._update();
+					return result;
+				} catch (error) {
+					this._logger.error(`[acquireTokenInteractive] [${this._clientId}] [${request.authority}] [${request.scopes?.join(' ')}] error: ${error}`);
+					throw error;
 				}
-				// Force an update so that the account cache is updated.
-				// TODO:@TylerLeonhardt The problem is, we use the sequencer for
-				// change events but we _don't_ use it for the accounts cache.
-				// We should probably use it for the accounts cache as well.
-				await this._update();
-				return result;
 			})
 		);
 	}
@@ -203,7 +210,7 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
 		});
 		if (result) {
 			// this._setupRefresh(result);
-			if (this._isBrokerAvailable && result.account) {
+			if (this.isBrokerAvailable && result.account) {
 				await this._accountAccess.setAllowedAccess(result.account, true);
 			}
 		}
@@ -211,14 +218,14 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
 	}
 
 	removeAccount(account: AccountInfo): Promise<void> {
-		if (this._isBrokerAvailable) {
+		if (this.isBrokerAvailable) {
 			return this._accountAccess.setAllowedAccess(account, false);
 		}
 		return this._sequencer.queue(() => this._pca.getTokenCache().removeAccount(account));
 	}
 
 	private _registerOnSecretStorageChanged() {
-		if (this._isBrokerAvailable) {
+		if (this.isBrokerAvailable) {
 			return this._accountAccess.onDidAccountAccessChange(() => this._sequencer.queue(() => this._update()));
 		}
 		return this._secretStorageCachePlugin.onDidChange(() => this._sequencer.queue(() => this._update()));
@@ -258,7 +265,7 @@ export class CachedPublicClientApplication implements ICachedPublicClientApplica
 		// 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) {
+		if (this.isBrokerAvailable) {
 			after = after.filter(a => this._accountAccess.isAllowedAccess(a));
 		}
 		this._accounts = after;

extensions/microsoft-authentication/src/node/flows.ts

@@ -9,8 +9,9 @@ import { ICachedPublicClientApplication } from '../common/publicClientCache';
 import { UriHandlerLoopbackClient } from '../common/loopbackClientAndOpener';
 import { UriEventHandler } from '../UriEventHandler';
 import { loopbackTemplate } from './loopbackTemplate';
+import { Config } from '../common/config';
 
-const redirectUri = 'https://vscode.dev/redirect';
+const DEFAULT_REDIRECT_URI = 'https://vscode.dev/redirect';
 
 export const enum ExtensionHost {
 	WebWorker,
@@ -49,6 +50,10 @@ class DefaultLoopbackFlow implements IMsalFlow {
 
 	async trigger({ cachedPca, authority, scopes, claims, loginHint, windowHandle, logger }: IMsalFlowTriggerOptions): Promise<AuthenticationResult> {
 		logger.info('Trying default msal flow...');
+		let redirectUri: string | undefined;
+		if (cachedPca.isBrokerAvailable && process.platform === 'darwin') {
+			redirectUri = Config.macOSBrokerRedirectUri;
+		}
 		return await cachedPca.acquireTokenInteractive({
 			openBrowser: async (url: string) => { await env.openExternal(Uri.parse(url)); },
 			scopes,
@@ -58,7 +63,8 @@ class DefaultLoopbackFlow implements IMsalFlow {
 			loginHint,
 			prompt: loginHint ? undefined : 'select_account',
 			windowHandle,
-			claims
+			claims,
+			redirectUri
 		});
 	}
 }
@@ -72,7 +78,11 @@ class UrlHandlerFlow implements IMsalFlow {
 
 	async trigger({ cachedPca, authority, scopes, claims, loginHint, windowHandle, logger, uriHandler }: IMsalFlowTriggerOptions): Promise<AuthenticationResult> {
 		logger.info('Trying protocol handler flow...');
-		const loopbackClient = new UriHandlerLoopbackClient(uriHandler, redirectUri, logger);
+		const loopbackClient = new UriHandlerLoopbackClient(uriHandler, DEFAULT_REDIRECT_URI, logger);
+		let redirectUri: string | undefined;
+		if (cachedPca.isBrokerAvailable && process.platform === 'darwin') {
+			redirectUri = Config.macOSBrokerRedirectUri;
+		}
 		return await cachedPca.acquireTokenInteractive({
 			openBrowser: (url: string) => loopbackClient.openBrowser(url),
 			scopes,
@@ -81,7 +91,8 @@ class UrlHandlerFlow implements IMsalFlow {
 			loginHint,
 			prompt: loginHint ? undefined : 'select_account',
 			windowHandle,
-			claims
+			claims,
+			redirectUri
 		});
 	}
 }
@@ -93,10 +104,12 @@ const allFlows: IMsalFlow[] = [
 
 export interface IMsalFlowQuery {
 	extensionHost: ExtensionHost;
+	isBrokerSupported: boolean;
 }
 
 export function getMsalFlows(query: IMsalFlowQuery): IMsalFlow[] {
-	return allFlows.filter(flow => {
+	const flows = [];
+	for (const flow of allFlows) {
 		let useFlow: boolean = true;
 		switch (query.extensionHost) {
 			case ExtensionHost.Remote:
@@ -106,6 +119,13 @@ export function getMsalFlows(query: IMsalFlowQuery): IMsalFlow[] {
 				useFlow &&= flow.options.supportsWebWorkerExtensionHost;
 				break;
 		}
-		return useFlow;
-	});
+		if (useFlow) {
+			flows.push(flow);
+			if (query.isBrokerSupported) {
+				// If broker is supported, only use the first valid flow
+				return flows;
+			}
+		}
+	}
+	return flows;
 }