From d20a9d0657726e3adc7f9288084ac82e77969eb9 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 3 Mar 2023 12:38:15 -0800 Subject: [PATCH 1/2] Fix AuthLibrary detection and use MSAL by default --- .../accountManagement/browser/accountDialog.ts | 8 +++----- .../browser/accountManagementService.ts | 18 ++++++++---------- .../browser/accountManagementService.test.ts | 3 ++- .../services/accountManagement/utils.ts | 18 ++++++++++++++++++ .../connection/browser/connectionWidget.ts | 9 ++++----- 5 files changed, 35 insertions(+), 21 deletions(-) create mode 100644 src/sql/workbench/services/accountManagement/utils.ts diff --git a/src/sql/workbench/services/accountManagement/browser/accountDialog.ts b/src/sql/workbench/services/accountManagement/browser/accountDialog.ts index d8f6fa343d7c..e847e6ffea63 100644 --- a/src/sql/workbench/services/accountManagement/browser/accountDialog.ts +++ b/src/sql/workbench/services/accountManagement/browser/accountDialog.ts @@ -48,11 +48,9 @@ import { Iterable } from 'vs/base/common/iterator'; import { LoadingSpinner } from 'sql/base/browser/ui/loadingSpinner/loadingSpinner'; import { Tenant, TenantListDelegate, TenantListRenderer } from 'sql/workbench/services/accountManagement/browser/tenantListRenderer'; import { IAccountManagementService } from 'sql/platform/accounts/common/interfaces'; +import { ADAL_AUTH_LIBRARY, AuthLibrary, getAuthLibrary } from 'sql/workbench/services/accountManagement/utils'; export const VIEWLET_ID = 'workbench.view.accountpanel'; -export type AuthLibrary = 'ADAL' | 'MSAL'; -export const MSAL_AUTH_LIBRARY: AuthLibrary = 'MSAL'; // default -export const ADAL_AUTH_LIBRARY: AuthLibrary = 'ADAL'; export class AccountPaneContainer extends ViewPaneContainer { } @@ -392,7 +390,7 @@ export class AccountDialog extends Modal { this._splitView!.layout(DOM.getContentHeight(this._container!)); // Set the initial items of the list - const authLibrary: AuthLibrary = this._configurationService.getValue('azure.authenticationLibrary'); + const authLibrary: AuthLibrary = getAuthLibrary(this._configurationService); let updatedAccounts: azdata.Account[]; if (authLibrary) { updatedAccounts = filterAccounts(newProvider.initialAccounts, authLibrary); @@ -443,7 +441,7 @@ export class AccountDialog extends Modal { if (!providerMapping || !providerMapping.view) { return; } - const authLibrary: AuthLibrary = this._configurationService.getValue('azure.authenticationLibrary'); + const authLibrary: AuthLibrary = getAuthLibrary(this._configurationService); let updatedAccounts: azdata.Account[]; if (authLibrary) { updatedAccounts = filterAccounts(args.accountList, authLibrary); diff --git a/src/sql/workbench/services/accountManagement/browser/accountManagementService.ts b/src/sql/workbench/services/accountManagement/browser/accountManagementService.ts index a495756ae03c..8fad515d2b6b 100644 --- a/src/sql/workbench/services/accountManagement/browser/accountManagementService.ts +++ b/src/sql/workbench/services/accountManagement/browser/accountManagementService.ts @@ -26,7 +26,8 @@ import { INotificationService, Severity, INotification } from 'vs/platform/notif import { Action } from 'vs/base/common/actions'; import { DisposableStore } from 'vs/base/common/lifecycle'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; -import { ADAL_AUTH_LIBRARY, AuthLibrary, filterAccounts, MSAL_AUTH_LIBRARY } from 'sql/workbench/services/accountManagement/browser/accountDialog'; +import { filterAccounts } from 'sql/workbench/services/accountManagement/browser/accountDialog'; +import { ADAL_AUTH_LIBRARY, MSAL_AUTH_LIBRARY, AuthLibrary, AZURE_AUTH_LIBRARY_CONFIG, getAuthLibrary } from 'sql/workbench/services/accountManagement/utils'; export class AccountManagementService implements IAccountManagementService { // CONSTANTS /////////////////////////////////////////////////////////// @@ -41,7 +42,6 @@ export class AccountManagementService implements IAccountManagementService { private _autoOAuthDialogController?: AutoOAuthDialogController; private _mementoContext?: Memento; protected readonly disposables = new DisposableStore(); - private readonly configurationService: IConfigurationService; // EVENT EMITTERS ////////////////////////////////////////////////////// private _addAccountProviderEmitter: Emitter; @@ -61,7 +61,7 @@ export class AccountManagementService implements IAccountManagementService { @IOpenerService private _openerService: IOpenerService, @ILogService private readonly _logService: ILogService, @INotificationService private readonly _notificationService: INotificationService, - @IConfigurationService configurationService: IConfigurationService + @IConfigurationService private _configurationService: IConfigurationService ) { this._mementoContext = new Memento(AccountManagementService.ACCOUNT_MEMENTO, this._storageService); const mementoObj = this._mementoContext.getMemento(StorageScope.GLOBAL, StorageTarget.MACHINE); @@ -71,11 +71,10 @@ export class AccountManagementService implements IAccountManagementService { this._addAccountProviderEmitter = new Emitter(); this._removeAccountProviderEmitter = new Emitter(); this._updateAccountListEmitter = new Emitter(); - this.configurationService = configurationService; // Determine authentication library in use, to support filtering accounts respectively. // When this value is changed a restart is required so there isn't a need to dynamically update this value at runtime. - this._authLibrary = this.configurationService.getValue('azure.authenticationLibrary'); + this._authLibrary = getAuthLibrary(this._configurationService); _storageService.onWillSaveState(() => this.shutdown()); this.registerListeners(); @@ -169,7 +168,6 @@ export class AccountManagementService implements IAccountManagementService { if (result.accountModified) { this.spliceModifiedAccount(provider, result.changedAccount); } - this.fireAccountListUpdate(provider, result.accountAdded); } finally { notificationHandler.close(); @@ -519,7 +517,7 @@ export class AccountManagementService implements IAccountManagementService { }); } - const authLibrary: AuthLibrary = this.configurationService.getValue('azure.authenticationLibrary'); + const authLibrary: AuthLibrary = getAuthLibrary(this._configurationService) let updatedAccounts: azdata.Account[] if (authLibrary) { updatedAccounts = filterAccounts(provider.accounts, authLibrary); @@ -543,9 +541,9 @@ export class AccountManagementService implements IAccountManagementService { } private registerListeners(): void { - this.disposables.add(this.configurationService.onDidChangeConfiguration(async e => { - if (e.affectsConfiguration('azure.authenticationLibrary')) { - const authLibrary: AuthLibrary = this.configurationService.getValue('azure.authenticationLibrary') ?? MSAL_AUTH_LIBRARY; + this.disposables.add(this._configurationService.onDidChangeConfiguration(async e => { + if (e.affectsConfiguration(AZURE_AUTH_LIBRARY_CONFIG)) { + const authLibrary: AuthLibrary = getAuthLibrary(this._configurationService); let accounts = await this._accountStore.getAllAccounts(); if (accounts) { let updatedAccounts = await this.filterAndMergeAccounts(accounts, authLibrary); diff --git a/src/sql/workbench/services/accountManagement/test/browser/accountManagementService.test.ts b/src/sql/workbench/services/accountManagement/test/browser/accountManagementService.test.ts index a9155ebba8cc..9b0b178adf5d 100644 --- a/src/sql/workbench/services/accountManagement/test/browser/accountManagementService.test.ts +++ b/src/sql/workbench/services/accountManagement/test/browser/accountManagementService.test.ts @@ -534,7 +534,8 @@ function getTestState(): AccountManagementState { const testConfigurationService = new TestConfigurationService(); // Create the account management service - let ams = new AccountManagementService(mockInstantiationService.object, new TestStorageService(), undefined!, undefined!, undefined!, testNotificationService, testConfigurationService); + let ams = new AccountManagementService(mockInstantiationService.object, new TestStorageService(), + undefined, undefined, undefined, testNotificationService, testConfigurationService); // Wire up event handlers let evUpdate = new EventVerifierSingle(); diff --git a/src/sql/workbench/services/accountManagement/utils.ts b/src/sql/workbench/services/accountManagement/utils.ts new file mode 100644 index 000000000000..f6f09b1f0e6b --- /dev/null +++ b/src/sql/workbench/services/accountManagement/utils.ts @@ -0,0 +1,18 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the Source EULA. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; + +export const AZURE_AUTH_LIBRARY_CONFIG = 'azure.authenticationLibrary'; + +export type AuthLibrary = 'ADAL' | 'MSAL'; +export const MSAL_AUTH_LIBRARY: AuthLibrary = 'MSAL'; +export const ADAL_AUTH_LIBRARY: AuthLibrary = 'ADAL'; + +export const DEFAULT_AUTH_LIBRARY: AuthLibrary = MSAL_AUTH_LIBRARY; + +export function getAuthLibrary(configurationService: IConfigurationService): AuthLibrary { + return configurationService.getValue(AZURE_AUTH_LIBRARY_CONFIG) || DEFAULT_AUTH_LIBRARY; +} diff --git a/src/sql/workbench/services/connection/browser/connectionWidget.ts b/src/sql/workbench/services/connection/browser/connectionWidget.ts index 240232eaa692..e0efe073344e 100644 --- a/src/sql/workbench/services/connection/browser/connectionWidget.ts +++ b/src/sql/workbench/services/connection/browser/connectionWidget.ts @@ -36,10 +36,11 @@ import Severity from 'vs/base/common/severity'; import { ConnectionStringOptions } from 'sql/platform/capabilities/common/capabilitiesService'; import { isFalsyOrWhitespace } from 'vs/base/common/strings'; import { IConfigurationService } from 'vs/platform/configuration/common/configuration'; -import { AuthLibrary, filterAccounts } from 'sql/workbench/services/accountManagement/browser/accountDialog'; +import { filterAccounts } from 'sql/workbench/services/accountManagement/browser/accountDialog'; import { AuthenticationType, Actions } from 'sql/platform/connection/common/constants'; import { AdsWidget } from 'sql/base/browser/ui/adsWidget'; import { createCSSRule } from 'vs/base/browser/dom'; +import { AuthLibrary, getAuthLibrary } from 'sql/workbench/services/accountManagement/utils'; const ConnectionStringText = localize('connectionWidget.connectionString', "Connection string"); @@ -112,7 +113,6 @@ export class ConnectionWidget extends lifecycle.Disposable { color: undefined, description: undefined, }; - private readonly configurationService: IConfigurationService; constructor(options: azdata.ConnectionOption[], callbacks: IConnectionComponentCallbacks, providerName: string, @@ -122,7 +122,7 @@ export class ConnectionWidget extends lifecycle.Disposable { @IAccountManagementService private _accountManagementService: IAccountManagementService, @ILogService protected _logService: ILogService, @IErrorMessageService private _errorMessageService: IErrorMessageService, - @IConfigurationService configurationService: IConfigurationService + @IConfigurationService private _configurationService: IConfigurationService ) { super(); this._callbacks = callbacks; @@ -142,7 +142,6 @@ export class ConnectionWidget extends lifecycle.Disposable { } this._providerName = providerName; this._connectionStringOptions = this._connectionManagementService.getProviderProperties(this._providerName).connectionStringOptions; - this.configurationService = configurationService; } protected getAuthTypeDefault(option: azdata.ConnectionOption, os: OperatingSystem): string { @@ -689,7 +688,7 @@ export class ConnectionWidget extends lifecycle.Disposable { let oldSelection = this._azureAccountDropdown.value; const accounts = await this._accountManagementService.getAccounts(); const updatedAccounts = accounts.filter(a => a.key.providerId.startsWith('azure')); - const authLibrary: AuthLibrary = this.configurationService.getValue('azure.authenticationLibrary'); + const authLibrary: AuthLibrary = getAuthLibrary(this._configurationService); if (authLibrary) { this._azureAccountList = filterAccounts(updatedAccounts, authLibrary); } From 8a5911e160c7ec53c663f862c6d0a6ef317f7051 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 3 Mar 2023 13:31:06 -0800 Subject: [PATCH 2/2] Fix tests --- .../test/browser/accountManagementService.test.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/sql/workbench/services/accountManagement/test/browser/accountManagementService.test.ts b/src/sql/workbench/services/accountManagement/test/browser/accountManagementService.test.ts index 9b0b178adf5d..8e75f17e3a02 100644 --- a/src/sql/workbench/services/accountManagement/test/browser/accountManagementService.test.ts +++ b/src/sql/workbench/services/accountManagement/test/browser/accountManagementService.test.ts @@ -33,7 +33,8 @@ const noAccountProvider: azdata.AccountProviderMetadata = { const account: azdata.Account = { key: { providerId: hasAccountProvider.id, - accountId: 'testAccount1' + accountId: 'testAccount1', + authLibrary: 'MSAL' }, displayInfo: { displayName: 'Test Account 1', @@ -320,7 +321,12 @@ suite('Account Management Service Tests:', () => { return ams.getAccountsForProvider(hasAccountProvider.id) .then(result => { // Then: I should get back the list of accounts - assert.strictEqual(result, accountList); + // Since account are filtered by AuthLibrary and list is prepared again, they are not strict equal. + // We compare strict equality of actual accounts here. + assert.strictEqual(accountList.length, result.length); + for (var i = 0; i < accountList.length; i++) { + assert.strictEqual(result[i], accountList[i]); + } }); });