Updated credentials system (#17888)

* linting

* added flags

* remove testing values

* format

* format doc

* tested in linux

* remove unused interface

* comments

* review comments

* clean imports

* pr comments

* format doc

* changed promise location

* insiders december

* pr comments

* test ado change

* fix test

* comment out code for hygiene

* remove unused imports

* test creds from client only

* remove unused import

* trying enabling keytar

* trying enabling keytar

* disable in correct script

* print statements

* remove print statements

* check mock output

* add linux check

* remove print statements

extensions/mssql/src/credentialstore/credentialstore.ts

@@ -3,63 +3,13 @@
  *  Licensed under the Source EULA. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import { SqlOpsDataClient, ClientOptions, SqlOpsFeature } from 'dataprotocol-client';
+import { SqlOpsDataClient, ClientOptions } from 'dataprotocol-client';
 import { IConfig } from '@microsoft/ads-service-downloader';
-import { ServerOptions, RPCMessageType, ClientCapabilities, ServerCapabilities, TransportKind } from 'vscode-languageclient';
-import { Disposable } from 'vscode';
-import * as UUID from 'vscode-languageclient/lib/utils/uuid';
-import * as azdata from 'azdata';
-
-import * as Contracts from './contracts';
+import { ServerOptions, TransportKind } from 'vscode-languageclient';
 import * as Constants from './constants';
 import * as Utils from '../utils';
-
-class CredentialsFeature extends SqlOpsFeature<any> {
-
-	private static readonly messagesTypes: RPCMessageType[] = [
-		Contracts.DeleteCredentialRequest.type,
-		Contracts.SaveCredentialRequest.type,
-		Contracts.ReadCredentialRequest.type
-	];
-
-	constructor(client: SqlOpsDataClient) {
-		super(client, CredentialsFeature.messagesTypes);
-	}
-
-	fillClientCapabilities(capabilities: ClientCapabilities): void {
-		Utils.ensure(Utils.ensure(capabilities, 'credentials')!, 'credentials')!.dynamicRegistration = true;
-	}
-
-	initialize(capabilities: ServerCapabilities): void {
-		this.register(this.messages, {
-			id: UUID.generateUuid(),
-			registerOptions: undefined
-		});
-	}
-
-	protected registerProvider(options: any): Disposable {
-		const client = this._client;
-
-		let readCredential = (credentialId: string): Thenable<azdata.Credential> => {
-			return client.sendRequest(Contracts.ReadCredentialRequest.type, { credentialId, password: undefined });
-		};
-
-		let saveCredential = (credentialId: string, password: string): Thenable<boolean> => {
-			return client.sendRequest(Contracts.SaveCredentialRequest.type, { credentialId, password });
-		};
-
-		let deleteCredential = (credentialId: string): Thenable<boolean> => {
-			return client.sendRequest(Contracts.DeleteCredentialRequest.type, { credentialId, password: undefined });
-		};
-
-		return azdata.credentials.registerProvider({
-			deleteCredential,
-			readCredential,
-			saveCredential,
-			handle: 0
-		});
-	}
-}
+import { SqlCredentialService } from './sqlCredentialService';
+import { AppContext } from '../appContext';
 
 /**
  * Implements a credential storage for Windows, Mac (darwin), or Linux.
@@ -69,18 +19,24 @@ class CredentialsFeature extends SqlOpsFeature<any> {
 export class CredentialStore {
 	private _client: SqlOpsDataClient;
 	private _config: IConfig;
+	private _logPath: string;
 
-	constructor(private logPath: string, baseConfig: IConfig) {
+	constructor(
+		private context: AppContext,
+		baseConfig: IConfig
+	) {
 		if (baseConfig) {
 			this._config = JSON.parse(JSON.stringify(baseConfig));
 			this._config.executableFiles = ['MicrosoftSqlToolsCredentials.exe', 'MicrosoftSqlToolsCredentials'];
 		}
+		this.context = context;
+		this._logPath = this.context.extensionContext.logPath;
 	}
 
 	public async start(): Promise<void> {
 		let clientOptions: ClientOptions = {
 			providerId: Constants.providerId,
-			features: [CredentialsFeature]
+			features: [SqlCredentialService.asFeature(this.context)]
 		};
 		const serverPath = await Utils.getOrDownloadServer(this._config);
 		const serverOptions = this.generateServerOptions(serverPath);
@@ -95,7 +51,7 @@ export class CredentialStore {
 	}
 
 	private generateServerOptions(executablePath: string): ServerOptions {
-		let launchArgs = Utils.getCommonLaunchArgsAndCleanupOldLogFiles(this.logPath, 'credentialstore.log', executablePath);
+		let launchArgs = Utils.getCommonLaunchArgsAndCleanupOldLogFiles(this._logPath, 'credentialstore.log', executablePath);
 		return { command: executablePath, args: launchArgs, transport: TransportKind.stdio };
 	}
 }

extensions/mssql/src/credentialstore/sqlCredentialService.ts

@@ -0,0 +1,91 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the Source EULA. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import { AppContext } from '../appContext';
+import { SqlOpsDataClient, ISqlOpsFeature, SqlOpsFeature } from 'dataprotocol-client';
+import * as Utils from '../utils';
+import { ClientCapabilities, RPCMessageType, ServerCapabilities } from 'vscode-languageclient';
+import * as Contracts from './contracts';
+import { Disposable, SecretStorage } from 'vscode';
+import * as azdata from 'azdata';
+import * as UUID from 'vscode-languageclient/lib/utils/uuid';
+
+export class SqlCredentialService extends SqlOpsFeature<any> {
+
+	private static readonly messagesTypes: RPCMessageType[] = [
+		Contracts.DeleteCredentialRequest.type,
+		Contracts.SaveCredentialRequest.type,
+		Contracts.ReadCredentialRequest.type
+	];
+
+	public static asFeature(context: AppContext): ISqlOpsFeature {
+		return class extends SqlCredentialService {
+			private _secretStorage: SecretStorage;
+
+			constructor(client: SqlOpsDataClient) {
+				super(context, client);
+				this._secretStorage = context.extensionContext.secrets;
+			}
+
+			override fillClientCapabilities(capabilities: ClientCapabilities): void {
+				Utils.ensure(Utils.ensure(capabilities, 'credentials')!, 'credentials')!.dynamicRegistration = true;
+			}
+
+			override async initialize(capabilities: ServerCapabilities): Promise<void> {
+				this.register(this.messages, {
+					id: UUID.generateUuid(),
+					registerOptions: undefined
+				});
+			}
+
+
+			protected override registerProvider(options: any): Disposable {
+				let readCredential = async (credentialId: string): Promise<azdata.Credential> => {
+					return this._client.sendRequest(Contracts.ReadCredentialRequest.type, { credentialId, password: undefined });
+				};
+
+				let saveCredential = async (credentialId: string, password: string): Promise<boolean> => {
+					if (Utils.isLinux) {
+						/**
+						 * This is only done for linux because this is going to be
+						 * the default credential system for linux in the next release
+						 */
+						await this._secretStorage.store(credentialId, password);
+					}
+					return this._client.sendRequest(Contracts.SaveCredentialRequest.type, { credentialId, password });
+				};
+
+				let deleteCredential = async (credentialId: string): Promise<boolean> => {
+					if (Utils.isLinux) {
+						try {
+							await this._secretStorage.delete(credentialId);
+						} catch (e) {
+							console.log('credential does not exist in native secret store');
+						}
+					}
+					return this._client.sendRequest(Contracts.DeleteCredentialRequest.type, { credentialId, password: undefined });
+				};
+
+				return azdata.credentials.registerProvider({
+					deleteCredential,
+					readCredential,
+					saveCredential,
+					handle: 0
+				});
+			}
+		};
+	}
+
+	fillClientCapabilities(capabilities: ClientCapabilities): void { }
+
+	initialize(capabilities: ServerCapabilities): void { }
+
+	protected registerProvider(options: any): Disposable { return undefined; }
+
+	private constructor(context: AppContext, protected readonly client: SqlOpsDataClient) {
+		super(client, SqlCredentialService.messagesTypes);
+	}
+
+}

extensions/mssql/src/sqlToolsServer.ts

@@ -25,6 +25,7 @@ import { LanguageExtensionService } from './languageExtension/languageExtensionS
 import { SqlAssessmentService } from './sqlAssessment/sqlAssessmentService';
 import { NotebookConvertService } from './notebookConvert/notebookConvertService';
 import { SqlMigrationService } from './sqlMigration/sqlMigrationService';
+import { SqlCredentialService } from './credentialstore/sqlCredentialService';
 
 const localize = nls.loadMessageBundle();
 const outputChannel = vscode.window.createOutputChannel(Constants.serviceName);
@@ -86,7 +87,7 @@ export class SqlToolsServer {
 	}
 
 	private activateFeatures(context: AppContext): Promise<void> {
-		const credsStore = new CredentialStore(context.extensionContext.logPath, this.config);
+		const credsStore = new CredentialStore(context, this.config);
 		const resourceProvider = new AzureResourceProvider(context.extensionContext.logPath, this.config);
 		this.disposables.push(credsStore);
 		this.disposables.push(resourceProvider);
@@ -163,6 +164,7 @@ function getClientOptions(context: AppContext): ClientOptions {
 			NotebookConvertService.asFeature(context),
 			ProfilerFeature,
 			SqlMigrationService.asFeature(context),
+			SqlCredentialService.asFeature(context),
 			TableDesignerFeature
 		],
 		outputChannel: new CustomOutputChannel()

extensions/mssql/src/utils.ts

@@ -21,6 +21,12 @@ const configLogFilesRemovalLimit = 'logFilesRemovalLimit';
 const extensionConfigSectionName = 'mssql';
 const configLogDebugInfo = 'logDebugInfo';
 
+/**
+ *
+ * @returns Whether the current OS is linux or not
+ */
+export const isLinux = os.platform() === 'linux';
+
 // The function is a duplicate of \src\paths.js. IT would be better to import path.js but it doesn't
 // work for now because the extension is running in different process.
 export function getAppDataPath() {
@@ -64,8 +70,7 @@ export function getConfigLogFilesRemovalLimit(): number {
 	let config = getConfiguration();
 	if (config) {
 		return Number((config[configLogFilesRemovalLimit]).toFixed(0));
-	}
-	else {
+	} else {
 		return undefined;
 	}
 }
@@ -74,8 +79,7 @@ export function getConfigLogRetentionSeconds(): number {
 	let config = getConfiguration();
 	if (config) {
 		return Number((config[configLogRetentionMinutes] * 60).toFixed(0));
-	}
-	else {
+	} else {
 		return undefined;
 	}
 }
@@ -84,8 +88,7 @@ export function getConfigTracingLevel(): string {
 	let config = getConfiguration();
 	if (config) {
 		return config[configTracingLevel];
-	}
-	else {
+	} else {
 		return undefined;
 	}
 }

extensions/notebook/src/jupyter/jupyterSessionManager.ts

@@ -323,6 +323,7 @@ export class JupyterSession implements nb.ISession {
 					}
 				} else {
 					clusterController = await getClusterController(controllerEndpoint.endpoint, 'integrated');
+
 				}
 
 				let gatewayEndpoint: bdc.IEndpointModel = endpoints?.find(ep => ep.name.toLowerCase() === KNOX_ENDPOINT_GATEWAY);
@@ -340,6 +341,7 @@ export class JupyterSession implements nb.ISession {
 				Logger.log(`Parsed knox host and port ${JSON.stringify(gatewayHostAndPort)}`);
 				connectionProfile.options[KNOX_ENDPOINT_SERVER] = gatewayHostAndPort.host;
 				connectionProfile.options[KNOX_ENDPOINT_PORT] = gatewayHostAndPort.port;
+
 			}
 			else {
 				throw new Error(providerNotValidError);

extensions/notebook/src/test/model/sessionManager.test.ts

@@ -23,7 +23,7 @@ import { ExtensionContextHelper } from '../../common/extensionContextHelper';
 import { AppContext } from '../../common/appContext';
 import uuid = require('uuid');
 
-export class TestClusterController implements bdc.IClusterController {
+class TestClusterController implements bdc.IClusterController {
 	getClusterConfig(): Promise<any> {
 		return Promise.resolve({});
 	}
@@ -254,80 +254,82 @@ describe('Jupyter Session', function (): void {
 	});
 
 	it('should configure connection correctly for MSSQL and SqlLogin auth type', async function (): Promise<void> {
-		let connectionProfile: IConnectionProfile = {
-			authenticationType: '',
-			connectionName: '',
-			databaseName: '',
-			id: 'id',
-			providerName: 'MSSQL',
-			options: {
-				authenticationType: 'SqlLogin',
-			},
-			password: '',
-			savePassword: false,
-			saveProfile: false,
-			serverName: '',
-			userName: ''
-		};
-		let futureMock = TypeMoq.Mock.ofType(FutureStub);
-		let kernelMock = TypeMoq.Mock.ofType(KernelStub);
-		kernelMock.setup(k => k.name).returns(() => 'spark');
-		kernelMock.setup(m => m.requestExecute(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => futureMock.object);
-		mockJupyterSession.setup(s => s.kernel).returns(() => kernelMock.object);
-		let credentials = { [ConnectionOptionSpecialType.password]: 'password' };
-		sinon.stub(connection, 'getCredentials').returns(Promise.resolve(credentials));
+		const isLinux = os.platform() === 'linux';
+		if (!isLinux) {
+			let connectionProfile: IConnectionProfile = {
+				authenticationType: '',
+				connectionName: '',
+				databaseName: '',
+				id: 'id',
+				providerName: 'MSSQL',
+				options: {
+					authenticationType: 'SqlLogin',
+				},
+				password: '',
+				savePassword: false,
+				saveProfile: false,
+				serverName: '',
+				userName: ''
+			};
+			let futureMock = TypeMoq.Mock.ofType(FutureStub);
+			let kernelMock = TypeMoq.Mock.ofType(KernelStub);
+			kernelMock.setup(k => k.name).returns(() => 'spark');
+			kernelMock.setup(m => m.requestExecute(TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => futureMock.object);
+			mockJupyterSession.setup(s => s.kernel).returns(() => kernelMock.object);
+			let creds = { [ConnectionOptionSpecialType.password]: 'password' };
+			sinon.stub(connection, 'getCredentials').returns(Promise.resolve(creds));
 
-		// Set up connection info to big data cluster
-		const mockServerInfo: ServerInfo = {
-			serverMajorVersion: 0,
-			serverMinorVersion: 0,
-			serverReleaseVersion: 0,
-			engineEditionId: 0,
-			serverVersion: '',
-			serverLevel: '',
-			serverEdition: '',
-			isCloud: false,
-			azureVersion: 0,
-			osVersion: '',
-			cpuCount: 0,
-			physicalMemoryInMb: -1,
-			options: {
-				isBigDataCluster: true
-			}
-		};
-		const mockGatewayEndpoint: bdc.IEndpointModel = {
-			name: 'gateway',
-			description: '',
-			endpoint: '',
-			protocol: '',
-		};
-		const mockControllerEndpoint: bdc.IEndpointModel = {
-			name: 'controller',
-			description: '',
-			endpoint: '',
-			protocol: '',
-		};
-		const mockHostAndIp: utils.HostAndIp = {
-			host: '127.0.0.1',
-			port: '1337'
-		};
-		const mockClustercontroller = new TestClusterController();
-		mockClustercontroller.username = 'admin';
-		mockClustercontroller.password = uuid.v4();
-		let mockBdcExtension: TypeMoq.IMock<bdc.IExtension> = TypeMoq.Mock.ofType<bdc.IExtension>();
-		let mockExtension: TypeMoq.IMock<vscode.Extension<any>> = TypeMoq.Mock.ofType<vscode.Extension<any>>();
-		mockBdcExtension.setup(m => m.getClusterController(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => mockClustercontroller);
-		mockBdcExtension.setup((m: any) => m.then).returns(() => mockBdcExtension);
-		mockExtension.setup(m => m.activate()).returns(() => Promise.resolve(mockBdcExtension.object));
-		mockExtension.setup((m: any) => m.then).returns(() => mockExtension);
-		sinon.stub(vscode.extensions, 'getExtension').returns(mockExtension.object);
-		sinon.stub(connection, 'getServerInfo').returns(Promise.resolve(mockServerInfo));
-		sinon.stub(utils, 'getClusterEndpoints').returns([mockGatewayEndpoint, mockControllerEndpoint]);
-		sinon.stub(utils, 'getHostAndPortFromEndpoint').returns(mockHostAndIp);
-
-		await session.configureConnection(connectionProfile);
-		should(connectionProfile.options['host']).equal(mockHostAndIp.host);
-		should(connectionProfile.options['knoxport']).equal(mockHostAndIp.port);
+			// Set up connection info to big data cluster
+			const mockServerInfo: ServerInfo = {
+				serverMajorVersion: 0,
+				serverMinorVersion: 0,
+				serverReleaseVersion: 0,
+				engineEditionId: 0,
+				serverVersion: '',
+				serverLevel: '',
+				serverEdition: '',
+				isCloud: false,
+				azureVersion: 0,
+				osVersion: '',
+				cpuCount: 0,
+				physicalMemoryInMb: -1,
+				options: {
+					isBigDataCluster: true
+				}
+			};
+			const mockGatewayEndpoint: bdc.IEndpointModel = {
+				name: 'gateway',
+				description: '',
+				endpoint: '',
+				protocol: '',
+			};
+			const mockControllerEndpoint: bdc.IEndpointModel = {
+				name: 'controller',
+				description: '',
+				endpoint: '',
+				protocol: '',
+			};
+			const mockHostAndIp: utils.HostAndIp = {
+				host: '127.0.0.1',
+				port: '1337'
+			};
+			const mockClustercontroller = new TestClusterController();
+			mockClustercontroller.username = 'admin';
+			mockClustercontroller.password = uuid.v4();
+			let mockBdcExtension: TypeMoq.IMock<bdc.IExtension> = TypeMoq.Mock.ofType<bdc.IExtension>();
+			let mockExtension: TypeMoq.IMock<vscode.Extension<any>> = TypeMoq.Mock.ofType<vscode.Extension<any>>();
+			mockBdcExtension.setup(m => m.getClusterController(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => mockClustercontroller);
+			mockBdcExtension.setup((m: any) => m.then).returns(() => mockBdcExtension);
+			mockExtension.setup(m => m.activate()).returns(() => Promise.resolve(mockBdcExtension.object));
+			mockExtension.setup((m: any) => m.then).returns(() => mockExtension);
+			sinon.stub(vscode.extensions, 'getExtension').returns(mockExtension.object);
+			sinon.stub(connection, 'getServerInfo').returns(Promise.resolve(mockServerInfo));
+			sinon.stub(utils, 'getClusterEndpoints').returns([mockGatewayEndpoint, mockControllerEndpoint]);
+			sinon.stub(utils, 'getHostAndPortFromEndpoint').returns(mockHostAndIp);
+			await session.configureConnection(connectionProfile);
+			should(connectionProfile.options['host']).equal(mockHostAndIp.host);
+			should(connectionProfile.options['knoxport']).equal(mockHostAndIp.port);
+		}
 	});
 
 	it('configure connection should throw error if there is no connection to big data cluster', async function (): Promise<void> {

scripts/test-extensions-unit.bat

@@ -54,7 +54,7 @@ if "%ADS_TEST_GREP%" == "" (
 	SET ADS_TEST_INVERT_GREP=1
 )
 
-set ALL_PLATFORMS_API_TESTS_EXTRA_ARGS=--disable-telemetry --crash-reporter-directory=%VSCODECRASHDIR% --no-cached-data --disable-updates --disable-keytar --user-data-dir=%VSCODEUSERDATADIR% --remote-debugging-port=9222 --extensions-dir=%VSCODEEXTENSIONSDIR%
+set ALL_PLATFORMS_API_TESTS_EXTRA_ARGS=--disable-telemetry --crash-reporter-directory=%VSCODECRASHDIR% --no-cached-data --disable-updates --user-data-dir=%VSCODEUSERDATADIR% --remote-debugging-port=9222 --extensions-dir=%VSCODEEXTENSIONSDIR%
 
 echo ***************************************************
 echo *** starting admin tool extension windows tests ***

scripts/test-extensions-unit.js

@@ -72,7 +72,7 @@ for (const ext of argv.extensions) {
 	console.log(`VSCODEUSERDATADIR : ${VSCODEUSERDATADIR}`);
 	console.log(`VSCODEEXTENSIONSDIR : ${VSCODEEXTENSIONSDIR}`);
 
-	const command = `${process.env.INTEGRATION_TEST_ELECTRON_PATH} ${LINUX_EXTRA_ARGS} --extensionDevelopmentPath=${path.join(__dirname, '..', 'extensions', ext)} --extensionTestsPath=${path.join(__dirname, '..', 'extensions', ext, 'out', 'test')} --user-data-dir=${VSCODEUSERDATADIR} --extensions-dir=${VSCODEEXTENSIONSDIR} --remote-debugging-port=9222 --disable-telemetry --disable-crash-reporter --disable-updates --no-cached-data --disable-keytar`;
+	const command = `${process.env.INTEGRATION_TEST_ELECTRON_PATH} ${LINUX_EXTRA_ARGS} --extensionDevelopmentPath=${path.join(__dirname, '..', 'extensions', ext)} --extensionTestsPath=${path.join(__dirname, '..', 'extensions', ext, 'out', 'test')} --user-data-dir=${VSCODEUSERDATADIR} --extensions-dir=${VSCODEEXTENSIONSDIR} --remote-debugging-port=9222 --disable-telemetry --disable-crash-reporter --disable-updates --no-cached-data`;
 	console.log(`Command used: ${command}`);
 
 	if (os.platform() === 'darwin') {

scripts/test-extensions-unit.sh

@@ -87,7 +87,7 @@ cd $ROOT
 echo "VSCODEUSERDATADIR : '$VSCODEUSERDATADIR'"
 echo "VSCODEEXTDIR : '$VSCODEEXTDIR'"
 
-ALL_PLATFORMS_API_TESTS_EXTRA_ARGS="--disable-telemetry --crash-reporter-directory=$VSCODECRASHDIR --no-cached-data --disable-updates --disable-keytar --disable-extensions --user-data-dir=$VSCODEUSERDATADIR --extensions-dir=$VSCODEEXTDIR"
+ALL_PLATFORMS_API_TESTS_EXTRA_ARGS="--disable-telemetry --crash-reporter-directory=$VSCODECRASHDIR --no-cached-data --disable-updates --disable-extensions --user-data-dir=$VSCODEUSERDATADIR --extensions-dir=$VSCODEEXTDIR"
 
 echo ***************************************************
 echo *** starting admin tool extension windows tests ***

scripts/test-integration.sh

@@ -66,7 +66,7 @@ after_suite
 
 # Tests in the extension host
 
-ALL_PLATFORMS_API_TESTS_EXTRA_ARGS="--disable-telemetry --crash-reporter-directory=$VSCODECRASHDIR --no-cached-data --disable-updates --disable-keytar --disable-extensions --disable-workspace-trust --user-data-dir=$VSCODEUSERDATADIR"
+ALL_PLATFORMS_API_TESTS_EXTRA_ARGS="--disable-telemetry --crash-reporter-directory=$VSCODECRASHDIR --no-cached-data --disable-updates --disable-extensions --disable-workspace-trust --user-data-dir=$VSCODEUSERDATADIR"
 
 # {{SQL CARBON EDIT}} Don't run tests for unused extensions
 # "$INTEGRATION_TEST_ELECTRON_PATH" $LINUX_EXTRA_ARGS $ROOT/extensions/vscode-api-tests/testWorkspace --enable-proposed-api=vscode.vscode-api-tests --extensionDevelopmentPath=$ROOT/extensions/vscode-api-tests --extensionTestsPath=$ROOT/extensions/vscode-api-tests/out/singlefolder-tests $ALL_PLATFORMS_API_TESTS_EXTRA_ARGS