From c2b23a1b68a868dba7a8f44946e502d505395463 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Thu, 6 Jul 2023 13:40:48 +0200 Subject: [PATCH] chore: added migration step to migrate keytar secrets to vscode SecretStorage - VSCODE-435 (#552) --- src/connectionController.ts | 363 ++++++---- src/storage/storageController.ts | 31 +- src/telemetry/telemetryService.ts | 9 +- src/test/suite/connectionController.test.ts | 620 +++++++++++++++++- .../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, 891 insertions(+), 159 deletions(-) diff --git a/src/connectionController.ts b/src/connectionController.ts index 2dae8e855..0da160909 100644 --- a/src/connectionController.ts +++ b/src/connectionController.ts @@ -4,7 +4,6 @@ import { ConnectionInfo, ConnectionOptions, getConnectionTitle, - ConnectionSecrets, extractSecrets, mergeSecrets, connect, @@ -22,11 +21,14 @@ import formatError from './utils/formatError'; import LegacyConnectionModel from './views/webview-app/connection-model/legacy-connection-model'; import { StorageLocation, - ConnectionsFromStorage, + SecretStorageLocationType, + 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 +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?: SecretStorageLocationType; connectionOptions?: ConnectionOptions; connectionModel?: LegacyConnectionModel; } @@ -69,21 +72,42 @@ interface ConnectionQuickPicks { data: { type: NewConnectionType; connectionId?: string }; } -interface ConnectionSecretsInfo { - connectionId: string; - secrets: ConnectionSecrets; -} +type StoreConnectionInfoWithConnectionModel = StoreConnectionInfo & + Required>; type StoreConnectionInfoWithConnectionOptions = StoreConnectionInfo & Required>; +type StoreConnectionInfoWithSecretStorageLocation = StoreConnectionInfo & + Required>; + +type MigratedStoreConnectionInfoWithConnectionOptions = + StoreConnectionInfoWithConnectionOptions & + StoreConnectionInfoWithSecretStorageLocation; + +type FailedMigrationConnectionDescriptor = { + connectionId: string; + 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), // on the workspace, or globally in vscode. _connections: { - [connectionId: string]: StoreConnectionInfoWithConnectionOptions; - } = {}; + [connectionId: string]: MigratedStoreConnectionInfoWithConnectionOptions; + } = Object.create(null); _activeDataService: DataService | null = null; _storageController: StorageController; @@ -120,142 +144,201 @@ 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 = Object.entries({ + ...this._storageController.get( + StorageVariables.GLOBAL_SAVED_CONNECTIONS, + StorageLocation.GLOBAL + ), + ...this._storageController.get( + StorageVariables.WORKSPACE_SAVED_CONNECTIONS, + StorageLocation.WORKSPACE + ), + }); + + // 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; + }, + [] ); + + // 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 (connectionsThatDidNotMigrate.length) { + log.error( + `Could not migrate secrets for ${connectionsThatDidNotMigrate.length} connections`, + connectionsThatDidNotMigrate + ); + this._telemetryService.track( + TelemetryEventTypes.KEYTAR_SECRETS_MIGRATION_FAILED, + { + totalConnections: globalAndWorkspaceConnections.length, + connectionsWithFailedKeytarMigration: + connectionsThatDidNotMigrate.length, + } + ); + void vscode.window.showInformationMessage( + keytarMigrationFailedMessage(connectionsThatDidNotMigrate.length) + ); + } + } + + async _getConnectionInfoWithSecrets( + connectionInfo: StoreConnectionInfo + ): Promise { + try { + if (connectionInfo.connectionModel) { + return await this._migrateConnectionWithConnectionModel( + connectionInfo as StoreConnectionInfoWithConnectionModel + ); + } + + if (!connectionInfo.secretStorageLocation) { + return await this._migrateConnectionWithKeytarSecrets( + 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 _migrateConnectionWithConnectionModel( + 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 connectionInfoWithSecrets = { id: savedConnectionInfo.id, name: savedConnectionInfo.name, storageLocation: savedConnectionInfo.storageLocation, + secretStorageLocation: SecretStorageLocation.SecretStorage, connectionOptions: newConnectionInfoWithSecrets.connectionOptions, }; - await this._saveConnection(newSavedConnectionInfoWithSecrets); - - return newSavedConnectionInfoWithSecrets; + await this._saveConnectionWithSecrets(connectionInfoWithSecrets); + return connectionInfoWithSecrets; } - 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 _migrateConnectionWithKeytarSecrets( + 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._saveConnectionWithSecrets(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, + }, + secrets + ); + + return { + ...connectionInfo, + connectionOptions: connectionInfoWithSecrets.connectionOptions, + }; } async connectWithURI(): Promise { @@ -357,26 +440,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 _saveConnectionWithSecrets( + newStoreConnectionInfoWithSecrets: MigratedStoreConnectionInfoWithConnectionOptions + ): Promise { // We don't want to store secrets to disc. const { connectionInfo: safeConnectionInfo, secrets } = extractSecrets( newStoreConnectionInfoWithSecrets as ConnectionInfo @@ -385,11 +451,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; } @@ -405,10 +470,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._saveConnectionWithSecrets( + newConnectionInfo + ); this._connections[savedConnectionInfo.id] = { ...savedConnectionInfo, @@ -600,8 +668,15 @@ export default class ConnectionController { } private async _removeSecretsFromKeychain(connectionId: string) { - if (ext.keytarModule) { - await ext.keytarModule.deletePassword(this._serviceName, connectionId); + 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 + // has been out for some time. + try { + await ext.keytarModule?.deletePassword(this._serviceName, connectionId); + } catch (error) { + log.error('Failed to remove secret from keytar', error); } } diff --git a/src/storage/storageController.ts b/src/storage/storageController.ts index 2c9583e9c..6467e3648 100644 --- a/src/storage/storageController.ts +++ b/src/storage/storageController.ts @@ -32,6 +32,15 @@ export type ConnectionsFromStorage = { [connectionId: string]: StoreConnectionInfo; }; +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; [StorageVariables.GLOBAL_ANONYMOUS_ID]: string; @@ -48,11 +57,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 +135,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 +255,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..e774d6308 100644 --- a/src/telemetry/telemetryService.ts +++ b/src/telemetry/telemetryService.ts @@ -68,6 +68,11 @@ type PlaygroundLoadedTelemetryEventProperties = { file_type?: string; }; +type KeytarSecretsMigrationFailedProperties = { + totalConnections: number; + connectionsWithFailedKeytarMigration: 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 073c4fc91..2ce61cac4 100644 --- a/src/test/suite/connectionController.test.ts +++ b/src/test/suite/connectionController.test.ts @@ -9,12 +9,14 @@ 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'; 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 +30,9 @@ import { TEST_USER_USERNAME, TEST_USER_PASSWORD, } from './dbTestHelper'; +import KeytarStub from './keytarStub'; +import { ext } from '../../extensionConstants'; +import { KeytarInterface } from '../../utils/keytar'; const testDatabaseConnectionName = 'localhost:27018'; const testDatabaseURI2WithTimeout = @@ -52,10 +57,14 @@ suite('Connection Controller Test Suite', function () { telemetryService: 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 +363,7 @@ suite('Connection Controller Test Suite', function () { name: 'tester', connectionOptions: { connectionString: TEST_DATABASE_URI }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }, }; @@ -521,6 +531,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 @@ -684,6 +750,7 @@ suite('Connection Controller Test Suite', function () { name: `test${i}`, connectionOptions: { connectionString: TEST_DATABASE_URI }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; } @@ -733,6 +800,7 @@ suite('Connection Controller Test Suite', function () { name: 'asdfasdg', connectionOptions: { connectionString: testDatabaseURI2WithTimeout }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; void testConnectionController.connectWithConnectionId(connectionId); @@ -757,6 +825,7 @@ suite('Connection Controller Test Suite', function () { name: 'asdfasdg', connectionOptions: { connectionString: TEST_DATABASE_URI }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; sandbox.replace( @@ -786,7 +855,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('_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', @@ -810,7 +879,7 @@ suite('Connection Controller Test Suite', function () { }, }; const newSavedConnectionInfoWithSecrets = - await testConnectionController._migratePreviouslySavedConnection( + await testConnectionController._migrateConnectionWithConnectionModel( oldSavedConnectionInfo ); @@ -818,6 +887,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', @@ -825,7 +895,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('_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', @@ -862,7 +932,7 @@ suite('Connection Controller Test Suite', function () { }; const newSavedConnectionInfoWithSecrets = - await testConnectionController._migratePreviouslySavedConnection( + await testConnectionController._migrateConnectionWithConnectionModel( oldSavedConnectionInfo ); @@ -870,6 +940,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', @@ -877,7 +948,7 @@ suite('Connection Controller Test Suite', function () { }); }); - test('_migratePreviouslySavedConnection 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', @@ -922,7 +993,7 @@ suite('Connection Controller Test Suite', function () { fakeSaveConnection ); - await testConnectionController._migratePreviouslySavedConnection( + await testConnectionController._migrateConnectionWithConnectionModel( oldSavedConnectionInfo ); @@ -960,6 +1031,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', @@ -968,7 +1040,7 @@ suite('Connection Controller Test Suite', function () { sandbox.replace( testConnectionController, - '_migratePreviouslySavedConnection', + '_migrateConnectionWithConnectionModel', fakeMigratePreviouslySavedConnection ); @@ -984,6 +1056,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', @@ -1002,7 +1075,7 @@ suite('Connection Controller Test Suite', function () { sandbox.replace( testConnectionController, - '_migratePreviouslySavedConnection', + '_migrateConnectionWithConnectionModel', fakeMigratePreviouslySavedConnection ); @@ -1150,4 +1223,533 @@ 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.strictEqual( + 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.strictEqual( + 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.strictEqual( + updatedConnection.secretStorageLocation, + SecretStorageLocation.Keytar + ); + }); + + 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 + 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('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, + }, + }); + }); + } + ); + + 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.strictEqual( + savedConnections.every( + ({ secretStorageLocation }) => + secretStorageLocation === SecretStorageLocation.SecretStorage + ), + true + ); + + 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.strictEqual( + savedConnectionsAfterFreshLoad[1].connectionOptions?.connectionString.includes( + TEST_USER_PASSWORD + ), + true + ); + }); + }); + + 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.strictEqual(isConnectionChanged, true); + }); + + 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({ + ...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(); + + // Notified to user + assert.strictEqual(showInformationMessageStub.calledOnce, true); + assert.deepStrictEqual(showInformationMessageStub.lastCall.args, [ + keytarMigrationFailedMessage(1), + ]); + + // Tracked + assert.strictEqual(trackStub.calledOnce, true); + assert.deepStrictEqual(trackStub.lastCall.args, [ + 'Keytar Secrets Migration Failed', + { 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); + }); + }); }); diff --git a/src/test/suite/editors/collectionDocumentsProvider.test.ts b/src/test/suite/editors/collectionDocumentsProvider.test.ts index 755f88d5b..a853526b0 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'; @@ -403,12 +406,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 1b958cbe7..71fc9e14a 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'; @@ -839,6 +840,7 @@ suite('MDBExtensionController Test Suite', function () { connectionOptions: { connectionString: 'mongodb://localhost' }, name: 'NAAAME', storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; const testTreeItem = getTestConnectionTreeItem({ @@ -869,6 +871,7 @@ suite('MDBExtensionController Test Suite', function () { name: 'NAAAME', connectionOptions: { connectionString: 'mongodb://localhost' }, storageLocation: StorageLocation.NONE, + secretStorageLocation: SecretStorageLocation.SecretStorage, }; const testTreeItem = getTestConnectionTreeItem({ 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[] => {