Handle errors fetching subscriptions from subset of tenants (#16765)

extensions/azurecore/src/azureResource/commands.ts

@@ -5,14 +5,12 @@
 
 import * as azdata from 'azdata';
 import * as vscode from 'vscode';
-import { TokenCredentials } from '@azure/ms-rest-js';
 import * as nls from 'vscode-nls';
 const localize = nls.loadMessageBundle();
 
 import { AppContext } from '../appContext';
 import { azureResource } from 'azureResource';
 import { TreeNode } from './treeNode';
-import { AzureResourceCredentialError } from './errors';
 import { AzureResourceTreeProvider } from './tree/treeProvider';
 import { AzureResourceAccountTreeNode } from './tree/accountTreeNode';
 import { IAzureResourceSubscriptionService, IAzureResourceSubscriptionFilterService, IAzureTerminalService } from '../azureResource/interfaces';
@@ -20,6 +18,7 @@ import { AzureResourceServiceNames } from './constants';
 import { AzureAccount, Tenant } from 'azurecore';
 import { FlatAccountTreeNode } from './tree/flatAccountTreeNode';
 import { ConnectionDialogTreeProvider } from './tree/connectionDialogTreeProvider';
+import { AzureResourceErrorMessageUtil } from './utils';
 
 export function registerAzureResourceCommands(appContext: AppContext, azureViewTree: AzureResourceTreeProvider, connectionDialogTree: ConnectionDialogTreeProvider): void {
 	const trees = [azureViewTree, connectionDialogTree];
@@ -109,21 +108,14 @@ export function registerAzureResourceCommands(appContext: AppContext, azureViewT
 		const subscriptionService = appContext.getService<IAzureResourceSubscriptionService>(AzureResourceServiceNames.subscriptionService);
 		const subscriptionFilterService = appContext.getService<IAzureResourceSubscriptionFilterService>(AzureResourceServiceNames.subscriptionFilterService);
 
-		const subscriptions = [];
+		let subscriptions: azureResource.AzureResourceSubscription[] = [];
 		if (subscriptions.length === 0) {
 			try {
-
+				subscriptions = await subscriptionService.getAllSubscriptions(account);
-				for (const tenant of account.properties.tenants) {
-					const response = await azdata.accounts.getAccountSecurityToken(account, tenant.id, azdata.AzureResource.ResourceManagement);
-
-					const token = response.token;
-					const tokenType = response.tokenType;
-
-					subscriptions.push(...await subscriptionService.getSubscriptions(account, new TokenCredentials(token, tokenType), tenant.id));
-				}
 			} catch (error) {
 				account.isStale = true;
-				throw new AzureResourceCredentialError(localize('azure.resource.selectsubscriptions.credentialError', "Failed to get credential for account {0}. Please refresh the account.", account.displayInfo.displayName), error);
+				vscode.window.showErrorMessage(AzureResourceErrorMessageUtil.getErrorMessage(error));
+				return;
 			}
 		}
 

extensions/azurecore/src/azureResource/errors.ts

@@ -3,11 +3,14 @@
  *  Licensed under the Source EULA. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-export class AzureResourceCredentialError extends Error {
+import * as nls from 'vscode-nls';
+const localize = nls.loadMessageBundle();
+
+export class AzureSubscriptionError extends Error {
 	constructor(
-		message: string,
+		accountName: string,
-		public readonly innerError: Error
+		public readonly errors: Error[]
 	) {
-		super(message);
+		super(localize('azure.subscriptionError', "Failed to get subscriptions for account {0}. Please refresh the account.", accountName));
 	}
 }

extensions/azurecore/src/azureResource/interfaces.ts

@@ -12,6 +12,7 @@ import { AzureAccount, Tenant } from 'azurecore';
 
 export interface IAzureResourceSubscriptionService {
 	getSubscriptions(account: Account, credential: msRest.ServiceClientCredentials, tenantId: string): Promise<azureResource.AzureResourceSubscription[]>;
+	getAllSubscriptions(account: Account): Promise<azureResource.AzureResourceSubscription[]>;
 }
 
 export interface IAzureResourceSubscriptionFilterService {

extensions/azurecore/src/azureResource/services/subscriptionService.ts

@@ -3,14 +3,30 @@
  *  Licensed under the Source EULA. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import { Account } from 'azdata';
+import * as azdata from 'azdata';
+import * as vscode from 'vscode';
 import { SubscriptionClient } from '@azure/arm-subscriptions';
 
 import { azureResource } from 'azureResource';
 import { IAzureResourceSubscriptionService } from '../interfaces';
+import { TokenCredentials } from '@azure/ms-rest-js';
+import { AzureSubscriptionError } from '../errors';
+import { AzureResourceErrorMessageUtil } from '../utils';
+
+import * as nls from 'vscode-nls';
+import { AzureAccount } from 'azurecore';
+const localize = nls.loadMessageBundle();
 
 export class AzureResourceSubscriptionService implements IAzureResourceSubscriptionService {
-	public async getSubscriptions(account: Account, credential: any, tenantId: string): Promise<azureResource.AzureResourceSubscription[]> {
+	/**
+	 * Gets all of the subscriptions for the specified account using the specified credential. This assumes that the credential passed is for
+	 * the specified tenant - which the subscriptions returned will be associated with.
+	 * @param account The account to get the subscriptions for
+	 * @param credential The credential to use for querying the subscriptions
+	 * @param tenantId The ID of the tenant these subscriptions are for
+	 * @returns The list of all subscriptions on this account for the specified tenant
+	 */
+	public async getSubscriptions(account: azdata.Account, credential: any, tenantId: string): Promise<azureResource.AzureResourceSubscription[]> {
 		const subscriptions: azureResource.AzureResourceSubscription[] = [];
 
 		const subClient = new SubscriptionClient(credential, { baseUri: account.properties.providerSettings.settings.armResource.endpoint });
@@ -23,4 +39,33 @@ export class AzureResourceSubscriptionService implements IAzureResourceSubscript
 
 		return subscriptions;
 	}
+
+	/**
+	 * Gets all subscriptions for all tenants of the given account. Any errors that occur while fetching the subscriptions for each tenant
+	 * will be displayed to the user, but this function will only throw an error if it's unable to fetch any subscriptions.
+	 * @param account The account to get the subscriptions for
+	 * @returns The list of all subscriptions on this account that were able to be retrieved
+	 */
+	public async getAllSubscriptions(account: AzureAccount): Promise<azureResource.AzureResourceSubscription[]> {
+		const subscriptions: azureResource.AzureResourceSubscription[] = [];
+		let gotSubscriptions = false;
+		const errors: Error[] = [];
+
+		for (const tenant of account.properties.tenants) {
+			try {
+				const token = await azdata.accounts.getAccountSecurityToken(account, tenant.id, azdata.AzureResource.ResourceManagement);
+				subscriptions.push(...(await this.getSubscriptions(account, new TokenCredentials(token.token, token.tokenType), tenant.id) || <azureResource.AzureResourceSubscription[]>[]));
+				gotSubscriptions = true;
+			} catch (error) {
+				const errorMsg = localize('azure.resource.tenantSubscriptionsError', "Failed to get subscriptions for account {0} (tenant '{1}'). {2}", account.key.accountId, tenant.id, AzureResourceErrorMessageUtil.getErrorMessage(error));
+				console.warn(errorMsg);
+				errors.push(error);
+				vscode.window.showWarningMessage(errorMsg);
+			}
+		}
+		if (!gotSubscriptions) {
+			throw new AzureSubscriptionError(account.key.accountId, errors);
+		}
+		return subscriptions;
+	}
 }

extensions/azurecore/src/azureResource/tree/accountTreeNode.ts

@@ -5,7 +5,6 @@
 
 import * as vscode from 'vscode';
 import * as azdata from 'azdata';
-import { TokenCredentials } from '@azure/ms-rest-js';
 
 import * as nls from 'vscode-nls';
 const localize = nls.loadMessageBundle();
@@ -13,7 +12,7 @@ const localize = nls.loadMessageBundle();
 import { AppContext } from '../../appContext';
 import { azureResource } from 'azureResource';
 import { TreeNode } from '../treeNode';
-import { AzureResourceCredentialError } from '../errors';
+import { AzureSubscriptionError } from '../errors';
 import { AzureResourceContainerTreeNodeBase } from './baseTreeNodes';
 import { AzureResourceItemType, AzureResourceServiceNames } from '../constants';
 import { AzureResourceSubscriptionTreeNode } from './subscriptionTreeNode';
@@ -44,17 +43,8 @@ export class AzureResourceAccountTreeNode extends AzureResourceContainerTreeNode
 			let subscriptions: azureResource.AzureResourceSubscription[] = [];
 
 			if (this._isClearingCache) {
-				try {
+				subscriptions = await this._subscriptionService.getAllSubscriptions(this.account);
-					for (const tenant of this.account.properties.tenants) {
-						const token = await azdata.accounts.getAccountSecurityToken(this.account, tenant.id, azdata.AzureResource.ResourceManagement);
-
-						subscriptions.push(...(await this._subscriptionService.getSubscriptions(this.account, new TokenCredentials(token.token, token.tokenType), tenant.id) || <azureResource.AzureResourceSubscription[]>[]));
-					}
-				} catch (error) {
-					throw new AzureResourceCredentialError(localize('azure.resource.tree.accountTreeNode.credentialError', "Failed to get credential for account {0}. Please refresh the account.", this.account.key.accountId), error);
-				}
 				this.updateCache<azureResource.AzureResourceSubscription[]>(subscriptions);
-
 				this._isClearingCache = false;
 			} else {
 				subscriptions = await this.getCachedSubscriptions();
@@ -93,7 +83,7 @@ export class AzureResourceAccountTreeNode extends AzureResourceContainerTreeNode
 				return subTreeNodes.sort((a, b) => a.subscription.name.localeCompare(b.subscription.name));
 			}
 		} catch (error) {
-			if (error instanceof AzureResourceCredentialError) {
+			if (error instanceof AzureSubscriptionError) {
 				vscode.commands.executeCommand('azure.resource.signin');
 			}
 			return [AzureResourceMessageTreeNode.create(AzureResourceErrorMessageUtil.getErrorMessage(error), this)];

extensions/azurecore/src/azureResource/tree/flatAccountTreeNode.ts

@@ -5,7 +5,6 @@
 
 import * as vscode from 'vscode';
 import * as azdata from 'azdata';
-import { TokenCredentials } from '@azure/ms-rest-js';
 
 import * as nls from 'vscode-nls';
 const localize = nls.loadMessageBundle();
@@ -13,7 +12,7 @@ const localize = nls.loadMessageBundle();
 import { AppContext } from '../../appContext';
 import { azureResource } from 'azureResource';
 import { TreeNode } from '../treeNode';
-import { AzureResourceCredentialError } from '../errors';
+import { AzureSubscriptionError } from '../errors';
 import { AzureResourceContainerTreeNodeBase } from './baseTreeNodes';
 import { AzureResourceItemType, AzureResourceServiceNames } from '../constants';
 import { IAzureResourceTreeChangeHandler } from './treeChangeHandler';
@@ -119,15 +118,7 @@ async function getSubscriptionInfo(account: AzureAccount, subscriptionService: I
 	total: number,
 	selected: number
 }> {
-	let subscriptions: azureResource.AzureResourceSubscription[] = [];
+	let subscriptions = await subscriptionService.getAllSubscriptions(account);
-	try {
-		for (const tenant of account.properties.tenants) {
-			const token = await azdata.accounts.getAccountSecurityToken(account, tenant.id, azdata.AzureResource.ResourceManagement);
-			subscriptions.push(...(await subscriptionService.getSubscriptions(account, new TokenCredentials(token.token, token.tokenType), tenant.id) || <azureResource.AzureResourceSubscription[]>[]));
-		}
-	} catch (error) {
-		throw new AzureResourceCredentialError(localize('azure.resource.tree.accountTreeNode.credentialError', "Failed to get credential for account {0}. Please go to the accounts dialog and refresh the account.", account.key.accountId), error);
-	}
 	const total = subscriptions.length;
 	let selected = total;
 
@@ -219,13 +210,13 @@ class FlatAccountTreeNodeLoader {
 				}
 			}
 		} catch (error) {
-			if (error instanceof AzureResourceCredentialError) {
+			if (error instanceof AzureSubscriptionError) {
 				vscode.commands.executeCommand('azure.resource.signin');
 			}
 			// http status code 429 means "too many requests"
 			// use a custom error message for azure resource graph api throttling error to make it more actionable for users.
 			const errorMessage = error?.statusCode === 429 ? localize('azure.resource.throttleerror', "Requests from this account have been throttled. To retry, please select a smaller number of subscriptions.") : AzureResourceErrorMessageUtil.getErrorMessage(error);
-			vscode.window.showErrorMessage(localize('azure.resource.tree.loadresourceerror', "An error occured while loading Azure resources: {0}", errorMessage));
+			vscode.window.showErrorMessage(localize('azure.resource.tree.loadresourceerror', "An error occurred while loading Azure resources: {0}", errorMessage));
 		}
 
 		this._isLoading = false;

extensions/azurecore/src/test/azureResource/tree/accountTreeNode.test.ts

@@ -9,7 +9,6 @@ import * as azdata from 'azdata';
 import * as vscode from 'vscode';
 import * as sinon from 'sinon';
 import 'mocha';
-import { TokenCredentials } from '@azure/ms-rest-js';
 import { AppContext } from '../../../appContext';
 
 import { azureResource } from 'azureResource';
@@ -31,7 +30,6 @@ let mockCacheService: TypeMoq.IMock<IAzureResourceCacheService>;
 let mockSubscriptionService: TypeMoq.IMock<IAzureResourceSubscriptionService>;
 let mockSubscriptionFilterService: TypeMoq.IMock<IAzureResourceSubscriptionFilterService>;
 let mockAppContext: AppContext;
-let getSecurityTokenStub: sinon.SinonStub;
 let mockTreeChangeHandler: TypeMoq.IMock<IAzureResourceTreeChangeHandler>;
 
 // Mock test data
@@ -82,9 +80,6 @@ const mockToken = {
 	tokenType: 'Bearer'
 };
 
-
-const mockCredential = new TokenCredentials(mockToken.token, mockToken.tokenType);
-
 let mockSubscriptionCache: azureResource.AzureResourceSubscription[] = [];
 
 describe('AzureResourceAccountTreeNode.info', function (): void {
@@ -103,7 +98,6 @@ describe('AzureResourceAccountTreeNode.info', function (): void {
 		mockAppContext.registerService<IAzureResourceSubscriptionService>(AzureResourceServiceNames.subscriptionService, mockSubscriptionService.object);
 		mockAppContext.registerService<IAzureResourceSubscriptionFilterService>(AzureResourceServiceNames.subscriptionFilterService, mockSubscriptionFilterService.object);
 
-		getSecurityTokenStub = sinon.stub(azdata.accounts, 'getAccountSecurityToken').returns(Promise.resolve(mockToken));
 		mockCacheService.setup((o) => o.generateKey(TypeMoq.It.isAnyString())).returns(() => generateGuid());
 		mockCacheService.setup((o) => o.get(TypeMoq.It.isAnyString())).returns(() => mockSubscriptionCache);
 		mockCacheService.setup((o) => o.update(TypeMoq.It.isAnyString(), TypeMoq.It.isAny())).returns(() => mockSubscriptionCache = mockSubscriptions);
@@ -134,7 +128,7 @@ describe('AzureResourceAccountTreeNode.info', function (): void {
 	});
 
 	it('Should be correct when there are subscriptions listed.', async function (): Promise<void> {
-		mockSubscriptionService.setup((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId)).returns(() => Promise.resolve(mockSubscriptions));
+		mockSubscriptionService.setup((o) => o.getAllSubscriptions(mockAccount)).returns(() => Promise.resolve(mockSubscriptions));
 		mockSubscriptionFilterService.setup((o) => o.getSelectedSubscriptions(mockAccount)).returns(() => Promise.resolve(undefined));
 
 		const accountTreeNodeLabel = `${mockAccount.displayInfo.displayName} (${mockSubscriptions.length} / ${mockSubscriptions.length} subscriptions)`;
@@ -154,7 +148,7 @@ describe('AzureResourceAccountTreeNode.info', function (): void {
 	});
 
 	it('Should be correct when there are subscriptions filtered.', async function (): Promise<void> {
-		mockSubscriptionService.setup((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId)).returns(() => Promise.resolve(mockSubscriptions));
+		mockSubscriptionService.setup((o) => o.getAllSubscriptions(mockAccount)).returns(() => Promise.resolve(mockSubscriptions));
 		mockSubscriptionFilterService.setup((o) => o.getSelectedSubscriptions(mockAccount)).returns(() => Promise.resolve(mockFilteredSubscriptions));
 
 		const accountTreeNodeLabel = `${mockAccount.displayInfo.displayName} (${mockFilteredSubscriptions.length} / ${mockSubscriptions.length} subscriptions)`;
@@ -201,14 +195,14 @@ describe('AzureResourceAccountTreeNode.getChildren', function (): void {
 	});
 
 	it('Should load subscriptions from scratch and update cache when it is clearing cache.', async function (): Promise<void> {
-		mockSubscriptionService.setup((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId)).returns(() => Promise.resolve(mockSubscriptions));
+		mockSubscriptionService.setup((o) => o.getAllSubscriptions(mockAccount)).returns(() => Promise.resolve(mockSubscriptions));
 		mockSubscriptionFilterService.setup((o) => o.getSelectedSubscriptions(mockAccount)).returns(() => Promise.resolve([]));
 
 		const accountTreeNode = new AzureResourceAccountTreeNode(mockAccount, mockAppContext, mockTreeChangeHandler.object);
 
 		const children = await accountTreeNode.getChildren();
 
-		mockSubscriptionService.verify((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId), TypeMoq.Times.once());
+		mockSubscriptionService.verify((o) => o.getAllSubscriptions(mockAccount), TypeMoq.Times.once());
 		mockCacheService.verify((o) => o.get(TypeMoq.It.isAnyString()), TypeMoq.Times.exactly(0));
 		mockCacheService.verify((o) => o.update(TypeMoq.It.isAnyString(), TypeMoq.It.isAny()), TypeMoq.Times.once());
 		mockSubscriptionFilterService.verify((o) => o.getSelectedSubscriptions(mockAccount), TypeMoq.Times.once());
@@ -234,7 +228,7 @@ describe('AzureResourceAccountTreeNode.getChildren', function (): void {
 	});
 
 	it('Should load subscriptions from cache when it is not clearing cache.', async function (): Promise<void> {
-		mockSubscriptionService.setup((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId)).returns(() => Promise.resolve(mockSubscriptions));
+		mockSubscriptionService.setup((o) => o.getAllSubscriptions(mockAccount)).returns(() => Promise.resolve(mockSubscriptions));
 		mockSubscriptionFilterService.setup((o) => o.getSelectedSubscriptions(mockAccount)).returns(() => Promise.resolve(undefined));
 
 		const accountTreeNode = new AzureResourceAccountTreeNode(mockAccount, mockAppContext, mockTreeChangeHandler.object);
@@ -243,7 +237,7 @@ describe('AzureResourceAccountTreeNode.getChildren', function (): void {
 		const children = await accountTreeNode.getChildren();
 
 
-		mockSubscriptionService.verify((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId), TypeMoq.Times.once());
+		mockSubscriptionService.verify((o) => o.getAllSubscriptions(mockAccount), TypeMoq.Times.once());
 		mockCacheService.verify((o) => o.get(TypeMoq.It.isAnyString()), TypeMoq.Times.once());
 		mockCacheService.verify((o) => o.update(TypeMoq.It.isAnyString(), TypeMoq.It.isAny()), TypeMoq.Times.once());
 
@@ -255,7 +249,7 @@ describe('AzureResourceAccountTreeNode.getChildren', function (): void {
 	});
 
 	it('Should handle when there is no subscriptions.', async function (): Promise<void> {
-		mockSubscriptionService.setup((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId)).returns(() => Promise.resolve(undefined));
+		mockSubscriptionService.setup((o) => o.getAllSubscriptions(mockAccount)).returns(() => Promise.resolve([]));
 
 		const accountTreeNode = new AzureResourceAccountTreeNode(mockAccount, mockAppContext, mockTreeChangeHandler.object);
 
@@ -271,7 +265,7 @@ describe('AzureResourceAccountTreeNode.getChildren', function (): void {
 	});
 
 	it('Should honor subscription filtering.', async function (): Promise<void> {
-		mockSubscriptionService.setup((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId)).returns(() => Promise.resolve(mockSubscriptions));
+		mockSubscriptionService.setup((o) => o.getAllSubscriptions(mockAccount)).returns(() => Promise.resolve(mockSubscriptions));
 		mockSubscriptionFilterService.setup((o) => o.getSelectedSubscriptions(mockAccount)).returns(() => Promise.resolve(mockFilteredSubscriptions));
 
 		const accountTreeNode = new AzureResourceAccountTreeNode(mockAccount, mockAppContext, mockTreeChangeHandler.object);
@@ -289,7 +283,7 @@ describe('AzureResourceAccountTreeNode.getChildren', function (): void {
 	});
 
 	it('Should handle errors.', async function (): Promise<void> {
-		mockSubscriptionService.setup((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId)).returns(() => Promise.resolve(mockSubscriptions));
+		mockSubscriptionService.setup((o) => o.getAllSubscriptions(mockAccount)).returns(() => Promise.resolve(mockSubscriptions));
 
 		const mockError = 'Test error';
 		mockSubscriptionFilterService.setup((o) => o.getSelectedSubscriptions(mockAccount)).returns(() => { throw new Error(mockError); });
@@ -298,8 +292,7 @@ describe('AzureResourceAccountTreeNode.getChildren', function (): void {
 
 		const children = await accountTreeNode.getChildren();
 
-		should(getSecurityTokenStub.calledTwice).be.true('getSecurityToken should have been called exactly twice - once per subscription');
+		mockSubscriptionService.verify((o) => o.getAllSubscriptions(mockAccount), TypeMoq.Times.once());
-		mockSubscriptionService.verify((o) => o.getSubscriptions(mockAccount, mockCredential, mockTenantId), TypeMoq.Times.once());
 		mockSubscriptionFilterService.verify((o) => o.getSelectedSubscriptions(mockAccount), TypeMoq.Times.once());
 		mockCacheService.verify((o) => o.get(TypeMoq.It.isAnyString()), TypeMoq.Times.never());
 		mockCacheService.verify((o) => o.update(TypeMoq.It.isAnyString(), TypeMoq.It.isAny()), TypeMoq.Times.once());