laurent22 · laurent22 · Aug 8, 2024 · Jun 3, 2024 · Jun 5, 2024 · Jun 6, 2024
diff --git a/.eslintignore b/.eslintignore
@@ -1071,8 +1071,10 @@ packages/lib/services/interop/Module.js
 packages/lib/services/interop/types.js
 packages/lib/services/joplinCloudUtils.js
 packages/lib/services/joplinServer/personalizedUserContentBaseUrl.js
+packages/lib/services/keychain/KeychainService.test.js
 packages/lib/services/keychain/KeychainService.js
 packages/lib/services/keychain/KeychainServiceDriver.dummy.js
+packages/lib/services/keychain/KeychainServiceDriver.electron.js
 packages/lib/services/keychain/KeychainServiceDriver.mobile.js
 packages/lib/services/keychain/KeychainServiceDriver.node.js
 packages/lib/services/keychain/KeychainServiceDriverBase.js

diff --git a/.gitignore b/.gitignore
@@ -1050,8 +1050,10 @@ packages/lib/services/interop/Module.js
 packages/lib/services/interop/types.js
 packages/lib/services/joplinCloudUtils.js
 packages/lib/services/joplinServer/personalizedUserContentBaseUrl.js
+packages/lib/services/keychain/KeychainService.test.js
 packages/lib/services/keychain/KeychainService.js
 packages/lib/services/keychain/KeychainServiceDriver.dummy.js
+packages/lib/services/keychain/KeychainServiceDriver.electron.js
 packages/lib/services/keychain/KeychainServiceDriver.mobile.js
 packages/lib/services/keychain/KeychainServiceDriver.node.js
 packages/lib/services/keychain/KeychainServiceDriverBase.js

diff --git a/packages/app-desktop/bridge.ts b/packages/app-desktop/bridge.ts
@@ -1,7 +1,7 @@
 import ElectronAppWrapper from './ElectronAppWrapper';
 import shim from '@joplin/lib/shim';
 import { _, setLocale } from '@joplin/lib/locale';
-import { BrowserWindow, nativeTheme, nativeImage, shell, dialog, MessageBoxSyncOptions } from 'electron';
+import { BrowserWindow, nativeTheme, nativeImage, shell, dialog, MessageBoxSyncOptions, safeStorage } from 'electron';
 import { dirname, toSystemSlashes } from '@joplin/lib/path-utils';
 import { fileUriToPath } from '@joplin/utils/url';
 import { urlDecode } from '@joplin/lib/string-utils';
@@ -485,6 +485,21 @@ export class Bridge {
 		return nativeImage.createFromPath(path);
 	}
 
+	public safeStorage = {
+		isEncryptionAvailable() {
+			return safeStorage.isEncryptionAvailable();
+		},
+		encryptString(data: string) {
+			return safeStorage.encryptString(data).toString('base64');
+		},
+		decryptString(base64Data: string) {
+			return safeStorage.decryptString(Buffer.from(base64Data, 'base64'));
+		},
+
+		getSelectedStorageBackend() {
+			return safeStorage.getSelectedStorageBackend();
+		},
+	};
 }
 
 let bridge_: Bridge = null;

diff --git a/packages/app-desktop/main.js b/packages/app-desktop/main.js
@@ -18,7 +18,7 @@ const registerCustomProtocols = require('./utils/customProtocols/registerCustomP
 // our case it's a string like "@joplin/app-desktop". It's also supposed to
 // check the productName key but is not doing it, so here set the
 // application name to the right string.
-electronApp.name = packageInfo.name;
+electronApp.setName(packageInfo.name);
 
 process.on('unhandledRejection', (reason, p) => {
 	console.error('Unhandled promise rejection', p, 'reason:', reason);

diff --git a/packages/app-mobile/root.tsx b/packages/app-mobile/root.tsx
@@ -615,7 +615,7 @@ async function initialize(dispatch: Dispatch) {
 		reg.logger().info('Database is ready.');
 		reg.logger().info('Loading settings...');
 
-		await loadKeychainServiceAndSettings(KeychainServiceDriverMobile);
+		await loadKeychainServiceAndSettings([KeychainServiceDriverMobile]);
 		await migrateMasterPassword();
 
 		if (!Setting.value('clientId')) Setting.setValue('clientId', uuid.create());

diff --git a/packages/lib/BaseApplication.ts b/packages/lib/BaseApplication.ts
@@ -4,8 +4,8 @@ import shim from './shim';
 const { setupProxySettings } = require('./shim-init-node');
 import BaseService from './services/BaseService';
 import reducer, { getNotesParent, serializeNotesParent, setStore, State } from './reducer';
-import KeychainServiceDriver from './services/keychain/KeychainServiceDriver.node';
-import KeychainServiceDriverDummy from './services/keychain/KeychainServiceDriver.dummy';
+import KeychainServiceDriverNode from './services/keychain/KeychainServiceDriver.node';
+import KeychainServiceDriverElectron from './services/keychain/KeychainServiceDriver.electron';
 import { setLocale } from './locale';
 import KvStore from './services/KvStore';
 import SyncTargetJoplinServer from './SyncTargetJoplinServer';
@@ -754,10 +754,13 @@ export default class BaseApplication {
 
 		reg.setDb(this.database_);
 		BaseModel.setDb(this.database_);
+		KvStore.instance().setDb(reg.db());
 
 		setRSA(RSA);
 
-		await loadKeychainServiceAndSettings(options.keychainEnabled ? KeychainServiceDriver : KeychainServiceDriverDummy);
+		await loadKeychainServiceAndSettings(
+			options.keychainEnabled ? [KeychainServiceDriverElectron, KeychainServiceDriverNode] : [],
+		);
 		await migrateMasterPassword();
 		await handleSyncStartupOperation();
 
@@ -841,7 +844,6 @@ export default class BaseApplication {
 
 		BaseItem.revisionService_ = RevisionService.instance();
 
-		KvStore.instance().setDb(reg.db());
 
 		BaseItem.encryptionService_ = EncryptionService.instance();
 		BaseItem.shareService_ = ShareService.instance();

diff --git a/packages/lib/models/Setting.ts b/packages/lib/models/Setting.ts
@@ -142,7 +142,6 @@ export type SettingMetadataSection = {
 export type MetadataBySection = SettingMetadataSection[];
 
 class Setting extends BaseModel {
-
 	public static schemaUrl = 'https://joplinapp.org/schema/settings.json';
 
 	// For backward compatibility
@@ -974,19 +973,10 @@ class Setting extends BaseModel {
 				// Also we don't control what happens on the keychain - the values can be edited or deleted
 				// outside the application. For that reason, we rewrite it every time the values are saved,
 				// even if, internally, they haven't changed.
-				// As an optimisation, we check if the value exists on the keychain before writing it again.
 				try {
 					const passwordName = `setting.${s.key}`;
-					const currentValue = await this.keychainService().password(passwordName);
-					if (currentValue !== valueAsString) {
-						const wasSet = await this.keychainService().setPassword(passwordName, valueAsString);
-						if (wasSet) continue;
-					} else {
-						// The value is already in the keychain - so nothing to do
-						// Make sure to `continue` here otherwise it will save the password
-						// in clear text in the database.
-						continue;
-					}
+					const wasSet = await this.keychainService().setPassword(passwordName, valueAsString);
+					if (wasSet) continue;
 				} catch (error) {
 					logger.error(`Could not set setting on the keychain. Will be saved to database instead: ${s.key}:`, error);
 				}

diff --git a/packages/lib/models/settings/builtInMetadata.ts b/packages/lib/models/settings/builtInMetadata.ts
@@ -920,6 +920,7 @@ const builtInMetadata = (Setting: typeof SettingType) => {
 		collapsedFolderIds: { value: [] as string[], type: SettingItemType.Array, public: false },
 
 		'keychain.supported': { value: -1, type: SettingItemType.Int, public: false },
+		'keychain.lastAvailableDrivers': { value: [] as string[], type: SettingItemType.Array, public: false },
 		'db.ftsEnabled': { value: -1, type: SettingItemType.Int, public: false },
 		'db.fuzzySearchEnabled': { value: -1, type: SettingItemType.Int, public: false },
 		'encryption.enabled': { value: false, type: SettingItemType.Bool, public: false },

diff --git a/packages/lib/services/SettingUtils.ts b/packages/lib/services/SettingUtils.ts
@@ -4,6 +4,9 @@ import KeychainService from './keychain/KeychainService';
 import Setting from '../models/Setting';
 import uuid from '../uuid';
 import { migrateLocalSyncInfo } from './synchronizer/syncInfoUtils';
+import KeychainServiceDriverBase from './keychain/KeychainServiceDriverBase';
+
+type KeychainServiceDriverConstructor = new (appId: string, clientId: string)=> KeychainServiceDriverBase;
 
 // This function takes care of initialising both the keychain service and settings.
 //
@@ -13,11 +16,12 @@ import { migrateLocalSyncInfo } from './synchronizer/syncInfoUtils';
 // In other words, it's not possible to load the settings without the KS service and it's not
 // possible to initialise the KS service without the settings.
 // The solution is to fetch just the client ID directly from the database.
-// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
-export async function loadKeychainServiceAndSettings(KeychainServiceDriver: any) {
+export async function loadKeychainServiceAndSettings(keychainServiceDrivers: KeychainServiceDriverConstructor[]) {
 	const clientIdSetting = await Setting.loadOne('clientId');
 	const clientId = clientIdSetting ? clientIdSetting.value : uuid.create();
-	KeychainService.instance().initialize(new KeychainServiceDriver(Setting.value('appId'), clientId));
+	await KeychainService.instance().initialize(
+		keychainServiceDrivers.map(Driver => new Driver(Setting.value('appId'), clientId)),
+	);
 	Setting.setKeychainService(KeychainService.instance());
 	await Setting.load();
 

diff --git a/packages/lib/services/keychain/KeychainService.test.ts b/packages/lib/services/keychain/KeychainService.test.ts
@@ -0,0 +1,128 @@
+import Setting from '../../models/Setting';
+import shim from '../../shim';
+import { switchClient, setupDatabaseAndSynchronizer } from '../../testing/test-utils';
+import KeychainService from './KeychainService';
+import KeychainServiceDriverDummy from './KeychainServiceDriver.dummy';
+import KeychainServiceDriverElectron from './KeychainServiceDriver.electron';
+import KeychainServiceDriverNode from './KeychainServiceDriver.node';
+
+interface SafeStorageMockOptions {
+	isEncryptionAvailable?: ()=> boolean;
+	encryptString?: (str: string)=> Promise<string|null>;
+	decryptString?: (str: string)=> Promise<string|null>;
+}
+
+const mockSafeStorage = ({ // Safe storage
+	isEncryptionAvailable = jest.fn(() => true),
+	encryptString = jest.fn(async s => (`e:${s}`)),
+	decryptString = jest.fn(async s => s.substring(2)),
+}: SafeStorageMockOptions) => {
+	shim.electronBridge = () => ({
+		safeStorage: {
+			isEncryptionAvailable,
+			encryptString,
+			decryptString,
+			getSelectedStorageBackend: () => 'mock',
+		},
+	});
+};
+
+const mockKeytar = () => {
+	const storage = new Map<string, string>();
+
+	const keytarMock = {
+		getPassword: jest.fn(async (key, client) => {
+			return storage.get(`${client}--${key}`);
+		}),
+		setPassword: jest.fn(async (key, client, password) => {
+			if (!password) throw new Error('Keytar doesn\'t support empty passwords.');
+			storage.set(`${client}--${key}`, password);
+		}),
+		deletePassword: jest.fn(async (key, client) => {
+			storage.delete(`${client}--${key}`);
+		}),
+	};
+	shim.keytar = () => keytarMock;
+	return keytarMock;
+};
+
+const makeDrivers = () => [
+	new KeychainServiceDriverElectron(Setting.value('appId'), Setting.value('clientId')),
+	new KeychainServiceDriverNode(Setting.value('appId'), Setting.value('clientId')),
+];
+
+describe('KeychainService', () => {
+	beforeEach(async () => {
+		await setupDatabaseAndSynchronizer(0);
+		await switchClient(0);
+		Setting.setValue('keychain.supported', 1);
+		shim.electronBridge = null;
+		shim.keytar = null;
+	});
+
+	test('should copy keys from keytar to safeStorage', async () => {
+		const keytarMock = mockKeytar();
+		await KeychainService.instance().initialize(makeDrivers());
+
+		// Set a secure setting
+		Setting.setValue('encryption.masterPassword', 'testing');
+		await Setting.saveAll();
+
+		mockSafeStorage({});
+
+		await KeychainService.instance().initialize(makeDrivers());
+		await Setting.load();
+		expect(Setting.value('encryption.masterPassword')).toBe('testing');
+
+		await Setting.saveAll();
+
+		// For now, passwords should not be removed from old backends -- this allows
+		// users to revert to an earlier version of Joplin without data loss.
+		expect(keytarMock.deletePassword).not.toHaveBeenCalled();
+
+		expect(shim.electronBridge().safeStorage.encryptString).toHaveBeenCalled();
+		expect(shim.electronBridge().safeStorage.encryptString).toHaveBeenCalledWith('testing');
+
+		await Setting.load();
+		expect(Setting.value('encryption.masterPassword')).toBe('testing');
+	});
+
+	test('should use keytar when safeStorage is unavailable', async () => {
+		const keytarMock = mockKeytar();
+		await KeychainService.instance().initialize(makeDrivers());
+
+		Setting.setValue('encryption.masterPassword', 'test-password');
+		await Setting.saveAll();
+		expect(keytarMock.setPassword).toHaveBeenCalledWith(
+			`${Setting.value('appId')}.setting.encryption.masterPassword`,
+			`${Setting.value('clientId')}@joplin`,
+			'test-password',
+		);
+
+		await Setting.load();
+		expect(Setting.value('encryption.masterPassword')).toBe('test-password');
+	});
+
+	test('should re-check for keychain support when a new driver is added', async () => {
+		mockKeytar();
+		mockSafeStorage({});
+		Setting.setValue('keychain.supported', -1);
+
+		await KeychainService.instance().initialize([
+			new KeychainServiceDriverDummy(Setting.value('appId'), Setting.value('clientId')),
+		]);
+		await KeychainService.instance().detectIfKeychainSupported();
+
+		expect(Setting.value('keychain.supported')).toBe(0);
+
+		// Should re-run the check after keytar and safeStorage are available.
+		await KeychainService.instance().initialize(makeDrivers());
+		await KeychainService.instance().detectIfKeychainSupported();
+		expect(Setting.value('keychain.supported')).toBe(1);
+
+		// Should re-run the check if safeStorage and keytar are both no longer available.
+		await KeychainService.instance().initialize([]);
+		await KeychainService.instance().detectIfKeychainSupported();
+		expect(Setting.value('keychain.supported')).toBe(0);
+	});
+});