From f08637381c617a66a9b2f6daf3661c37f31cac5d Mon Sep 17 00:00:00 2001 From: Basit Date: Wed, 15 Nov 2023 11:17:00 +0300 Subject: [PATCH 01/10] migrate to safeStorage --- packages/compass/src/main/application.ts | 2 + .../src/connection-storage.spec.ts | 292 +++++++++++++++--- .../src/connection-storage.ts | 171 ++++++++-- 3 files changed, 403 insertions(+), 62 deletions(-) diff --git a/packages/compass/src/main/application.ts b/packages/compass/src/main/application.ts index a15a1195d6d..d6fb8453b1b 100644 --- a/packages/compass/src/main/application.ts +++ b/packages/compass/src/main/application.ts @@ -86,6 +86,8 @@ class CompassApplication { // ConnectionStorage offers import/export which is used via CLI as well. ConnectionStorage.init(); + await ConnectionStorage.migrateToSafeStorage(); + if (mode === 'CLI') { return; } diff --git a/packages/connection-storage/src/connection-storage.spec.ts b/packages/connection-storage/src/connection-storage.spec.ts index 2d64f9f178c..e9b92c78485 100644 --- a/packages/connection-storage/src/connection-storage.spec.ts +++ b/packages/connection-storage/src/connection-storage.spec.ts @@ -6,7 +6,10 @@ import os from 'os'; import { UUID } from 'bson'; import { sortBy } from 'lodash'; -import { ConnectionStorage } from './connection-storage'; +import { + ConnectionStorage, + type ConnectionWithLegacyProps, +} from './connection-storage'; import type { ConnectionInfo } from './connection-info'; import Sinon from 'sinon'; @@ -33,14 +36,23 @@ function getConnectionInfo(props: Partial = {}) { async function writeFakeConnection( tmpDir: string, - connection: { connectionInfo: ConnectionInfo } + connection: ConnectionWithLegacyProps ) { - const filePath = getConnectionFilePath(tmpDir, connection.connectionInfo.id); + const _id = connection._id || connection.connectionInfo?.id; + if (!_id) { + throw new Error('Connection should have _id or connectionInfo.id'); + } + const filePath = getConnectionFilePath(tmpDir, _id); const connectionsDir = path.dirname(filePath); await fs.mkdir(connectionsDir, { recursive: true }); await fs.writeFile(filePath, JSON.stringify(connection)); } +async function readConnection(tmpDir: string, id: string) { + const content = await fs.readFile(getConnectionFilePath(tmpDir, id), 'utf-8'); + return JSON.parse(content); +} + const maxAllowedConnections = 10; describe('ConnectionStorage', function () { @@ -75,6 +87,239 @@ describe('ConnectionStorage', function () { } }); + describe('migrateToSafeStorage', function () { + context('does not migrate connections', function () { + it('when there are no connections', async function () { + await ConnectionStorage.migrateToSafeStorage(); + const connections = await ConnectionStorage.loadAll(); + expect(connections).to.deep.equal([]); + }); + + it('when there are only legacy connections', async function () { + await writeFakeConnection(tmpDir, connection1270); + + const encryptSecretsSpy = Sinon.spy( + ConnectionStorage, + 'encryptSecrets' + ); + const getKeytarCredentialsSpy = Sinon.spy( + ConnectionStorage, + 'getKeytarCredentials' + ); + + await ConnectionStorage.migrateToSafeStorage(); + + expect( + encryptSecretsSpy.called, + 'it does not try to encrypt' + ).to.be.false; + expect( + getKeytarCredentialsSpy.called, + 'it does not try to get secrets from keychain' + ).to.be.false; + }); + + it('when there are only migrated connections', async function () { + const connectionInfo1 = getConnectionInfo(); + const connectionInfo2 = getConnectionInfo(); + + await writeFakeConnection(tmpDir, { + _id: connectionInfo1.id, + connectionInfo: connectionInfo1, + version: 1, + }); + await writeFakeConnection(tmpDir, { + _id: connectionInfo2.id, + connectionInfo: connectionInfo2, + version: 1, + }); + + const encryptSecretsSpy = Sinon.spy( + ConnectionStorage, + 'encryptSecrets' + ); + const getKeytarCredentialsSpy = Sinon.spy( + ConnectionStorage, + 'getKeytarCredentials' + ); + + await ConnectionStorage.migrateToSafeStorage(); + + expect( + encryptSecretsSpy.called, + 'it does not try to encrypt' + ).to.be.false; + expect( + getKeytarCredentialsSpy.called, + 'it does not try to get secrets from keychain' + ).to.be.false; + + const expectedConnection1 = await readConnection( + tmpDir, + connectionInfo1.id + ); + const expectedConnection2 = await readConnection( + tmpDir, + connectionInfo2.id + ); + + expect(expectedConnection1).to.deep.equal({ + _id: connectionInfo1.id, + connectionInfo: connectionInfo1, + version: 1, + }); + + expect(expectedConnection2).to.deep.equal({ + _id: connectionInfo2.id, + connectionInfo: connectionInfo2, + version: 1, + }); + }); + }); + + context('migrates connections', function () { + it('when there are connections and secrets in keychain', async function () { + const connectionInfo1 = getConnectionInfo(); + const connectionInfo2 = getConnectionInfo(); + await writeFakeConnection(tmpDir, { + connectionInfo: connectionInfo1, + }); + await writeFakeConnection(tmpDir, { + connectionInfo: connectionInfo2, + }); + + // Keytar fake + Sinon.replace( + ConnectionStorage, + 'getKeytarCredentials', + Sinon.fake.returns({ + [connectionInfo1.id]: { + password: 'password1', + }, + [connectionInfo2.id]: { + password: 'password2', + }, + }) + ); + + // safeStorage.encryptString fake + Sinon.replace( + ConnectionStorage, + 'encryptSecrets', + Sinon.fake.returns('encrypted-password') + ); + + await ConnectionStorage.migrateToSafeStorage(); + + const expectedConnection1 = await readConnection( + tmpDir, + connectionInfo1.id + ); + const expectedConnection2 = await readConnection( + tmpDir, + connectionInfo2.id + ); + + expect(expectedConnection1).to.deep.equal({ + _id: connectionInfo1.id, + connectionInfo: connectionInfo1, + connectionSecrets: 'encrypted-password', + version: 1, + }); + expect(expectedConnection2).to.deep.equal({ + _id: connectionInfo2.id, + connectionInfo: connectionInfo2, + connectionSecrets: 'encrypted-password', + version: 1, + }); + }); + it('when there are connections and no secrets in keychain', async function () { + const connectionInfo1 = getConnectionInfo(); + const connectionInfo2 = getConnectionInfo(); + await writeFakeConnection(tmpDir, { + connectionInfo: connectionInfo1, + }); + await writeFakeConnection(tmpDir, { + connectionInfo: connectionInfo2, + }); + + // Keytar fake + Sinon.replace( + ConnectionStorage, + 'getKeytarCredentials', + Sinon.fake.returns({}) + ); + + // Since there're no secrets in keychain, we do not expect to call safeStorage.encryptString + // and connection.connectionSecrets should be undefined + + await ConnectionStorage.migrateToSafeStorage(); + + const expectedConnection1 = await readConnection( + tmpDir, + connectionInfo1.id + ); + const expectedConnection2 = await readConnection( + tmpDir, + connectionInfo2.id + ); + + expect(expectedConnection1).to.deep.equal({ + _id: connectionInfo1.id, + connectionInfo: connectionInfo1, + version: 1, + }); + expect(expectedConnection2).to.deep.equal({ + _id: connectionInfo2.id, + connectionInfo: connectionInfo2, + version: 1, + }); + }); + + it('when there are any unmigrated connections', async function () { + const connectionInfo1 = getConnectionInfo(); + const connectionInfo2 = getConnectionInfo(); + await writeFakeConnection(tmpDir, { + _id: connectionInfo1.id, + connectionInfo: connectionInfo1, + version: 1, + }); + await writeFakeConnection(tmpDir, { + connectionInfo: connectionInfo2, + }); + + // Keytar fake + Sinon.replace( + ConnectionStorage, + 'getKeytarCredentials', + Sinon.fake.returns({}) + ); + + await ConnectionStorage.migrateToSafeStorage(); + + const expectedConnection1 = await readConnection( + tmpDir, + connectionInfo1.id + ); + const expectedConnection2 = await readConnection( + tmpDir, + connectionInfo2.id + ); + + expect(expectedConnection1).to.deep.equal({ + _id: connectionInfo1.id, + connectionInfo: connectionInfo1, + version: 1, + }); + expect(expectedConnection2).to.deep.equal({ + _id: connectionInfo2.id, + connectionInfo: connectionInfo2, + version: 1, + }); + }); + }); + }); + describe('loadAll', function () { it('should load an empty array with no connections', async function () { const connections = await ConnectionStorage.loadAll(); @@ -463,27 +708,12 @@ describe('ConnectionStorage', function () { }); it('returns true if there are favorite legacy connections', async function () { - const _id = new UUID().toString(); - - // Save a legacy connection (connection without connectionInfo) - const filePath = getConnectionFilePath(tmpDir, _id); - // As we are saving a legacy connection here, we can not use Storage.save as - // it requries connectionInfo. And the internals of fs ensure the subdir exists, - // here we are creating it manually. - await fs.mkdir(path.join(tmpDir, 'Connections'), { recursive: true }); - await fs.writeFile( - filePath, - JSON.stringify({ - _id, - isFavorite: true, - name: 'Local 1', - hosts: [{ host: 'localhost', port: 27017 }], - readPreference: 'primary', - port: 27017, - hostname: 'localhost', - }) - ); - + // Write a legacy connection (connection without connectionInfo, which is favorite and has a name) + await writeFakeConnection(tmpDir, { + _id: new UUID().toString(), + isFavorite: true, + name: 'Local 1', + }); const getLegacyConnections = await ConnectionStorage.getLegacyConnections(); expect(getLegacyConnections).to.deep.equal([{ name: 'Local 1' }]); @@ -583,18 +813,8 @@ describe('ConnectionStorage', function () { }); describe('supports connections from older version of compass', function () { - const storeConnection = async (connection: any) => { - const connectionFolder = path.join(tmpDir, 'Connections'); - const connectionPath = path.join( - connectionFolder, - `${connection._id}.json` - ); - await fs.mkdir(connectionFolder, { recursive: true }); - await fs.writeFile(connectionPath, JSON.stringify(connection)); - }; - it('correctly identifies connection as legacy connection', async function () { - await storeConnection(connection1270); + await writeFakeConnection(tmpDir, connection1270); const expectedConnection = await ConnectionStorage.load({ id: connection1270._id, }); @@ -612,7 +832,7 @@ describe('ConnectionStorage', function () { for (const version in connections) { const connection = connections[version]; - await storeConnection(connection); + await writeFakeConnection(tmpDir, connection); const expectedConnection = await ConnectionStorage.load({ id: connection._id, }); diff --git a/packages/connection-storage/src/connection-storage.ts b/packages/connection-storage/src/connection-storage.ts index af38b1a664e..267a3a62d66 100644 --- a/packages/connection-storage/src/connection-storage.ts +++ b/packages/connection-storage/src/connection-storage.ts @@ -1,6 +1,7 @@ import type { HadronIpcMain } from 'hadron-ipc'; import { ipcMain } from 'hadron-ipc'; import keytar from 'keytar'; +import { safeStorage } from 'electron'; import type { ConnectionInfo } from './connection-info'; import { createLoggerAndTelemetry } from '@mongodb-js/compass-logging'; @@ -25,7 +26,8 @@ import type { } from './import-export-connection'; import { UserData, z } from '@mongodb-js/compass-user-data'; -const { log, mongoLogId } = createLoggerAndTelemetry('CONNECTION-STORAGE'); +const { log, mongoLogId, track } = + createLoggerAndTelemetry('CONNECTION-STORAGE'); type ConnectionLegacyProps = { _id?: string; @@ -33,9 +35,13 @@ type ConnectionLegacyProps = { name?: string; }; -type ConnectionWithLegacyProps = { +type ConnectionProps = { connectionInfo?: ConnectionInfo; -} & ConnectionLegacyProps; + version?: number; + connectionSecrets?: string; +}; + +export type ConnectionWithLegacyProps = ConnectionProps & ConnectionLegacyProps; const ConnectionSchema: z.Schema = z .object({ @@ -61,6 +67,8 @@ const ConnectionSchema: z.Schema = z }), }) .optional(), + version: z.number().optional(), + connectionSecrets: z.string().optional(), }) .passthrough(); @@ -71,6 +79,7 @@ export class ConnectionStorage { | undefined = ipcMain; private static readonly maxAllowedRecentConnections = 10; + private static readonly version = 1; private static userData: UserData; private constructor() { @@ -98,11 +107,128 @@ export class ConnectionStorage { this.calledOnce = true; } - private static mapStoredConnectionToConnectionInfo( - storedConnectionInfo: ConnectionInfo, + static async migrateToSafeStorage() { + // If user lands on this version of Compass with legacy connections (using storage mixin), + // we will ignore those and only migrate connections that have connectionInfo. + const connections = (await this.getConnections()).filter( + (x) => x.connectionInfo + ); + if (connections.length === 0) { + log.info( + mongoLogId(1_001_000_200), + 'Connection Storage', + 'No connections to migrate' + ); + return; + } + + if ( + connections.filter((x) => 'version' in x && x.version === this.version) + .length === connections.length + ) { + log.info( + mongoLogId(1_001_000_205), + 'Connection Storage', + 'Connections already migrated' + ); + return; + } + + // Get the secrets from keychain. If there're no secrets, we still want to + // migrate the connects to a new version. + const secrets = await this.getKeytarCredentials(); + + // Connections that are not migrated yet or that failed to migrate, + // are not expected to have a version property. + const unmigratedConnections = connections.filter((x) => !('version' in x)); + log.info( + mongoLogId(1_001_000_206), + 'Connection Storage', + 'Migrating connections', + { + num_connections: unmigratedConnections.length, + num_secrets: Object.keys(secrets).length, + } + ); + + let numFailedToMigrate = 0; + + for (const { connectionInfo } of unmigratedConnections) { + // We have connectionInfo as we are limiting this migration to + // connections that have connectionInfo (not legacy connections) + const _id = connectionInfo!.id; + const connectionSecrets = secrets[_id]; + + try { + await this.userData.write(_id, { + _id, + connectionInfo, // connectionInfo is without secrets as its direclty read from storage + connectionSecrets: this.encryptSecrets(connectionSecrets), + version: this.version, + }); + } catch (e) { + numFailedToMigrate++; + log.error( + mongoLogId(1_001_000_207), + 'Connection Storage', + 'Failed to migrate connection', + { message: (e as Error).message, connectionId: _id } + ); + } + } + + log.info( + mongoLogId(1_001_000_267), + 'Connection Storage', + 'Migration complete', + { + num_saved_connections: + unmigratedConnections.length - numFailedToMigrate, + num_failed_connections: numFailedToMigrate, + } + ); + + if (numFailedToMigrate > 0) { + track('Keytar Secrets Migration Failed', { + num_saved_connections: + unmigratedConnections.length - numFailedToMigrate, + num_failed_connections: numFailedToMigrate, + }); + } + } + + private static encryptSecrets( secrets?: ConnectionSecrets - ): ConnectionInfo { - const connectionInfo = mergeSecrets(storedConnectionInfo, secrets); + ): string | undefined { + return Object.keys(secrets ?? {}).length > 0 + ? safeStorage.encryptString(JSON.stringify(secrets)).toString('base64') + : undefined; + } + + private static decryptSecrets( + secrets?: string + ): ConnectionSecrets | undefined { + return secrets + ? JSON.parse(safeStorage.decryptString(Buffer.from(secrets, 'base64'))) + : undefined; + } + + private static mapStoredConnectionToConnectionInfo({ + connectionInfo: storedConnectionInfo, + connectionSecrets: storedConnectionSecrets, + }: ConnectionProps): ConnectionInfo { + let secrets: ConnectionSecrets | undefined = undefined; + try { + secrets = this.decryptSecrets(storedConnectionSecrets); + } catch (e) { + log.error( + mongoLogId(1_001_000_208), + 'Connection Storage', + 'Failed to decrypt secrets', + { message: (e as Error).message } + ); + } + const connectionInfo = mergeSecrets(storedConnectionInfo!, secrets); return deleteCompassAppNameParam(connectionInfo); } @@ -165,19 +291,13 @@ export class ConnectionStorage { } = {}): Promise { throwIfAborted(signal); try { - const [connections, secrets] = await Promise.all([ - this.getConnections(), - this.getKeytarCredentials(), - ]); + const connections = await this.getConnections(); return ( connections // Ignore legacy connections and make sure connection has a connection string. .filter((x) => x.connectionInfo?.connectionOptions?.connectionString) - .map(({ connectionInfo }) => - this.mapStoredConnectionToConnectionInfo( - connectionInfo!, - secrets[connectionInfo!.id] - ) + .map((connection) => + this.mapStoredConnectionToConnectionInfo(connection) ) ); } catch (err) { @@ -214,25 +334,24 @@ export class ConnectionStorage { } else { const { secrets, connectionInfo: connectionInfoWithoutSecrets } = extractSecrets(connectionInfo); - await this.userData.write(connectionInfo.id, { - connectionInfo: connectionInfoWithoutSecrets, - _id: connectionInfo.id, - }); + let connectionSecrets: string | undefined = undefined; try { - await keytar.setPassword( - getKeytarServiceName(), - connectionInfo.id, - JSON.stringify({ secrets }, null, 2) - ); + connectionSecrets = this.encryptSecrets(secrets); } catch (e) { log.error( mongoLogId(1_001_000_202), 'Connection Storage', - 'Failed to save secrets to keychain', + 'Failed to encrypt secrets', { message: (e as Error).message } ); } + await this.userData.write(connectionInfo.id, { + _id: connectionInfo.id, + connectionInfo: connectionInfoWithoutSecrets, + connectionSecrets, + version: this.version, + }); } await this.afterConnectionHasBeenSaved(connectionInfo); } catch (err) { From b05fc4dc95c72725d309edefea95b0552681b923 Mon Sep 17 00:00:00 2001 From: Basit Date: Wed, 15 Nov 2023 13:13:51 +0300 Subject: [PATCH 02/10] clean up --- .../src/connection-storage.spec.ts | 75 +++++++++---------- 1 file changed, 37 insertions(+), 38 deletions(-) diff --git a/packages/connection-storage/src/connection-storage.spec.ts b/packages/connection-storage/src/connection-storage.spec.ts index e9b92c78485..84b047f416b 100644 --- a/packages/connection-storage/src/connection-storage.spec.ts +++ b/packages/connection-storage/src/connection-storage.spec.ts @@ -88,6 +88,15 @@ describe('ConnectionStorage', function () { }); describe('migrateToSafeStorage', function () { + let sandbox: Sinon.SinonSandbox; + beforeEach(function () { + sandbox = Sinon.createSandbox(); + }); + + afterEach(function () { + sandbox.restore(); + }); + context('does not migrate connections', function () { it('when there are no connections', async function () { await ConnectionStorage.migrateToSafeStorage(); @@ -98,13 +107,13 @@ describe('ConnectionStorage', function () { it('when there are only legacy connections', async function () { await writeFakeConnection(tmpDir, connection1270); - const encryptSecretsSpy = Sinon.spy( + const encryptSecretsSpy = sandbox.spy( ConnectionStorage, - 'encryptSecrets' + 'encryptSecrets' ); - const getKeytarCredentialsSpy = Sinon.spy( + const getKeytarCredentialsSpy = sandbox.spy( ConnectionStorage, - 'getKeytarCredentials' + 'getKeytarCredentials' ); await ConnectionStorage.migrateToSafeStorage(); @@ -134,13 +143,13 @@ describe('ConnectionStorage', function () { version: 1, }); - const encryptSecretsSpy = Sinon.spy( + const encryptSecretsSpy = sandbox.spy( ConnectionStorage, - 'encryptSecrets' + 'encryptSecrets' ); - const getKeytarCredentialsSpy = Sinon.spy( + const getKeytarCredentialsSpy = sandbox.spy( ConnectionStorage, - 'getKeytarCredentials' + 'getKeytarCredentials' ); await ConnectionStorage.migrateToSafeStorage(); @@ -188,26 +197,20 @@ describe('ConnectionStorage', function () { connectionInfo: connectionInfo2, }); - // Keytar fake - Sinon.replace( - ConnectionStorage, - 'getKeytarCredentials', - Sinon.fake.returns({ - [connectionInfo1.id]: { - password: 'password1', - }, - [connectionInfo2.id]: { - password: 'password2', - }, - }) - ); + // Keytar stub + sandbox.stub(ConnectionStorage, 'getKeytarCredentials').returns({ + [connectionInfo1.id]: { + password: 'password1', + }, + [connectionInfo2.id]: { + password: 'password2', + }, + }); - // safeStorage.encryptString fake - Sinon.replace( - ConnectionStorage, - 'encryptSecrets', - Sinon.fake.returns('encrypted-password') - ); + // safeStorage.encryptString stub + sandbox + .stub(ConnectionStorage, 'encryptSecrets') + .returns('encrypted-password'); await ConnectionStorage.migrateToSafeStorage(); @@ -244,11 +247,9 @@ describe('ConnectionStorage', function () { }); // Keytar fake - Sinon.replace( - ConnectionStorage, - 'getKeytarCredentials', - Sinon.fake.returns({}) - ); + sandbox + .stub(ConnectionStorage, 'getKeytarCredentials') + .returns({}); // Since there're no secrets in keychain, we do not expect to call safeStorage.encryptString // and connection.connectionSecrets should be undefined @@ -288,12 +289,10 @@ describe('ConnectionStorage', function () { connectionInfo: connectionInfo2, }); - // Keytar fake - Sinon.replace( - ConnectionStorage, - 'getKeytarCredentials', - Sinon.fake.returns({}) - ); + // Keytar stub + sandbox + .stub(ConnectionStorage, 'getKeytarCredentials') + .returns({}); await ConnectionStorage.migrateToSafeStorage(); From 325a7bfe4afd766baabd049b98e4603ba09466ef Mon Sep 17 00:00:00 2001 From: Basit Date: Wed, 15 Nov 2023 14:47:26 +0300 Subject: [PATCH 03/10] npm check --- package-lock.json | 2 ++ packages/connection-storage/package.json | 1 + 2 files changed, 3 insertions(+) diff --git a/package-lock.json b/package-lock.json index c8bac9c8f49..00678c2bf9a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -47607,6 +47607,7 @@ "@mongodb-js/compass-user-data": "^0.1.9", "@mongodb-js/compass-utils": "^0.5.5", "bson": "^6.0.0", + "electron": "^25.9.4", "hadron-ipc": "^3.2.4", "keytar": "^7.9.0", "lodash": "^4.17.21", @@ -60590,6 +60591,7 @@ "bson": "^6.0.0", "chai": "^4.3.6", "depcheck": "^1.4.1", + "electron": "^25.9.4", "eslint": "^7.25.0", "hadron-ipc": "^3.2.4", "keytar": "^7.9.0", diff --git a/packages/connection-storage/package.json b/packages/connection-storage/package.json index c8ae96ebd93..6353e9bf969 100644 --- a/packages/connection-storage/package.json +++ b/packages/connection-storage/package.json @@ -56,6 +56,7 @@ "@mongodb-js/compass-user-data": "^0.1.9", "@mongodb-js/compass-utils": "^0.5.5", "bson": "^6.0.0", + "electron": "^25.9.4", "hadron-ipc": "^3.2.4", "keytar": "^7.9.0", "lodash": "^4.17.21", From 7d055e0ceb031894ffd1eb55dea4fe0225dd9f38 Mon Sep 17 00:00:00 2001 From: Basit Date: Wed, 15 Nov 2023 16:45:31 +0300 Subject: [PATCH 04/10] update mongoLogIds --- packages/connection-storage/src/connection-storage.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/connection-storage/src/connection-storage.ts b/packages/connection-storage/src/connection-storage.ts index 267a3a62d66..efa0c8774ea 100644 --- a/packages/connection-storage/src/connection-storage.ts +++ b/packages/connection-storage/src/connection-storage.ts @@ -115,7 +115,7 @@ export class ConnectionStorage { ); if (connections.length === 0) { log.info( - mongoLogId(1_001_000_200), + mongoLogId(1_001_000_270), 'Connection Storage', 'No connections to migrate' ); @@ -127,7 +127,7 @@ export class ConnectionStorage { .length === connections.length ) { log.info( - mongoLogId(1_001_000_205), + mongoLogId(1_001_000_271), 'Connection Storage', 'Connections already migrated' ); @@ -142,7 +142,7 @@ export class ConnectionStorage { // are not expected to have a version property. const unmigratedConnections = connections.filter((x) => !('version' in x)); log.info( - mongoLogId(1_001_000_206), + mongoLogId(1_001_000_272), 'Connection Storage', 'Migrating connections', { @@ -169,7 +169,7 @@ export class ConnectionStorage { } catch (e) { numFailedToMigrate++; log.error( - mongoLogId(1_001_000_207), + mongoLogId(1_001_000_273), 'Connection Storage', 'Failed to migrate connection', { message: (e as Error).message, connectionId: _id } @@ -178,7 +178,7 @@ export class ConnectionStorage { } log.info( - mongoLogId(1_001_000_267), + mongoLogId(1_001_000_274), 'Connection Storage', 'Migration complete', { From 17c3103353b2cc12fb8fb24bcf43fef839d68102 Mon Sep 17 00:00:00 2001 From: Basit Date: Thu, 16 Nov 2023 09:27:36 +0300 Subject: [PATCH 05/10] assert using ts --- packages/connection-storage/src/connection-storage.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/connection-storage/src/connection-storage.ts b/packages/connection-storage/src/connection-storage.ts index efa0c8774ea..b7b185ffe72 100644 --- a/packages/connection-storage/src/connection-storage.ts +++ b/packages/connection-storage/src/connection-storage.ts @@ -111,7 +111,10 @@ export class ConnectionStorage { // If user lands on this version of Compass with legacy connections (using storage mixin), // we will ignore those and only migrate connections that have connectionInfo. const connections = (await this.getConnections()).filter( - (x) => x.connectionInfo + ( + x + ): x is ConnectionWithLegacyProps & + Required> => !!x.connectionInfo ); if (connections.length === 0) { log.info( @@ -154,9 +157,7 @@ export class ConnectionStorage { let numFailedToMigrate = 0; for (const { connectionInfo } of unmigratedConnections) { - // We have connectionInfo as we are limiting this migration to - // connections that have connectionInfo (not legacy connections) - const _id = connectionInfo!.id; + const _id = connectionInfo.id; const connectionSecrets = secrets[_id]; try { From a081b149f62a88fb2d7b1a18259fdee409701954 Mon Sep 17 00:00:00 2001 From: Basit Date: Thu, 16 Nov 2023 09:28:12 +0300 Subject: [PATCH 06/10] handle rejection when migrating --- packages/compass/src/main/application.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/compass/src/main/application.ts b/packages/compass/src/main/application.ts index d6fb8453b1b..53213006780 100644 --- a/packages/compass/src/main/application.ts +++ b/packages/compass/src/main/application.ts @@ -21,7 +21,8 @@ import { setupTheme } from './theme'; import { setupProtocolHandlers } from './protocol-handling'; import { ConnectionStorage } from '@mongodb-js/connection-storage/main'; -const { debug, track } = createLoggerAndTelemetry('COMPASS-MAIN'); +const { debug, log, track, mongoLogId } = + createLoggerAndTelemetry('COMPASS-MAIN'); type ExitHandler = () => Promise; type CompassApplicationMode = 'CLI' | 'GUI'; @@ -86,7 +87,16 @@ class CompassApplication { // ConnectionStorage offers import/export which is used via CLI as well. ConnectionStorage.init(); - await ConnectionStorage.migrateToSafeStorage(); + try { + await ConnectionStorage.migrateToSafeStorage(); + } catch (e) { + log.error( + mongoLogId(1_001_000_275), + 'SafeStorage Migration', + 'Failed to migrate connections', + { message: (e as Error).message } + ); + } if (mode === 'CLI') { return; From 575ce5235ae22d8a35db375cf074166ec9345755 Mon Sep 17 00:00:00 2001 From: Basit Date: Thu, 16 Nov 2023 17:15:09 +0300 Subject: [PATCH 07/10] add tests for connection import and clean up existing ones --- .../src/connection-storage.spec.ts | 257 +++++++++++++++--- .../test/fixtures/compass-connections.ts | 106 ++++++++ 2 files changed, 323 insertions(+), 40 deletions(-) create mode 100644 packages/connection-storage/test/fixtures/compass-connections.ts diff --git a/packages/connection-storage/src/connection-storage.spec.ts b/packages/connection-storage/src/connection-storage.spec.ts index 84b047f416b..d0586a54de2 100644 --- a/packages/connection-storage/src/connection-storage.spec.ts +++ b/packages/connection-storage/src/connection-storage.spec.ts @@ -16,6 +16,7 @@ import Sinon from 'sinon'; import connection1270 from './../test/fixtures/favorite_connection_1.27.0.json'; import connection1310 from './../test/fixtures/favorite_connection_1.31.0.json'; import connection1380 from './../test/fixtures/favorite_connection_1.38.0.json'; +import * as exportedConnectionFixtures from './../test/fixtures/compass-connections'; function getConnectionFilePath(tmpDir: string, id: string): string { const connectionsDir = path.join(tmpDir, 'Connections'); @@ -56,7 +57,6 @@ async function readConnection(tmpDir: string, id: string) { const maxAllowedConnections = 10; describe('ConnectionStorage', function () { - const initialKeytarEnvValue = process.env.COMPASS_E2E_DISABLE_KEYCHAIN_USAGE; const ipcMain = ConnectionStorage['ipcMain']; let tmpDir: string; @@ -69,8 +69,6 @@ describe('ConnectionStorage', function () { createHandle: Sinon.stub(), }; ConnectionStorage.init(tmpDir); - - process.env.COMPASS_E2E_DISABLE_KEYCHAIN_USAGE = 'true'; }); afterEach(async function () { @@ -79,12 +77,23 @@ describe('ConnectionStorage', function () { ConnectionStorage['calledOnce'] = false; ConnectionStorage['ipcMain'] = ipcMain; + }); - if (initialKeytarEnvValue) { - process.env.COMPASS_E2E_DISABLE_KEYCHAIN_USAGE = initialKeytarEnvValue; - } else { - delete process.env.COMPASS_E2E_DISABLE_KEYCHAIN_USAGE; - } + it('reminds us to remove keytar in', function () { + const testDate = new Date('2023-11-17T00:00:00.000Z'); + + const endOfKeytarDate = new Date(testDate); + endOfKeytarDate.setMonth(endOfKeytarDate.getMonth() + 3); + + // Based on milestone #2 of Move from keytar to Electron safeStorage, + // we should remove keytar in 3 months from now. And as such, we are + // intentionally failing this test to remind us to remove keytar. + // If we want to continue using keytar for now, check with the product and + // please update this test accordingly. + expect( + new Date(), + 'Expected to have keytar removed completely by now. If we want to continue using it for now, please update this test.' + ).to.be.lessThan(endOfKeytarDate); }); describe('migrateToSafeStorage', function () { @@ -317,6 +326,92 @@ describe('ConnectionStorage', function () { }); }); }); + + context( + 'imports already exported connections from compass using keytar', + function () { + const exportedConnections = [ + { + label: 'connection with plain secrets', + data: exportedConnectionFixtures.connectionsWithPlainSecrets, + }, + { + label: 'connection with encrypted secrets', + data: exportedConnectionFixtures.connectionsWithEncryptedSecrets, + importOptions: { + passphrase: 'password', + }, + }, + ]; + + for (const exportUseCase of exportedConnections) { + it(exportUseCase.label, async function () { + const countOfConnections = exportUseCase.data.connections.length; + // Import + { + // When importing connections, we will encrypt secrets using safeStorage. + // So, we mock the *encryptSecrets* implementation to not trigger keychain. + const encryptSecretsStub = sandbox + .stub(ConnectionStorage, 'encryptSecrets') + .returns('encrypted-password'); + // Import + await ConnectionStorage.importConnections({ + content: JSON.stringify(exportUseCase.data), + options: exportUseCase.importOptions, + }); + expect( + encryptSecretsStub.getCalls().map((x) => x.args), + `makes ${countOfConnections} calls with correct arguments when encrypting` + ).to.deep.equal( + Array.from({ length: countOfConnections }).map(() => [ + { password: 'password' }, + ]) + ); + } + + // Read imported connections using ConnectionStorage + { + // When reading, we will mock *decryptSecrets* implementation. + const decryptSecretsStub = sandbox + .stub(ConnectionStorage, 'decryptSecrets') + .returns({ password: 'password' }); + + const connections = await ConnectionStorage.loadAll(); + + expect(connections).to.have.lengthOf(countOfConnections); + + expect( + decryptSecretsStub.getCalls().map((x) => x.args), + `makes ${countOfConnections} calls with correct arguments when decrypting` + ).to.deep.equal( + Array.from({ length: countOfConnections }).map(() => [ + 'encrypted-password', + ]) + ); + } + + // Read imported connections using fs from disk + { + const connections = await Promise.all( + exportUseCase.data.connections.map(({ id }) => + readConnection(tmpDir, id) + ) + ); + + expect(connections).to.have.lengthOf(countOfConnections); + + for (const connection of connections) { + expect(connection).to.have.a.property('version', 1); + expect(connection).to.have.a.property( + 'connectionSecrets', + 'encrypted-password' + ); + } + } + }); + } + } + ); }); describe('loadAll', function () { @@ -525,42 +620,112 @@ describe('ConnectionStorage', function () { }); expect.fail('Expected connection string to be required.'); } catch (e) { - expect(e).to.have.nested.property( - 'errors[0].message', - 'Connection string is required.' + expect(e).to.have.property( + 'message', + 'Invalid scheme, expected connection string to start with "mongodb://" or "mongodb+srv://"' ); } }); - // In tests we can not use keytar and have it disabled. When saving any data, - // its completely stored on disk without anything removed. - it('it stores all the fleOptions on disk', async function () { - const id = new UUID().toString(); - const connectionInfo = { - id, - connectionOptions: { - connectionString: 'mongodb://localhost:27017', - fleOptions: { - storeCredentials: false, - autoEncryption: { - keyVaultNamespace: 'db.coll', - kmsProviders: { - local: { - key: 'my-key', + context('it stores all the fleOptions on disk', function () { + it('removes secrets when storeCredentials is false', async function () { + const id = new UUID().toString(); + const connectionInfo = { + id, + connectionOptions: { + connectionString: 'mongodb://localhost:27017/', + fleOptions: { + storeCredentials: false, + autoEncryption: { + keyVaultNamespace: 'db.coll', + kmsProviders: { + local: { + key: 'my-key', + }, }, }, }, }, - }, - }; - await ConnectionStorage.save({ connectionInfo }); + }; - const content = await fs.readFile( - getConnectionFilePath(tmpDir, id), - 'utf-8' - ); - const { connectionInfo: expectedConnectionInfo } = JSON.parse(content); - expect(expectedConnectionInfo).to.deep.equal(connectionInfo); + // // Stub encryptSecrets so that we do not call electron.safeStorage.encrypt + // // and make assertions on that. + const encryptSecretsStub = Sinon.stub( + ConnectionStorage, + 'encryptSecrets' + ).returns(undefined); + + await ConnectionStorage.save({ connectionInfo }); + + const expectedConnection = await readConnection(tmpDir, id); + connectionInfo.connectionOptions.fleOptions.autoEncryption.kmsProviders = + {} as any; + expect(expectedConnection).to.deep.equal({ + _id: connectionInfo.id, + connectionInfo, + version: 1, + }); + + expect(encryptSecretsStub.calledOnce).to.be.true; + expect( + encryptSecretsStub.firstCall.firstArg, + 'it should not store any secrets' + ).to.deep.equal({}); + }); + + it('encrypt and store secrets when storeCredentials is true', async function () { + const id = new UUID().toString(); + const connectionInfo = { + id, + connectionOptions: { + connectionString: 'mongodb://localhost:27017/', + fleOptions: { + storeCredentials: true, + autoEncryption: { + keyVaultNamespace: 'db.coll', + kmsProviders: { + local: { + key: 'my-key', + }, + }, + }, + }, + }, + }; + + // Stub encryptSecrets so that we do not call electron.safeStorage.encrypt + // and make assertions on that. + const encryptSecretsStub = Sinon.stub( + ConnectionStorage, + 'encryptSecrets' + ).returns('encrypted-data'); + + await ConnectionStorage.save({ connectionInfo }); + + const expectedConnection = await readConnection(tmpDir, id); + connectionInfo.connectionOptions.fleOptions.autoEncryption.kmsProviders = + {} as any; + expect(expectedConnection).to.deep.equal({ + _id: connectionInfo.id, + connectionInfo, + connectionSecrets: 'encrypted-data', + version: 1, + }); + + expect(encryptSecretsStub.calledOnce).to.be.true; + expect( + encryptSecretsStub.firstCall.firstArg, + 'it should store secrets' + ).to.deep.equal({ + autoEncryption: { + kmsProviders: { + local: { + key: 'my-key', + }, + }, + }, + }); + }); }); it('saves a connection with _id', async function () { @@ -571,6 +736,13 @@ describe('ConnectionStorage', function () { expect(e).to.be.instanceOf(Error); } + // Stub encryptSecrets so that we do not call electron.safeStorage.encrypt + // and make assertions on that. + const encryptSecretsStub = Sinon.stub( + ConnectionStorage, + 'encryptSecrets' + ).returns('encrypted-password'); + await ConnectionStorage.save({ connectionInfo: { id, @@ -580,19 +752,24 @@ describe('ConnectionStorage', function () { }, }); - const savedConnection = JSON.parse( - await fs.readFile(getConnectionFilePath(tmpDir, id), 'utf-8') - ); - + const savedConnection = await readConnection(tmpDir, id); expect(savedConnection).to.deep.equal({ connectionInfo: { id, connectionOptions: { - connectionString: 'mongodb://root:password@localhost:27017', + connectionString: 'mongodb://root@localhost:27017/', }, }, + connectionSecrets: 'encrypted-password', + version: 1, _id: id, }); + + expect(encryptSecretsStub.calledOnce).to.be.true; + expect( + encryptSecretsStub.firstCall.firstArg, + 'it encrypts the connection secrets correctly' + ).to.deep.equal({ password: 'password' }); }); context(`max allowed connections ${maxAllowedConnections}`, function () { diff --git a/packages/connection-storage/test/fixtures/compass-connections.ts b/packages/connection-storage/test/fixtures/compass-connections.ts new file mode 100644 index 00000000000..f9cc255d108 --- /dev/null +++ b/packages/connection-storage/test/fixtures/compass-connections.ts @@ -0,0 +1,106 @@ +// These connections have been encrypted with *password* passphrase. +// The password of these connections is *password* +// Exported on Compass 1.40.4 +export const connectionsWithEncryptedSecrets = { + type: 'Compass Connections', + version: { + $numberInt: '1', + }, + connections: [ + { + id: '30bc83dd-a988-4553-b379-89fac70f3f5a', + favorite: { + name: 'Compass 1.39.3', + color: 'color6', + }, + connectionOptions: { + connectionString: + 'mongodb://compass1393@localhost:27017/?authMechanism=DEFAULT', + }, + connectionSecrets: + 'AAFSEEDlKaJFiXzDE1UFQ9/iokaH3j1t7TOCXUsy6sG+yxASG2WOfTP8rbg0g5XqrOcm6D+IpTni5QxpTXChGkrwngRVOWwmGmHkAItGNlZaR0AGaw==', + }, + { + id: '5231cafe-b280-43ac-977b-f95619fb0892', + favorite: { + name: 'Compass 1.39.0', + color: 'color3', + }, + connectionOptions: { + connectionString: + 'mongodb://compass1390@localhost:27017/?authMechanism=DEFAULT', + }, + connectionSecrets: + 'AAFj3cDh/I7e17O0rMczLJBqzNaSHBwbhlzPCek+G43M5H61EOLXGcvMUZpMr+0CYYWl5xSFBaWOX+qCDZDYCsd3Md+HL32F4o78QZDd5XeXA2fjFw==', + }, + { + id: '594f1301-30db-4b4b-a341-432bb915cc0b', + favorite: { + name: 'Compass 1.38.2', + color: 'color10', + }, + connectionOptions: { + connectionString: + 'mongodb://compass1382@localhost:27017/?authMechanism=DEFAULT', + }, + connectionSecrets: + 'AAEqe3aNaVhTg+90sm2ESKElVyFDwAoRiXu6D2C+Saelo4jDAqwzH+jOQqmwF0CTLDBjsnKjlKJKxfrzhakgZxy6LgyxGUmM/yrICy/eUjIL75oJOA==', + }, + { + id: '9005e859-cf99-4b29-8837-ca9d4bf3b8bf', + favorite: { + name: 'Compass 1.39.2', + color: 'color6', + }, + connectionOptions: { + connectionString: + 'mongodb://compass1392@localhost:27017/?authMechanism=DEFAULT', + }, + connectionSecrets: + 'AAHK7dlGMo8VUT8owt7rbtOhaevPP2ArrAkg+5WablYAAcMQFrxDlFUAambct6uolnpVhTEzfQED2kM5tdmgj8wQiuM0yFvmHNPIU3x/ukZthDJWfA==', + }, + ], +} as const; + +// Exported on Compass 1.40.4 +export const connectionsWithPlainSecrets = { + type: 'Compass Connections', + version: { + $numberInt: '1', + }, + connections: [ + { + id: '3985f01b-417a-448c-91f5-6b04303a26be', + favorite: { + name: 'Compass 1.36.4', + color: 'color5', + }, + connectionOptions: { + connectionString: + 'mongodb://compass1364:password@localhost:27017/?authMechanism=DEFAULT', + }, + }, + { + id: '5231cafe-b280-43ac-977b-f95619fb0892', + favorite: { + name: 'Compass 1.39.0', + color: 'color3', + }, + connectionOptions: { + connectionString: + 'mongodb://compass1390:password@localhost:27017/?authMechanism=DEFAULT', + }, + }, + { + id: '594f1301-30db-4b4b-a341-432bb915cc0b', + favorite: { + name: 'Compass 1.38.2', + color: 'color10', + }, + connectionOptions: { + connectionString: + 'mongodb://compass1382:password@localhost:27017/?authMechanism=DEFAULT', + }, + }, + ], +} as const; From df48b1bffc7ac8136fe35a45425aca2e190d7a9c Mon Sep 17 00:00:00 2001 From: Basit Date: Tue, 28 Nov 2023 11:02:33 +0100 Subject: [PATCH 08/10] ts as any assert --- .../src/connection-storage.spec.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/connection-storage/src/connection-storage.spec.ts b/packages/connection-storage/src/connection-storage.spec.ts index 84b047f416b..c684f8b523f 100644 --- a/packages/connection-storage/src/connection-storage.spec.ts +++ b/packages/connection-storage/src/connection-storage.spec.ts @@ -109,11 +109,11 @@ describe('ConnectionStorage', function () { const encryptSecretsSpy = sandbox.spy( ConnectionStorage, - 'encryptSecrets' + 'encryptSecrets' as any ); const getKeytarCredentialsSpy = sandbox.spy( ConnectionStorage, - 'getKeytarCredentials' + 'getKeytarCredentials' as any ); await ConnectionStorage.migrateToSafeStorage(); @@ -145,11 +145,11 @@ describe('ConnectionStorage', function () { const encryptSecretsSpy = sandbox.spy( ConnectionStorage, - 'encryptSecrets' + 'encryptSecrets' as any ); const getKeytarCredentialsSpy = sandbox.spy( ConnectionStorage, - 'getKeytarCredentials' + 'getKeytarCredentials' as any ); await ConnectionStorage.migrateToSafeStorage(); @@ -198,7 +198,7 @@ describe('ConnectionStorage', function () { }); // Keytar stub - sandbox.stub(ConnectionStorage, 'getKeytarCredentials').returns({ + sandbox.stub(ConnectionStorage, 'getKeytarCredentials' as any).returns({ [connectionInfo1.id]: { password: 'password1', }, @@ -209,7 +209,7 @@ describe('ConnectionStorage', function () { // safeStorage.encryptString stub sandbox - .stub(ConnectionStorage, 'encryptSecrets') + .stub(ConnectionStorage, 'encryptSecrets' as any) .returns('encrypted-password'); await ConnectionStorage.migrateToSafeStorage(); @@ -248,7 +248,7 @@ describe('ConnectionStorage', function () { // Keytar fake sandbox - .stub(ConnectionStorage, 'getKeytarCredentials') + .stub(ConnectionStorage, 'getKeytarCredentials' as any) .returns({}); // Since there're no secrets in keychain, we do not expect to call safeStorage.encryptString @@ -291,7 +291,7 @@ describe('ConnectionStorage', function () { // Keytar stub sandbox - .stub(ConnectionStorage, 'getKeytarCredentials') + .stub(ConnectionStorage, 'getKeytarCredentials' as any) .returns({}); await ConnectionStorage.migrateToSafeStorage(); From f413645ff2852160654b913e3c1ade33e1bac5a7 Mon Sep 17 00:00:00 2001 From: Basit Date: Tue, 28 Nov 2023 15:20:59 +0100 Subject: [PATCH 09/10] dep check --- package-lock.json | 3 ++- packages/connection-storage/package.json | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index fc106da3d79..2b1e59e5039 100644 --- a/package-lock.json +++ b/package-lock.json @@ -47946,6 +47946,7 @@ "@mongodb-js/compass-user-data": "^0.1.9", "@mongodb-js/compass-utils": "^0.5.5", "bson": "^6.2.0", + "electron": "^25.9.6", "hadron-ipc": "^3.2.4", "keytar": "^7.9.0", "lodash": "^4.17.21", @@ -61065,7 +61066,7 @@ "bson": "^6.2.0", "chai": "^4.3.6", "depcheck": "^1.4.1", - "electron": "^25.9.4", + "electron": "^25.9.6", "eslint": "^7.25.0", "hadron-ipc": "^3.2.4", "keytar": "^7.9.0", diff --git a/packages/connection-storage/package.json b/packages/connection-storage/package.json index 805e02c7ebf..83c15f3db1b 100644 --- a/packages/connection-storage/package.json +++ b/packages/connection-storage/package.json @@ -56,6 +56,7 @@ "@mongodb-js/compass-user-data": "^0.1.9", "@mongodb-js/compass-utils": "^0.5.5", "bson": "^6.2.0", + "electron": "^25.9.6", "hadron-ipc": "^3.2.4", "keytar": "^7.9.0", "lodash": "^4.17.21", From 6a12e6935acb33ea49e367e2499b414b557e4957 Mon Sep 17 00:00:00 2001 From: Basit Date: Tue, 28 Nov 2023 15:24:41 +0100 Subject: [PATCH 10/10] ts as any assert --- .../connection-storage/src/connection-storage.spec.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/connection-storage/src/connection-storage.spec.ts b/packages/connection-storage/src/connection-storage.spec.ts index f0e016dbbb4..b8277fb27f9 100644 --- a/packages/connection-storage/src/connection-storage.spec.ts +++ b/packages/connection-storage/src/connection-storage.spec.ts @@ -352,7 +352,7 @@ describe('ConnectionStorage', function () { // When importing connections, we will encrypt secrets using safeStorage. // So, we mock the *encryptSecrets* implementation to not trigger keychain. const encryptSecretsStub = sandbox - .stub(ConnectionStorage, 'encryptSecrets') + .stub(ConnectionStorage, 'encryptSecrets' as any) .returns('encrypted-password'); // Import await ConnectionStorage.importConnections({ @@ -373,7 +373,7 @@ describe('ConnectionStorage', function () { { // When reading, we will mock *decryptSecrets* implementation. const decryptSecretsStub = sandbox - .stub(ConnectionStorage, 'decryptSecrets') + .stub(ConnectionStorage, 'decryptSecrets' as any) .returns({ password: 'password' }); const connections = await ConnectionStorage.loadAll(); @@ -652,7 +652,7 @@ describe('ConnectionStorage', function () { // // and make assertions on that. const encryptSecretsStub = Sinon.stub( ConnectionStorage, - 'encryptSecrets' + 'encryptSecrets' as any ).returns(undefined); await ConnectionStorage.save({ connectionInfo }); @@ -697,7 +697,7 @@ describe('ConnectionStorage', function () { // and make assertions on that. const encryptSecretsStub = Sinon.stub( ConnectionStorage, - 'encryptSecrets' + 'encryptSecrets' as any ).returns('encrypted-data'); await ConnectionStorage.save({ connectionInfo }); @@ -740,7 +740,7 @@ describe('ConnectionStorage', function () { // and make assertions on that. const encryptSecretsStub = Sinon.stub( ConnectionStorage, - 'encryptSecrets' + 'encryptSecrets' as any ).returns('encrypted-password'); await ConnectionStorage.save({