All: Fixes #8254: Improve selection of active E2EE key

packages/lib/services/e2ee/utils.test.ts

@@ -1,6 +1,6 @@
-import { afterAllCleanUp, setupDatabaseAndSynchronizer, switchClient, encryptionService, expectNotThrow, expectThrow, kvStore } from '../../testing/test-utils';
+import { afterAllCleanUp, setupDatabaseAndSynchronizer, switchClient, encryptionService, expectNotThrow, expectThrow, kvStore, msleep } from '../../testing/test-utils';
 import MasterKey from '../../models/MasterKey';
-import { migrateMasterPassword, resetMasterPassword, showMissingMasterKeyMessage, updateMasterPassword } from './utils';
+import { activeMasterKeySanityCheck, migrateMasterPassword, resetMasterPassword, showMissingMasterKeyMessage, updateMasterPassword } from './utils';
 import { localSyncInfo, masterKeyById, masterKeyEnabled, setActiveMasterKeyId, setMasterKeyEnabled, setPpk } from '../synchronizer/syncInfoUtils';
 import Setting from '../../models/Setting';
 import { generateKeyPair, ppkPasswordIsValid } from './ppk';
@@ -147,4 +147,36 @@ describe('e2ee/utils', () => {
 		expect(localSyncInfo().ppk.publicKey).not.toBe(previousPpk.publicKey);
 	});
 
+	it('should fix active key selection issues - 1', async () => {
+		const masterPassword1 = '111111';
+		Setting.setValue('encryption.masterPassword', masterPassword1);
+		const mk1 = await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1));
+		await msleep(1);
+		await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1));
+		await msleep(1);
+		const mk3 = await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1));
+		setActiveMasterKeyId(mk1.id);
+		setMasterKeyEnabled(mk1.id, false);
+
+		activeMasterKeySanityCheck();
+
+		const syncInfo = localSyncInfo();
+		expect(syncInfo.activeMasterKeyId).toBe(mk3.id);
+	});
+
+	it('should fix active key selection issues - 2', async () => {
+		// Should not do anything if the active key is already an enabled one.
+		const masterPassword1 = '111111';
+		Setting.setValue('encryption.masterPassword', masterPassword1);
+		const mk1 = await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1));
+		await msleep(1);
+		await MasterKey.save(await encryptionService().generateMasterKey(masterPassword1));
+		setActiveMasterKeyId(mk1.id);
+
+		activeMasterKeySanityCheck();
+
+		const syncInfo = localSyncInfo();
+		expect(syncInfo.activeMasterKeyId).toBe(mk1.id);
+	});
+
 });

packages/lib/services/e2ee/utils.ts

@@ -4,7 +4,7 @@ import MasterKey from '../../models/MasterKey';
 import Setting from '../../models/Setting';
 import { MasterKeyEntity } from './types';
 import EncryptionService from './EncryptionService';
-import { getActiveMasterKey, getActiveMasterKeyId, localSyncInfo, masterKeyEnabled, saveLocalSyncInfo, setEncryptionEnabled, SyncInfo } from '../synchronizer/syncInfoUtils';
+import { getActiveMasterKey, getActiveMasterKeyId, localSyncInfo, masterKeyEnabled, saveLocalSyncInfo, setActiveMasterKeyId, setEncryptionEnabled, SyncInfo } from '../synchronizer/syncInfoUtils';
 import JoplinError from '../../JoplinError';
 import { generateKeyPair, pkReencryptPrivateKey, ppkPasswordIsValid } from './ppk';
 import KvStore from '../KvStore';
@@ -131,6 +131,8 @@ export async function findMasterKeyPassword(service: EncryptionService, masterKe
 }
 
 export async function loadMasterKeysFromSettings(service: EncryptionService) {
+	activeMasterKeySanityCheck();
+
 	const masterKeys = await MasterKey.all();
 	const activeMasterKeyId = getActiveMasterKeyId();
 
@@ -153,6 +155,35 @@ export async function loadMasterKeysFromSettings(service: EncryptionService) {
 	logger.info(`Loaded master keys: ${service.loadedMasterKeysCount()}`);
 }
 
+// In some rare cases (normally should no longer be possible), a disabled master
+// key end up being the active one (the one used to encrypt data). This sanity
+// check resolves this by making an enabled key the active one.
+export const activeMasterKeySanityCheck = () => {
+	const syncInfo = localSyncInfo();
+	const activeMasterKeyId = syncInfo.activeMasterKeyId;
+	const enabledMasterKeys = syncInfo.masterKeys.filter(mk => masterKeyEnabled(mk));
+	if (!enabledMasterKeys.length) return;
+
+	if (enabledMasterKeys.find(mk => mk.id === activeMasterKeyId)) {
+		logger.info('activeMasterKeySanityCheck: Active key is an enabled key - nothing to do');
+		return;
+	}
+
+	logger.info('activeMasterKeySanityCheck: Active key is **not** an enabled key - selecting a different key as the active key...');
+
+	const latestMasterKey = enabledMasterKeys.reduce((acc: MasterKeyEntity, current: MasterKeyEntity) => {
+		if (current.created_time > acc.created_time) {
+			return current;
+		} else {
+			return acc;
+		}
+	});
+
+	logger.info('activeMasterKeySanityCheck: Selected new active key:', latestMasterKey);
+
+	setActiveMasterKeyId(latestMasterKey.id);
+};
+
 export function showMissingMasterKeyMessage(syncInfo: SyncInfo, notLoadedMasterKeys: string[]) {
 	if (!syncInfo.masterKeys.length) return false;
 

packages/lib/services/synchronizer/syncInfoUtils.test.ts

@@ -120,4 +120,38 @@ describe('syncInfoUtils', () => {
 		expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('2');
 	});
 
+	it('should merge sync target info, but should not make a disabled key the active one', async () => {
+		const syncInfo1 = new SyncInfo();
+		syncInfo1.masterKeys = [{
+			id: '1',
+			content: 'content1',
+			hasBeenUsed: true,
+			enabled: 0,
+		}];
+		syncInfo1.activeMasterKeyId = '1';
+
+		await msleep(1);
+
+		const syncInfo2 = new SyncInfo();
+		syncInfo2.masterKeys = [{
+			id: '2',
+			content: 'content2',
+			enabled: 1,
+			hasBeenUsed: false,
+		}];
+		syncInfo2.activeMasterKeyId = '2';
+
+		// Normally, if one master key has been used (1) and the other not (2),
+		// it should select the one that's been used regardless of timestamps.
+		// **However**, if the key 1 has been disabled by user, it should
+		// **not** be picked as the active one. Instead it should use key 2,
+		// because it's still enabled.
+		expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('2');
+
+		// If both key are disabled, we go back to the original logic, where we
+		// select the key that's been used.
+		syncInfo2.masterKeys[0].enabled = 0;
+		expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('1');
+	});
+
 });

packages/lib/services/synchronizer/syncInfoUtils.ts

@@ -115,13 +115,20 @@ export function localSyncInfoFromState(state: State): SyncInfo {
 // has already been used to encrypt data. In this case, at the moment we compare
 // local and remote sync info (before synchronising the data), key1.hasBeenUsed
 // is true, but key2.hasBeenUsed is false.
+//
+// 2023-05-30: Additionally, if one key is enabled and the other is not, we
+// always pick the enabled one regardless of usage.
 const mergeActiveMasterKeys = (s1: SyncInfo, s2: SyncInfo, output: SyncInfo) => {
 	const activeMasterKey1 = getActiveMasterKey(s1);
 	const activeMasterKey2 = getActiveMasterKey(s2);
 	let doDefaultAction = false;
 
 	if (activeMasterKey1 && activeMasterKey2) {
-		if (activeMasterKey1.hasBeenUsed && !activeMasterKey2.hasBeenUsed) {
+		if (masterKeyEnabled(activeMasterKey1) && !masterKeyEnabled(activeMasterKey2)) {
+			output.setWithTimestamp(s1, 'activeMasterKeyId');
+		} else if (!masterKeyEnabled(activeMasterKey1) && masterKeyEnabled(activeMasterKey2)) {
+			output.setWithTimestamp(s2, 'activeMasterKeyId');
+		} else if (activeMasterKey1.hasBeenUsed && !activeMasterKey2.hasBeenUsed) {
 			output.setWithTimestamp(s1, 'activeMasterKeyId');
 		} else if (!activeMasterKey1.hasBeenUsed && activeMasterKey2.hasBeenUsed) {
 			output.setWithTimestamp(s2, 'activeMasterKeyId');