From 6bd8e90fb726babbf212ab52bf3109cd4a731508 Mon Sep 17 00:00:00 2001 From: Tyler James Leonhardt Date: Fri, 13 Sep 2024 12:57:12 -0700 Subject: [PATCH] Misc fixes for Sovereign Clouds (#228591) * Misc fixes for Sovereign Clouds * For now, use the URL handler since the main flow doesn't work right now because the localhost redirect url needs to be in those environments * Includes the name of the cloud in the PCAs so that we have separation between the auth providers * extra logging for the URL Handler * fix tests --- .../src/common/loopbackClientAndOpener.ts | 9 +- .../test/loopbackClientAndOpener.test.ts | 4 +- .../src/node/authProvider.ts | 84 ++++++++++++------- .../src/node/publicClientCache.ts | 19 +++-- 4 files changed, 70 insertions(+), 46 deletions(-) diff --git a/extensions/microsoft-authentication/src/common/loopbackClientAndOpener.ts b/extensions/microsoft-authentication/src/common/loopbackClientAndOpener.ts index 4a455ea50f7..3fbb0340037 100644 --- a/extensions/microsoft-authentication/src/common/loopbackClientAndOpener.ts +++ b/extensions/microsoft-authentication/src/common/loopbackClientAndOpener.ts @@ -5,7 +5,7 @@ import type { ILoopbackClient, ServerAuthorizationCodeResponse } from '@azure/msal-node'; import type { UriEventHandler } from '../UriEventHandler'; -import { env, Uri } from 'vscode'; +import { env, LogOutputChannel, Uri } from 'vscode'; import { toPromise } from './async'; export interface ILoopbackClientAndOpener extends ILoopbackClient { @@ -15,12 +15,13 @@ export interface ILoopbackClientAndOpener extends ILoopbackClient { export class UriHandlerLoopbackClient implements ILoopbackClientAndOpener { constructor( private readonly _uriHandler: UriEventHandler, - private readonly _redirectUri: string + private readonly _redirectUri: string, + private readonly _logger: LogOutputChannel ) { } - async listenForAuthCode(successTemplate?: string, errorTemplate?: string): Promise { - console.log(successTemplate, errorTemplate); + async listenForAuthCode(): Promise { const url = await toPromise(this._uriHandler.event); + this._logger.debug(`Received URL event. Authority: ${url.authority}`); const result = new URL(url.toString(true)); return { diff --git a/extensions/microsoft-authentication/src/common/test/loopbackClientAndOpener.test.ts b/extensions/microsoft-authentication/src/common/test/loopbackClientAndOpener.test.ts index 5c55567d1d0..69d7afaa38a 100644 --- a/extensions/microsoft-authentication/src/common/test/loopbackClientAndOpener.test.ts +++ b/extensions/microsoft-authentication/src/common/test/loopbackClientAndOpener.test.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import * as assert from 'assert'; -import { env, Uri } from 'vscode'; +import { env, Uri, window } from 'vscode'; import * as sinon from 'sinon'; import { UriHandlerLoopbackClient } from '../loopbackClientAndOpener'; import { UriEventHandler } from '../../UriEventHandler'; @@ -22,7 +22,7 @@ suite('UriHandlerLoopbackClient', () => { envStub.openExternal.resolves(true); envStub.asExternalUri.callThrough(); uriHandler = new UriEventHandler(); - client = new UriHandlerLoopbackClient(uriHandler, redirectUri); + client = new UriHandlerLoopbackClient(uriHandler, redirectUri, window.createOutputChannel('test', { log: true })); }); teardown(() => { diff --git a/extensions/microsoft-authentication/src/node/authProvider.ts b/extensions/microsoft-authentication/src/node/authProvider.ts index bbccad73a44..20c8bf305a2 100644 --- a/extensions/microsoft-authentication/src/node/authProvider.ts +++ b/extensions/microsoft-authentication/src/node/authProvider.ts @@ -50,7 +50,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); + this._publicClientManager = new CachedPublicClientApplicationManager( + context.globalState, + context.secrets, + this._logger, + this._env.name + ); const accountChangeEvent = this._eventBufferer.wrapEvent( this._publicClientManager.onDidAccountsChange, (last, newEvent) => { @@ -148,40 +153,52 @@ export class MsalAuthProvider implements AuthenticationProvider { this._logger.info('[createSession]', `[${scopeData.scopeStr}]`, 'starting'); const cachedPca = await this.getOrCreatePublicClientApplication(scopeData.clientId, scopeData.tenant); - let result: AuthenticationResult; - try { - result = await cachedPca.acquireTokenInteractive({ - openBrowser: async (url: string) => { await env.openExternal(Uri.parse(url)); }, - scopes: scopeData.scopesToSend, - // The logic for rendering one or the other of these templates is in the - // template itself, so we pass the same one for both. - successTemplate: loopbackTemplate, - errorTemplate: loopbackTemplate - }); - } catch (e) { - if (e instanceof CancellationError) { - const yes = l10n.t('Yes'); - const result = await window.showErrorMessage( - l10n.t('Having trouble logging in?'), - { - modal: true, - detail: l10n.t('Would you like to try a different way to sign in to your Microsoft account? ({0})', 'protocol handler') - }, - yes - ); - if (!result) { + let result: AuthenticationResult | undefined; + + // Currently, the http://localhost redirect URI is only in the AzureCloud environment... even though I did make the change in the SovereignCloud environments... + // TODO: Remove this check when the change is in all environments. + let useLoopBack = this._env !== Environment.AzureCloud && scopeData.clientId === 'aebc6443-996d-45c2-90f0-388ff96faa56'; + if (!useLoopBack) { + try { + result = await cachedPca.acquireTokenInteractive({ + openBrowser: async (url: string) => { await env.openExternal(Uri.parse(url)); }, + scopes: scopeData.scopesToSend, + // The logic for rendering one or the other of these templates is in the + // template itself, so we pass the same one for both. + successTemplate: loopbackTemplate, + errorTemplate: loopbackTemplate + }); + } catch (e) { + if (e instanceof CancellationError) { + const yes = l10n.t('Yes'); + const result = await window.showErrorMessage( + l10n.t('Having trouble logging in?'), + { + modal: true, + detail: l10n.t('Would you like to try a different way to sign in to your Microsoft account? ({0})', 'protocol handler') + }, + yes + ); + if (!result) { + this._telemetryReporter.sendLoginFailedEvent(); + throw e; + } + } + // This error comes from the backend and is likely not due to the user's machine + // failing to open a port or something local that would require us to try the + // URL handler loopback client. + if (e instanceof ServerError) { this._telemetryReporter.sendLoginFailedEvent(); throw e; } + + // The user wants to try the loopback client or we got an error likely due to spinning up the server + useLoopBack = true; } - // This error comes from the backend and is likely not due to the user's machine - // failing to open a port or something local that would require us to try the - // URL handler loopback client. - if (e instanceof ServerError) { - this._telemetryReporter.sendLoginFailedEvent(); - throw e; - } - const loopbackClient = new UriHandlerLoopbackClient(this._uriHandler, redirectUri); + } + + if (useLoopBack) { + const loopbackClient = new UriHandlerLoopbackClient(this._uriHandler, redirectUri, this._logger); try { result = await cachedPca.acquireTokenInteractive({ openBrowser: (url: string) => loopbackClient.openBrowser(url), @@ -194,6 +211,11 @@ export class MsalAuthProvider implements AuthenticationProvider { } } + if (!result) { + this._telemetryReporter.sendLoginFailedEvent(); + throw new Error('No result returned from MSAL'); + } + const session = this.sessionFromAuthenticationResult(result, scopeData.originalScopes); this._telemetryReporter.sendLoginEvent(session.scopes); this._logger.info('[createSession]', `[${scopeData.scopeStr}]`, 'returned session'); diff --git a/extensions/microsoft-authentication/src/node/publicClientCache.ts b/extensions/microsoft-authentication/src/node/publicClientCache.ts index 5d9830bb741..fc6ce38e975 100644 --- a/extensions/microsoft-authentication/src/node/publicClientCache.ts +++ b/extensions/microsoft-authentication/src/node/publicClientCache.ts @@ -27,9 +27,10 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient constructor( private readonly _globalMemento: Memento, private readonly _secretStorage: SecretStorage, - private readonly _logger: LogOutputChannel + private readonly _logger: LogOutputChannel, + cloudName: string ) { - this._pcasSecretStorage = new PublicClientApplicationsSecretStorage(_secretStorage); + this._pcasSecretStorage = new PublicClientApplicationsSecretStorage(_secretStorage, cloudName); this._disposable = Disposable.from( this._pcasSecretStorage, this._registerSecretStorageHandler(), @@ -190,18 +191,18 @@ export class CachedPublicClientApplicationManager implements ICachedPublicClient } class PublicClientApplicationsSecretStorage { - private static key = 'publicClientApplications'; - private _disposable: Disposable; private readonly _onDidChangeEmitter = new EventEmitter; readonly onDidChange: Event = this._onDidChangeEmitter.event; - constructor(private readonly _secretStorage: SecretStorage) { + private readonly _key = `publicClientApplications-${this._cloudName}`; + + constructor(private readonly _secretStorage: SecretStorage, private readonly _cloudName: string) { this._disposable = Disposable.from( this._onDidChangeEmitter, this._secretStorage.onDidChange(e => { - if (e.key === PublicClientApplicationsSecretStorage.key) { + if (e.key === this._key) { this._onDidChangeEmitter.fire(); } }) @@ -209,7 +210,7 @@ class PublicClientApplicationsSecretStorage { } async get(): Promise { - const value = await this._secretStorage.get(PublicClientApplicationsSecretStorage.key); + const value = await this._secretStorage.get(this._key); if (!value) { return undefined; } @@ -217,11 +218,11 @@ class PublicClientApplicationsSecretStorage { } store(value: string[]): Thenable { - return this._secretStorage.store(PublicClientApplicationsSecretStorage.key, JSON.stringify(value)); + return this._secretStorage.store(this._key, JSON.stringify(value)); } delete(): Thenable { - return this._secretStorage.delete(PublicClientApplicationsSecretStorage.key); + return this._secretStorage.delete(this._key); } dispose() {