self-hosted/joplin
1
0
Fork 0
mirror of https://github.com/laurent22/joplin.git synced 2025-08-13 22:12:50 +02:00
Code Issues Releases Activity

All: Fixes #1433: Some resources could incorrectly be deleted even though they are still present in a note. Also added additional verifications before deleting a resource.

Browse Source
This commit is contained in:
Laurent Cozic
2019-04-21 13:49:40 +01:00
parent 220f5d0967
commit a5ee120281
4 changed files with 144 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files

69
CliClient/tests/services_ResourceService.js
View File

@@ -1,7 +1,7 @@
require('app-module-path').addPath(__dirname);
const { time } = require('lib/time-utils.js');
const { asyncTest, fileContentEqual, setupDatabase, setupDatabaseAndSynchronizer, db, synchronizer, fileApi, sleep, clearDatabase, switchClient, syncTargetId, objectsEqual, checkThrowAsync } = require('test-utils.js');
const { asyncTest, resourceService, decryptionWorker, encryptionService, loadEncryptionMasterKey, allSyncTargetItemsEncrypted, fileContentEqual, setupDatabase, setupDatabaseAndSynchronizer, db, synchronizer, fileApi, sleep, clearDatabase, switchClient, syncTargetId, objectsEqual, checkThrowAsync } = require('test-utils.js');
const InteropService = require('lib/services/InteropService.js');
const Folder = require('lib/models/Folder.js');
const Note = require('lib/models/Note.js');
@@ -15,6 +15,7 @@ const fs = require('fs-extra');
const ArrayUtils = require('lib/ArrayUtils');
const ObjectUtils = require('lib/ObjectUtils');
const { shim } = require('lib/shim.js');
const SearchEngine = require('lib/services/SearchEngine');
process.on('unhandledRejection', (reason, p) => {
console.log('Unhandled Rejection at: Promise', p, 'reason:', reason);
@@ -37,6 +38,7 @@ describe('services_ResourceService', function() {
beforeEach(async (done) => {
await setupDatabaseAndSynchronizer(1);
await setupDatabaseAndSynchronizer(2);
await switchClient(1);
done();
});
@@ -146,4 +148,69 @@ describe('services_ResourceService', function() {
expect(before.last_seen_time).toBe(after.last_seen_time);
}));
it('should not delete resources that are associated with an encrypted note', asyncTest(async () => {
// https://github.com/laurent22/joplin/issues/1433
//
// Client 1 and client 2 have E2EE setup.
//
// - Client 1 creates note N1 and add resource R1 to it
// - Client 1 syncs
// - Client 2 syncs and get N1
// - Client 2 add resource R2 to N1
// - Client 2 syncs
// - Client 1 syncs
// - Client 1 runs resource indexer - but because N1 hasn't been decrypted yet, it found that R1 is no longer associated with any note
// - Client 1 decrypts notes, but too late
//
// Eventually R1 is deleted because service thinks that it was at some point associated with a note, but no longer.
const masterKey = await loadEncryptionMasterKey();
await encryptionService().enableEncryption(masterKey, '123456');
await encryptionService().loadMasterKeysFromSettings();
let folder1 = await Folder.save({ title: "folder1" });
let note1 = await Note.save({ title: 'ma note', parent_id: folder1.id });
await shim.attachFileToNote(note1, __dirname + '/../tests/support/photo.jpg'); // R1
await resourceService().indexNoteResources();
await synchronizer().start();
expect(await allSyncTargetItemsEncrypted()).toBe(true);
await switchClient(2);
await synchronizer().start();
await encryptionService().enableEncryption(masterKey, '123456');
await encryptionService().loadMasterKeysFromSettings();
await decryptionWorker().start();
{
const n1 = await Note.load(note1.id);
await shim.attachFileToNote(n1, __dirname + '/../tests/support/photo.jpg'); // R2
}
await synchronizer().start();
await switchClient(1);
await synchronizer().start();
await resourceService().indexNoteResources();
await resourceService().deleteOrphanResources(0); // Previously, R1 would be deleted here because it's not indexed
expect((await Resource.all()).length).toBe(2);
}));
it('should double-check if the resource is still linked before deleting it', asyncTest(async () => {
SearchEngine.instance().setDb(db()); // /!\ Note that we use the global search engine here, which we shouldn't but will work for now
let folder1 = await Folder.save({ title: "folder1" });
let note1 = await Note.save({ title: 'ma note', parent_id: folder1.id });
note1 = await shim.attachFileToNote(note1, __dirname + '/../tests/support/photo.jpg');
await resourceService().indexNoteResources();
const bodyWithResource = note1.body;
await Note.save({ id: note1.id, body: '' });
await resourceService().indexNoteResources();
await Note.save({ id: note1.id, body: bodyWithResource });
await SearchEngine.instance().syncTables();
await resourceService().deleteOrphanResources(0);
expect((await Resource.all()).length).toBe(1); // It should not have deleted the resource
const nr = (await NoteResource.all())[0];
expect(!!nr.is_associated).toBe(true); // And it should have fixed the situation by re-indexing the note content
}));
});

34
CliClient/tests/synchronizer.js
View File

@@ -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, checkThrowAsync, asyncTest } = require('test-utils.js');
const { setupDatabase, allSyncTargetItemsEncrypted, setupDatabaseAndSynchronizer, db, synchronizer, fileApi, sleep, clearDatabase, switchClient, syncTargetId, encryptionService, loadEncryptionMasterKey, fileContentEqual, decryptionWorker, checkThrowAsync, asyncTest } = require('test-utils.js');
const { shim } = require('lib/shim.js');
const fs = require('fs-extra');
const Folder = require('lib/models/Folder.js');
@@ -29,38 +29,6 @@ async function allItems() {
return folders.concat(notes);
}
async function allSyncTargetItemsEncrypted() {
const list = await fileApi().list();
const files = list.items;
//console.info(Setting.value('resourceDir'));
let totalCount = 0;
let encryptedCount = 0;
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;
totalCount++;
if (remoteContent.type_ === BaseModel.TYPE_RESOURCE) {
const content = await fileApi().get('.resource/' + remoteContent.id);
totalCount++;
if (content.substr(0, 5) === 'JED01') output = encryptedCount++;
}
if (!!remoteContent.encryption_applied) encryptedCount++;
}
if (!totalCount) throw new Error('No encryptable item on sync target');
return totalCount === encryptedCount;
}
async function localItemsSameAsRemote(locals, expect) {
let error = null;
try {

42
CliClient/tests/test-utils.js
View File

@@ -30,6 +30,7 @@ const SyncTargetNextcloud = require('lib/SyncTargetNextcloud.js');
const SyncTargetDropbox = require('lib/SyncTargetDropbox.js');
const EncryptionService = require('lib/services/EncryptionService.js');
const DecryptionWorker = require('lib/services/DecryptionWorker.js');
const ResourceService = require('lib/services/ResourceService.js');
const WebDavApi = require('lib/WebDavApi');
const DropboxApi = require('lib/DropboxApi');
@@ -37,6 +38,7 @@ let databases_ = [];
let synchronizers_ = [];
let encryptionServices_ = [];
let decryptionWorkers_ = [];
let resourceServices_ = [];
let fileApi_ = null;
let currentClient_ = 1;
@@ -102,6 +104,8 @@ function sleep(n) {
}
async function switchClient(id) {
if (!databases_[id]) throw new Error('Call setupDatabaseAndSynchronizer(' + id + ') first!!');
await time.msleep(sleepTime); // Always leave a little time so that updated_time properties don't overlap
await Setting.saveAll();
@@ -198,6 +202,7 @@ async function setupDatabaseAndSynchronizer(id = null) {
encryptionServices_[id] = new EncryptionService();
decryptionWorkers_[id] = new DecryptionWorker();
decryptionWorkers_[id].setEncryptionService(encryptionServices_[id]);
resourceServices_[id] = new ResourceService();
await fileApi().clearRoot();
}
@@ -222,6 +227,11 @@ function decryptionWorker(id = null) {
return decryptionWorkers_[id];
}
function resourceService(id = null) {
if (id === null) id = currentClient_;
return resourceServices_[id];
}
async function loadEncryptionMasterKey(id = null, useExisting = false) {
const service = encryptionService(id);
@@ -314,4 +324,34 @@ function asyncTest(callback) {
}
}
module.exports = { setupDatabase, setupDatabaseAndSynchronizer, db, synchronizer, fileApi, sleep, clearDatabase, switchClient, syncTargetId, objectsEqual, checkThrowAsync, encryptionService, loadEncryptionMasterKey, fileContentEqual, decryptionWorker, asyncTest };
async function allSyncTargetItemsEncrypted() {
const list = await fileApi().list();
const files = list.items;
let totalCount = 0;
let encryptedCount = 0;
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;
totalCount++;
if (remoteContent.type_ === BaseModel.TYPE_RESOURCE) {
const content = await fileApi().get('.resource/' + remoteContent.id);
totalCount++;
if (content.substr(0, 5) === 'JED01') output = encryptedCount++;
}
if (!!remoteContent.encryption_applied) encryptedCount++;
}
if (!totalCount) throw new Error('No encryptable item on sync target');
return totalCount === encryptedCount;
}
module.exports = { resourceService, allSyncTargetItemsEncrypted, setupDatabase, setupDatabaseAndSynchronizer, db, synchronizer, fileApi, sleep, clearDatabase, switchClient, syncTargetId, objectsEqual, checkThrowAsync, encryptionService, loadEncryptionMasterKey, fileContentEqual, decryptionWorker, asyncTest };

38
ReactNativeClient/lib/services/ResourceService.js
View File

@@ -4,9 +4,11 @@ const Note = require('lib/models/Note');
const Resource = require('lib/models/Resource');
const BaseModel = require('lib/BaseModel');
const BaseService = require('lib/services/BaseService');
const SearchEngine = require('lib/services/SearchEngine');
const Setting = require('lib/models/Setting');
const { shim } = require('lib/shim');
const ItemChangeUtils = require('lib/services/ItemChangeUtils');
const { sprintf } = require('sprintf-js');
class ResourceService extends BaseService {
@@ -15,6 +17,8 @@ class ResourceService extends BaseService {
await ItemChange.waitForAllSaved();
let foundNoteWithEncryption = false;
while (true) {
const changes = await ItemChange.modelSelectAll(`
SELECT id, item_id, type
@@ -28,7 +32,7 @@ class ResourceService extends BaseService {
if (!changes.length) break;
const noteIds = changes.map(a => a.item_id);
const notes = await Note.modelSelectAll('SELECT id, title, body FROM notes WHERE id IN ("' + noteIds.join('","') + '")');
const notes = await Note.modelSelectAll('SELECT id, title, body, encryption_applied FROM notes WHERE id IN ("' + noteIds.join('","') + '")');
const noteById = (noteId) => {
for (let i = 0; i < notes.length; i++) {
@@ -47,9 +51,18 @@ class ResourceService extends BaseService {
if (change.type === ItemChange.TYPE_CREATE || change.type === ItemChange.TYPE_UPDATE) {
const note = noteById(change.item_id);
if (!!note.encryption_applied) {
// If we hit an encrypted note, abort processing for now.
// Note will eventually get decrypted and processing can resume then.
// This is a limitation of the change tracking system - we cannot skip a change
// and keep processing the rest since we only keep track of "lastProcessedChangeId".
foundNoteWithEncryption = true;
break;
}
if (note) {
const resourceIds = await Note.linkedResourceIds(note.body);
await NoteResource.setAssociatedResources(note.id, resourceIds);
await this.setAssociatedResources_(note);
} else {
this.logger().warn('ResourceService::indexNoteResources: A change was recorded for a note that has been deleted: ' + change.item_id);
}
@@ -61,6 +74,8 @@ class ResourceService extends BaseService {
Setting.setValue('resourceService.lastProcessedChangeId', change.id);
}
if (foundNoteWithEncryption) break;
}
await Setting.saveAll();
@@ -72,11 +87,26 @@ class ResourceService extends BaseService {
this.logger().info('ResourceService::indexNoteResources: Completed');
}
async setAssociatedResources_(note) {
const resourceIds = await Note.linkedResourceIds(note.body);
await NoteResource.setAssociatedResources(note.id, resourceIds);
}
async deleteOrphanResources(expiryDelay = null) {
const resourceIds = await NoteResource.orphanResources(expiryDelay);
this.logger().info('ResourceService::deleteOrphanResources:', resourceIds);
for (let i = 0; i < resourceIds.length; i++) {
await Resource.delete(resourceIds[i]);
const resourceId = resourceIds[i];
const results = await SearchEngine.instance().search(resourceId);
if (results.length) {
const note = await Note.load(results[0].id);
if (note) {
this.logger().info(sprintf('ResourceService::deleteOrphanResources: Skipping deletion of resource %s because it is still referenced in note %s. Re-indexing note content to fix the issue.', resourceId, note.id));
await this.setAssociatedResources_(note);
}
} else {
await Resource.delete(resourceId);
}
}
}