From 20c290aa912724223d4e013f2a1064a3588be158 Mon Sep 17 00:00:00 2001 From: Christopher Suh Date: Thu, 2 Mar 2023 15:38:12 -0800 Subject: [PATCH] Better error messages from connection failures in MSAL (#22065) * better error messages from connection failures in MSAL\, fix typo * cleanup * pr comments * rename error interface * address pr comments * update api * fix typings * change one more typing * fix build * fix tests --- .../src/account-provider/auths/azureAuth.ts | 16 +++++++--- .../account-provider/azureAccountProvider.ts | 32 ++++++++++++++++--- src/sql/azdata.proposed.d.ts | 14 ++++++-- .../browser/accountManagementService.ts | 12 +++++-- .../connection/browser/connectionWidget.ts | 2 +- 5 files changed, 59 insertions(+), 17 deletions(-) diff --git a/extensions/azurecore/src/account-provider/auths/azureAuth.ts b/extensions/azurecore/src/account-provider/auths/azureAuth.ts index eab7ec53f7..25041a2ff7 100644 --- a/extensions/azurecore/src/account-provider/auths/azureAuth.ts +++ b/extensions/azurecore/src/account-provider/auths/azureAuth.ts @@ -322,7 +322,7 @@ export abstract class AzureAuth implements vscode.Disposable { * @param azureResource * @returns The authentication result, including the access token */ - public async getTokenMsal(accountId: string, azureResource: azdata.AzureResource, tenantId: string): Promise { + public async getTokenMsal(accountId: string, azureResource: azdata.AzureResource, tenantId: string): Promise { const resource = this.resources.find(s => s.azureResourceId === azureResource); if (!resource) { Logger.error(`Error: Could not fetch the azure resource ${azureResource} `); @@ -363,11 +363,17 @@ export abstract class AzureAuth implements vscode.Disposable { displayName: '' }; return this.handleInteractionRequiredMsal(tenant, resource); - } else if (e.name === 'ClientAuthError') { - Logger.error(e.message); + } else { + if (e.name === 'ClientAuthError') { + Logger.verbose('[ClientAuthError] Failed to silently acquire token'); + } + return { + canceled: false, + name: e.name, + errorCode: e.errorCode, + errorMessage: e.errorMessage || e.message + } } - Logger.error('Failed to silently acquire token, not InteractionRequiredAuthError'); - return null; } } diff --git a/extensions/azurecore/src/account-provider/azureAccountProvider.ts b/extensions/azurecore/src/account-provider/azureAccountProvider.ts index 9079806bb5..d6b04fbc1a 100644 --- a/extensions/azurecore/src/account-provider/azureAccountProvider.ts +++ b/extensions/azurecore/src/account-provider/azureAccountProvider.ts @@ -13,7 +13,7 @@ import { AzureAccount } from 'azurecore'; import { Deferred } from './interfaces'; -import { PublicClientApplication } from '@azure/msal-node'; +import { AuthenticationResult, PublicClientApplication } from '@azure/msal-node'; import { SimpleTokenCache } from './simpleTokenCache'; import { Logger } from '../utils/Logger'; import { MultiTenantTokenResponse, Token, AzureAuth } from './auths/azureAuth'; @@ -148,10 +148,7 @@ export class AzureAccountProvider implements azdata.AccountProvider, vscode.Disp if (this.authLibrary === Constants.AuthLibrary.MSAL) { tenantId = tenantId || account.properties.owningTenant.id; let authResult = await azureAuth.getTokenMsal(account.key.accountId, resource, tenantId); - if (!authResult || !authResult.account || !authResult.account.idTokenClaims) { - Logger.error(`MSAL: getToken call failed`); - throw Error('Failed to get token'); - } else { + if (this.isAuthenticationResult(authResult) && authResult.account && authResult.account.idTokenClaims) { const token: Token = { key: authResult.account.homeAccountId, token: authResult.accessToken, @@ -159,6 +156,14 @@ export class AzureAccountProvider implements azdata.AccountProvider, vscode.Disp expiresOn: authResult.account.idTokenClaims.exp }; return token; + } else { + Logger.error(`MSAL: getToken call failed`); + // Throw error with MSAL-specific code/message, else throw generic error message + if (this.isProviderError(authResult)) { + throw new Error(localize('msalTokenError', `{0} occurred when acquiring token. \n{1}`, authResult.errorCode, authResult.errorMessage)); + } else { + throw new Error(localize('genericTokenError', 'Failed to get token')); + } } } else { // fallback to ADAL as default return azureAuth.getAccountSecurityTokenAdal(account, tenantId, resource); @@ -171,6 +176,23 @@ export class AzureAccountProvider implements azdata.AccountProvider, vscode.Disp } } + private isAuthenticationResult(result: AuthenticationResult | azdata.ProviderError | null): result is AuthenticationResult { + if (result) { + return typeof (result).accessToken === 'string'; + } else { + return false; + } + } + + private isProviderError(result: AuthenticationResult | azdata.ProviderError | null): result is azdata.ProviderError { + if (result) { + return typeof (result).errorMessage === 'string'; + } else { + return false; + } + } + + private async _getSecurityToken(account: AzureAccount, resource: azdata.AzureResource): Promise { void vscode.window.showInformationMessage(localize('azure.deprecatedGetSecurityToken', "A call was made to azdata.accounts.getSecurityToken, this method is deprecated and will be removed in future releases. Please use getAccountSecurityToken instead.")); const azureAccount = account as AzureAccount; diff --git a/src/sql/azdata.proposed.d.ts b/src/sql/azdata.proposed.d.ts index c8413066c5..9d0456360f 100644 --- a/src/sql/azdata.proposed.d.ts +++ b/src/sql/azdata.proposed.d.ts @@ -432,18 +432,26 @@ declare module 'azdata' { azurePortalEndpoint?: string; } - export interface PromptFailedResult { + export interface PromptFailedResult extends ProviderError { } + + export interface ProviderError { /** - * Error code used for non-user cancelled sign in errors + * Error name + */ + name?: string; + + /** + * Error code */ errorCode?: string; /** - * Error message used for non-user cancelled sign in errors + * Error message */ errorMessage?: string; } + export namespace diagnostics { /** * Represents a diagnostics provider of accounts. diff --git a/src/sql/workbench/services/accountManagement/browser/accountManagementService.ts b/src/sql/workbench/services/accountManagement/browser/accountManagementService.ts index ce9a024352..a495756ae0 100644 --- a/src/sql/workbench/services/accountManagement/browser/accountManagementService.ts +++ b/src/sql/workbench/services/accountManagement/browser/accountManagementService.ts @@ -128,7 +128,7 @@ export class AccountManagementService implements IAccountManagementService { */ public addAccount(providerId: string): Promise { const closeAction: Action = new Action('closeAddingAccount', localize('accountManagementService.close', "Close"), undefined, true); - + const genericAccountErrorMessage = localize('addAccountFailedGenericMessage', 'Adding account failed, check Azure Accounts log for more info.') const loginNotification: INotification = { severity: Severity.Info, message: localize('loggingIn', "Adding account..."), @@ -148,13 +148,19 @@ export class AccountManagementService implements IAccountManagementService { if (accountResult.canceled === true) { return; } else { - throw new Error(localize('addAccountFailedMessage', `${0} \nError Message: ${1}`, accountResult.errorCode, accountResult.errorMessage)); + if (accountResult.errorCode && accountResult.errorMessage) { + throw new Error(localize('addAccountFailedCodeMessage', `{0} \nError Message: {1}`, accountResult.errorCode, accountResult.errorMessage)); + } else if (accountResult.errorMessage) { + throw new Error(localize('addAccountFailedMessage', `{0}`, accountResult.errorMessage)); + } else { + throw new Error(genericAccountErrorMessage); + } } } let result = await this._accountStore.addOrUpdate(accountResult); if (!result) { this._logService.error('adding account failed'); - throw new Error(localize('addAccountFailedGeneric', 'Adding account failed, check Azure Accounts log for more info.')); + throw new Error(genericAccountErrorMessage); } if (result.accountAdded) { // Add the account to the list diff --git a/src/sql/workbench/services/connection/browser/connectionWidget.ts b/src/sql/workbench/services/connection/browser/connectionWidget.ts index eb5dcc48a7..240232eaa6 100644 --- a/src/sql/workbench/services/connection/browser/connectionWidget.ts +++ b/src/sql/workbench/services/connection/browser/connectionWidget.ts @@ -561,7 +561,7 @@ export class ConnectionWidget extends lifecycle.Disposable { if (this._azureAccountDropdown) { this._register(styler.attachSelectBoxStyler(this._azureAccountDropdown, this._themeService)); this._register(this._azureAccountDropdown.onDidSelect(() => { - this.onAzureAccountSelected().catch(err => this._logService.error(`Unexpeted error handling Azure Account dropdown click : ${err}`)); + this.onAzureAccountSelected().catch(err => this._logService.error(`Unexpected error handling Azure Account dropdown click : ${err}`)); })); }