From 6f122ee399c9854a658e00f2564aec2053acb041 Mon Sep 17 00:00:00 2001 From: Basit <1305718+mabaasit@users.noreply.github.com> Date: Tue, 28 Nov 2023 17:19:13 +0100 Subject: [PATCH] feat(connection-storage): migrate from keytar COMPASS-6854 (#5115) --- package-lock.json | 2 + packages/compass/src/main/application.ts | 14 +- packages/connection-storage/package.json | 1 + .../src/connection-storage.spec.ts | 291 +++++++++++++++--- .../src/connection-storage.ts | 172 +++++++++-- 5 files changed, 417 insertions(+), 63 deletions(-) diff --git a/package-lock.json b/package-lock.json index f0db62da074..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,6 +61066,7 @@ "bson": "^6.2.0", "chai": "^4.3.6", "depcheck": "^1.4.1", + "electron": "^25.9.6", "eslint": "^7.25.0", "hadron-ipc": "^3.2.4", "keytar": "^7.9.0", diff --git a/packages/compass/src/main/application.ts b/packages/compass/src/main/application.ts index a15a1195d6d..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,6 +87,17 @@ class CompassApplication { // ConnectionStorage offers import/export which is used via CLI as well. ConnectionStorage.init(); + 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; } 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", diff --git a/packages/connection-storage/src/connection-storage.spec.ts b/packages/connection-storage/src/connection-storage.spec.ts index 2d64f9f178c..c684f8b523f 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,238 @@ 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(); + 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 = sandbox.spy( + ConnectionStorage, + 'encryptSecrets' as any + ); + const getKeytarCredentialsSpy = sandbox.spy( + ConnectionStorage, + 'getKeytarCredentials' as any + ); + + 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 = sandbox.spy( + ConnectionStorage, + 'encryptSecrets' as any + ); + const getKeytarCredentialsSpy = sandbox.spy( + ConnectionStorage, + 'getKeytarCredentials' as any + ); + + 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 stub + sandbox.stub(ConnectionStorage, 'getKeytarCredentials' as any).returns({ + [connectionInfo1.id]: { + password: 'password1', + }, + [connectionInfo2.id]: { + password: 'password2', + }, + }); + + // safeStorage.encryptString stub + sandbox + .stub(ConnectionStorage, 'encryptSecrets' as any) + .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 + sandbox + .stub(ConnectionStorage, 'getKeytarCredentials' as any) + .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 stub + sandbox + .stub(ConnectionStorage, 'getKeytarCredentials' as any) + .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 +707,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 +812,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 +831,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..b7b185ffe72 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,129 @@ 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 is ConnectionWithLegacyProps & + Required> => !!x.connectionInfo + ); + if (connections.length === 0) { + log.info( + mongoLogId(1_001_000_270), + '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_271), + '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_272), + 'Connection Storage', + 'Migrating connections', + { + num_connections: unmigratedConnections.length, + num_secrets: Object.keys(secrets).length, + } + ); + + let numFailedToMigrate = 0; + + for (const { connectionInfo } of unmigratedConnections) { + 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_273), + 'Connection Storage', + 'Failed to migrate connection', + { message: (e as Error).message, connectionId: _id } + ); + } + } + + log.info( + mongoLogId(1_001_000_274), + '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 +292,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 +335,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) {