Mobile,Desktop: Resolves #11872: Explain why items could not be decrypted (#12048)

packages/app-mobile/components/screens/status.tsx

@@ -1,16 +1,17 @@
 import * as React from 'react';
 
-import { View, Text, Button, FlatList, TextStyle, StyleSheet } from 'react-native';
+import { View, Text, Button, FlatList, TextStyle, StyleSheet, Role } from 'react-native';
 import Setting from '@joplin/lib/models/Setting';
 import { connect } from 'react-redux';
 import { ScreenHeader } from '../ScreenHeader';
-import ReportService, { ReportSection } from '@joplin/lib/services/ReportService';
+import ReportService, { ReportItemType, ReportSection } from '@joplin/lib/services/ReportService';
 import { _ } from '@joplin/lib/locale';
 import { BaseScreenComponent } from '../base-screen';
 import { themeStyle } from '../global-style';
 import { AppState } from '../../utils/types';
 import checkDisabledSyncItemsNotification from '@joplin/lib/services/synchronizer/utils/checkDisabledSyncItemsNotification';
 import { Dispatch } from 'redux';
+import Icon from '../Icon';
 
 interface Props {
 	themeId: number;
@@ -21,6 +22,86 @@ interface State {
 	report: ReportSection[];
 }
 
+interface ProcessedLine {
+	key: string;
+	text?: string;
+	isSection?: boolean;
+	isDivider?: boolean;
+	retryAllHandler?: ()=> void;
+	retryHandler?: ()=> void;
+	ignoreHandler?: ()=> void;
+	listItems?: ProcessedLine[];
+}
+
+type OnRefreshScreen = ()=> Promise<void>;
+
+const processReport = (report: ReportSection[], refreshScreen: OnRefreshScreen, dispatch: Dispatch, baseStyle: TextStyle) => {
+	const lines: ProcessedLine[] = [];
+	let currentList: ProcessedLine[]|null = null;
+
+	for (let i = 0; i < report.length; i++) {
+		const section = report[i];
+
+		let style: TextStyle = { ...baseStyle };
+		style.fontWeight = 'bold';
+		if (i > 0) style.paddingTop = 20;
+		lines.push({ key: `section_${i}`, isSection: true, text: section.title });
+		if (section.canRetryAll) {
+			lines.push({ key: `retry_all_${i}`, text: '', retryAllHandler: section.retryAllHandler });
+		}
+
+		for (const n in section.body) {
+			if (!section.body.hasOwnProperty(n)) continue;
+			style = { ...baseStyle };
+			const item = section.body[n];
+
+			let text = '';
+
+			let retryHandler = null;
+			let ignoreHandler = null;
+			if (typeof item === 'object') {
+				if (item.canRetry) {
+					retryHandler = async () => {
+						await item.retryHandler();
+						await refreshScreen();
+					};
+				}
+				if (item.canIgnore) {
+					ignoreHandler = async () => {
+						await item.ignoreHandler();
+						await refreshScreen();
+						await checkDisabledSyncItemsNotification((action) => dispatch(action));
+					};
+				}
+				if (item.type === ReportItemType.OpenList) {
+					currentList = [];
+				} else if (item.type === ReportItemType.CloseList) {
+					lines.push({ key: `list_${i}_${n}`, listItems: currentList });
+					currentList = null;
+				}
+				text = item.text;
+			} else {
+				text = item;
+			}
+
+			const line = { key: `item_${i}_${n}`, text: text, retryHandler, ignoreHandler };
+			if (currentList) {
+				// The OpenList item, for example, might be empty and should be skipped:
+				const hasContent = line.text || retryHandler || ignoreHandler;
+				if (hasContent) {
+					currentList.push(line);
+				}
+			} else {
+				lines.push(line);
+			}
+		}
+
+		lines.push({ key: `divider2_${i}`, isDivider: true });
+	}
+
+	return lines;
+};
+
 class StatusScreenComponent extends BaseScreenComponent<Props, State> {
 	public constructor(props: Props) {
 		super(props);
@@ -52,15 +133,11 @@ class StatusScreenComponent extends BaseScreenComponent<Props, State> {
 				marginLeft: 2,
 				marginRight: 2,
 			},
-		});
+			retryAllButton: {
-	}
+				flexGrow: 0,
-
+				alignSelf: 'flex-start',
-	public override render() {
+			},
-		const theme = themeStyle(this.props.themeId);
+			baseStyle: {
-		const styles = this.styles();
-
-		const renderBody = (report: ReportSection[]) => {
-			const baseStyle = {
 				paddingLeft: 6,
 				paddingRight: 6,
 				paddingTop: 2,
@@ -68,70 +145,49 @@ class StatusScreenComponent extends BaseScreenComponent<Props, State> {
 				flex: 0,
 				color: theme.color,
 				fontSize: theme.fontSize,
-			};
+				alignSelf: 'center',
-
+			},
-			const lines = [];
+			listWrapper: {
-
+				paddingBottom: 5,
-			for (let i = 0; i < report.length; i++) {
+			},
-				const section = report[i];
+			listBullet: {
-
+				fontSize: theme.fontSize / 3,
-				let style: TextStyle = { ...baseStyle };
+				color: theme.color,
-				style.fontWeight = 'bold';
+				alignSelf: 'center',
-				if (i > 0) style.paddingTop = 20;
+				justifyContent: 'center',
-				lines.push({ key: `section_${i}`, isSection: true, text: section.title });
+				flexGrow: 0,
-				if (section.canRetryAll) {
+				marginStart: 12,
-					lines.push({ key: `retry_all_${i}`, text: '', retryAllHandler: section.retryAllHandler });
+				marginEnd: 2,
+			},
+			divider: {
+				borderBottomWidth: 1,
+				borderBottomColor: theme.dividerColor,
+				marginTop: 20,
+				marginBottom: 20,
+			},
+		});
 	}
 
-				for (const n in section.body) {
+	public override render() {
-					if (!section.body.hasOwnProperty(n)) continue;
+		const styles = this.styles();
-					style = { ...baseStyle };
-					const item = section.body[n];
 
-					let text = '';
+		const renderItem = (item: ProcessedLine, inList: boolean) => {
-
+			const style: TextStyle = { ...styles.baseStyle };
-					let retryHandler = null;
-					let ignoreHandler = null;
-					if (typeof item === 'object') {
-						if (item.canRetry) {
-							retryHandler = async () => {
-								await item.retryHandler();
-								await this.refreshScreen();
-							};
-						}
-						if (item.canIgnore) {
-							ignoreHandler = async () => {
-								await item.ignoreHandler();
-								await this.refreshScreen();
-								await checkDisabledSyncItemsNotification((action) => this.props.dispatch(action));
-							};
-						}
-						text = item.text;
-					} else {
-						text = item;
-					}
-
-					lines.push({ key: `item_${i}_${n}`, text: text, retryHandler, ignoreHandler });
-				}
-
-				lines.push({ key: `divider2_${i}`, isDivider: true });
-			}
-
-			return (
-				<FlatList
-					data={lines}
-					renderItem={({ item }) => {
-						const style: TextStyle = { ...baseStyle };
 
+			let textRole: Role|null = undefined;
+			const text = item.text;
 			if (item.isSection === true) {
 				style.fontWeight = 'bold';
 				style.marginBottom = 5;
+				textRole = 'heading';
+			} else if (inList) {
+				textRole = 'listitem';
 			}
 
 			style.flex = 1;
 
 			const retryAllButton = item.retryAllHandler ? (
-							<View style={{ flex: 0 }}>
+				<View style={styles.retryAllButton}>
 					<Button title={_('Retry All')} onPress={item.retryAllHandler} />
 				</View>
 			) : null;
@@ -148,18 +204,36 @@ class StatusScreenComponent extends BaseScreenComponent<Props, State> {
 				</View>
 			) : null;
 
+			const textComponent = text ? <Text style={style} role={textRole}>{text}</Text> : null;
 			if (item.isDivider) {
-							return <View style={{ borderBottomWidth: 1, borderBottomColor: theme.dividerColor, marginTop: 20, marginBottom: 20 }} />;
+				return <View style={styles.divider} role='separator' key={item.key} />;
+			} else if (item.listItems) {
+				return <View role='list' style={styles.listWrapper} key={item.key}>
+					{textComponent}
+					{item.listItems.map(item => renderItem(item, true))}
+				</View>;
 			} else {
 				return (
-								<View style={{ flex: 1, flexDirection: 'row' }}>
+					<View style={{ flex: 1, flexDirection: 'row' }} key={item.key}>
-									<Text style={style}>{item.text}</Text>
+						{inList ? <Icon style={styles.listBullet} name='fas fa-circle' accessibilityLabel={null} /> : null}
+						{textComponent}
 						{ignoreButton}
 						{retryAllButton}
 						{retryButton}
 					</View>
 				);
 			}
+		};
+
+		const renderBody = (report: ReportSection[]) => {
+			const baseStyle = styles.baseStyle;
+			const lines = processReport(report, () => this.refreshScreen(), this.props.dispatch, baseStyle);
+
+			return (
+				<FlatList
+					data={lines}
+					renderItem={({ item }) => {
+						return renderItem(item, false);
 					}}
 				/>
 			);

packages/lib/services/DecryptionWorker.ts

@@ -18,6 +18,16 @@ interface DecryptionResult {
 	error: any;
 }
 
+// Key for use with the KvStore.
+const decryptionErrorKeyPrefix = 'decryptErrorLabel:';
+const decryptionErrorKey = (type: number, id: string) => {
+	return `${decryptionErrorKeyPrefix}${type}:${id}`;
+};
+const decryptionCounterKeyPrefix = 'decrypt:';
+const decryptionCounterKey = (type: number, id: string) => {
+	return `${decryptionCounterKeyPrefix}${type}:${id}`;
+};
+
 export default class DecryptionWorker {
 
 	public static instance_: DecryptionWorker = null;
@@ -96,24 +106,29 @@ export default class DecryptionWorker {
 	}
 
 	public async decryptionDisabledItems() {
-		let items = await this.kvStore().searchByPrefix('decrypt:');
+		let items = await this.kvStore().searchByPrefix(decryptionCounterKeyPrefix);
 		items = items.filter(item => item.value > this.maxDecryptionAttempts_);
-		items = items.map(item => {
+		return await Promise.all(items.map(async item => {
 			const s = item.key.split(':');
+			const type_ = Number(s[1]);
+			const id = s[2];
+			const errorDescription = await this.kvStore().value<string>(decryptionErrorKey(type_, id));
 			return {
-				type_: Number(s[1]),
+				type_,
-				id: s[2],
+				id,
+				reason: errorDescription,
 			};
-		});
+		}));
-		return items;
 	}
 
-	public async clearDisabledItem(typeId: string, itemId: string) {
+	public async clearDisabledItem(typeId: number, itemId: string) {
-		await this.kvStore().deleteValue(`decrypt:${typeId}:${itemId}`);
+		await this.kvStore().deleteValue(decryptionCounterKey(typeId, itemId));
+		await this.kvStore().deleteValue(decryptionErrorKey(typeId, itemId));
 	}
 
 	public async clearDisabledItems() {
-		await this.kvStore().deleteByPrefix('decrypt:');
+		await this.kvStore().deleteByPrefix(decryptionCounterKeyPrefix);
+		await this.kvStore().deleteByPrefix(decryptionErrorKeyPrefix);
 	}
 
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
@@ -193,10 +208,14 @@ export default class DecryptionWorker {
 						itemCount: items.length,
 					});
 
-					const counterKey = `decrypt:${item.type_}:${item.id}`;
+					const counterKey = decryptionCounterKey(item.type_, item.id);
+					const errorKey = decryptionErrorKey(item.type_, item.id);
 
 					const clearDecryptionCounter = async () => {
 						await this.kvStore().deleteValue(counterKey);
+						// The decryption error key stores the reason for the decryption counter's value.
+						// As such, the error should be reset when the decryption counter is reset:
+						await this.kvStore().deleteValue(errorKey);
 					};
 
 					// Don't log in production as it results in many messages when importing many items
@@ -253,6 +272,8 @@ export default class DecryptionWorker {
 							throw error;
 						}
 
+						await this.kvStore().setValue(errorKey, String(error));
+
 						if (options.errorHandler === 'log') {
 							this.logger().warn(`DecryptionWorker: error for: ${item.id} (${ItemClass.tableName()})`, error);
 							this.logger().debug('Item with error:', item);

packages/lib/services/KvStore.ts

@@ -6,6 +6,13 @@ enum ValueType {
 	Text = 2,
 }
 
+interface KvStoreKeyValue {
+	key: string;
+	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Partial refactoring of old code from before rule was applied
+	value: any;
+	type: ValueType;
+}
+
 export default class KvStore extends BaseService {
 
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
@@ -110,7 +117,7 @@ export default class KvStore extends BaseService {
 		}
 	}
 
-	public async searchByPrefix(prefix: string) {
+	public async searchByPrefix(prefix: string): Promise<KvStoreKeyValue[]> {
 		const results = await this.db().selectAll('SELECT `key`, `value`, `type` FROM key_values WHERE `key` LIKE ?', [`${prefix}%`]);
 		return this.formatValues_(results);
 	}

packages/lib/services/ReportService.test.ts

@@ -1,11 +1,15 @@
 import { _ } from '../locale';
 import ReportService, { ReportSection } from './ReportService';
-import { createNTestNotes, decryptionWorker, setupDatabaseAndSynchronizer, supportDir, switchClient, syncTargetId, synchronizer, synchronizerStart } from '../testing/test-utils';
+import { createNTestNotes, decryptionWorker, encryptionService, loadEncryptionMasterKey, setupDatabaseAndSynchronizer, supportDir, switchClient, syncTargetId, synchronizer, synchronizerStart } from '../testing/test-utils';
 import Folder from '../models/Folder';
 import BaseItem from '../models/BaseItem';
-import DecryptionWorker from './DecryptionWorker';
 import Note from '../models/Note';
 import shim from '../shim';
+import SyncTargetRegistry from '../SyncTargetRegistry';
+import { loadMasterKeysFromSettings, setupAndEnableEncryption } from './e2ee/utils';
+import Setting from '../models/Setting';
+import DecryptionWorker from './DecryptionWorker';
+import { ModelType } from '../BaseModel';
 
 
 const firstSectionWithTitle = (report: ReportSection[], title: string) => {
@@ -22,6 +26,10 @@ const getIgnoredSection = (report: ReportSection[]) => {
 	return firstSectionWithTitle(report, _('Ignored items that cannot be synchronised'));
 };
 
+const getDecryptionErrorSection = (report: ReportSection[]): ReportSection|null => {
+	return firstSectionWithTitle(report, _('Items that cannot be decrypted'));
+};
+
 const sectionBodyToText = (section: ReportSection) => {
 	return section.body.map(item => {
 		if (typeof item === 'string') {
@@ -32,13 +40,71 @@ const sectionBodyToText = (section: ReportSection) => {
 	}).join('\n');
 };
 
+const getListItemsInBodyStartingWith = (section: ReportSection, keyPrefix: string) => {
+	return section.body.filter(item =>
+		typeof item !== 'string' && item.type === 'openList' && item.key.startsWith(keyPrefix),
+	);
+};
+
+const addCannotDecryptNotes = async (corruptedNoteCount: number) => {
+	await switchClient(2);
+
+	const notes = [];
+	for (let i = 0; i < corruptedNoteCount; i++) {
+		notes.push(await Note.save({ title: `Note ${i}` }));
+	}
+
+	await synchronizerStart();
+	await switchClient(1);
+	await synchronizerStart();
+
+	// First, simulate a broken note and check that the decryption worker
+	// gives up decrypting after a number of tries. This is mainly relevant
+	// for data that crashes the mobile application - we don't want to keep
+	// decrypting these.
+
+	for (const note of notes) {
+		await Note.save({ id: note.id, encryption_cipher_text: 'bad' });
+	}
+
+	return notes.map(note => note.id);
+};
+
+const addRemoteNotes = async (noteCount: number) => {
+	await switchClient(2);
+
+	const notes = [];
+	for (let i = 0; i < noteCount; i++) {
+		notes.push(await Note.save({ title: `Test Note ${i}` }));
+	}
+
+	await synchronizerStart();
+	await switchClient(1);
+
+	return notes.map(note => note.id);
+};
+
+const setUpLocalAndRemoteEncryption = async () => {
+	await switchClient(2);
+
+	// Encryption setup
+	const masterKey = await loadEncryptionMasterKey();
+	await setupAndEnableEncryption(encryptionService(), masterKey, '123456');
+	await synchronizerStart();
+
+	// Give both clients the same master key
+	await switchClient(1);
+	await synchronizerStart();
+
+	Setting.setObjectValue('encryption.passwordCache', masterKey.id, '123456');
+	await loadMasterKeysFromSettings(encryptionService());
+};
+
 describe('ReportService', () => {
 	beforeEach(async () => {
 		await setupDatabaseAndSynchronizer(1);
 		await setupDatabaseAndSynchronizer(2);
 		await switchClient(1);
-		// For compatibility with code that calls DecryptionWorker.instance()
-		DecryptionWorker.instance_ = decryptionWorker();
 	});
 
 	it('should move sync errors to the "ignored" section after clicking "ignore"', async () => {
@@ -129,6 +195,7 @@ describe('ReportService', () => {
 		let report = await service.status(syncTargetId());
 
 		const unsyncableSection = getCannotSyncSection(report);
+		expect(unsyncableSection).not.toBeNull();
 		expect(sectionBodyToText(unsyncableSection)).toContain('could not be downloaded');
 
 		// Item for the download error should be ignorable
@@ -159,4 +226,85 @@ describe('ReportService', () => {
 		expect(getIgnoredSection(report)).toBeNull();
 		expect(getCannotSyncSection(report)).toBeNull();
 	});
+
+	it('should associate decryption failures with error message headers when errors are known', async () => {
+		await setUpLocalAndRemoteEncryption();
+
+		const service = new ReportService();
+		const syncTargetId = SyncTargetRegistry.nameToId('joplinServer');
+		let report = await service.status(syncTargetId);
+
+		// Initially, should not have a "cannot be decrypted section"
+		expect(getDecryptionErrorSection(report)).toBeNull();
+
+		const corruptedNoteIds = await addCannotDecryptNotes(4);
+		await addRemoteNotes(10);
+		await synchronizerStart();
+
+		for (let i = 0; i < 3; i++) {
+			report = await service.status(syncTargetId);
+			expect(getDecryptionErrorSection(report)).toBeNull();
+
+			// .start needs to be run multiple times for items to be disabled and thus
+			// added to the report
+			await decryptionWorker().start();
+		}
+
+		// After adding corrupted notes, it should have such a section.
+		report = await service.status(syncTargetId);
+		const decryptionErrorsSection = getDecryptionErrorSection(report);
+		expect(decryptionErrorsSection).not.toBeNull();
+
+		// There should be a list of errors (all errors are known)
+		const errorLists = getListItemsInBodyStartingWith(decryptionErrorsSection, 'itemsWithError');
+		expect(errorLists).toHaveLength(1);
+
+		// There should, however, be testIds.length ReportItems with the IDs of the notes.
+		const decryptionErrorsText = sectionBodyToText(decryptionErrorsSection);
+		for (const noteId of corruptedNoteIds) {
+			expect(decryptionErrorsText).toContain(noteId);
+		}
+	});
+
+	it('should not associate decryption failures with error message headers when errors are unknown', async () => {
+		const decryption = decryptionWorker();
+
+		// Create decryption errors:
+		const testIds = ['0123456789012345601234567890123456', '0123456789012345601234567890123457', '0123456789012345601234567890123458'];
+
+		// Adds items to the decryption error list **without also adding the reason**. This matches
+		// the format of older decryption errors.
+		const addIdsToDecryptionErrorList = async (worker: DecryptionWorker, ids: string[]) => {
+			for (const id of ids) {
+				// A value that is more than the maximum number of attempts:
+				const numDecryptionAttempts = 3;
+
+				// Add the failure manually so that the error message is unknown
+				await worker.kvStore().setValue(
+					`decrypt:${ModelType.Note}:${id}`, numDecryptionAttempts,
+				);
+			}
+		};
+
+		await addIdsToDecryptionErrorList(decryption, testIds);
+
+		const service = new ReportService();
+		const syncTargetId = SyncTargetRegistry.nameToId('joplinServer');
+		const report = await service.status(syncTargetId);
+
+		// Report should have an "Items that cannot be decrypted" section
+		const decryptionErrorSection = getDecryptionErrorSection(report);
+		expect(decryptionErrorSection).not.toBeNull();
+
+		// There should not be any lists of errors (no errors associated with the item).
+		const errorLists = getListItemsInBodyStartingWith(decryptionErrorSection, 'itemsWithError');
+		expect(errorLists).toHaveLength(0);
+
+		// There should be items with the correct messages:
+		const expectedMessages = testIds.map(id => `Note: ${id}`);
+		const bodyText = sectionBodyToText(decryptionErrorSection);
+		for (const message of expectedMessages) {
+			expect(bodyText).toContain(message);
+		}
+	});
 });

packages/lib/services/ReportService.ts

@@ -16,7 +16,7 @@ enum CanRetryType {
 	ItemSync = 'itemSync',
 }
 
-enum ReportItemType {
+export enum ReportItemType {
 	OpenList = 'openList',
 	CloseList = 'closeList',
 }
@@ -255,17 +255,51 @@ export default class ReportService {
 
 			section.body.push('');
 
+			const errorMessagesToItems: Map<string, ReportItem[]> = new Map();
+
 			for (let i = 0; i < decryptionDisabledItems.length; i++) {
 				const row = decryptionDisabledItems[i];
-				section.body.push({
+
-					text: _('%s: %s', toTitleCase(BaseModel.modelTypeToName(row.type_)), row.id),
+				const resourceTypeName = toTitleCase(BaseModel.modelTypeToName(row.type_));
+				const message = _('%s: %s', resourceTypeName, row.id);
+
+				const item: ReportItem = {
+					text: message,
 					canRetry: true,
 					canRetryType: CanRetryType.E2EE,
 					retryHandler: async () => {
 						await DecryptionWorker.instance().clearDisabledItem(row.type_, row.id);
 						void DecryptionWorker.instance().scheduleStart();
 					},
-				});
+				};
+
+				const itemError = row.reason;
+				if (itemError) {
+					// If the error message is known, postpone adding the report item.
+					// Instead, add it under the error message as a heading
+					if (errorMessagesToItems.has(itemError)) {
+						errorMessagesToItems.get(itemError).push(item);
+					} else {
+						errorMessagesToItems.set(itemError, [item]);
+					}
+				} else {
+					// If there's no known error, add directly:
+					section.body.push(item);
+				}
+			}
+
+			// Categorize any items under each known error:
+			let errorIdx = 0;
+			for (const itemError of errorMessagesToItems.keys()) {
+				section.body.push(_('Items with error: %s', itemError));
+
+				errorIdx++;
+				section.body.push({ type: ReportItemType.OpenList, key: `itemsWithError${errorIdx}` });
+
+				// Add all items associated with the header
+				section.body.push(...errorMessagesToItems.get(itemError));
+
+				section.body.push({ type: ReportItemType.CloseList });
 			}
 
 			section = this.addRetryAllHandler(section);

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

@@ -409,7 +409,7 @@ describe('Synchronizer.e2ee', () => {
 		expect(disabledItems.length).toBe(1);
 		expect(disabledItems[0].id).toBe(note.id);
 
-		expect((await kvStore().all()).length).toBe(1);
+		expect((await kvStore().searchByPrefix('decrypt:')).length).toBe(1);
 		await kvStore().clear();
 
 		// Now check that if it fails once but succeed the second time, the note

packages/lib/testing/test-utils.ts

@@ -287,6 +287,7 @@ async function switchClient(id: number, options: any = null) {
 	Resource.encryptionService_ = encryptionServices_[id];
 	BaseItem.revisionService_ = revisionServices_[id];
 	ResourceFetcher.instance_ = resourceFetchers_[id];
+	DecryptionWorker.instance_ = decryptionWorker(id);
 
 	await Setting.reset();
 	Setting.settingFilename = settingFilename(id);
@@ -549,7 +550,7 @@ function revisionService(id: number = null) {
 function decryptionWorker(id: number = null) {
 	if (id === null) id = currentClient_;
 	const o = decryptionWorkers_[id];
-	o.setKvStore(kvStore(id));
+	o?.setKvStore(kvStore(id));
 	return o;
 }