All: Allow disabling encryption and added more test cases

CliClient/app/command-encrypt-config.js

@@ -29,7 +29,7 @@ class Command extends BaseCommand {
 			const service = new EncryptionService();
 			let masterKey = await service.generateMasterKey(password);
 			masterKey = await MasterKey.save(masterKey);
-			await service.initializeEncryption(masterKey, password);
+			await service.enableEncryption(masterKey, password);
 		}
 	}
 

CliClient/tests/synchronizer.js

@@ -1,7 +1,7 @@
 require('app-module-path').addPath(__dirname);
 
 const { time } = require('lib/time-utils.js');
-const { setupDatabase, setupDatabaseAndSynchronizer, db, synchronizer, fileApi, sleep, clearDatabase, switchClient, syncTargetId, encryptionService, loadEncryptionMasterKey, fileContentEqual, decryptionWorker } = require('test-utils.js');
+const { setupDatabase, setupDatabaseAndSynchronizer, db, synchronizer, fileApi, sleep, clearDatabase, switchClient, syncTargetId, encryptionService, loadEncryptionMasterKey, fileContentEqual, decryptionWorker, checkThrowAsync } = require('test-utils.js');
 const { shim } = require('lib/shim.js');
 const Folder = require('lib/models/Folder.js');
 const Note = require('lib/models/Note.js');
@@ -26,6 +26,24 @@ async function allItems() {
 	return folders.concat(notes);
 }
 
+async function allSyncTargetItemsEncrypted() {
+	const list = await fileApi().list();
+	const files = list.items;
+
+	let output = false;
+	for (let i = 0; i < files.length; i++) {
+		const file = files[i];
+		const remoteContentString = await fileApi().get(file.path);
+		const remoteContent = await BaseItem.unserialize(remoteContentString);
+		const ItemClass = BaseItem.itemClass(remoteContent);
+
+		if (!ItemClass.encryptionSupported()) continue;
+		if (!!remoteContent.encryption_applied) output = true;
+	}	
+
+	return output;
+}
+
 async function localItemsSameAsRemote(locals, expect) {
 	try {
 		let files = await fileApi().list();
@@ -56,13 +74,19 @@ async function localItemsSameAsRemote(locals, expect) {
 	}
 }
 
+let insideBeforeEach = false;
+
 describe('Synchronizer', function() {
 
-	beforeEach( async (done) => {
+	beforeEach(async (done) => {
+		insideBeforeEach = true;
+
 		await setupDatabaseAndSynchronizer(1);
 		await setupDatabaseAndSynchronizer(2);
 		await switchClient(1);
 		done();
+
+		insideBeforeEach = false;
 	});
 
 	it('should create remote items', async (done) => {
@@ -605,7 +629,10 @@ describe('Synchronizer', function() {
 		await switchClient(2);
 
 		await synchronizer().start();
-		if (withEncryption) await loadEncryptionMasterKey(null, true);
+		if (withEncryption) {
+			await loadEncryptionMasterKey(null, true);
+			await decryptionWorker().start();
+		}
 		let note2 = await Note.load(note1.id);
 		note2.todo_completed = time.unixMs()-1;
 		await Note.save(note2);
@@ -654,6 +681,12 @@ describe('Synchronizer', function() {
 		done();
 	});
 
+	it('should always handle conflict if local or remote are encrypted', async (done) => {
+		await ignorableNoteConflictTest(true);
+
+		done();
+	});
+
 	it('items should be downloaded again when user cancels in the middle of delta operation', async (done) => {
 		let folder1 = await Folder.save({ title: "folder1" });
 		let note1 = await Note.save({ title: "un", is_todo: 1, parent_id: folder1.id });
@@ -746,12 +779,6 @@ describe('Synchronizer', function() {
 		done();
 	});
 
-	it('should always handle conflict if local or remote are encrypted', async (done) => {
-		await ignorableNoteConflictTest(true);
-
-		done();
-	});
-
 	it('should enable encryption automatically when downloading new master key (and none was previously available)', async (done) => {
 		// Enable encryption on client 1 and sync an item
 		Setting.setValue('encryption.enabled', true);
@@ -776,7 +803,7 @@ describe('Synchronizer', function() {
 		// If we sync now, nothing should be sent to target since we don't have a password.
 		// Technically it's incorrect to set the property of an encrypted variable but it allows confirming
 		// that encryption doesn't work if user hasn't supplied a password.
-		let folder1_2 = await Folder.save({ id: folder1.id, title: "change test" });
+		await BaseItem.forceSync(folder1.id);
 		await synchronizer().start();
 
 		await switchClient(1);
@@ -814,7 +841,6 @@ describe('Synchronizer', function() {
 
 	it('should encrypt existing notes too when enabling E2EE', async (done) => {
 		// First create a folder, without encryption enabled, and sync it
-		const service = encryptionService();
 		let folder1 = await Folder.save({ title: "folder1" });
 		await synchronizer().start();
 		let files = await fileApi().list()
@@ -822,10 +848,10 @@ describe('Synchronizer', function() {
 		expect(content.indexOf('folder1') >= 0).toBe(true)
 
 		// Then enable encryption and sync again
-		let masterKey = await service.generateMasterKey('123456');
+		let masterKey = await encryptionService().generateMasterKey('123456');
 		masterKey = await MasterKey.save(masterKey);
-		await service.initializeEncryption(masterKey, '123456');
-		await service.loadMasterKeysFromSettings();
+		await encryptionService().enableEncryption(masterKey, '123456');
+		await encryptionService().loadMasterKeysFromSettings();
 		await synchronizer().start();
 		
 		// Even though the folder has not been changed it should have been synced again so that
@@ -849,11 +875,14 @@ describe('Synchronizer', function() {
 		let resource1 = (await Resource.all())[0];
 		let resourcePath1 = Resource.fullPath(resource1);
 		await synchronizer().start();
+		expect((await fileApi().list()).items.length).toBe(3);
 
 		await switchClient(2);
 
 		await synchronizer().start();
-		let resource1_2 = (await Resource.all())[0];
+		let allResources = await Resource.all();
+		expect(allResources.length).toBe(1);
+		let resource1_2 = allResources[0];
 		let resourcePath1_2 = Resource.fullPath(resource1_2);
 
 		expect(resource1_2.id).toBe(resource1.id);
@@ -888,4 +917,62 @@ describe('Synchronizer', function() {
 		done();
 	});
 
+	it('should upload decrypted items to sync target after encryption disabled', async (done) => {
+		Setting.setValue('encryption.enabled', true);
+		const masterKey = await loadEncryptionMasterKey();
+
+		let folder1 = await Folder.save({ title: "folder1" });
+		await synchronizer().start();
+
+		let allEncrypted = await allSyncTargetItemsEncrypted();
+		expect(allEncrypted).toBe(true);
+
+		await encryptionService().disableEncryption();
+
+		await synchronizer().start();
+		allEncrypted = await allSyncTargetItemsEncrypted();
+		expect(allEncrypted).toBe(false);
+
+		done();
+	});
+
+	it('should not upload any item if encryption was enabled, and items have not been decrypted, and then encryption disabled', async (done) => {
+		// For some reason I can't explain, this test is sometimes executed before beforeEach is finished
+		// which means it's going to fail in unexpected way. So the loop below wait for beforeEach to be done.
+		while (insideBeforeEach) await time.msleep(100);
+
+		Setting.setValue('encryption.enabled', true);
+		const masterKey = await loadEncryptionMasterKey();
+
+		let folder1 = await Folder.save({ title: "folder1" });
+		await synchronizer().start();
+
+		await switchClient(2);
+
+		await synchronizer().start();
+		expect(Setting.value('encryption.enabled')).toBe(true);
+
+		// If we try to disable encryption now, it should throw an error because some items are
+		// currently encrypted. They must be decrypted first so that they can be sent as
+		// plain text to the sync target.
+		let hasThrown = await checkThrowAsync(async () => await encryptionService().disableEncryption());
+		expect(hasThrown).toBe(true);
+
+		// Now supply the password, and decrypt the items
+		Setting.setObjectKey('encryption.passwordCache', masterKey.id, '123456');
+		await encryptionService().loadMasterKeysFromSettings();	
+		await decryptionWorker().start();
+
+		// Try to disable encryption again
+		hasThrown = await checkThrowAsync(async () => await encryptionService().disableEncryption());
+		expect(hasThrown).toBe(false);
+
+		// If we sync now the target should receive the decrypted items
+		await synchronizer().start();
+		allEncrypted = await allSyncTargetItemsEncrypted();
+		expect(allEncrypted).toBe(false);
+
+		done();
+	});
+
 });

CliClient/tests/test-utils.js

@@ -98,7 +98,7 @@ async function switchClient(id) {
 	return Setting.load();
 }
 
-function clearDatabase(id = null) {
+async function clearDatabase(id = null) {
 	if (id === null) id = currentClient_;
 
 	let queries = [
@@ -114,31 +114,53 @@ function clearDatabase(id = null) {
 		'DELETE FROM sync_items',
 	];
 
-	return databases_[id].transactionExecBatch(queries);
+	await databases_[id].transactionExecBatch(queries);
 }
 
-function setupDatabase(id = null) {
+async function setupDatabase(id = null) {
 	if (id === null) id = currentClient_;
 
+	Setting.cancelScheduleSave();
+	Setting.cache_ = null;
+
 	if (databases_[id]) {
-		return clearDatabase(id).then(() => {
-			return Setting.load();
-		});
+		await clearDatabase(id);
+		await Setting.load();
+		return;
 	}
 
 	const filePath = __dirname + '/data/test-' + id + '.sqlite';
-	// Setting.setConstant('resourceDir', RNFetchBlob.fs.dirs.DocumentDir);
-	return fs.unlink(filePath).catch(() => {
+
+	try {
+		await fs.unlink(filePath);
+	} catch (error) {
 		// Don't care if the file doesn't exist
-	}).then(() => {
-		databases_[id] = new JoplinDatabase(new DatabaseDriverNode());
-		// databases_[id].setLogger(logger);
-		// console.info(filePath);
-		return databases_[id].open({ name: filePath }).then(() => {
-			BaseModel.db_ = databases_[id];
-			return setupDatabase(id);
-		});
-	});
+	};
+
+	databases_[id] = new JoplinDatabase(new DatabaseDriverNode());
+	await databases_[id].open({ name: filePath });
+
+	BaseModel.db_ = databases_[id];
+	await Setting.load();
+	//return setupDatabase(id);
+
+
+
+	// return databases_[id].open({ name: filePath }).then(() => {
+	// 	BaseModel.db_ = databases_[id];
+	// 	return setupDatabase(id);
+	// });
+
+
+	// return fs.unlink(filePath).catch(() => {
+	// 	// Don't care if the file doesn't exist
+	// }).then(() => {
+	// 	databases_[id] = new JoplinDatabase(new DatabaseDriverNode());
+	// 	return databases_[id].open({ name: filePath }).then(() => {
+	// 		BaseModel.db_ = databases_[id];
+	// 		return setupDatabase(id);
+	// 	});
+	// });
 }
 
 function resourceDir(id = null) {
@@ -151,6 +173,9 @@ async function setupDatabaseAndSynchronizer(id = null) {
 
 	await setupDatabase(id);
 
+	EncryptionService.instance_ = null;
+	DecryptionWorker.instance_ = null;
+
 	await fs.remove(resourceDir(id));
 	await fs.mkdirp(resourceDir(id), 0o755);
 

ReactNativeClient/lib/BaseModel.js

@@ -96,8 +96,11 @@ class BaseModel {
 		return options;
 	}
 
-	static count() {
-		return this.db().selectOne('SELECT count(*) as total FROM `' + this.tableName() + '`').then((r) => {
+	static count(options = null) {
+		if (!options) options = {};
+		let sql = 'SELECT count(*) as total FROM `' + this.tableName() + '`';
+		if (options.where) sql += ' WHERE ' + options.where;
+		return this.db().selectOne(sql).then((r) => {
 			return r ? r['total'] : 0;
 		});
 	}

ReactNativeClient/lib/database.js

@@ -104,29 +104,38 @@ class Database {
 		return this.tryCall('exec', sql, params);
 	}
 
-	transactionExecBatch(queries) {
-		if (queries.length <= 0) return Promise.resolve();
+	async transactionExecBatch(queries) {
+		if (queries.length <= 0) return;
 
 		if (queries.length == 1) {
 			let q = this.wrapQuery(queries[0]);
-			return this.exec(q.sql, q.params);
+			await this.exec(q.sql, q.params);
+			return;
 		}
 
 		// There can be only one transaction running at a time so queue
 		// any new transaction here.
 		if (this.inTransaction_) {
-			return new Promise((resolve, reject) => {
-				let iid = setInterval(() => {
-					if (!this.inTransaction_) {
-						clearInterval(iid);
-						this.transactionExecBatch(queries).then(() => {
-							resolve();
-						}).catch((error) => {
-							reject(error);
-						});
-					}
-				}, 100);
-			});
+			while (true) {
+				await time.msleep(100);
+				if (!this.inTransaction_) {
+					this.inTransaction_ = true;
+					break;
+				}
+			}
+
+			// return new Promise((resolve, reject) => {
+			// 	let iid = setInterval(() => {
+			// 		if (!this.inTransaction_) {
+			// 			clearInterval(iid);
+			// 			this.transactionExecBatch(queries).then(() => {
+			// 				resolve();
+			// 			}).catch((error) => {
+			// 				reject(error);
+			// 			});
+			// 		}
+			// 	}, 100);
+			// });
 		}
 
 		this.inTransaction_ = true;
@@ -134,17 +143,62 @@ class Database {
 		queries.splice(0, 0, 'BEGIN TRANSACTION');
 		queries.push('COMMIT'); // Note: ROLLBACK is currently not supported
 
-		let chain = [];
 		for (let i = 0; i < queries.length; i++) {
 			let query = this.wrapQuery(queries[i]);
-			chain.push(() => {
-				return this.exec(query.sql, query.params);
-			});
+			await this.exec(query.sql, query.params);
 		}
 
-		return promiseChain(chain).then(() => {
-			this.inTransaction_ = false;
-		});
+		this.inTransaction_ = false;
+
+		// return promiseChain(chain).then(() => {
+		// 	this.inTransaction_ = false;
+		// });
+
+
+
+
+
+
+		// if (queries.length <= 0) return Promise.resolve();
+
+		// if (queries.length == 1) {
+		// 	let q = this.wrapQuery(queries[0]);
+		// 	return this.exec(q.sql, q.params);
+		// }
+
+		// // There can be only one transaction running at a time so queue
+		// // any new transaction here.
+		// if (this.inTransaction_) {
+		// 	return new Promise((resolve, reject) => {
+		// 		let iid = setInterval(() => {
+		// 			if (!this.inTransaction_) {
+		// 				clearInterval(iid);
+		// 				this.transactionExecBatch(queries).then(() => {
+		// 					resolve();
+		// 				}).catch((error) => {
+		// 					reject(error);
+		// 				});
+		// 			}
+		// 		}, 100);
+		// 	});
+		// }
+
+		// this.inTransaction_ = true;
+
+		// queries.splice(0, 0, 'BEGIN TRANSACTION');
+		// queries.push('COMMIT'); // Note: ROLLBACK is currently not supported
+
+		// let chain = [];
+		// for (let i = 0; i < queries.length; i++) {
+		// 	let query = this.wrapQuery(queries[i]);
+		// 	chain.push(() => {
+		// 		return this.exec(query.sql, query.params);
+		// 	});
+		// }
+
+		// return promiseChain(chain).then(() => {
+		// 	this.inTransaction_ = false;
+		// });
 	}
 
 	static enumId(type, s) {

ReactNativeClient/lib/models/BaseItem.js

@@ -258,7 +258,13 @@ class BaseItem extends BaseModel {
 	static async serializeForSync(item) {
 		const ItemClass = this.itemClass(item);
 		let serialized = await ItemClass.serialize(item);
-		if (!Setting.value('encryption.enabled') || !ItemClass.encryptionSupported()) return serialized;
+		if (!Setting.value('encryption.enabled') || !ItemClass.encryptionSupported()) {
+			// Sanity check - normally not possible
+			if (!!item.encryption_applied) throw new Error('Item is encrypted but encryption is currently disabled');
+			return serialized;
+		}
+
+		if (!!item.encryption_applied) { const e = new Error('Trying to encrypt item that is already encrypted'); e.code = 'cannotEncryptEncrypted'; throw e; }
 
 		const cipherText = await this.encryptionService().encryptString(serialized);
 
@@ -343,6 +349,20 @@ class BaseItem extends BaseModel {
 		return output;
 	}
 
+	static async hasEncryptedItems() {
+		const classNames = this.encryptableItemClassNames();
+
+		for (let i = 0; i < classNames.length; i++) {
+			const className = classNames[i];
+			const ItemClass = this.getClass(className);
+
+			const count = await ItemClass.count({ where: 'encryption_applied = 1' });
+			if (count) return true;
+		}
+
+		return false;
+	}
+
 	static async itemsThatNeedDecryption(exclusions = [], limit = 100) {
 		const classNames = this.encryptableItemClassNames();
 
@@ -568,6 +588,14 @@ class BaseItem extends BaseModel {
 		}
 	}
 
+	static async forceSync(itemId) {
+		await this.db().exec('UPDATE sync_items SET force_sync = 1 WHERE item_id = ?', [itemId]);
+	}
+
+	static async forceSyncAll() {
+		await this.db().exec('UPDATE sync_items SET force_sync = 1');
+	}
+
 	static async save(o, options = null) {
 		if (!options) options = {};
 

ReactNativeClient/lib/models/Resource.js

@@ -81,10 +81,14 @@ class Resource extends BaseItem {
 		const plainTextPath = this.fullPath(resource);
 
 		if (!Setting.value('encryption.enabled')) {
-			if (resource.encryption_blob_encrypted) {
-				resource.encryption_blob_encrypted = 0;
-				await Resource.save(resource, { autoTimestamp: false });
-			}
+			// Sanity check - normally not possible
+			if (!!resource.encryption_blob_encrypted) throw new Error('Trying to access encrypted resource but encryption is currently disabled');
+
+			// // TODO: why is it set to 0 without decrypting first?
+			// if (resource.encryption_blob_encrypted) {
+			// 	resource.encryption_blob_encrypted = 0;
+			// 	await Resource.save(resource, { autoTimestamp: false });
+			// }
 			return { path: plainTextPath, resource: resource };
 		}
 

ReactNativeClient/lib/services/EncryptionService.js

@@ -3,6 +3,7 @@ const { shim } = require('lib/shim.js');
 const Setting = require('lib/models/Setting.js');
 const MasterKey = require('lib/models/MasterKey');
 const BaseItem = require('lib/models/BaseItem');
+const { _ } = require('lib/locale.js');
 
 function hexPad(s, length) {
 	return padLeft(s, length, '0');
@@ -33,7 +34,7 @@ class EncryptionService {
 		return this.logger_;
 	}
 
-	async initializeEncryption(masterKey, password = null) {
+	async enableEncryption(masterKey, password = null) {
 		Setting.setValue('encryption.enabled', true);
 		Setting.setValue('encryption.activeMasterKeyId', masterKey.id);
 
@@ -43,9 +44,21 @@ class EncryptionService {
 			Setting.setValue('encryption.passwordCache', passwordCache);		
 		}
 
+		// Mark only the non-encrypted ones for sync since, if there are encrypted ones,
+		// it means they come from the sync target and are already encrypted over there.
 		await BaseItem.markAllNonEncryptedForSync();
 	}
 
+	async disableEncryption() {
+		const hasEncryptedItems = await BaseItem.hasEncryptedItems();
+		if (hasEncryptedItems) throw new Error(_('Encryption cannot currently be disabled because some items are still encrypted. Please wait for all the items to be decrypted and try again.'));
+		
+		Setting.setValue('encryption.enabled', false);
+		// The only way to make sure everything gets decrypted on the sync target is
+		// to re-sync everything.
+		await BaseItem.forceSyncAll();
+	}
+
 	async loadMasterKeysFromSettings() {
 		if (!Setting.value('encryption.enabled')) {
 			this.unloadAllMasterKeys();

ReactNativeClient/lib/synchronizer.js

@@ -217,7 +217,6 @@ class Synchronizer {
 					if (donePaths.indexOf(path) > 0) throw new Error(sprintf('Processing a path that has already been done: %s. sync_time was not updated?', path));
 
 					let remote = await this.api().stat(path);
-					//let content = await ItemClass.serializeForSync(local);
 					let action = null;
 					let updateSyncTimeOnly = true;
 					let reason = '';					
@@ -561,9 +560,9 @@ class Synchronizer {
 				await BaseItem.deleteOrphanSyncItems();
 			}
 		} catch (error) {
-			if (error && error.code === 'noActiveMasterKey') {
-				// Don't log an error for this as this is a common
-				// condition that the UI should report anyway.
+			if (error && ['cannotEncryptEncrypted', 'noActiveMasterKey'].indexOf(error.code) >= 0) {
+				// Only log an info statement for this since this is a common condition that is reported
+				// in the application, and needs to be resolved by the user
 				this.logger().info(error.message);
 			} else {
 				this.logger().error(error);
@@ -583,7 +582,7 @@ class Synchronizer {
 			const mk = await MasterKey.latest();
 			if (mk) {
 				this.logger().info('Using master key: ', mk);
-				await this.encryptionService().initializeEncryption(mk);
+				await this.encryptionService().enableEncryption(mk);
 				await this.encryptionService().loadMasterKeysFromSettings();
 				this.logger().info('Encryption has been enabled with downloaded master key as active key. However, note that no password was initially supplied. It will need to be provided by user.');
 			}