diff --git a/extensions/azurecore/src/azureResource/commands.ts b/extensions/azurecore/src/azureResource/commands.ts index 54a7f1588f..622773ab48 100644 --- a/extensions/azurecore/src/azureResource/commands.ts +++ b/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(AzureResourceServiceNames.subscriptionService); const subscriptionFilterService = appContext.getService(AzureResourceServiceNames.subscriptionFilterService); - const subscriptions = []; + let subscriptions: azureResource.AzureResourceSubscription[] = []; if (subscriptions.length === 0) { try { - - 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)); - } + subscriptions = await subscriptionService.getAllSubscriptions(account); } 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; } } diff --git a/extensions/azurecore/src/azureResource/errors.ts b/extensions/azurecore/src/azureResource/errors.ts index 7400d4c3d5..d2674e2763 100644 --- a/extensions/azurecore/src/azureResource/errors.ts +++ b/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, - public readonly innerError: Error + accountName: string, + public readonly errors: Error[] ) { - super(message); + super(localize('azure.subscriptionError', "Failed to get subscriptions for account {0}. Please refresh the account.", accountName)); } } diff --git a/extensions/azurecore/src/azureResource/interfaces.ts b/extensions/azurecore/src/azureResource/interfaces.ts index 387ed7b93a..4c58026dce 100644 --- a/extensions/azurecore/src/azureResource/interfaces.ts +++ b/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; + getAllSubscriptions(account: Account): Promise; } export interface IAzureResourceSubscriptionFilterService { diff --git a/extensions/azurecore/src/azureResource/services/subscriptionService.ts b/extensions/azurecore/src/azureResource/services/subscriptionService.ts index ce77a4f959..060e09b6d2 100644 --- a/extensions/azurecore/src/azureResource/services/subscriptionService.ts +++ b/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 { + /** + * 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 { 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 { + 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) || [])); + 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; + } } diff --git a/extensions/azurecore/src/azureResource/tree/accountTreeNode.ts b/extensions/azurecore/src/azureResource/tree/accountTreeNode.ts index cf73c021c0..7484d6a6df 100644 --- a/extensions/azurecore/src/azureResource/tree/accountTreeNode.ts +++ b/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 { - 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) || [])); - } - } 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); - } + subscriptions = await this._subscriptionService.getAllSubscriptions(this.account); this.updateCache(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)]; diff --git a/extensions/azurecore/src/azureResource/tree/flatAccountTreeNode.ts b/extensions/azurecore/src/azureResource/tree/flatAccountTreeNode.ts index a146028ade..7e86e8475e 100644 --- a/extensions/azurecore/src/azureResource/tree/flatAccountTreeNode.ts +++ b/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[] = []; - 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) || [])); - } - } 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); - } + let subscriptions = await subscriptionService.getAllSubscriptions(account); 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; diff --git a/extensions/azurecore/src/test/azureResource/tree/accountTreeNode.test.ts b/extensions/azurecore/src/test/azureResource/tree/accountTreeNode.test.ts index 7b13b11ad1..d5d0e8e88e 100644 --- a/extensions/azurecore/src/test/azureResource/tree/accountTreeNode.test.ts +++ b/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; let mockSubscriptionService: TypeMoq.IMock; let mockSubscriptionFilterService: TypeMoq.IMock; let mockAppContext: AppContext; -let getSecurityTokenStub: sinon.SinonStub; let mockTreeChangeHandler: TypeMoq.IMock; // 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(AzureResourceServiceNames.subscriptionService, mockSubscriptionService.object); mockAppContext.registerService(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 { - 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 { - 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 { - 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 { - 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 { - 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 { - 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 { - 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.getSubscriptions(mockAccount, mockCredential, mockTenantId), TypeMoq.Times.once()); + mockSubscriptionService.verify((o) => o.getAllSubscriptions(mockAccount), 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());