From 5f85bb784cec15f8b5ff18423af853d3681dd615 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 13:04:16 +0200 Subject: [PATCH 01/17] chore: added migration step to migrate keytar secrets to vscode secret storage --- src/connectionController.ts | 322 ++++++++------- src/storage/storageController.ts | 27 +- src/telemetry/telemetryService.ts | 9 +- src/test/suite/connectionController.test.ts | 365 +++++++++++++++++- .../collectionDocumentsProvider.test.ts | 7 +- .../suite/explorer/explorerController.test.ts | 4 + src/test/suite/mdbExtensionController.test.ts | 3 + src/test/suite/stubs.ts | 13 +- 8 files changed, 602 insertions(+), 148 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index 40ee3d947..f66f115f5 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -4,7 +4,6 @@ import { ConnectionInfo, ConnectionOptions, getConnectionTitle, - ConnectionSecrets, extractSecrets, mergeSecrets, connect, @@ -22,11 +21,13 @@ import formatError from './utils/formatError'; import LegacyConnectionModel from './views/webview-app/connection-model/legacy-connection-model'; import { StorageLocation, - ConnectionsFromStorage, + SecretStorageLocation, } from './storage/storageController'; import { StorageController, StorageVariables } from './storage'; import { StatusView } from './views'; -import TelemetryService from './telemetry/telemetryService'; +import TelemetryService, { + TelemetryEventTypes, +} from './telemetry/telemetryService'; import LINKS from './utils/links'; // eslint-disable-next-line @typescript-eslint/no-var-requires const packageJSON = require('../package.json'); @@ -50,6 +51,7 @@ export interface StoreConnectionInfo { id: string; // Connection model id or a new uuid. name: string; // Possibly user given name, not unique. storageLocation: StorageLocation; + secretStorageLocation?: SecretStorageLocation; connectionOptions?: ConnectionOptions; connectionModel?: LegacyConnectionModel; } @@ -69,20 +71,24 @@ interface ConnectionQuickPicks { data: { type: NewConnectionType; connectionId?: string }; } -interface ConnectionSecretsInfo { - connectionId: string; - secrets: ConnectionSecrets; -} +type StoreConnectionInfoWithConnectionModel = StoreConnectionInfo & + Required>; type StoreConnectionInfoWithConnectionOptions = StoreConnectionInfo & Required>; +type MigratedStoreConnectionInfo = StoreConnectionInfo & + Required>; + +type MigratedStoreConnectionInfoWithConnectionOptions = + StoreConnectionInfoWithConnectionOptions & MigratedStoreConnectionInfo; + export default class ConnectionController { // This is a map of connection ids to their configurations. // These connections can be saved on the session (runtime), // on the workspace, or globally in vscode. _connections: { - [connectionId: string]: StoreConnectionInfoWithConnectionOptions; + [connectionId: string]: MigratedStoreConnectionInfoWithConnectionOptions; } = {}; _activeDataService: DataService | null = null; _storageController: StorageController; @@ -116,142 +122,185 @@ export default class ConnectionController { this._telemetryService = telemetryService; } - async _migratePreviouslySavedConnection( - savedConnectionInfo: StoreConnectionInfo - ): Promise { - if (!savedConnectionInfo.connectionModel) { - throw new Error( - 'The connectionModel object is missing in saved connection info.' + async loadSavedConnections(): Promise { + const globalAndWorkspaceConnections = { + ...this._storageController.get( + StorageVariables.GLOBAL_SAVED_CONNECTIONS, + StorageLocation.GLOBAL + ), + ...this._storageController.get( + StorageVariables.WORKSPACE_SAVED_CONNECTIONS, + StorageLocation.WORKSPACE + ), + }; + + let connectionsDidChange = false; + const connectionsWithSecretsInKeytar: Array<{ + connectionId: string; + connectionName: string; + }> = []; + const totalConnectionEntries = Object.entries( + globalAndWorkspaceConnections + ); + for (const [connectionId, connectionInfo] of totalConnectionEntries) { + const connectionInfoWithSecret = await this._getConnectionInfoWithSecrets( + connectionInfo ); + + if (connectionInfoWithSecret) { + connectionsDidChange = true; + this._connections[connectionId] = connectionInfoWithSecret; + + if ( + connectionInfoWithSecret.secretStorageLocation === + SecretStorageLocation.Keytar + ) { + connectionsWithSecretsInKeytar.push({ + connectionId, + connectionName: connectionInfo.name, + }); + } + } } + if (connectionsDidChange) { + this.eventEmitter.emit(DataServiceEventTypes.CONNECTIONS_DID_CHANGE); + } + + if (connectionsWithSecretsInKeytar.length) { + log.error('Could not migrate secrets for a few connections', connectionsWithSecretsInKeytar); + this._telemetryService.track( + TelemetryEventTypes.KEYTAR_SECRETS_MIGRATION_FAILED, + { + totalConnections: totalConnectionEntries.length, + connectionsWithSecretsInKeytar: connectionsWithSecretsInKeytar.length, + } + ); + void vscode.window.showInformationMessage( + [ + 'Could not migrate secrets for a few connections. Please review the following connections:', + connectionsWithSecretsInKeytar + .map(({ connectionName }) => connectionName) + .join(', '), + ].join('\n') + ); + } + } + + async _getConnectionInfoWithSecrets( + connectionInfo: StoreConnectionInfo + ): Promise { + try { + if (connectionInfo.connectionModel) { + return this._migratePreviouslySavedConnection( + connectionInfo as StoreConnectionInfoWithConnectionModel + ); + } + + if (!connectionInfo.secretStorageLocation) { + return this._migrateKeytarSecrets( + connectionInfo as StoreConnectionInfoWithConnectionOptions + ); + } + + // We tried migrating this connection earlier but failed because Keytar was not + // available. So we return simply the connection without secrets. + if ( + connectionInfo.secretStorageLocation === SecretStorageLocation.Keytar + ) { + return connectionInfo as MigratedStoreConnectionInfoWithConnectionOptions; + } + + const unparsedSecrets = + (await this._storageController.getSecret(connectionInfo.id)) ?? ''; + + return this._mergedConnectionInfoWithSecrets( + connectionInfo as MigratedStoreConnectionInfoWithConnectionOptions, + unparsedSecrets + ); + } catch (error) { + log.error('Error while retrieving connection info', error); + return undefined; + } + } + + async _migratePreviouslySavedConnection( + savedConnectionInfo: StoreConnectionInfoWithConnectionModel + ): Promise { // Transform a raw connection model from storage to an ampersand model. const newConnectionInfoWithSecrets = convertConnectionModelToInfo( savedConnectionInfo.connectionModel ); - // Further use connectionOptions instead of connectionModel. - const newSavedConnectionInfoWithSecrets = { + const connectionInfoWithSecret = { id: savedConnectionInfo.id, name: savedConnectionInfo.name, storageLocation: savedConnectionInfo.storageLocation, + secretStorageLocation: SecretStorageLocation.SecretStorage, connectionOptions: newConnectionInfoWithSecrets.connectionOptions, }; - await this._saveConnection(newSavedConnectionInfoWithSecrets); - - return newSavedConnectionInfoWithSecrets; + await this._saveConnectionWithSecret(connectionInfoWithSecret); + return connectionInfoWithSecret; } - async _getConnectionInfoWithSecrets( - savedConnectionInfo: StoreConnectionInfo - ): Promise { - // Migrate previously saved connections to a new format. - // Save only secrets to keychain. - // Remove connectionModel and use connectionOptions instead. - if (savedConnectionInfo.connectionModel) { - try { - return await this._migratePreviouslySavedConnection( - savedConnectionInfo - ); - } catch (error) { - // Here we're lenient when loading connections in case their - // connections have become corrupted. - log.error('Migrating previously saved connections failed', error); - return; - } - } - - // If connection has a new format already and keytar module is undefined. - // Return saved connection as it is. + async _migrateKeytarSecrets( + savedConnectionInfo: StoreConnectionInfoWithConnectionOptions + ): Promise { + // If the Keytar module is not available, we simply mark the connections + // storage as Keytar and return if (!ext.keytarModule) { - log.error( - 'Getting connection info with secrets failed because VSCode extension keytar module is undefined' + log.error('Could not migrate Keytar secrets, module not found'); + return await this._storageController.saveConnection( + { + ...savedConnectionInfo, + secretStorageLocation: SecretStorageLocation.Keytar, + } ); - return savedConnectionInfo as StoreConnectionInfoWithConnectionOptions; } - try { - const unparsedSecrets = await ext.keytarModule.getPassword( + // If there is nothing in keytar, we will save an empty object as secrets in + // new storage and mark this connection as migrated + const keytarSecrets = + (await ext.keytarModule.getPassword( this._serviceName, savedConnectionInfo.id - ); - - // Ignore empty secrets. - if (!unparsedSecrets) { - return savedConnectionInfo as StoreConnectionInfoWithConnectionOptions; - } + )) || '{}'; - const secrets = JSON.parse(unparsedSecrets); - const connectionOptions = savedConnectionInfo.connectionOptions; - const connectionInfoWithSecrets = mergeSecrets( + const migratedConnectionInfoWithSecrets = + this._mergedConnectionInfoWithSecrets( { - id: savedConnectionInfo.id, - connectionOptions, - } as ConnectionInfo, - secrets + ...savedConnectionInfo, + secretStorageLocation: SecretStorageLocation.SecretStorage, + }, + keytarSecrets ); - return { - ...savedConnectionInfo, - connectionOptions: connectionInfoWithSecrets.connectionOptions, - }; - } catch (error) { - // Here we're lenient when loading connections in case their - // connections have become corrupted. - log.error('Getting connection info with secrets failed', error); - return; - } - } - - private async _loadSavedConnectionsByStore( - savedConnections: ConnectionsFromStorage - ): Promise { - if (!savedConnections || !Object.keys(savedConnections).length) { - return; - } - - /** User connections are being saved both in: - * 1. Vscode global/workspace storage (without secrets) + keychain (secrets) - * 2. Memory of the extension (with secrets) - */ - await Promise.all( - Object.keys(savedConnections).map(async (connectionId) => { - // Get connection info from vscode storage and merge with secrets. - const connectionInfoWithSecrets = - await this._getConnectionInfoWithSecrets( - savedConnections[connectionId] - ); - - // Save connection info with secrets to extension memory. - if (connectionInfoWithSecrets) { - this._connections[connectionId] = connectionInfoWithSecrets; - } + await this._saveConnectionWithSecret(migratedConnectionInfoWithSecrets); - this.eventEmitter.emit(DataServiceEventTypes.CONNECTIONS_DID_CHANGE); - }) - ); + return migratedConnectionInfoWithSecrets; } - async loadSavedConnections(): Promise { - await Promise.all([ - (async () => { - // Try to pull in the connections previously saved in the global storage of vscode. - const existingGlobalConnections = this._storageController.get( - StorageVariables.GLOBAL_SAVED_CONNECTIONS, - StorageLocation.GLOBAL - ); - await this._loadSavedConnectionsByStore(existingGlobalConnections); - })(), - (async () => { - // Try to pull in the connections previously saved in the workspace storage of vscode. - const existingWorkspaceConnections = this._storageController.get( - StorageVariables.WORKSPACE_SAVED_CONNECTIONS, - StorageLocation.WORKSPACE - ); - await this._loadSavedConnectionsByStore(existingWorkspaceConnections); - })(), - ]); + _mergedConnectionInfoWithSecrets< + T extends StoreConnectionInfoWithConnectionOptions + >(connectionInfo: T, unparsedSecrets: string) { + if (!unparsedSecrets) { + return connectionInfo; + } + + const secrets = JSON.parse(unparsedSecrets); + const connectionInfoWithSecrets = mergeSecrets( + { + id: connectionInfo.id, + connectionOptions: connectionInfo.connectionOptions, + } as ConnectionInfo, + secrets + ); + + return { + ...connectionInfo, + connectionOptions: connectionInfoWithSecrets.connectionOptions, + }; } async connectWithURI(): Promise { @@ -353,26 +402,9 @@ export default class ConnectionController { }); } - private async _saveSecretsToKeychain({ - connectionId, - secrets, - }: ConnectionSecretsInfo): Promise { - if (!ext.keytarModule) { - return; - } - - const secretsAsString = JSON.stringify(secrets); - - await ext.keytarModule.setPassword( - this._serviceName, - connectionId, - secretsAsString - ); - } - - private async _saveConnection( - newStoreConnectionInfoWithSecrets: StoreConnectionInfo - ): Promise { + private async _saveConnectionWithSecret( + newStoreConnectionInfoWithSecrets: MigratedStoreConnectionInfoWithConnectionOptions + ): Promise { // We don't want to store secrets to disc. const { connectionInfo: safeConnectionInfo, secrets } = extractSecrets( newStoreConnectionInfoWithSecrets as ConnectionInfo @@ -381,11 +413,10 @@ export default class ConnectionController { ...newStoreConnectionInfoWithSecrets, connectionOptions: safeConnectionInfo.connectionOptions, // The connection info without secrets. }); - - await this._saveSecretsToKeychain({ - connectionId: savedConnectionInfo.id, - secrets, // Only secrets. - }); + await this._storageController.setSecret( + savedConnectionInfo.id, + JSON.stringify(secrets) + ); return savedConnectionInfo; } @@ -401,10 +432,13 @@ export default class ConnectionController { // To begin we just store it on the session, the storage controller // handles changing this based on user preference. storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, connectionOptions: originalConnectionInfo.connectionOptions, }; - const savedConnectionInfo = await this._saveConnection(newConnectionInfo); + const savedConnectionInfo = await this._saveConnectionWithSecret( + newConnectionInfo + ); this._connections[savedConnectionInfo.id] = { ...savedConnectionInfo, @@ -596,9 +630,15 @@ export default class ConnectionController { } private async _removeSecretsFromKeychain(connectionId: string) { + // Even though we migrated to SecretStorage from keytar, we are still removing + // secrets from keytar to make sure that we don't leave any left-overs when a + // connection is removed. This block can safely be removed after our migration + // has been out for some time. if (ext.keytarModule) { await ext.keytarModule.deletePassword(this._serviceName, connectionId); } + + await this._storageController.deleteSecret(connectionId); } async removeSavedConnection(connectionId: string): Promise { diff --git a/src/storage/storageController.ts b/src/storage/storageController.ts index 2c9583e9c..b27673795 100644 --- a/src/storage/storageController.ts +++ b/src/storage/storageController.ts @@ -32,6 +32,11 @@ export type ConnectionsFromStorage = { [connectionId: string]: StoreConnectionInfo; }; +export enum SecretStorageLocation { + Keytar = 'vscode.Keytar', + SecretStorage = 'vscode.SecretStorage', +} + interface StorageVariableContents { [StorageVariables.GLOBAL_USER_ID]: string; [StorageVariables.GLOBAL_ANONYMOUS_ID]: string; @@ -48,11 +53,14 @@ export default class StorageController { [StorageLocation.WORKSPACE]: vscode.Memento; }; + _secretStorage: vscode.SecretStorage; + constructor(context: vscode.ExtensionContext) { this._storage = { [StorageLocation.GLOBAL]: context.globalState, [StorageLocation.WORKSPACE]: context.workspaceState, }; + this._secretStorage = context.secrets; } get( @@ -123,9 +131,9 @@ export default class StorageController { ); } - async saveConnection( - storeConnectionInfo: StoreConnectionInfo - ): Promise { + async saveConnection( + storeConnectionInfo: T + ): Promise { const dontShowSaveLocationPrompt = vscode.workspace .getConfiguration('mdb.connectionSaving') .get('hideOptionToChooseWhereToSaveNewConnections'); @@ -243,4 +251,17 @@ export default class StorageController { return StorageLocation.NONE; } + + async getSecret(key: string): Promise { + return (await this._secretStorage.get(key)) ?? null; + } + + async deleteSecret(key: string): Promise { + await this._secretStorage.delete(key); + return true; + } + + async setSecret(key: string, value: string): Promise { + await this._secretStorage.store(key, value); + } } diff --git a/src/telemetry/telemetryService.ts b/src/telemetry/telemetryService.ts index 9937c4f28..78cac92e3 100644 --- a/src/telemetry/telemetryService.ts +++ b/src/telemetry/telemetryService.ts @@ -68,6 +68,11 @@ type PlaygroundLoadedTelemetryEventProperties = { file_type?: string; }; +type KeytarSecretsMigrationFailedProperties = { + totalConnections: number; + connectionsWithSecretsInKeytar: number; +}; + export type TelemetryEventProperties = | PlaygroundTelemetryEventProperties | LinkClickedTelemetryEventProperties @@ -78,7 +83,8 @@ export type TelemetryEventProperties = | QueryExportedTelemetryEventProperties | PlaygroundCreatedTelemetryEventProperties | PlaygroundSavedTelemetryEventProperties - | PlaygroundLoadedTelemetryEventProperties; + | PlaygroundLoadedTelemetryEventProperties + | KeytarSecretsMigrationFailedProperties; export enum TelemetryEventTypes { PLAYGROUND_CODE_EXECUTED = 'Playground Code Executed', @@ -92,6 +98,7 @@ export enum TelemetryEventTypes { QUERY_EXPORTED = 'Query Exported', AGGREGATION_EXPORTED = 'Aggregation Exported', PLAYGROUND_CREATED = 'Playground Created', + KEYTAR_SECRETS_MIGRATION_FAILED = 'Keytar Secrets Migration Failed', } /** diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index 9865d7b54..4c3e3b01f 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -15,6 +15,7 @@ import { StorageController, StorageVariables } from '../../storage'; import { StorageLocation, DefaultSavingLocations, + SecretStorageLocation, } from '../../storage/storageController'; import READ_PREFERENCES from '../../views/webview-app/connection-model/constants/read-preferences'; import SSH_TUNNEL_TYPES from '../../views/webview-app/connection-model/constants/ssh-tunnel-types'; @@ -28,6 +29,8 @@ import { TEST_USER_USERNAME, TEST_USER_PASSWORD, } from './dbTestHelper'; +import KeytarStub from './keytarStub'; +import { ext } from '../../extensionConstants'; const testDatabaseConnectionName = 'localhost:27018'; const testDatabaseURI2WithTimeout = @@ -52,10 +55,14 @@ suite('Connection Controller Test Suite', function () { testTelemetryService ); let showErrorMessageStub: SinonStub; + let showInformationMessageStub: SinonStub; const sandbox = sinon.createSandbox(); beforeEach(() => { - sandbox.stub(vscode.window, 'showInformationMessage'); + showInformationMessageStub = sandbox.stub( + vscode.window, + 'showInformationMessage' + ); showErrorMessageStub = sandbox.stub(vscode.window, 'showErrorMessage'); }); @@ -354,6 +361,7 @@ suite('Connection Controller Test Suite', function () { name: 'tester', connectionOptions: { connectionString: TEST_DATABASE_URI }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }, }; @@ -684,6 +692,7 @@ suite('Connection Controller Test Suite', function () { name: `test${i}`, connectionOptions: { connectionString: TEST_DATABASE_URI }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; } @@ -733,6 +742,7 @@ suite('Connection Controller Test Suite', function () { name: 'asdfasdg', connectionOptions: { connectionString: testDatabaseURI2WithTimeout }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; void testConnectionController.connectWithConnectionId(connectionId); @@ -757,6 +767,7 @@ suite('Connection Controller Test Suite', function () { name: 'asdfasdg', connectionOptions: { connectionString: TEST_DATABASE_URI }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; sandbox.replace( @@ -818,6 +829,7 @@ suite('Connection Controller Test Suite', function () { id: '1d700f37-ba57-4568-9552-0ea23effea89', name: 'localhost:27017', storageLocation: 'GLOBAL', + secretStorageLocation: SecretStorageLocation.SecretStorage, connectionOptions: { connectionString: 'mongodb://localhost:27017/?readPreference=primary&ssl=false&directConnection=true', @@ -870,6 +882,7 @@ suite('Connection Controller Test Suite', function () { id: 'fb210b47-f85d-4823-8552-aa6d7825156b', name: 'host.u88dd.test.test', storageLocation: 'WORKSPACE', + secretStorageLocation: SecretStorageLocation.SecretStorage, connectionOptions: { connectionString: 'mongodb+srv://username:password@compass-data-sets.e06dc.mongodb.net/test?authSource=admin&replicaSet=host-shard-0&readPreference=primary&appname=mongodb-vscode+0.6.14&ssl=true', @@ -960,6 +973,7 @@ suite('Connection Controller Test Suite', function () { id: '1d700f37-ba57-4568-9552-0ea23effea89', name: 'localhost:27017', storageLocation: 'GLOBAL', + secretStorageLocation: SecretStorageLocation.SecretStorage, connectionOptions: { connectionString: 'mongodb://localhost:27017/?readPreference=primary&ssl=false', @@ -984,6 +998,7 @@ suite('Connection Controller Test Suite', function () { id: '1d700f37-ba57-4568-9552-0ea23effea89', name: 'localhost:27017', storageLocation: StorageLocation.GLOBAL, + secretStorageLocation: SecretStorageLocation.SecretStorage, connectionOptions: { connectionString: 'mongodb://localhost:27017/?readPreference=primary&ssl=false', @@ -1150,4 +1165,352 @@ suite('Connection Controller Test Suite', function () { }); assert.strictEqual(connectionString, expectedConnectionStringWithProxy); }); + + suite('loadSavedConnections', () => { + const serviceName = 'mdb.vscode.savedConnections'; + const extensionSandbox = sinon.createSandbox(); + const keytarSandbox = sinon.createSandbox(); + const testSandbox = sinon.createSandbox(); + let keytarStub: KeytarStub; + + beforeEach(() => { + // To fake a successful auth connection + testSandbox.replace( + testConnectionController, + '_connect', + testSandbox.stub().resolves({ successfullyConnected: true }) + ); + }); + + afterEach(() => { + testSandbox.restore(); + keytarSandbox.restore(); + extensionSandbox.restore(); + }); + + suite( + 'when there are connections requiring secrets migrations from Keytar', + () => { + beforeEach(() => { + // Replace the storage location with keytar to have some connection + // secrets in keytar before the tests + keytarStub = new KeytarStub(); + keytarSandbox.replace( + testStorageController, + 'getSecret', + (key: string) => keytarStub.getPassword(serviceName, key) + ); + keytarSandbox.replace( + testStorageController, + 'setSecret', + (key: string, value: string) => + keytarStub.setPassword(serviceName, key, value) + ); + keytarSandbox.replace( + testStorageController, + 'deleteSecret', + (key: string) => keytarStub.deletePassword(serviceName, key) + ); + + // Also replace the saveConnection method on StorageController to remove + // the secretStorage field while saving the connections because we are + // initially faking that we store in keytar to actually run the + // migration code + const originalSaveConnectionFn = + testStorageController.saveConnection.bind(testStorageController); + keytarSandbox.replace( + testStorageController, + 'saveConnection', + (connectionInfo) => { + const connectionInfoWithoutSecretStorage = { + ...connectionInfo, + }; + delete connectionInfoWithoutSecretStorage.secretStorageLocation; + return originalSaveConnectionFn( + connectionInfoWithoutSecretStorage + ); + } + ); + }); + + [ + { + name: 'should be able to migrate secrets and load a connection that does not have any credentials', + uri: TEST_DATABASE_URI, + expectedSecret: '{}', + }, + { + name: 'should be able to migrate secrets and load a connection that has credentials', + uri: TEST_DATABASE_URI_USER, + expectedSecret: JSON.stringify({ password: TEST_USER_PASSWORD }), + }, + ].forEach(({ name, uri, expectedSecret }) => { + test(name, async () => { + // We replace the keytar module with our stub to make sure that later, + // during migration, we are able to find the secrets in the correct + // place + extensionSandbox.replace(ext, 'keytarModule', keytarStub); + + await testConnectionController.addNewConnectionStringAndConnect( + uri + ); + + // Make sure we actually saved in keytar and that there is nothing in secretStorage + const [savedConnection] = + testConnectionController.getSavedConnections(); + assert.strictEqual( + await (ext.keytarModule as KeytarStub).getPassword( + serviceName, + savedConnection.id + ), + expectedSecret + ); + assert.strictEqual( + await testStorageController._secretStorage.get( + savedConnection.id + ), + undefined + ); + + // Also assert that our connections do not have secretStorage field in them + assert.strictEqual( + 'secretStorageLocation' in savedConnection, + false + ); + + // Reset the modification done by the keytarSandbox + keytarSandbox.restore(); + + // Disconnect and clear the connections + await testConnectionController.disconnect(); + testConnectionController.clearAllConnections(); + + // Load all connections now, after this the migration is expected to be + // finished + await testConnectionController.loadSavedConnections(); + const [updatedConnection] = + testConnectionController.getSavedConnections(); + + // Assert that we have our secrets now in SecretStorage + assert.strictEqual( + await testStorageController.getSecret(savedConnection.id), + expectedSecret + ); + assert( + updatedConnection.secretStorageLocation === + SecretStorageLocation.SecretStorage + ); + }); + }); + + test('should be able to load a connection for which there was nothing found in keytar (user removed the secrets manually)', async () => { + // We replace the keytar module with our stub to make sure that later, + // during migration, we are able to find the secrets in the correct + // place + extensionSandbox.replace(ext, 'keytarModule', keytarStub); + + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + const expectedSecret = JSON.stringify({ + password: TEST_USER_PASSWORD, + }); + + // Make sure we actually saved in keytar and that there is nothing in secretStorage + const [savedConnection] = + testConnectionController.getSavedConnections(); + assert.strictEqual( + await (ext.keytarModule as KeytarStub).getPassword( + serviceName, + savedConnection.id + ), + expectedSecret + ); + assert.strictEqual( + await testStorageController._secretStorage.get(savedConnection.id), + undefined + ); + + // Here we are manually removing the secret from keytar so that it is + // not found during migration + await (ext.keytarModule as KeytarStub).deletePassword( + serviceName, + savedConnection.id + ); + + // Reset the modification done by the keytarSandbox + keytarSandbox.restore(); + + // Disconnect and clear the connections + await testConnectionController.disconnect(); + testConnectionController.clearAllConnections(); + + // Load all connections now, after this the migration is expected to be + // finished + await testConnectionController.loadSavedConnections(); + const [updatedConnection] = + testConnectionController.getSavedConnections(); + + // Assert that we have an empty object as secrets now in SecretStorage + // because we removed the original secrets above + assert.strictEqual( + await testStorageController.getSecret(savedConnection.id), + '{}' + ); + assert( + updatedConnection.secretStorageLocation === + SecretStorageLocation.SecretStorage + ); + }); + + test('should be able to load a connection (in broken state) even when keytar module is gone', async () => { + // Here we try to mimick that the keytar module is gone away + extensionSandbox.replace(ext, 'keytarModule', null as any); + + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + // Make sure we actually saved in keytar and that there is nothing in + // secretStorage + const [savedConnection] = + testConnectionController.getSavedConnections(); + assert.strictEqual( + await keytarStub.getPassword(serviceName, savedConnection.id), + JSON.stringify({ password: TEST_USER_PASSWORD }) + ); + assert.strictEqual( + await testStorageController._secretStorage.get(savedConnection.id), + undefined + ); + + // Reset the modification done by the keytarSandbox + keytarSandbox.restore(); + + // Disconnect and clear the connections + await testConnectionController.disconnect(); + testConnectionController.clearAllConnections(); + + // Load all connections now, after this the migration is expected to be + // finished + await testConnectionController.loadSavedConnections(); + const [updatedConnection] = + testConnectionController.getSavedConnections(); + + // Assert that we have nothing now in our SecretStorage + assert.strictEqual( + await testStorageController.getSecret(savedConnection.id), + null + ); + assert( + updatedConnection.secretStorageLocation === + SecretStorageLocation.Keytar + ); + }); + } + ); + + suite('when connection secrets are already in SecretStorage', () => { + afterEach(() => { + testSandbox.restore(); + }); + + test('should be able to load connection with its secrets', async () => { + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI + ); + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + // By default the connection secrets are already stored in SecretStorage + const savedConnections = testConnectionController.getSavedConnections(); + assert( + savedConnections.every( + ({ secretStorageLocation }) => + secretStorageLocation === SecretStorageLocation.SecretStorage + ) + ); + + await testConnectionController.disconnect(); + testConnectionController.clearAllConnections(); + + await testConnectionController.loadSavedConnections(); + const savedConnectionsAfterFreshLoad = + testConnectionController.getSavedConnections(); + assert.deepStrictEqual( + savedConnections, + testConnectionController.getSavedConnections() + ); + + // Additionally make sure that we are retrieving secrets properly + assert( + savedConnectionsAfterFreshLoad[1].connectionOptions?.connectionString.includes( + TEST_USER_PASSWORD + ) + ); + }); + }); + + test('should fire a CONNECTIONS_DID_CHANGE event if connections are loaded successfully', async () => { + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + await testConnectionController.disconnect(); + testConnectionController.clearAllConnections(); + + let isConnectionChanged = false; + testConnectionController.addEventListener( + DataServiceEventTypes.CONNECTIONS_DID_CHANGE, + () => { + isConnectionChanged = true; + } + ); + + await testConnectionController.loadSavedConnections(); + assert(isConnectionChanged); + }); + + test('should track and also notify the users of failed keytar secrets migration', async () => { + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI + ); + const [savedConnection] = testConnectionController.getSavedConnections(); + + testConnectionController.clearAllConnections(); + + testSandbox.replace( + testConnectionController, + '_getConnectionInfoWithSecrets', + () => + Promise.resolve({ + ...savedConnection, + secretStorageLocation: SecretStorageLocation.Keytar, + } as any) + ); + + const fakeTrack = sinon.stub(); + testSandbox.replace(testTelemetryService, 'track', fakeTrack); + + await testConnectionController.loadSavedConnections(); + + // Notified to user + assert(showInformationMessageStub.calledOnce); + assert.deepStrictEqual(showInformationMessageStub.lastCall.args, [ + [ + 'Could not migrate secrets for a few connections. Please review the following connections:', + savedConnection.name, + ].join('\n'), + ]); + + // Tracked + assert(fakeTrack.calledOnce); + assert.deepStrictEqual(fakeTrack.lastCall.args, [ + 'Keytar Secrets Migration Failed', + { totalConnections: 1, connectionsWithSecretsInKeytar: 1 }, + ]); + }); + }); }); diff --git a/src/test/suite/editors/collectionDocumentsProvider.test.ts b/src/test/suite/editors/collectionDocumentsProvider.test.ts index afc368500..36483fcbb 100644 --- a/src/test/suite/editors/collectionDocumentsProvider.test.ts +++ b/src/test/suite/editors/collectionDocumentsProvider.test.ts @@ -13,7 +13,10 @@ import ConnectionController from '../../../connectionController'; import EditDocumentCodeLensProvider from '../../../editors/editDocumentCodeLensProvider'; import { StatusView } from '../../../views'; import { StorageController } from '../../../storage'; -import { StorageLocation } from '../../../storage/storageController'; +import { + SecretStorageLocation, + StorageLocation, +} from '../../../storage/storageController'; import TelemetryService from '../../../telemetry/telemetryService'; import { TEST_DATABASE_URI } from '../dbTestHelper'; import { ExtensionContextStub, mockTextEditor } from '../stubs'; @@ -480,12 +483,14 @@ suite('Collection Documents Provider Test Suite', () => { name: 'localhost', connectionOptions: { connectionString: TEST_DATABASE_URI }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }, [secondConnectionId]: { id: secondConnectionId, name: 'compass', connectionOptions: { connectionString: TEST_DATABASE_URI }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }, }; diff --git a/src/test/suite/explorer/explorerController.test.ts b/src/test/suite/explorer/explorerController.test.ts index 3257b8c6c..7fbaa99c8 100644 --- a/src/test/suite/explorer/explorerController.test.ts +++ b/src/test/suite/explorer/explorerController.test.ts @@ -5,6 +5,7 @@ import sinon from 'sinon'; import { DefaultSavingLocations, + SecretStorageLocation, StorageLocation, } from '../../../storage/storageController'; import { mdbTestExtension } from '../stubbableMdbExtension'; @@ -59,6 +60,7 @@ suite('Explorer Controller Test Suite', function () { connectionOptions: { connectionString: 'mongodb://localhost' }, name: 'testConnectionName', storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }, }; testConnectionController.setConnnecting(true); @@ -198,6 +200,7 @@ suite('Explorer Controller Test Suite', function () { name: 'aaa', id: 'aaa', storageLocation: StorageLocation.WORKSPACE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; testConnectionController._connections.zzz = { @@ -206,6 +209,7 @@ suite('Explorer Controller Test Suite', function () { name: 'zzz', id: 'zzz', storageLocation: StorageLocation.WORKSPACE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; const connectionsItems = await treeController.getChildren(); diff --git a/src/test/suite/mdbExtensionController.test.ts b/src/test/suite/mdbExtensionController.test.ts index d79b037fd..302754e15 100644 --- a/src/test/suite/mdbExtensionController.test.ts +++ b/src/test/suite/mdbExtensionController.test.ts @@ -21,6 +21,7 @@ import IndexListTreeItem from '../../explorer/indexListTreeItem'; import { mdbTestExtension } from './stubbableMdbExtension'; import { mockTextEditor } from './stubs'; import { + SecretStorageLocation, StorageLocation, StorageVariables, } from '../../storage/storageController'; @@ -966,6 +967,7 @@ suite('MDBExtensionController Test Suite', function () { connectionOptions: { connectionString: 'mongodb://localhost' }, name: 'NAAAME', storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; const testTreeItem = new ConnectionTreeItem( @@ -1004,6 +1006,7 @@ suite('MDBExtensionController Test Suite', function () { name: 'NAAAME', connectionOptions: { connectionString: 'mongodb://localhost' }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; const testTreeItem = new ConnectionTreeItem( diff --git a/src/test/suite/stubs.ts b/src/test/suite/stubs.ts index f3d573765..b6cf7a890 100644 --- a/src/test/suite/stubs.ts +++ b/src/test/suite/stubs.ts @@ -34,6 +34,7 @@ class ExtensionContextStub implements vscode.ExtensionContext { storageUri; globalStorageUri; logUri; + _secrets: Record = {}; secrets; extension; @@ -45,7 +46,17 @@ class ExtensionContextStub implements vscode.ExtensionContext { this.globalStoragePath = ''; this.logPath = ''; this.subscriptions = []; - this.secrets = {}; + this.secrets = { + get: (key: string) => { + return this._secrets[key]; + }, + store: (key: string, value: string) => { + this._secrets[key] = value; + }, + delete: (key: string) => { + delete this._secrets[key]; + }, + }; this.extension = ''; this.workspaceState = { keys: (): readonly string[] => { From 7e0e4c717bf843cb7ad9e0b66c386cff193aec41 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 13:45:25 +0200 Subject: [PATCH 02/17] prettier reformatting --- src/connectionController.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index f66f115f5..e12bc5471 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -168,7 +168,10 @@ export default class ConnectionController { } if (connectionsWithSecretsInKeytar.length) { - log.error('Could not migrate secrets for a few connections', connectionsWithSecretsInKeytar); + log.error( + 'Could not migrate secrets for a few connections', + connectionsWithSecretsInKeytar + ); this._telemetryService.track( TelemetryEventTypes.KEYTAR_SECRETS_MIGRATION_FAILED, { From f62455411e9236a3070123baf76805e32e76d7fa Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 15:42:10 +0200 Subject: [PATCH 03/17] PR feedback - better variable --- src/connectionController.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index e12bc5471..d17f577b1 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -123,7 +123,7 @@ export default class ConnectionController { } async loadSavedConnections(): Promise { - const globalAndWorkspaceConnections = { + const globalAndWorkspaceConnections = Object.entries({ ...this._storageController.get( StorageVariables.GLOBAL_SAVED_CONNECTIONS, StorageLocation.GLOBAL @@ -132,17 +132,14 @@ export default class ConnectionController { StorageVariables.WORKSPACE_SAVED_CONNECTIONS, StorageLocation.WORKSPACE ), - }; + }); let connectionsDidChange = false; const connectionsWithSecretsInKeytar: Array<{ connectionId: string; connectionName: string; }> = []; - const totalConnectionEntries = Object.entries( - globalAndWorkspaceConnections - ); - for (const [connectionId, connectionInfo] of totalConnectionEntries) { + for (const [connectionId, connectionInfo] of globalAndWorkspaceConnections) { const connectionInfoWithSecret = await this._getConnectionInfoWithSecrets( connectionInfo ); @@ -175,7 +172,7 @@ export default class ConnectionController { this._telemetryService.track( TelemetryEventTypes.KEYTAR_SECRETS_MIGRATION_FAILED, { - totalConnections: totalConnectionEntries.length, + totalConnections: globalAndWorkspaceConnections.length, connectionsWithSecretsInKeytar: connectionsWithSecretsInKeytar.length, } ); From dcd2a11049b4867a0dc3f9e544ee59899b7a6c89 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 15:58:38 +0200 Subject: [PATCH 04/17] PR feedback - replaces usage of assert with assert.strictEqual --- src/test/suite/connectionController.test.ts | 34 +++++++++++---------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index 4c3e3b01f..204470f39 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -1296,9 +1296,9 @@ suite('Connection Controller Test Suite', function () { await testStorageController.getSecret(savedConnection.id), expectedSecret ); - assert( - updatedConnection.secretStorageLocation === - SecretStorageLocation.SecretStorage + assert.strictEqual( + updatedConnection.secretStorageLocation, + SecretStorageLocation.SecretStorage ); }); }); @@ -1358,9 +1358,9 @@ suite('Connection Controller Test Suite', function () { await testStorageController.getSecret(savedConnection.id), '{}' ); - assert( - updatedConnection.secretStorageLocation === - SecretStorageLocation.SecretStorage + assert.strictEqual( + updatedConnection.secretStorageLocation, + SecretStorageLocation.SecretStorage ); }); @@ -1403,9 +1403,9 @@ suite('Connection Controller Test Suite', function () { await testStorageController.getSecret(savedConnection.id), null ); - assert( - updatedConnection.secretStorageLocation === - SecretStorageLocation.Keytar + assert.strictEqual( + updatedConnection.secretStorageLocation, + SecretStorageLocation.Keytar ); }); } @@ -1426,11 +1426,12 @@ suite('Connection Controller Test Suite', function () { // By default the connection secrets are already stored in SecretStorage const savedConnections = testConnectionController.getSavedConnections(); - assert( + assert.strictEqual( savedConnections.every( ({ secretStorageLocation }) => secretStorageLocation === SecretStorageLocation.SecretStorage - ) + ), + true ); await testConnectionController.disconnect(); @@ -1445,10 +1446,11 @@ suite('Connection Controller Test Suite', function () { ); // Additionally make sure that we are retrieving secrets properly - assert( + assert.strictEqual( savedConnectionsAfterFreshLoad[1].connectionOptions?.connectionString.includes( TEST_USER_PASSWORD - ) + ), + true ); }); }); @@ -1470,7 +1472,7 @@ suite('Connection Controller Test Suite', function () { ); await testConnectionController.loadSavedConnections(); - assert(isConnectionChanged); + assert.strictEqual(isConnectionChanged, true); }); test('should track and also notify the users of failed keytar secrets migration', async () => { @@ -1497,7 +1499,7 @@ suite('Connection Controller Test Suite', function () { await testConnectionController.loadSavedConnections(); // Notified to user - assert(showInformationMessageStub.calledOnce); + assert.strictEqual(showInformationMessageStub.calledOnce, true); assert.deepStrictEqual(showInformationMessageStub.lastCall.args, [ [ 'Could not migrate secrets for a few connections. Please review the following connections:', @@ -1506,7 +1508,7 @@ suite('Connection Controller Test Suite', function () { ]); // Tracked - assert(fakeTrack.calledOnce); + assert.strictEqual(fakeTrack.calledOnce, true); assert.deepStrictEqual(fakeTrack.lastCall.args, [ 'Keytar Secrets Migration Failed', { totalConnections: 1, connectionsWithSecretsInKeytar: 1 }, From 6c6741993d7fcc767b6b2fb960e96d92eb2c82f8 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 16:20:48 +0200 Subject: [PATCH 05/17] PR feedback - reposition removing secrets from vscode.SecretStorage above the keytar secret removal --- src/connectionController.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index d17f577b1..ba453b7a8 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -630,6 +630,7 @@ export default class ConnectionController { } private async _removeSecretsFromKeychain(connectionId: string) { + await this._storageController.deleteSecret(connectionId); // Even though we migrated to SecretStorage from keytar, we are still removing // secrets from keytar to make sure that we don't leave any left-overs when a // connection is removed. This block can safely be removed after our migration @@ -637,8 +638,6 @@ export default class ConnectionController { if (ext.keytarModule) { await ext.keytarModule.deletePassword(this._serviceName, connectionId); } - - await this._storageController.deleteSecret(connectionId); } async removeSavedConnection(connectionId: string): Promise { From eea1fcc0b40d486c12e26b1bb4900361cf72cf20 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 16:28:20 +0200 Subject: [PATCH 06/17] PR feedback - replace {} with Object.create(null) --- src/connectionController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index ba453b7a8..d01bdb0f9 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -89,7 +89,7 @@ export default class ConnectionController { // on the workspace, or globally in vscode. _connections: { [connectionId: string]: MigratedStoreConnectionInfoWithConnectionOptions; - } = {}; + } = Object.create(null); _activeDataService: DataService | null = null; _storageController: StorageController; From 29701581d61edd063f07e2284bbec41311c17cc2 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 17:40:06 +0200 Subject: [PATCH 07/17] PR feedback - renamed multiple migration method to make them more descriptive --- src/connectionController.ts | 8 ++++---- src/test/suite/connectionController.test.ts | 16 ++++++++-------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index d01bdb0f9..b94230d09 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -192,13 +192,13 @@ export default class ConnectionController { ): Promise { try { if (connectionInfo.connectionModel) { - return this._migratePreviouslySavedConnection( + return this._migrateConnectionsWithConnectionModel( connectionInfo as StoreConnectionInfoWithConnectionModel ); } if (!connectionInfo.secretStorageLocation) { - return this._migrateKeytarSecrets( + return this._migrateConnectionsWithKeytarSecrets( connectionInfo as StoreConnectionInfoWithConnectionOptions ); } @@ -224,7 +224,7 @@ export default class ConnectionController { } } - async _migratePreviouslySavedConnection( + async _migrateConnectionsWithConnectionModel( savedConnectionInfo: StoreConnectionInfoWithConnectionModel ): Promise { // Transform a raw connection model from storage to an ampersand model. @@ -244,7 +244,7 @@ export default class ConnectionController { return connectionInfoWithSecret; } - async _migrateKeytarSecrets( + async _migrateConnectionsWithKeytarSecrets( savedConnectionInfo: StoreConnectionInfoWithConnectionOptions ): Promise { // If the Keytar module is not available, we simply mark the connections diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index 204470f39..a3d004309 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -797,7 +797,7 @@ suite('Connection Controller Test Suite', function () { assert.strictEqual(testConnectionController.isCurrentlyConnected(), false); }); - test('_migratePreviouslySavedConnection converts an old previously saved connection model without secrets to a new connection info format', async () => { + test('_migrateConnectionsWithConnectionModel converts an old previously saved connection model without secrets to a new connection info format', async () => { const oldSavedConnectionInfo = { id: '1d700f37-ba57-4568-9552-0ea23effea89', name: 'localhost:27017', @@ -821,7 +821,7 @@ suite('Connection Controller Test Suite', function () { }, }; const newSavedConnectionInfoWithSecrets = - await testConnectionController._migratePreviouslySavedConnection( + await testConnectionController._migrateConnectionsWithConnectionModel( oldSavedConnectionInfo ); @@ -837,7 +837,7 @@ suite('Connection Controller Test Suite', function () { }); }); - test('_migratePreviouslySavedConnection converts an old previously saved connection model with secrets to a new connection info format', async () => { + test('_migrateConnectionsWithConnectionModel converts an old previously saved connection model with secrets to a new connection info format', async () => { const oldSavedConnectionInfo = { id: 'fb210b47-f85d-4823-8552-aa6d7825156b', name: 'host.u88dd.test.test', @@ -874,7 +874,7 @@ suite('Connection Controller Test Suite', function () { }; const newSavedConnectionInfoWithSecrets = - await testConnectionController._migratePreviouslySavedConnection( + await testConnectionController._migrateConnectionsWithConnectionModel( oldSavedConnectionInfo ); @@ -890,7 +890,7 @@ suite('Connection Controller Test Suite', function () { }); }); - test('_migratePreviouslySavedConnection does not store secrets to disc', async () => { + test('_migrateConnectionsWithConnectionModel does not store secrets to disc', async () => { const oldSavedConnectionInfo = { id: 'fb210b47-f85d-4823-8552-aa6d7825156b', name: 'host.u88dd.test.test', @@ -935,7 +935,7 @@ suite('Connection Controller Test Suite', function () { fakeSaveConnection ); - await testConnectionController._migratePreviouslySavedConnection( + await testConnectionController._migrateConnectionsWithConnectionModel( oldSavedConnectionInfo ); @@ -982,7 +982,7 @@ suite('Connection Controller Test Suite', function () { sandbox.replace( testConnectionController, - '_migratePreviouslySavedConnection', + '_migrateConnectionsWithConnectionModel', fakeMigratePreviouslySavedConnection ); @@ -1017,7 +1017,7 @@ suite('Connection Controller Test Suite', function () { sandbox.replace( testConnectionController, - '_migratePreviouslySavedConnection', + '_migrateConnectionsWithConnectionModel', fakeMigratePreviouslySavedConnection ); From b83d44bd5ffa2bd17dec8e0a55a740dd59d1944e Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 17:40:50 +0200 Subject: [PATCH 08/17] PR feedback - remove unnecessary type aliasing --- src/connectionController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index b94230d09..c2dfb1f63 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -293,7 +293,7 @@ export default class ConnectionController { { id: connectionInfo.id, connectionOptions: connectionInfo.connectionOptions, - } as ConnectionInfo, + }, secrets ); From 0742e4b979ecca21234a2a3aaff0dbb3fddf02d2 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 17:46:56 +0200 Subject: [PATCH 09/17] PR feedback - better error description: --- src/connectionController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index c2dfb1f63..60f30fa8d 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -166,7 +166,7 @@ export default class ConnectionController { if (connectionsWithSecretsInKeytar.length) { log.error( - 'Could not migrate secrets for a few connections', + `Could not migrate secrets for ${connectionsWithSecretsInKeytar.length} connections`, connectionsWithSecretsInKeytar ); this._telemetryService.track( @@ -178,7 +178,7 @@ export default class ConnectionController { ); void vscode.window.showInformationMessage( [ - 'Could not migrate secrets for a few connections. Please review the following connections:', + `Could not migrate secrets for ${connectionsWithSecretsInKeytar.length} connections. Please review the following connections:`, connectionsWithSecretsInKeytar .map(({ connectionName }) => connectionName) .join(', '), From cdd901871a327b1c7f6dc833e83f06f9292a3ed6 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 17:47:45 +0200 Subject: [PATCH 10/17] PR feedback - remove unnecessary variable to check for connection change event --- src/connectionController.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index 60f30fa8d..9b8465c55 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -134,7 +134,6 @@ export default class ConnectionController { ), }); - let connectionsDidChange = false; const connectionsWithSecretsInKeytar: Array<{ connectionId: string; connectionName: string; @@ -145,7 +144,6 @@ export default class ConnectionController { ); if (connectionInfoWithSecret) { - connectionsDidChange = true; this._connections[connectionId] = connectionInfoWithSecret; if ( @@ -160,7 +158,7 @@ export default class ConnectionController { } } - if (connectionsDidChange) { + if (Object.keys(this._connections).length) { this.eventEmitter.emit(DataServiceEventTypes.CONNECTIONS_DID_CHANGE); } From e6ea5bf83c5acef71bf5438cef50957034c62642 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Tue, 4 Jul 2023 18:41:18 +0200 Subject: [PATCH 11/17] PR feedback - use type unions instead of enums --- src/connectionController.ts | 3 ++- src/storage/storageController.ts | 12 ++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index 9b8465c55..1de1d88d7 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -21,6 +21,7 @@ import formatError from './utils/formatError'; import LegacyConnectionModel from './views/webview-app/connection-model/legacy-connection-model'; import { StorageLocation, + SecretStorageLocationType, SecretStorageLocation, } from './storage/storageController'; import { StorageController, StorageVariables } from './storage'; @@ -51,7 +52,7 @@ export interface StoreConnectionInfo { id: string; // Connection model id or a new uuid. name: string; // Possibly user given name, not unique. storageLocation: StorageLocation; - secretStorageLocation?: SecretStorageLocation; + secretStorageLocation?: SecretStorageLocationType; connectionOptions?: ConnectionOptions; connectionModel?: LegacyConnectionModel; } diff --git a/src/storage/storageController.ts b/src/storage/storageController.ts index b27673795..6467e3648 100644 --- a/src/storage/storageController.ts +++ b/src/storage/storageController.ts @@ -32,10 +32,14 @@ export type ConnectionsFromStorage = { [connectionId: string]: StoreConnectionInfo; }; -export enum SecretStorageLocation { - Keytar = 'vscode.Keytar', - SecretStorage = 'vscode.SecretStorage', -} +export const SecretStorageLocation = { + Keytar: 'vscode.Keytar', + SecretStorage: 'vscode.SecretStorage', +} as const; + +export type SecretStorageLocationType = + | typeof SecretStorageLocation.Keytar + | typeof SecretStorageLocation.SecretStorage; interface StorageVariableContents { [StorageVariables.GLOBAL_USER_ID]: string; From 1249543b5519ea913e59cbf493aaba838ff0c89e Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Wed, 5 Jul 2023 12:32:35 +0200 Subject: [PATCH 12/17] PR feedback - parallelise connection secrets fetch and track / notify only for failed migrations in the current run --- src/connectionController.ts | 102 +++++++++++++------- src/telemetry/telemetryService.ts | 2 +- src/test/suite/connectionController.test.ts | 75 +++++++++----- 3 files changed, 116 insertions(+), 63 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index 1de1d88d7..a0dd7824f 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -84,6 +84,11 @@ type MigratedStoreConnectionInfo = StoreConnectionInfo & type MigratedStoreConnectionInfoWithConnectionOptions = StoreConnectionInfoWithConnectionOptions & MigratedStoreConnectionInfo; +type FailedMigrationConnectionDescriptor = { + connectionId: string; + connectionName: string; +}; + export default class ConnectionController { // This is a map of connection ids to their configurations. // These connections can be saved on the session (runtime), @@ -135,50 +140,73 @@ export default class ConnectionController { ), }); - const connectionsWithSecretsInKeytar: Array<{ - connectionId: string; - connectionName: string; - }> = []; - for (const [connectionId, connectionInfo] of globalAndWorkspaceConnections) { - const connectionInfoWithSecret = await this._getConnectionInfoWithSecrets( - connectionInfo + // A list of connection ids that we could not migrate in previous load of + // connections because of Keytar not being available. + const connectionIdsThatDidNotMigrateEarlier = + globalAndWorkspaceConnections.reduce( + (ids, [connectionId, connectionInfo]) => { + if ( + connectionInfo.secretStorageLocation === + SecretStorageLocation.Keytar + ) { + return [...ids, connectionId]; + } + return ids; + }, + [] ); - if (connectionInfoWithSecret) { - this._connections[connectionId] = connectionInfoWithSecret; - - if ( - connectionInfoWithSecret.secretStorageLocation === - SecretStorageLocation.Keytar - ) { - connectionsWithSecretsInKeytar.push({ - connectionId, - connectionName: connectionInfo.name, - }); - } - } - } + // A list of connection descriptors that we could not migration in the + // current load of connections because of Keytar not being available. + const connectionsThatDidNotMigrate = ( + await Promise.all( + globalAndWorkspaceConnections.map< + Promise + >(async ([connectionId, connectionInfo]) => { + const connectionInfoWithSecrets = + await this._getConnectionInfoWithSecrets(connectionInfo); + if (!connectionInfoWithSecrets) { + return; + } + + this._connections[connectionId] = connectionInfoWithSecrets; + const connectionSecretsInKeytar = + connectionInfoWithSecrets.secretStorageLocation === + SecretStorageLocation.Keytar; + if ( + connectionSecretsInKeytar && + !connectionIdsThatDidNotMigrateEarlier.includes(connectionId) + ) { + return { + connectionId, + connectionName: connectionInfo.name, + }; + } + }) + ) + ).filter((conn): conn is FailedMigrationConnectionDescriptor => !!conn); if (Object.keys(this._connections).length) { this.eventEmitter.emit(DataServiceEventTypes.CONNECTIONS_DID_CHANGE); } - if (connectionsWithSecretsInKeytar.length) { + if (connectionsThatDidNotMigrate.length) { log.error( - `Could not migrate secrets for ${connectionsWithSecretsInKeytar.length} connections`, - connectionsWithSecretsInKeytar + `Could not migrate secrets for ${connectionsThatDidNotMigrate.length} connections`, + connectionsThatDidNotMigrate ); this._telemetryService.track( TelemetryEventTypes.KEYTAR_SECRETS_MIGRATION_FAILED, { totalConnections: globalAndWorkspaceConnections.length, - connectionsWithSecretsInKeytar: connectionsWithSecretsInKeytar.length, + connectionsWithFailedKeytarMigration: + connectionsThatDidNotMigrate.length, } ); void vscode.window.showInformationMessage( [ - `Could not migrate secrets for ${connectionsWithSecretsInKeytar.length} connections. Please review the following connections:`, - connectionsWithSecretsInKeytar + `Could not migrate secrets for ${connectionsThatDidNotMigrate.length} connections. Please review the following connections:`, + connectionsThatDidNotMigrate .map(({ connectionName }) => connectionName) .join(', '), ].join('\n') @@ -191,13 +219,13 @@ export default class ConnectionController { ): Promise { try { if (connectionInfo.connectionModel) { - return this._migrateConnectionsWithConnectionModel( + return this._migrateConnectionWithConnectionModel( connectionInfo as StoreConnectionInfoWithConnectionModel ); } if (!connectionInfo.secretStorageLocation) { - return this._migrateConnectionsWithKeytarSecrets( + return this._migrateConnectionWithKeytarSecrets( connectionInfo as StoreConnectionInfoWithConnectionOptions ); } @@ -223,7 +251,7 @@ export default class ConnectionController { } } - async _migrateConnectionsWithConnectionModel( + async _migrateConnectionWithConnectionModel( savedConnectionInfo: StoreConnectionInfoWithConnectionModel ): Promise { // Transform a raw connection model from storage to an ampersand model. @@ -231,7 +259,7 @@ export default class ConnectionController { savedConnectionInfo.connectionModel ); - const connectionInfoWithSecret = { + const connectionInfoWithSecrets = { id: savedConnectionInfo.id, name: savedConnectionInfo.name, storageLocation: savedConnectionInfo.storageLocation, @@ -239,11 +267,11 @@ export default class ConnectionController { connectionOptions: newConnectionInfoWithSecrets.connectionOptions, }; - await this._saveConnectionWithSecret(connectionInfoWithSecret); - return connectionInfoWithSecret; + await this._saveConnectionWithSecrets(connectionInfoWithSecrets); + return connectionInfoWithSecrets; } - async _migrateConnectionsWithKeytarSecrets( + async _migrateConnectionWithKeytarSecrets( savedConnectionInfo: StoreConnectionInfoWithConnectionOptions ): Promise { // If the Keytar module is not available, we simply mark the connections @@ -275,7 +303,7 @@ export default class ConnectionController { keytarSecrets ); - await this._saveConnectionWithSecret(migratedConnectionInfoWithSecrets); + await this._saveConnectionWithSecrets(migratedConnectionInfoWithSecrets); return migratedConnectionInfoWithSecrets; } @@ -401,7 +429,7 @@ export default class ConnectionController { }); } - private async _saveConnectionWithSecret( + private async _saveConnectionWithSecrets( newStoreConnectionInfoWithSecrets: MigratedStoreConnectionInfoWithConnectionOptions ): Promise { // We don't want to store secrets to disc. @@ -435,7 +463,7 @@ export default class ConnectionController { connectionOptions: originalConnectionInfo.connectionOptions, }; - const savedConnectionInfo = await this._saveConnectionWithSecret( + const savedConnectionInfo = await this._saveConnectionWithSecrets( newConnectionInfo ); diff --git a/src/telemetry/telemetryService.ts b/src/telemetry/telemetryService.ts index 78cac92e3..e774d6308 100644 --- a/src/telemetry/telemetryService.ts +++ b/src/telemetry/telemetryService.ts @@ -70,7 +70,7 @@ type PlaygroundLoadedTelemetryEventProperties = { type KeytarSecretsMigrationFailedProperties = { totalConnections: number; - connectionsWithSecretsInKeytar: number; + connectionsWithFailedKeytarMigration: number; }; export type TelemetryEventProperties = diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index a3d004309..40b448549 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -797,7 +797,7 @@ suite('Connection Controller Test Suite', function () { assert.strictEqual(testConnectionController.isCurrentlyConnected(), false); }); - test('_migrateConnectionsWithConnectionModel converts an old previously saved connection model without secrets to a new connection info format', async () => { + test('_migrateConnectionWithConnectionModel converts an old previously saved connection model without secrets to a new connection info format', async () => { const oldSavedConnectionInfo = { id: '1d700f37-ba57-4568-9552-0ea23effea89', name: 'localhost:27017', @@ -821,7 +821,7 @@ suite('Connection Controller Test Suite', function () { }, }; const newSavedConnectionInfoWithSecrets = - await testConnectionController._migrateConnectionsWithConnectionModel( + await testConnectionController._migrateConnectionWithConnectionModel( oldSavedConnectionInfo ); @@ -837,7 +837,7 @@ suite('Connection Controller Test Suite', function () { }); }); - test('_migrateConnectionsWithConnectionModel converts an old previously saved connection model with secrets to a new connection info format', async () => { + test('_migrateConnectionWithConnectionModel converts an old previously saved connection model with secrets to a new connection info format', async () => { const oldSavedConnectionInfo = { id: 'fb210b47-f85d-4823-8552-aa6d7825156b', name: 'host.u88dd.test.test', @@ -874,7 +874,7 @@ suite('Connection Controller Test Suite', function () { }; const newSavedConnectionInfoWithSecrets = - await testConnectionController._migrateConnectionsWithConnectionModel( + await testConnectionController._migrateConnectionWithConnectionModel( oldSavedConnectionInfo ); @@ -890,7 +890,7 @@ suite('Connection Controller Test Suite', function () { }); }); - test('_migrateConnectionsWithConnectionModel does not store secrets to disc', async () => { + test('_migrateConnectionWithConnectionModel does not store secrets to disc', async () => { const oldSavedConnectionInfo = { id: 'fb210b47-f85d-4823-8552-aa6d7825156b', name: 'host.u88dd.test.test', @@ -935,7 +935,7 @@ suite('Connection Controller Test Suite', function () { fakeSaveConnection ); - await testConnectionController._migrateConnectionsWithConnectionModel( + await testConnectionController._migrateConnectionWithConnectionModel( oldSavedConnectionInfo ); @@ -982,7 +982,7 @@ suite('Connection Controller Test Suite', function () { sandbox.replace( testConnectionController, - '_migrateConnectionsWithConnectionModel', + '_migrateConnectionWithConnectionModel', fakeMigratePreviouslySavedConnection ); @@ -1017,7 +1017,7 @@ suite('Connection Controller Test Suite', function () { sandbox.replace( testConnectionController, - '_migrateConnectionsWithConnectionModel', + '_migrateConnectionWithConnectionModel', fakeMigratePreviouslySavedConnection ); @@ -1475,43 +1475,68 @@ suite('Connection Controller Test Suite', function () { assert.strictEqual(isConnectionChanged, true); }); - test('should track and also notify the users of failed keytar secrets migration', async () => { - await testConnectionController.addNewConnectionStringAndConnect( - TEST_DATABASE_URI - ); - const [savedConnection] = testConnectionController.getSavedConnections(); - - testConnectionController.clearAllConnections(); + test('should track and also notify the users of unique failed keytar secrets migration (in the current load of extension)', async () => { + testSandbox.replace(testStorageController, 'get', (key, storage) => { + if ( + storage === StorageLocation.WORKSPACE || + key === StorageVariables.WORKSPACE_SAVED_CONNECTIONS + ) { + return {}; + } + return { + 'random-connection-1': { + id: 'random-connection-1', + name: 'localhost:27017', + storageLocation: 'GLOBAL', + secretStorageLocation: SecretStorageLocation.Keytar, + connectionOptions: { + connectionString: + 'mongodb://localhost:27017/?readPreference=primary&ssl=false', + }, + }, + 'random-connection-2': { + id: 'random-connection-2', + name: 'localhost:27018', + storageLocation: 'GLOBAL', + connectionOptions: { + connectionString: + 'mongodb://localhost:27018/?readPreference=primary&ssl=false', + }, + }, + } as any; + }); testSandbox.replace( testConnectionController, '_getConnectionInfoWithSecrets', - () => + (connectionInfo) => Promise.resolve({ - ...savedConnection, + ...connectionInfo, secretStorageLocation: SecretStorageLocation.Keytar, } as any) ); + const trackStub = testSandbox.stub(testTelemetryService, 'track'); - const fakeTrack = sinon.stub(); - testSandbox.replace(testTelemetryService, 'track', fakeTrack); - + // Load the connections + testConnectionController.clearAllConnections(); await testConnectionController.loadSavedConnections(); + const [, secondConnection] = + testConnectionController.getSavedConnections(); // Notified to user assert.strictEqual(showInformationMessageStub.calledOnce, true); assert.deepStrictEqual(showInformationMessageStub.lastCall.args, [ [ - 'Could not migrate secrets for a few connections. Please review the following connections:', - savedConnection.name, + 'Could not migrate secrets for 1 connections. Please review the following connections:', + secondConnection.name, ].join('\n'), ]); // Tracked - assert.strictEqual(fakeTrack.calledOnce, true); - assert.deepStrictEqual(fakeTrack.lastCall.args, [ + assert.strictEqual(trackStub.calledOnce, true); + assert.deepStrictEqual(trackStub.lastCall.args, [ 'Keytar Secrets Migration Failed', - { totalConnections: 1, connectionsWithSecretsInKeytar: 1 }, + { totalConnections: 2, connectionsWithFailedKeytarMigration: 1 }, ]); }); }); From 36e17c07fcea13a632a6d5976bb992450577d630 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Wed, 5 Jul 2023 14:21:04 +0200 Subject: [PATCH 13/17] PR feedback - changed misleading comment, added a test for verifying that no notification / tracking was sent if the failed migrations are not from current load of extension --- src/test/suite/connectionController.test.ts | 56 ++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index 40b448549..f96c178b3 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -1517,7 +1517,7 @@ suite('Connection Controller Test Suite', function () { ); const trackStub = testSandbox.stub(testTelemetryService, 'track'); - // Load the connections + // Clear any connections and load so we get our stubbed connections from above. testConnectionController.clearAllConnections(); await testConnectionController.loadSavedConnections(); const [, secondConnection] = @@ -1539,5 +1539,59 @@ suite('Connection Controller Test Suite', function () { { totalConnections: 2, connectionsWithFailedKeytarMigration: 1 }, ]); }); + + test('should neither track nor notify the user if none of the failed migrations are from current load of extension', async () => { + testSandbox.replace(testStorageController, 'get', (key, storage) => { + if ( + storage === StorageLocation.WORKSPACE || + key === StorageVariables.WORKSPACE_SAVED_CONNECTIONS + ) { + return {}; + } + + return { + 'random-connection-1': { + id: 'random-connection-1', + name: 'localhost:27017', + storageLocation: 'GLOBAL', + secretStorageLocation: SecretStorageLocation.Keytar, + connectionOptions: { + connectionString: + 'mongodb://localhost:27017/?readPreference=primary&ssl=false', + }, + }, + 'random-connection-2': { + id: 'random-connection-2', + name: 'localhost:27018', + storageLocation: 'GLOBAL', + secretStorageLocation: SecretStorageLocation.Keytar, + connectionOptions: { + connectionString: + 'mongodb://localhost:27018/?readPreference=primary&ssl=false', + }, + }, + } as any; + }); + testSandbox.replace( + testConnectionController, + '_getConnectionInfoWithSecrets', + (connectionInfo) => + Promise.resolve({ + ...connectionInfo, + secretStorageLocation: SecretStorageLocation.Keytar, + } as any) + ); + const trackStub = testSandbox.stub(testTelemetryService, 'track'); + + // Clear any connections and load so we get our stubbed connections from above. + testConnectionController.clearAllConnections(); + await testConnectionController.loadSavedConnections(); + + // No notification sent to the user + assert.strictEqual(showInformationMessageStub.notCalled, true); + + // No tracking done + assert.strictEqual(trackStub.notCalled, true); + }); }); }); From 808e60e8b6d1b60ee5d71103c2ff858bb8baaa7d Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Wed, 5 Jul 2023 17:20:43 +0200 Subject: [PATCH 14/17] PR feedback - catch possible errors from keytar.deletePassword --- src/connectionController.ts | 11 ++-- src/test/suite/connectionController.test.ts | 57 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index a0dd7824f..927b2082d 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -78,11 +78,12 @@ type StoreConnectionInfoWithConnectionModel = StoreConnectionInfo & type StoreConnectionInfoWithConnectionOptions = StoreConnectionInfo & Required>; -type MigratedStoreConnectionInfo = StoreConnectionInfo & +type StoreConnectionInfoWithSecretStorageLocation = StoreConnectionInfo & Required>; type MigratedStoreConnectionInfoWithConnectionOptions = - StoreConnectionInfoWithConnectionOptions & MigratedStoreConnectionInfo; + StoreConnectionInfoWithConnectionOptions & + StoreConnectionInfoWithSecretStorageLocation; type FailedMigrationConnectionDescriptor = { connectionId: string; @@ -662,8 +663,10 @@ export default class ConnectionController { // secrets from keytar to make sure that we don't leave any left-overs when a // connection is removed. This block can safely be removed after our migration // has been out for some time. - if (ext.keytarModule) { - await ext.keytarModule.deletePassword(this._serviceName, connectionId); + try { + await ext.keytarModule?.deletePassword(this._serviceName, connectionId); + } catch (error) { + log.error('Failed to remove secret from keytar', error); } } diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index f96c178b3..a0e0bcb59 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -31,6 +31,7 @@ import { } from './dbTestHelper'; import KeytarStub from './keytarStub'; import { ext } from '../../extensionConstants'; +import { KeytarInterface } from '../../utils/keytar'; const testDatabaseConnectionName = 'localhost:27018'; const testDatabaseURI2WithTimeout = @@ -529,6 +530,62 @@ suite('Connection Controller Test Suite', function () { assert.strictEqual(Object.keys(postGlobalStoreConnections).length, 0); }); + test('when a connection is removed, the secrets for that connection are also removed', async () => { + const keytarDeleteSpy = sandbox + .stub(ext.keytarModule as KeytarInterface, 'deletePassword') + .resolves(); + const secretStorageDeleteSpy = sandbox.spy( + testStorageController, + 'deleteSecret' + ); + + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + const [connection] = testConnectionController.getSavedConnections(); + await testConnectionController.removeSavedConnection(connection.id); + assert.strictEqual(keytarDeleteSpy.calledOnce, true); + assert.strictEqual(secretStorageDeleteSpy.calledOnce, true); + }); + + test('when a connection is removed, should be able to remove the secrets safely from SecretStorage even if keytar is not available', async () => { + sandbox.replace(ext, 'keytarModule', null as any); + const secretStorageDeleteSpy = sandbox.spy( + testStorageController, + 'deleteSecret' + ); + + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + const [connection] = testConnectionController.getSavedConnections(); + await testConnectionController.removeSavedConnection(connection.id); + assert.strictEqual(secretStorageDeleteSpy.calledOnce, true); + }); + + test('when a connection is removed, should be able to remove the secrets safely from SecretStorage even if keytar.deletePassword rejects', async () => { + const keytarDeleteSpy = sandbox + .stub(ext.keytarModule as KeytarInterface, 'deletePassword') + .rejects(); + const secretStorageDeleteSpy = sandbox.spy( + testStorageController, + 'deleteSecret' + ); + + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + const [connection] = testConnectionController.getSavedConnections(); + await assert.doesNotReject( + testConnectionController.removeSavedConnection(connection.id) + ); + assert.strictEqual(keytarDeleteSpy.calledOnce, true); + assert.strictEqual(secretStorageDeleteSpy.calledOnce, true); + }); + test('a saved to workspace connection can be renamed and loaded', async () => { await testConnectionController.loadSavedConnections(); await vscode.workspace From 4250dd6a6a984fff425c50701aa8ea67c4c4286e Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Wed, 5 Jul 2023 19:31:30 +0200 Subject: [PATCH 15/17] ensures that failure of keytar migration do not affect loading of other connections --- src/connectionController.ts | 4 +- src/test/suite/connectionController.test.ts | 105 ++++++++++++++++++++ 2 files changed, 107 insertions(+), 2 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index 927b2082d..9818c8067 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -220,13 +220,13 @@ export default class ConnectionController { ): Promise { try { if (connectionInfo.connectionModel) { - return this._migrateConnectionWithConnectionModel( + return await this._migrateConnectionWithConnectionModel( connectionInfo as StoreConnectionInfoWithConnectionModel ); } if (!connectionInfo.secretStorageLocation) { - return this._migrateConnectionWithKeytarSecrets( + return await this._migrateConnectionWithKeytarSecrets( connectionInfo as StoreConnectionInfoWithConnectionOptions ); } diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index a0e0bcb59..490470e5d 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -1465,6 +1465,111 @@ suite('Connection Controller Test Suite', function () { SecretStorageLocation.Keytar ); }); + + test.only('should be able to load other connections even if the _migrateConnectionWithKeytarSecrets throws', async () => { + // We replace the keytar module with our stub to make sure that later, + // during migration, we are able to find the secrets in the correct + // place + extensionSandbox.replace(ext, 'keytarModule', keytarStub); + + // Adding a few connections + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI + ); + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + const [firstConnection, secondConnection] = + testConnectionController.getSavedConnections(); + + // Clearing the connection and disconnect + await testConnectionController.disconnect(); + testConnectionController.clearAllConnections(); + + // Faking the failure of _migrateConnectionWithKeytarSecrets with an + // error thrown from keytar.getPassword + const originalGetPasswordFn = keytarStub.getPassword.bind(keytarStub); + testSandbox.replace(keytarStub, 'getPassword', (serviceName, key) => { + if (key === secondConnection.id) { + throw new Error('Something bad happened'); + } + return originalGetPasswordFn(serviceName, key); + }); + + // Now load all connections, it should not reject + await assert.doesNotReject( + testConnectionController.loadSavedConnections() + ); + + assert.deepStrictEqual(testConnectionController._connections, { + [firstConnection.id]: { + ...firstConnection, + secretStorageLocation: SecretStorageLocation.SecretStorage, + }, + }); + }); + + test.only('should be able to re-attempt migration for connections that failed in previous load and were not marked migrated', async () => { + // We replace the keytar module with our stub to make sure that later, + // during migration, we are able to find the secrets in the correct + // place + extensionSandbox.replace(ext, 'keytarModule', keytarStub); + + // Add a few connections + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI + ); + await testConnectionController.addNewConnectionStringAndConnect( + TEST_DATABASE_URI_USER + ); + + const [firstConnection, secondConnection] = + testConnectionController.getSavedConnections(); + + // Clear all connections and disconnect + await testConnectionController.disconnect(); + testConnectionController.clearAllConnections(); + + // Faking the failure of _migrateConnectionWithKeytarSecrets with an + // error thrown from keytar.getPassword + testSandbox.replace(keytarStub, 'getPassword', (service, key) => { + if (key === firstConnection.id) { + return Promise.resolve('{}'); + } + throw new Error('Something bad happened'); + }); + + // Now load all connections + await testConnectionController.loadSavedConnections(); + + // And only first connection should appear in our connection list + // because we don't include connections with failed keytar migration + assert.deepStrictEqual(testConnectionController._connections, { + [firstConnection.id]: { + ...firstConnection, + secretStorageLocation: SecretStorageLocation.SecretStorage, + }, + }); + + // Now reset the keytar method to original + testSandbox.restore(); + + // Load all connections again + await testConnectionController.loadSavedConnections(); + + // Now we should be able to see the migrated connection + assert.deepStrictEqual(testConnectionController._connections, { + [firstConnection.id]: { + ...firstConnection, + secretStorageLocation: SecretStorageLocation.SecretStorage, + }, + [secondConnection.id]: { + ...secondConnection, + secretStorageLocation: SecretStorageLocation.SecretStorage, + }, + }); + }); } ); From ee19bb111a8d0e68b1e946836819f603554bd277 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Wed, 5 Jul 2023 19:34:00 +0200 Subject: [PATCH 16/17] lint fixes --- src/test/suite/connectionController.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index 490470e5d..5304d66f9 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -1466,7 +1466,7 @@ suite('Connection Controller Test Suite', function () { ); }); - test.only('should be able to load other connections even if the _migrateConnectionWithKeytarSecrets throws', async () => { + test('should be able to load other connections even if the _migrateConnectionWithKeytarSecrets throws', async () => { // We replace the keytar module with our stub to make sure that later, // during migration, we are able to find the secrets in the correct // place @@ -1510,7 +1510,7 @@ suite('Connection Controller Test Suite', function () { }); }); - test.only('should be able to re-attempt migration for connections that failed in previous load and were not marked migrated', async () => { + test('should be able to re-attempt migration for connections that failed in previous load and were not marked migrated', async () => { // We replace the keytar module with our stub to make sure that later, // during migration, we are able to find the secrets in the correct // place From 8cca2be3b91799ad778824a4c4da5e4e192b0da7 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Wed, 5 Jul 2023 20:15:48 +0200 Subject: [PATCH 17/17] PR feedback - updated the copy of the message for failed migration --- src/connectionController.ts | 18 ++++++++++++------ src/test/suite/connectionController.test.ts | 8 ++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index 9818c8067..d2e57a3a0 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -90,6 +90,17 @@ type FailedMigrationConnectionDescriptor = { connectionName: string; }; +export const keytarMigrationFailedMessage = (failedConnections: number) => { + // Writing the message this way to keep it readable. + return [ + `Unable to migrate ${failedConnections} of your cluster connections from the deprecated Keytar to the VS Code SecretStorage.`, + 'Keytar is officially archived and not being maintained.', + 'In an effort to promote good security practices by not depending on an archived piece of software, VS Code removes Keytar shim in favour of built-in storage utility for secrets.', + 'Please review your connections and delete or recreate those with missing secrets.', + 'You can still access your secrets via OS keychain.', + ].join(' '); +}; + export default class ConnectionController { // This is a map of connection ids to their configurations. // These connections can be saved on the session (runtime), @@ -205,12 +216,7 @@ export default class ConnectionController { } ); void vscode.window.showInformationMessage( - [ - `Could not migrate secrets for ${connectionsThatDidNotMigrate.length} connections. Please review the following connections:`, - connectionsThatDidNotMigrate - .map(({ connectionName }) => connectionName) - .join(', '), - ].join('\n') + keytarMigrationFailedMessage(connectionsThatDidNotMigrate.length) ); } } diff --git a/src/test/suite/connectionController.test.ts b/src/test/suite/connectionController.test.ts index 5304d66f9..c8ebd42d9 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -9,6 +9,7 @@ import { connect } from 'mongodb-data-service'; import AUTH_STRATEGY_VALUES from '../../views/webview-app/connection-model/constants/auth-strategies'; import ConnectionController, { DataServiceEventTypes, + keytarMigrationFailedMessage, } from '../../connectionController'; import formatError from '../../utils/formatError'; import { StorageController, StorageVariables } from '../../storage'; @@ -1682,16 +1683,11 @@ suite('Connection Controller Test Suite', function () { // Clear any connections and load so we get our stubbed connections from above. testConnectionController.clearAllConnections(); await testConnectionController.loadSavedConnections(); - const [, secondConnection] = - testConnectionController.getSavedConnections(); // Notified to user assert.strictEqual(showInformationMessageStub.calledOnce, true); assert.deepStrictEqual(showInformationMessageStub.lastCall.args, [ - [ - 'Could not migrate secrets for 1 connections. Please review the following connections:', - secondConnection.name, - ].join('\n'), + keytarMigrationFailedMessage(1), ]); // Tracked