From 0e9fa85acc128ef0d9d18b1ba88da04b604368b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20Mart=C3=ADn?= Date: Sat, 30 May 2026 22:22:51 +0200 Subject: [PATCH] Support concurrent usage of multiple GitHub accounts for child extensions (#317821) --- .../authenticationExtensionsService.ts | 33 +++-- .../browser/authenticationMcpService.ts | 33 +++-- .../authenticationExtensionsService.test.ts | 116 ++++++++++++++++++ .../browser/authenticationMcpService.test.ts | 116 ++++++++++++++++++ 4 files changed, 282 insertions(+), 16 deletions(-) create mode 100644 src/vs/workbench/services/authentication/test/browser/authenticationExtensionsService.test.ts create mode 100644 src/vs/workbench/services/authentication/test/browser/authenticationMcpService.test.ts diff --git a/src/vs/workbench/services/authentication/browser/authenticationExtensionsService.ts b/src/vs/workbench/services/authentication/browser/authenticationExtensionsService.ts index ef2e32e2e15c8d..fdecf1c03b2b96 100644 --- a/src/vs/workbench/services/authentication/browser/authenticationExtensionsService.ts +++ b/src/vs/workbench/services/authentication/browser/authenticationExtensionsService.ts @@ -162,8 +162,7 @@ export class AuthenticationExtensionsService extends Disposable implements IAuth updateAccountPreference(extensionId: string, providerId: string, account: AuthenticationSessionAccount): void { const realExtensionId = ExtensionIdentifier.toKey(extensionId); - const parentExtensionId = this._inheritAuthAccountPreferenceChildToParent[realExtensionId] ?? realExtensionId; - const key = this._getKey(parentExtensionId, providerId); + const key = this._getKey(realExtensionId, providerId); // Store the preference in the workspace and application storage. This allows new workspaces to // have a preference set already to limit the number of prompts that are shown... but also allows @@ -171,22 +170,40 @@ export class AuthenticationExtensionsService extends Disposable implements IAuth this.storageService.store(key, account.label, StorageScope.WORKSPACE, StorageTarget.MACHINE); this.storageService.store(key, account.label, StorageScope.APPLICATION, StorageTarget.MACHINE); - const childrenExtensions = this._inheritAuthAccountPreferenceParentToChildren[parentExtensionId]; - const extensionIds = childrenExtensions ? [parentExtensionId, ...childrenExtensions] : [parentExtensionId]; + const isChild = !!this._inheritAuthAccountPreferenceChildToParent[realExtensionId]; + let extensionIds: string[]; + if (isChild) { + extensionIds = [realExtensionId]; + } else { + const childrenExtensions = this._inheritAuthAccountPreferenceParentToChildren[realExtensionId]; + extensionIds = childrenExtensions ? [realExtensionId, ...childrenExtensions] : [realExtensionId]; + } this._onDidAccountPreferenceChange.fire({ extensionIds, providerId }); } getAccountPreference(extensionId: string, providerId: string): string | undefined { const realExtensionId = ExtensionIdentifier.toKey(extensionId); - const key = this._getKey(this._inheritAuthAccountPreferenceChildToParent[realExtensionId] ?? realExtensionId, providerId); + const specificKey = this._getKey(realExtensionId, providerId); - // If a preference is set in the workspace, use that. Otherwise, use the global preference. - return this.storageService.get(key, StorageScope.WORKSPACE) ?? this.storageService.get(key, StorageScope.APPLICATION); + // If a preference is set for the specific extension, use that. + const specificPreference = this.storageService.get(specificKey, StorageScope.WORKSPACE) ?? this.storageService.get(specificKey, StorageScope.APPLICATION); + if (specificPreference !== undefined) { + return specificPreference; + } + + // Otherwise, fall back to the parent extension's preference if it exists. + const parentExtensionId = this._inheritAuthAccountPreferenceChildToParent[realExtensionId]; + if (parentExtensionId) { + const parentKey = this._getKey(parentExtensionId, providerId); + return this.storageService.get(parentKey, StorageScope.WORKSPACE) ?? this.storageService.get(parentKey, StorageScope.APPLICATION); + } + + return undefined; } removeAccountPreference(extensionId: string, providerId: string): void { const realExtensionId = ExtensionIdentifier.toKey(extensionId); - const key = this._getKey(this._inheritAuthAccountPreferenceChildToParent[realExtensionId] ?? realExtensionId, providerId); + const key = this._getKey(realExtensionId, providerId); // This won't affect any other workspaces that have a preference set, but it will remove the preference // for this workspace and the global preference. This is only paired with a call to updateSessionPreference... diff --git a/src/vs/workbench/services/authentication/browser/authenticationMcpService.ts b/src/vs/workbench/services/authentication/browser/authenticationMcpService.ts index b1ea1f1978d1ad..e1bfd75b745d12 100644 --- a/src/vs/workbench/services/authentication/browser/authenticationMcpService.ts +++ b/src/vs/workbench/services/authentication/browser/authenticationMcpService.ts @@ -219,8 +219,7 @@ export class AuthenticationMcpService extends Disposable implements IAuthenticat //#region Account/Session Preference updateAccountPreference(mcpServerId: string, providerId: string, account: AuthenticationSessionAccount): void { - const parentMcpServerId = this._inheritAuthAccountPreferenceChildToParent[mcpServerId] ?? mcpServerId; - const key = this._getKey(parentMcpServerId, providerId); + const key = this._getKey(mcpServerId, providerId); // Store the preference in the workspace and application storage. This allows new workspaces to // have a preference set already to limit the number of prompts that are shown... but also allows @@ -228,20 +227,38 @@ export class AuthenticationMcpService extends Disposable implements IAuthenticat this.storageService.store(key, account.label, StorageScope.WORKSPACE, StorageTarget.MACHINE); this.storageService.store(key, account.label, StorageScope.APPLICATION, StorageTarget.MACHINE); - const childrenMcpServers = this._inheritAuthAccountPreferenceParentToChildren[parentMcpServerId]; - const mcpServerIds = childrenMcpServers ? [parentMcpServerId, ...childrenMcpServers] : [parentMcpServerId]; + const isChild = !!this._inheritAuthAccountPreferenceChildToParent[mcpServerId]; + let mcpServerIds: string[]; + if (isChild) { + mcpServerIds = [mcpServerId]; + } else { + const childrenMcpServers = this._inheritAuthAccountPreferenceParentToChildren[mcpServerId]; + mcpServerIds = childrenMcpServers ? [mcpServerId, ...childrenMcpServers] : [mcpServerId]; + } this._onDidAccountPreferenceChange.fire({ mcpServerIds, providerId }); } getAccountPreference(mcpServerId: string, providerId: string): string | undefined { - const key = this._getKey(this._inheritAuthAccountPreferenceChildToParent[mcpServerId] ?? mcpServerId, providerId); + const specificKey = this._getKey(mcpServerId, providerId); - // If a preference is set in the workspace, use that. Otherwise, use the global preference. - return this.storageService.get(key, StorageScope.WORKSPACE) ?? this.storageService.get(key, StorageScope.APPLICATION); + // If a preference is set for the specific MCP server, use that. + const specificPreference = this.storageService.get(specificKey, StorageScope.WORKSPACE) ?? this.storageService.get(specificKey, StorageScope.APPLICATION); + if (specificPreference !== undefined) { + return specificPreference; + } + + // Otherwise, fall back to the parent MCP server's preference if it exists. + const parentMcpServerId = this._inheritAuthAccountPreferenceChildToParent[mcpServerId]; + if (parentMcpServerId) { + const parentKey = this._getKey(parentMcpServerId, providerId); + return this.storageService.get(parentKey, StorageScope.WORKSPACE) ?? this.storageService.get(parentKey, StorageScope.APPLICATION); + } + + return undefined; } removeAccountPreference(mcpServerId: string, providerId: string): void { - const key = this._getKey(this._inheritAuthAccountPreferenceChildToParent[mcpServerId] ?? mcpServerId, providerId); + const key = this._getKey(mcpServerId, providerId); // This won't affect any other workspaces that have a preference set, but it will remove the preference // for this workspace and the global preference. This is only paired with a call to updateSessionPreference... diff --git a/src/vs/workbench/services/authentication/test/browser/authenticationExtensionsService.test.ts b/src/vs/workbench/services/authentication/test/browser/authenticationExtensionsService.test.ts new file mode 100644 index 00000000000000..878c36d5506c9e --- /dev/null +++ b/src/vs/workbench/services/authentication/test/browser/authenticationExtensionsService.test.ts @@ -0,0 +1,116 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; +import { IStorageService } from '../../../../../platform/storage/common/storage.js'; +import { TestStorageService, TestActivityService } from '../../../../test/common/workbenchTestServices.js'; +import { IProductService } from '../../../../../platform/product/common/productService.js'; +import { IActivityService } from '../../../activity/common/activity.js'; +import { IDialogService } from '../../../../../platform/dialogs/common/dialogs.js'; +import { TestDialogService } from '../../../../../platform/dialogs/test/common/testDialogService.js'; +import { IQuickInputService } from '../../../../../platform/quickinput/common/quickInput.js'; +import { TestQuickInputService } from '../../../../test/browser/workbenchTestServices.js'; +import { IAuthenticationService } from '../../common/authentication.js'; +import { TestAuthenticationService, TestAccessService } from './authenticationQueryServiceMocks.js'; +import { IAuthenticationUsageService } from '../../browser/authenticationUsageService.js'; +import { IAuthenticationAccessService } from '../../browser/authenticationAccessService.js'; +import { AuthenticationExtensionsService } from '../../browser/authenticationExtensionsService.js'; + +class TestAuthUsageService implements IAuthenticationUsageService { + readonly _serviceBrand: undefined; + async initializeExtensionUsageCache(): Promise { } + async extensionUsesAuth(extensionId: string): Promise { return false; } + readAccountUsages(providerId: string, accountName: string): any[] { return []; } + removeAccountUsage(providerId: string, accountName: string): void { } + addAccountUsage(providerId: string, accountName: string, scopes: ReadonlyArray, extensionId: string, extensionName: string): void { } +} + +suite('AuthenticationExtensionsService - Hierarchical Preferences', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + + let extensionsService: AuthenticationExtensionsService; + let storageService: TestStorageService; + + setup(() => { + const instantiationService = disposables.add(new TestInstantiationService()); + + storageService = disposables.add(new TestStorageService()); + instantiationService.stub(IStorageService, storageService); + + instantiationService.stub(IActivityService, disposables.add(new TestActivityService())); + instantiationService.stub(IDialogService, new TestDialogService()); + instantiationService.stub(IQuickInputService, new TestQuickInputService()); + + const mockProductService: Partial = { + inheritAuthAccountPreference: { + 'parent-ext': ['child-ext-1', 'child-ext-2'] + } + }; + instantiationService.stub(IProductService, mockProductService); + + instantiationService.stub(IAuthenticationService, disposables.add(new TestAuthenticationService())); + instantiationService.stub(IAuthenticationUsageService, new TestAuthUsageService()); + instantiationService.stub(IAuthenticationAccessService, disposables.add(new TestAccessService())); + + extensionsService = disposables.add(instantiationService.createInstance(AuthenticationExtensionsService)); + }); + + test('default inheritance: child inherits parent preference', () => { + // Set preference on parent + extensionsService.updateAccountPreference('parent-ext', 'github', { id: 'user-a', label: 'account-a' }); + + // Child should inherit parent's preference + assert.strictEqual(extensionsService.getAccountPreference('child-ext-1', 'github'), 'account-a'); + assert.strictEqual(extensionsService.getAccountPreference('child-ext-2', 'github'), 'account-a'); + assert.strictEqual(extensionsService.getAccountPreference('parent-ext', 'github'), 'account-a'); + }); + + test('explicit override: child can use different preference than parent', () => { + // Set preference on parent (account-a) + extensionsService.updateAccountPreference('parent-ext', 'github', { id: 'user-a', label: 'account-a' }); + + // Set explicit preference on child-ext-1 (account-b) + extensionsService.updateAccountPreference('child-ext-1', 'github', { id: 'user-b', label: 'account-b' }); + + // child-ext-1 should use its own override + assert.strictEqual(extensionsService.getAccountPreference('child-ext-1', 'github'), 'account-b'); + + // parent-ext and child-ext-2 should still use parent's preference (account-a) + assert.strictEqual(extensionsService.getAccountPreference('parent-ext', 'github'), 'account-a'); + assert.strictEqual(extensionsService.getAccountPreference('child-ext-2', 'github'), 'account-a'); + }); + + test('explicit override: removing child preference falls back to parent preference', () => { + // Set preference on parent (account-a) and child (account-b) + extensionsService.updateAccountPreference('parent-ext', 'github', { id: 'user-a', label: 'account-a' }); + extensionsService.updateAccountPreference('child-ext-1', 'github', { id: 'user-b', label: 'account-b' }); + + assert.strictEqual(extensionsService.getAccountPreference('child-ext-1', 'github'), 'account-b'); + + // Remove child preference + extensionsService.removeAccountPreference('child-ext-1', 'github'); + + // child-ext-1 should fall back to parent preference + assert.strictEqual(extensionsService.getAccountPreference('child-ext-1', 'github'), 'account-a'); + }); + + test('removing parent preference does not remove child preference override', () => { + // Set preference on parent (account-a) and child (account-b) + extensionsService.updateAccountPreference('parent-ext', 'github', { id: 'user-a', label: 'account-a' }); + extensionsService.updateAccountPreference('child-ext-1', 'github', { id: 'user-b', label: 'account-b' }); + + // Remove parent preference + extensionsService.removeAccountPreference('parent-ext', 'github'); + + // child-ext-1 should still have its preference override (account-b) + assert.strictEqual(extensionsService.getAccountPreference('child-ext-1', 'github'), 'account-b'); + + // parent-ext and child-ext-2 should have no preference + assert.strictEqual(extensionsService.getAccountPreference('parent-ext', 'github'), undefined); + assert.strictEqual(extensionsService.getAccountPreference('child-ext-2', 'github'), undefined); + }); +}); diff --git a/src/vs/workbench/services/authentication/test/browser/authenticationMcpService.test.ts b/src/vs/workbench/services/authentication/test/browser/authenticationMcpService.test.ts new file mode 100644 index 00000000000000..8d9860196cce96 --- /dev/null +++ b/src/vs/workbench/services/authentication/test/browser/authenticationMcpService.test.ts @@ -0,0 +1,116 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; +import { IStorageService } from '../../../../../platform/storage/common/storage.js'; +import { TestStorageService, TestActivityService } from '../../../../test/common/workbenchTestServices.js'; +import { IProductService } from '../../../../../platform/product/common/productService.js'; +import { IActivityService } from '../../../activity/common/activity.js'; +import { IDialogService } from '../../../../../platform/dialogs/common/dialogs.js'; +import { TestDialogService } from '../../../../../platform/dialogs/test/common/testDialogService.js'; +import { IQuickInputService } from '../../../../../platform/quickinput/common/quickInput.js'; +import { TestQuickInputService } from '../../../../test/browser/workbenchTestServices.js'; +import { IAuthenticationService } from '../../common/authentication.js'; +import { TestAuthenticationService, TestMcpAccessService } from './authenticationQueryServiceMocks.js'; +import { IAuthenticationMcpUsageService } from '../../browser/authenticationMcpUsageService.js'; +import { IAuthenticationMcpAccessService } from '../../browser/authenticationMcpAccessService.js'; +import { AuthenticationMcpService } from '../../browser/authenticationMcpService.js'; + +class TestMcpUsageService implements IAuthenticationMcpUsageService { + readonly _serviceBrand: undefined; + async initializeUsageCache(): Promise { } + async hasUsedAuth(mcpServerId: string): Promise { return false; } + readAccountUsages(providerId: string, accountName: string): any[] { return []; } + removeAccountUsage(providerId: string, accountName: string): void { } + addAccountUsage(providerId: string, accountName: string, scopes: ReadonlyArray, mcpServerId: string, mcpServerName: string): void { } +} + +suite('AuthenticationMcpService - Hierarchical Preferences', () => { + const disposables = ensureNoDisposablesAreLeakedInTestSuite(); + + let mcpService: AuthenticationMcpService; + let storageService: TestStorageService; + + setup(() => { + const instantiationService = disposables.add(new TestInstantiationService()); + + storageService = disposables.add(new TestStorageService()); + instantiationService.stub(IStorageService, storageService); + + instantiationService.stub(IActivityService, disposables.add(new TestActivityService())); + instantiationService.stub(IDialogService, new TestDialogService()); + instantiationService.stub(IQuickInputService, new TestQuickInputService()); + + const mockProductService: Partial = { + inheritAuthAccountPreference: { + 'parent-mcp': ['child-mcp-1', 'child-mcp-2'] + } + }; + instantiationService.stub(IProductService, mockProductService); + + instantiationService.stub(IAuthenticationService, disposables.add(new TestAuthenticationService())); + instantiationService.stub(IAuthenticationMcpUsageService, new TestMcpUsageService()); + instantiationService.stub(IAuthenticationMcpAccessService, disposables.add(new TestMcpAccessService())); + + mcpService = disposables.add(instantiationService.createInstance(AuthenticationMcpService)); + }); + + test('default inheritance: child inherits parent preference', () => { + // Set preference on parent + mcpService.updateAccountPreference('parent-mcp', 'github', { id: 'user-a', label: 'account-a' }); + + // Child should inherit parent's preference + assert.strictEqual(mcpService.getAccountPreference('child-mcp-1', 'github'), 'account-a'); + assert.strictEqual(mcpService.getAccountPreference('child-mcp-2', 'github'), 'account-a'); + assert.strictEqual(mcpService.getAccountPreference('parent-mcp', 'github'), 'account-a'); + }); + + test('explicit override: child can use different preference than parent', () => { + // Set preference on parent (account-a) + mcpService.updateAccountPreference('parent-mcp', 'github', { id: 'user-a', label: 'account-a' }); + + // Set explicit preference on child-mcp-1 (account-b) + mcpService.updateAccountPreference('child-mcp-1', 'github', { id: 'user-b', label: 'account-b' }); + + // child-mcp-1 should use its own override + assert.strictEqual(mcpService.getAccountPreference('child-mcp-1', 'github'), 'account-b'); + + // parent-mcp and child-mcp-2 should still use parent's preference (account-a) + assert.strictEqual(mcpService.getAccountPreference('parent-mcp', 'github'), 'account-a'); + assert.strictEqual(mcpService.getAccountPreference('child-mcp-2', 'github'), 'account-a'); + }); + + test('explicit override: removing child preference falls back to parent preference', () => { + // Set preference on parent (account-a) and child (account-b) + mcpService.updateAccountPreference('parent-mcp', 'github', { id: 'user-a', label: 'account-a' }); + mcpService.updateAccountPreference('child-mcp-1', 'github', { id: 'user-b', label: 'account-b' }); + + assert.strictEqual(mcpService.getAccountPreference('child-mcp-1', 'github'), 'account-b'); + + // Remove child preference + mcpService.removeAccountPreference('child-mcp-1', 'github'); + + // child-mcp-1 should fall back to parent preference + assert.strictEqual(mcpService.getAccountPreference('child-mcp-1', 'github'), 'account-a'); + }); + + test('removing parent preference does not remove child preference override', () => { + // Set preference on parent (account-a) and child (account-b) + mcpService.updateAccountPreference('parent-mcp', 'github', { id: 'user-a', label: 'account-a' }); + mcpService.updateAccountPreference('child-mcp-1', 'github', { id: 'user-b', label: 'account-b' }); + + // Remove parent preference + mcpService.removeAccountPreference('parent-mcp', 'github'); + + // child-mcp-1 should still have its preference override (account-b) + assert.strictEqual(mcpService.getAccountPreference('child-mcp-1', 'github'), 'account-b'); + + // parent-mcp and child-mcp-2 should have no preference + assert.strictEqual(mcpService.getAccountPreference('parent-mcp', 'github'), undefined); + assert.strictEqual(mcpService.getAccountPreference('child-mcp-2', 'github'), undefined); + }); +});