Desktop: Resolves #6338: Improve E2EE usability when accidentally creating multiple keys (#6399)

packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx

@@ -137,7 +137,7 @@ const EncryptionConfigScreen = (props: Props) => {
 				return (
 					<td style={theme.textStyle}>
 						<input type="password" style={passwordStyle} value={password} onChange={event => onInputPasswordChange(mk, event.target.value)} />{' '}
-						<button style={theme.buttonStyle} onClick={() => onSavePasswordClick(mk, props.passwords)}>
+						<button style={theme.buttonStyle} onClick={() => onSavePasswordClick(mk, { ...props.passwords, ...inputPasswords })}>
 							{_('Save')}
 						</button>
 					</td>
@@ -268,7 +268,7 @@ const EncryptionConfigScreen = (props: Props) => {
 		const buttonTitle = CommandService.instance().label('openMasterPasswordDialog');
 
 		const needPasswordMessage = !needMasterPassword ? null : (
-			<p className="needpassword">{_('Your master password is needed to decrypt some of your data.')}<br/>{_('Please click on "%s" to proceed', buttonTitle)}</p>
+			<p className="needpassword">{_('Your password is needed to decrypt some of your data.')}<br/>{_('Please click on "%s" to proceed, or set the passwords in the "%s" list below.', buttonTitle, _('Encryption keys'))}</p>
 		);
 
 		return (

packages/app-desktop/gui/EncryptionConfigScreen/style.scss

@@ -5,6 +5,7 @@
 .manage-password-section > .status {
 	display: flex;
 	flex-direction: row;
+	align-items: center;
 }
 
 .manage-password-section > .needpassword {

packages/app-desktop/gui/MasterPasswordDialog/Dialog.tsx

@@ -34,6 +34,12 @@ export default function(props: Props) {
 	const [updatingPassword, setUpdatingPassword] = useState(false);
 	const [mode, setMode] = useState<Mode>(Mode.Set);
 
+	const showCurrentPassword = useMemo(() => {
+		if ([MasterPasswordStatus.NotSet, MasterPasswordStatus.Invalid].includes(status)) return false;
+		if (mode === Mode.Reset) return false;
+		return true;
+	}, [status]);
+
 	const onClose = useCallback(() => {
 		props.dispatch({
 			type: 'DIALOG_CLOSE',
@@ -63,7 +69,7 @@ export default function(props: Props) {
 			setUpdatingPassword(true);
 			try {
 				if (mode === Mode.Set) {
-					await updateMasterPassword(currentPassword, password1);
+					await updateMasterPassword(showCurrentPassword ? currentPassword : null, password1);
 				} else if (mode === Mode.Reset) {
 					await resetMasterPassword(EncryptionService.instance(), KvStore.instance(), ShareService.instance(), password1);
 				} else {
@@ -115,7 +121,7 @@ export default function(props: Props) {
 	}, [password1, password2, updatingPassword, needToRepeatPassword]);
 
 	useEffect(() => {
-		setShowPasswordForm(status === MasterPasswordStatus.NotSet);
+		setShowPasswordForm([MasterPasswordStatus.NotSet, MasterPasswordStatus.Invalid].includes(status));
 	}, [status]);
 
 	useAsyncEffect(async (event: AsyncEffectEvent) => {
@@ -131,8 +137,7 @@ export default function(props: Props) {
 
 	function renderPasswordForm() {
 		const renderCurrentPassword = () => {
-			if (status === MasterPasswordStatus.NotSet) return null;
-			if (mode === Mode.Reset) return null;
+			if (!showCurrentPassword) return null;
 
 			// If the master password is in the keychain we preload it into the
 			// field and allow displaying it. That way if the user has forgotten

packages/app-desktop/main.scss

@@ -275,4 +275,5 @@ Component-specific classes
 
 .master-password-dialog .fa-times {
 	color: var(--joplin-color-error);
+	margin-left: 5px;
 }

packages/lib/Synchronizer.ts

@@ -22,7 +22,7 @@ import TaskQueue from './TaskQueue';
 import ItemUploader from './services/synchronizer/ItemUploader';
 import { FileApi, RemoteItem } from './file-api';
 import JoplinDatabase from './JoplinDatabase';
-import { fetchSyncInfo, getActiveMasterKey, localSyncInfo, mergeSyncInfos, saveLocalSyncInfo, SyncInfo, syncInfoEquals, uploadSyncInfo } from './services/synchronizer/syncInfoUtils';
+import { fetchSyncInfo, getActiveMasterKey, localSyncInfo, mergeSyncInfos, saveLocalSyncInfo, setMasterKeyHasBeenUsed, SyncInfo, syncInfoEquals, uploadSyncInfo } from './services/synchronizer/syncInfoUtils';
 import { getMasterPassword, setupAndDisableEncryption, setupAndEnableEncryption } from './services/e2ee/utils';
 import { generateKeyPair } from './services/e2ee/ppk';
 import syncDebugLog from './services/synchronizer/syncDebugLog';
@@ -439,10 +439,13 @@ export default class Synchronizer {
 				let remoteInfo = await fetchSyncInfo(this.api());
 				logger.info('Sync target remote info:', remoteInfo);
 
+				let syncTargetIsNew = false;
+
 				if (!remoteInfo.version) {
 					logger.info('Sync target is new - setting it up...');
 					await this.migrationHandler().upgrade(Setting.value('syncVersion'));
 					remoteInfo = await fetchSyncInfo(this.api());
+					syncTargetIsNew = true;
 				}
 
 				logger.info('Sync target is already setup - checking it...');
@@ -455,11 +458,16 @@ export default class Synchronizer {
 
 				localInfo = await this.setPpkIfNotExist(localInfo, remoteInfo);
 
+				if (syncTargetIsNew && localInfo.activeMasterKeyId) {
+					localInfo = setMasterKeyHasBeenUsed(localInfo, localInfo.activeMasterKeyId);
+				}
+
 				// console.info('LOCAL', localInfo);
 				// console.info('REMOTE', remoteInfo);
 
 				if (!syncInfoEquals(localInfo, remoteInfo)) {
-					const newInfo = mergeSyncInfos(localInfo, remoteInfo);
+					let newInfo = mergeSyncInfos(localInfo, remoteInfo);
+					if (newInfo.activeMasterKeyId) newInfo = setMasterKeyHasBeenUsed(newInfo, newInfo.activeMasterKeyId);
 					const previousE2EE = localInfo.e2ee;
 					logger.info('Sync target info differs between local and remote - merging infos: ', newInfo.toObject());
 

packages/lib/services/e2ee/EncryptionService.ts

@@ -254,6 +254,7 @@ export default class EncryptionService {
 		model.created_time = now;
 		model.updated_time = now;
 		model.source_application = Setting.value('appId');
+		model.hasBeenUsed = false;
 
 		return model;
 	}

packages/lib/services/e2ee/types.ts

@@ -8,6 +8,7 @@ export interface MasterKeyEntity {
 	content?: string;
 	type_?: number;
 	enabled?: number;
+	hasBeenUsed?: boolean;
 }
 
 export type RSAKeyPair = any; // Depends on implementation

packages/lib/services/synchronizer/Synchronizer.e2ee.test.ts

@@ -9,7 +9,7 @@ import ResourceFetcher from '../../services/ResourceFetcher';
 import MasterKey from '../../models/MasterKey';
 import BaseItem from '../../models/BaseItem';
 import Synchronizer from '../../Synchronizer';
-import { getEncryptionEnabled, setEncryptionEnabled } from '../synchronizer/syncInfoUtils';
+import { fetchSyncInfo, getEncryptionEnabled, localSyncInfo, setEncryptionEnabled } from '../synchronizer/syncInfoUtils';
 import { loadMasterKeysFromSettings, setupAndDisableEncryption, setupAndEnableEncryption } from '../e2ee/utils';
 
 let insideBeforeEach = false;
@@ -73,6 +73,32 @@ describe('Synchronizer.e2ee', function() {
 		expect(!folder1_2.encryption_cipher_text).toBe(true);
 	}));
 
+	it('should mark the key has having been used when synchronising the first time', (async () => {
+		setEncryptionEnabled(true);
+		await loadEncryptionMasterKey();
+		await Folder.save({ title: 'folder1' });
+		await synchronizerStart();
+
+		const localInfo = localSyncInfo();
+		const remoteInfo = await fetchSyncInfo(fileApi());
+		expect(localInfo.masterKeys[0].hasBeenUsed).toBe(true);
+		expect(remoteInfo.masterKeys[0].hasBeenUsed).toBe(true);
+	}));
+
+	it('should mark the key has having been used when synchronising after enabling encryption', (async () => {
+		await Folder.save({ title: 'folder1' });
+		await synchronizerStart();
+
+		setEncryptionEnabled(true);
+		await loadEncryptionMasterKey();
+		await synchronizerStart();
+
+		const localInfo = localSyncInfo();
+		const remoteInfo = await fetchSyncInfo(fileApi());
+		expect(localInfo.masterKeys[0].hasBeenUsed).toBe(true);
+		expect(remoteInfo.masterKeys[0].hasBeenUsed).toBe(true);
+	}));
+
 	it('should enable encryption automatically when downloading new master key (and none was previously available)',(async () => {
 		// Enable encryption on client 1 and sync an item
 		setEncryptionEnabled(true);

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

@@ -1,6 +1,6 @@
-import { afterAllCleanUp, setupDatabaseAndSynchronizer, switchClient, encryptionService } from '../../testing/test-utils';
+import { afterAllCleanUp, setupDatabaseAndSynchronizer, switchClient, encryptionService, msleep } from '../../testing/test-utils';
 import MasterKey from '../../models/MasterKey';
-import { masterKeyEnabled, setMasterKeyEnabled, SyncInfo, syncInfoEquals } from './syncInfoUtils';
+import { masterKeyEnabled, mergeSyncInfos, setMasterKeyEnabled, SyncInfo, syncInfoEquals } from './syncInfoUtils';
 
 describe('syncInfoUtils', function() {
 
@@ -92,4 +92,33 @@ describe('syncInfoUtils', function() {
 		}
 	});
 
+	it('should merge sync target info and takes into account usage of master key - 1', async () => {
+		const syncInfo1 = new SyncInfo();
+		syncInfo1.masterKeys = [{
+			id: '1',
+			content: 'content1',
+			hasBeenUsed: true,
+		}];
+		syncInfo1.activeMasterKeyId = '1';
+
+		await msleep(1);
+
+		const syncInfo2 = new SyncInfo();
+		syncInfo2.masterKeys = [{
+			id: '2',
+			content: 'content2',
+			hasBeenUsed: false,
+		}];
+		syncInfo2.activeMasterKeyId = '2';
+
+		// If one master key has been used and the other not, it should select
+		// the one that's been used regardless of timestamps.
+		expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('1');
+
+		// If both master keys have been used it should rely on timestamp
+		// (latest modified is picked).
+		syncInfo2.masterKeys[0].hasBeenUsed = true;
+		expect(mergeSyncInfos(syncInfo1, syncInfo2).activeMasterKeyId).toBe('2');
+	});
+
 });

packages/lib/services/synchronizer/syncInfoUtils.ts

@@ -90,14 +90,62 @@ export function localSyncInfoFromState(state: State): SyncInfo {
 	return new SyncInfo(state.settings['syncInfoCache']);
 }
 
+// When deciding which master key should be active we should take into account
+// whether it's been used or not. If it's been used before it should most likely
+// remain the active one, regardless of timestamps. This is because the extra
+// key was most likely created by mistake by the user, in particular in this
+// kind of scenario:
+//
+// - Client 1 setup sync with sync target
+// - Client 1 enable encryption
+// - Client 1 sync
+//
+// Then user 2 does the same:
+//
+// - Client 2 setup sync with sync target
+// - Client 2 enable encryption
+// - Client 2 sync
+//
+// The problem is that enabling encryption was not needed since it was already
+// done (and recorded in info.json) on the sync target. As a result an extra key
+// has been created and it has been set as the active one, but we shouldn't use
+// it. Instead the key created by client 1 should be used and made active again.
+//
+// And we can do this using the "hasBeenUsed" field which tells us which keys
+// 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.
+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) {
+			output.setWithTimestamp(s1, 'activeMasterKeyId');
+		} else if (!activeMasterKey1.hasBeenUsed && activeMasterKey2.hasBeenUsed) {
+			output.setWithTimestamp(s2, 'activeMasterKeyId');
+		} else {
+			doDefaultAction = true;
+		}
+	} else {
+		doDefaultAction = true;
+	}
+
+	if (doDefaultAction) {
+		output.setWithTimestamp(s1.keyTimestamp('activeMasterKeyId') > s2.keyTimestamp('activeMasterKeyId') ? s1 : s2, 'activeMasterKeyId');
+	}
+};
+
 export function mergeSyncInfos(s1: SyncInfo, s2: SyncInfo): SyncInfo {
 	const output: SyncInfo = new SyncInfo();
 
 	output.setWithTimestamp(s1.keyTimestamp('e2ee') > s2.keyTimestamp('e2ee') ? s1 : s2, 'e2ee');
-	output.setWithTimestamp(s1.keyTimestamp('activeMasterKeyId') > s2.keyTimestamp('activeMasterKeyId') ? s1 : s2, 'activeMasterKeyId');
 	output.setWithTimestamp(s1.keyTimestamp('ppk') > s2.keyTimestamp('ppk') ? s1 : s2, 'ppk');
 	output.version = s1.version > s2.version ? s1.version : s2.version;
 
+	mergeActiveMasterKeys(s1, s2, output);
+
 	output.masterKeys = s1.masterKeys.slice();
 
 	for (const mk of s2.masterKeys) {
@@ -154,6 +202,14 @@ export class SyncInfo {
 		this.activeMasterKeyId_ = 'activeMasterKeyId' in s ? s.activeMasterKeyId : { value: '', updatedTime: 0 };
 		this.masterKeys_ = 'masterKeys' in s ? s.masterKeys : [];
 		this.ppk_ = 'ppk' in s ? s.ppk : { value: null, updatedTime: 0 };
+
+		// Migration for master keys that didn't have "hasBeenUsed" property -
+		// in that case we assume they've been used at least once.
+		for (const mk of this.masterKeys_) {
+			if (!('hasBeenUsed' in mk) || mk.hasBeenUsed === undefined) {
+				mk.hasBeenUsed = true;
+			}
+		}
 	}
 
 	public setWithTimestamp(fromSyncInfo: SyncInfo, propName: string) {
@@ -275,6 +331,21 @@ export function setMasterKeyEnabled(mkId: string, enabled: boolean = true) {
 	saveLocalSyncInfo(s);
 }
 
+export const setMasterKeyHasBeenUsed = (s: SyncInfo, mkId: string) => {
+	const idx = s.masterKeys.findIndex(mk => mk.id === mkId);
+	if (idx < 0) throw new Error(`No such master key: ${mkId}`);
+
+	s.masterKeys[idx] = {
+		...s.masterKeys[idx],
+		hasBeenUsed: true,
+		updated_time: Date.now(),
+	};
+
+	saveLocalSyncInfo(s);
+
+	return s;
+};
+
 export function masterKeyEnabled(mk: MasterKeyEntity): boolean {
 	if ('enabled' in mk) return !!mk.enabled;
 	return true;