From 85fad2360cff94d45287606b5449ca885503946d Mon Sep 17 00:00:00 2001 From: Cheena Malhotra <13396919+cheenamalhotra@users.noreply.github.com> Date: Thu, 3 Nov 2022 11:04:37 -0700 Subject: [PATCH] Fix tenant Id design for Azure accounts (#21086) --- .../src/account-provider/auths/azureAuth.ts | 25 +++++++++++------ extensions/azurecore/src/azurecore.d.ts | 14 +++++++--- .../account-provider/auths/azureAuth.test.ts | 1 + .../database/databaseTreeDataProvider.test.ts | 4 +++ .../databaseServerTreeDataProvider.test.ts | 4 +++ .../azureResource/resourceService.test.ts | 28 +++++++++++-------- .../azureResource/resourceTreeNode.test.ts | 4 +++ .../tree/accountTreeNode.test.ts | 14 +++++----- extensions/azurecore/tsconfig.json | 1 + 9 files changed, 64 insertions(+), 31 deletions(-) diff --git a/extensions/azurecore/src/account-provider/auths/azureAuth.ts b/extensions/azurecore/src/account-provider/auths/azureAuth.ts index c22ed412bc..28ea046a85 100644 --- a/extensions/azurecore/src/account-provider/auths/azureAuth.ts +++ b/extensions/azurecore/src/account-provider/auths/azureAuth.ts @@ -133,12 +133,6 @@ export abstract class AzureAuth implements vscode.Disposable { } } - private getHomeTenant(account: AzureAccount): Tenant { - // Home is defined by the API - // Lets pick the home tenant - and fall back to commonTenant if they don't exist - return account.properties.tenants.find(t => t.tenantCategory === 'Home') ?? account.properties.tenants[0] ?? this.commonTenant; - } - public async refreshAccess(account: AzureAccount): Promise { // Deprecated account - delete it. if (account.key.accountVersion !== AzureAuth.ACCOUNT_VERSION) { @@ -146,7 +140,10 @@ export abstract class AzureAuth implements vscode.Disposable { return account; } try { - const tenant = this.getHomeTenant(account); + // There can be multiple home tenants + // We want to return the one that owns the Azure account. + // Not doing so can result in token being issued for the wrong tenant + const tenant = account.properties.owningTenant; const tokenResult = await this.getAccountSecurityToken(account, tenant.id, azdata.AzureResource.MicrosoftResourceManagement); if (!tokenResult) { account.isStale = true; @@ -186,7 +183,14 @@ export abstract class AzureAuth implements vscode.Disposable { return undefined; } - const tenant = account.properties.tenants.find(t => t.id === tenantId); + if (!account.properties.owningTenant) { + // Should never happen + throw new AzureAuthError(localize('azure.owningTenantNotFound', "Owning Tenant information not found for account."), 'Owning tenant not found.', undefined); + } + + const tenant = account.properties.owningTenant?.id === tenantId + ? account.properties.owningTenant + : account.properties.tenants.find(t => t.id === tenantId); if (!tenant) { throw new AzureAuthError(localize('azure.tenantNotFound', "Specified tenant with ID '{0}' not found.", tenantId), `Tenant ${tenantId} not found.`, undefined); @@ -557,6 +561,10 @@ export abstract class AzureAuth implements vscode.Disposable { const name = tokenClaims.name ?? tokenClaims.email ?? tokenClaims.unique_name; const email = tokenClaims.email ?? tokenClaims.unique_name; + // Read more about tid > https://learn.microsoft.com/azure/active-directory/develop/id-tokens + const owningTenant = tenants.find(t => t.id === tokenClaims.tid) + ?? { 'id': tokenClaims.tid, 'displayName': 'Microsoft Account' }; + let displayName = name; if (email) { displayName = `${displayName} - ${email}`; @@ -596,6 +604,7 @@ export abstract class AzureAuth implements vscode.Disposable { properties: { providerSettings: this.metadata, isMsAccount: accountIssuer === 'msft', + owningTenant: owningTenant, tenants, azureAuthType: this.authType }, diff --git a/extensions/azurecore/src/azurecore.d.ts b/extensions/azurecore/src/azurecore.d.ts index 5c43cf92b4..56965dc916 100644 --- a/extensions/azurecore/src/azurecore.d.ts +++ b/extensions/azurecore/src/azurecore.d.ts @@ -38,11 +38,17 @@ declare module 'azurecore' { azureAuthType?: AzureAuthType providerSettings: AzureAccountProviderMetadata; + /** * Whether or not the account is a Microsoft account */ isMsAccount: boolean; + /** + * Represents the tenant that the user would be signing in to. For work and school accounts, the GUID is the immutable tenant ID of the organization that the user is signing in to. + * For sign-ins to the personal Microsoft account tenant (services like Xbox, Teams for Life, or Outlook), the value is 9188040d-6c67-4c5b-b112-36a304b66dad. + */ + owningTenant: Tenant; /** * A list of tenants (aka directories) that the account belongs to */ @@ -356,10 +362,10 @@ declare module 'azurecore' { } export interface IAzureResourceTreeDataProvider { - /** - * Gets the root tree item nodes for this provider - these will be used as - * direct children of the Account node in the Azure tree view. - */ + /** + * Gets the root tree item nodes for this provider - these will be used as + * direct children of the Account node in the Azure tree view. + */ getRootChildren(): Promise; /** * Gets the children for a given {@link IAzureResourceNode} diff --git a/extensions/azurecore/src/test/account-provider/auths/azureAuth.test.ts b/extensions/azurecore/src/test/account-provider/auths/azureAuth.test.ts index 68ae3027a4..b8ead5cae0 100644 --- a/extensions/azurecore/src/test/account-provider/auths/azureAuth.test.ts +++ b/extensions/azurecore/src/test/account-provider/auths/azureAuth.test.ts @@ -53,6 +53,7 @@ describe('Azure Authentication', function () { mockAccount = { isStale: false, properties: { + owningTenant: mockTenant, tenants: [mockTenant] } } as AzureAccount; diff --git a/extensions/azurecore/src/test/azureResource/providers/database/databaseTreeDataProvider.test.ts b/extensions/azurecore/src/test/azureResource/providers/database/databaseTreeDataProvider.test.ts index eb246e5a2a..28cb0ffb9b 100644 --- a/extensions/azurecore/src/test/azureResource/providers/database/databaseTreeDataProvider.test.ts +++ b/extensions/azurecore/src/test/azureResource/providers/database/databaseTreeDataProvider.test.ts @@ -35,6 +35,10 @@ const mockAccount: AzureAccount = { properties: { providerSettings: settings[0].metadata, isMsAccount: true, + owningTenant: { + id: 'tenantId', + displayName: 'tenantDisplayName', + }, tenants: [] }, isStale: false diff --git a/extensions/azurecore/src/test/azureResource/providers/databaseServer/databaseServerTreeDataProvider.test.ts b/extensions/azurecore/src/test/azureResource/providers/databaseServer/databaseServerTreeDataProvider.test.ts index f287b532e5..ce812eb1c7 100644 --- a/extensions/azurecore/src/test/azureResource/providers/databaseServer/databaseServerTreeDataProvider.test.ts +++ b/extensions/azurecore/src/test/azureResource/providers/databaseServer/databaseServerTreeDataProvider.test.ts @@ -35,6 +35,10 @@ const mockAccount: AzureAccount = { properties: { providerSettings: settings[0].metadata, isMsAccount: true, + owningTenant: { + id: 'tenantId', + displayName: 'tenantDisplayName', + }, tenants: [] }, isStale: false diff --git a/extensions/azurecore/src/test/azureResource/resourceService.test.ts b/extensions/azurecore/src/test/azureResource/resourceService.test.ts index c7567f256d..4e1be378a5 100644 --- a/extensions/azurecore/src/test/azureResource/resourceService.test.ts +++ b/extensions/azurecore/src/test/azureResource/resourceService.test.ts @@ -28,13 +28,17 @@ const mockAccount: AzureAccount = { properties: { providerSettings: settings[0].metadata, isMsAccount: true, + owningTenant: { + id: 'tenantId', + displayName: 'tenantDisplayName', + }, tenants: [] }, isStale: false }; const mockTenantId: string = 'mock_tenant'; -const mockSubscriptionId ='mock_subscription'; +const mockSubscriptionId = 'mock_subscription'; const mockSubscription: azureResource.AzureResourceSubscription = { id: mockSubscriptionId, @@ -50,7 +54,7 @@ let mockResourceProvider2: TypeMoq.IMock; let resourceService: AzureResourceService; -describe('AzureResourceService.listResourceProviderIds', function(): void { +describe('AzureResourceService.listResourceProviderIds', function (): void { beforeEach(() => { mockResourceTreeDataProvider1 = TypeMoq.Mock.ofType(); mockResourceTreeDataProvider1.setup((o) => o.getRootChildren()).returns(() => Promise.resolve([TypeMoq.Mock.ofType().object])); @@ -71,7 +75,7 @@ describe('AzureResourceService.listResourceProviderIds', function(): void { resourceService.areResourceProvidersLoaded = true; }); - it('Should be correct when registering providers.', async function(): Promise { + it('Should be correct when registering providers.', async function (): Promise { resourceService.registerResourceProvider(mockResourceProvider1.object); let providerIds = await resourceService.listResourceProviderIds(); should(providerIds).Array(); @@ -87,7 +91,7 @@ describe('AzureResourceService.listResourceProviderIds', function(): void { }); }); -describe('AzureResourceService.getRootChildren', function(): void { +describe('AzureResourceService.getRootChildren', function (): void { beforeEach(() => { mockResourceTreeDataProvider1 = TypeMoq.Mock.ofType(); mockResourceTreeDataProvider1.setup((o) => o.getRootChildren()).returns(() => Promise.resolve([TypeMoq.Mock.ofType().object])); @@ -101,13 +105,13 @@ describe('AzureResourceService.getRootChildren', function(): void { resourceService.areResourceProvidersLoaded = true; }); - it('Should be correct when provider id is correct.', async function(): Promise { + it('Should be correct when provider id is correct.', async function (): Promise { const children = await resourceService.getRootChildren(mockResourceProvider1.object.providerId, mockAccount, mockSubscription, mockTenantId); should(children).Array(); }); - it('Should throw exceptions when provider id is incorrect.', async function(): Promise { + it('Should throw exceptions when provider id is incorrect.', async function (): Promise { const providerId = 'non_existent_provider_id'; try { await resourceService.getRootChildren(providerId, mockAccount, mockSubscription, mockTenantId); @@ -120,7 +124,7 @@ describe('AzureResourceService.getRootChildren', function(): void { }); }); -describe('AzureResourceService.getChildren', function(): void { +describe('AzureResourceService.getChildren', function (): void { beforeEach(() => { mockResourceTreeDataProvider1 = TypeMoq.Mock.ofType(); mockResourceTreeDataProvider1.setup((o) => o.getRootChildren()).returns(() => Promise.resolve([TypeMoq.Mock.ofType().object])); @@ -135,12 +139,12 @@ describe('AzureResourceService.getChildren', function(): void { resourceService.areResourceProvidersLoaded = true; }); - it('Should be correct when provider id is correct.', async function(): Promise { + it('Should be correct when provider id is correct.', async function (): Promise { const children = await resourceService.getChildren(mockResourceProvider1.object.providerId, TypeMoq.It.isAny()); should(children).Array(); }); - it('Should throw exceptions when provider id is incorrect.', async function(): Promise { + it('Should throw exceptions when provider id is incorrect.', async function (): Promise { const providerId = 'non_existent_provider_id'; try { await resourceService.getRootChildren(providerId, mockAccount, mockSubscription, mockTenantId); @@ -153,7 +157,7 @@ describe('AzureResourceService.getChildren', function(): void { }); }); -describe('AzureResourceService.getTreeItem', function(): void { +describe('AzureResourceService.getTreeItem', function (): void { beforeEach(() => { mockResourceTreeDataProvider1 = TypeMoq.Mock.ofType(); mockResourceTreeDataProvider1.setup((o) => o.getRootChildren()).returns(() => Promise.resolve([TypeMoq.Mock.ofType().object])); @@ -168,12 +172,12 @@ describe('AzureResourceService.getTreeItem', function(): void { resourceService.areResourceProvidersLoaded = true; }); - it('Should be correct when provider id is correct.', async function(): Promise { + it('Should be correct when provider id is correct.', async function (): Promise { const treeItem = await resourceService.getTreeItem(mockResourceProvider1.object.providerId, TypeMoq.It.isAny()); should(treeItem).Object(); }); - it('Should throw exceptions when provider id is incorrect.', async function(): Promise { + it('Should throw exceptions when provider id is incorrect.', async function (): Promise { const providerId = 'non_existent_provider_id'; try { await resourceService.getRootChildren(providerId, mockAccount, mockSubscription, mockTenantId); diff --git a/extensions/azurecore/src/test/azureResource/resourceTreeNode.test.ts b/extensions/azurecore/src/test/azureResource/resourceTreeNode.test.ts index fd68d2c049..b4629dec3d 100644 --- a/extensions/azurecore/src/test/azureResource/resourceTreeNode.test.ts +++ b/extensions/azurecore/src/test/azureResource/resourceTreeNode.test.ts @@ -31,6 +31,10 @@ const mockAccount: AzureAccount = { properties: { providerSettings: settings[0].metadata, isMsAccount: true, + owningTenant: { + id: 'tenantId', + displayName: 'tenantDisplayName', + }, tenants: [] }, isStale: false diff --git a/extensions/azurecore/src/test/azureResource/tree/accountTreeNode.test.ts b/extensions/azurecore/src/test/azureResource/tree/accountTreeNode.test.ts index b8b8a4102b..ecba49937b 100644 --- a/extensions/azurecore/src/test/azureResource/tree/accountTreeNode.test.ts +++ b/extensions/azurecore/src/test/azureResource/tree/accountTreeNode.test.ts @@ -35,7 +35,10 @@ let mockTreeChangeHandler: TypeMoq.IMock; // Mock test data const mockTenantId = 'mock_tenant_id'; - +const mockTenant = { + id: mockTenantId, + displayName: 'Mock Tenant' +}; const mockAccount: AzureAccount = { key: { accountId: '97915f6d-84fa-4926-b60c-38db64327ad7', @@ -50,11 +53,9 @@ const mockAccount: AzureAccount = { }, properties: { tenants: [ - { - id: mockTenantId, - displayName: 'Mock Tenant' - } + mockTenant ], + owningTenant: mockTenant, providerSettings: { settings: allSettings[0].metadata.settings, id: 'azure', @@ -360,8 +361,7 @@ describe('AzureResourceAccountTreeNode.clearCache', function (): void { 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(() => - { + mockCacheService.setup((o) => o.update(TypeMoq.It.isAnyString(), TypeMoq.It.isAny())).returns(() => { mockSubscriptionCache = mockSubscriptions; return Promise.resolve(); }); diff --git a/extensions/azurecore/tsconfig.json b/extensions/azurecore/tsconfig.json index 3606c65ff1..bd78dc839e 100644 --- a/extensions/azurecore/tsconfig.json +++ b/extensions/azurecore/tsconfig.json @@ -3,6 +3,7 @@ "compilerOptions": { "outDir": "./out", "noUnusedParameters": false, + "sourceMap": true, "typeRoots": [ "./node_modules/@types" ],