diff --git a/packages/app-desktop/runForTesting.sh b/packages/app-desktop/runForTesting.sh index 20f1d0a62c..04df402328 100755 --- a/packages/app-desktop/runForTesting.sh +++ b/packages/app-desktop/runForTesting.sh @@ -123,6 +123,9 @@ do echo 'use "shared"' >> "$CMD_FILE" echo 'mknote "note 1"' >> "$CMD_FILE" echo 'mknote "note 2"' >> "$CMD_FILE" + echo 'mkbook --parent "shared" "sub"' >> "$CMD_FILE" + echo 'use "sub"' >> "$CMD_FILE" + echo 'mknote "note 3"' >> "$CMD_FILE" elif [[ $CMD == "reset" ]]; then diff --git a/packages/lib/Synchronizer.ts b/packages/lib/Synchronizer.ts index fd6e749ca3..9fa6993336 100644 --- a/packages/lib/Synchronizer.ts +++ b/packages/lib/Synchronizer.ts @@ -448,7 +448,7 @@ export default class Synchronizer { // Before synchronising make sure all share_id properties are set // correctly so as to share/unshare the right items. try { - await Folder.updateAllShareIds(this.resourceService()); + await Folder.updateAllShareIds(this.resourceService(), this.shareService_ ? this.shareService_.shares : []); if (this.shareService_) await this.shareService_.checkShareConsistency(); } catch (error) { if (error && error.code === ErrorCode.IsReadOnly) { diff --git a/packages/lib/models/Folder.sharing.test.ts b/packages/lib/models/Folder.sharing.test.ts index e6b3e3ac07..ef784fe524 100644 --- a/packages/lib/models/Folder.sharing.test.ts +++ b/packages/lib/models/Folder.sharing.test.ts @@ -6,6 +6,7 @@ import shim from '../shim'; import Resource from '../models/Resource'; import { FolderEntity, NoteEntity, ResourceEntity } from '../services/database/types'; import ResourceService from '../services/ResourceService'; +import { StateShare } from '../services/share/reducer'; const testImagePath = `${supportDir}/photo.jpg`; @@ -40,7 +41,7 @@ describe('models/Folder.sharing', () => { ]); await Folder.save({ id: folder.id, share_id: 'abcd1234' }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); const allItems = await allNotesFolders(); for (const item of allItems) { @@ -86,7 +87,7 @@ describe('models/Folder.sharing', () => { await Folder.save({ id: folder1.id, share_id: 'abcd1234' }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); folder1 = await Folder.loadByTitle('folder 1'); const folder2 = await Folder.loadByTitle('folder 2'); @@ -120,7 +121,7 @@ describe('models/Folder.sharing', () => { await Folder.save({ id: folder1.id, share_id: 'abcd1234' }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); folder1 = await Folder.loadByTitle('folder 1'); let folder2 = await Folder.loadByTitle('folder 2'); @@ -132,7 +133,7 @@ describe('models/Folder.sharing', () => { // Move the folder outside the shared folder await Folder.save({ id: folder2.id, parent_id: folder3.id }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); folder2 = await Folder.loadByTitle('folder 2'); expect(folder2.share_id).toBe(''); @@ -140,12 +141,53 @@ describe('models/Folder.sharing', () => { { await Folder.save({ id: folder2.id, parent_id: folder1.id }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); folder2 = await Folder.loadByTitle('folder 2'); expect(folder2.share_id).toBe('abcd1234'); } })); + it('should unshare a subfolder of a shared folder when it is moved to the root', (async () => { + let folder1 = await createFolderTree('', [ + { + title: 'folder 1', + children: [ + { + title: 'folder 2', + children: [], + }, + ], + }, + ]); + + await Folder.save({ id: folder1.id, share_id: 'abcd1234' }); + + const stateShares: StateShare[] = [ + { + folder_id: folder1.id, + id: 'abcd1234', + master_key_id: '', + note_id: '', + type: 3, + }, + ]; + + await Folder.updateAllShareIds(resourceService(), stateShares); + + folder1 = await Folder.loadByTitle('folder 1'); + let folder2 = await Folder.loadByTitle('folder 2'); + + expect(folder1.share_id).toBe('abcd1234'); + expect(folder2.share_id).toBe('abcd1234'); + + // Move the subfolder to the root + + await Folder.save({ id: folder2.id, parent_id: '' }); + await Folder.updateAllShareIds(resourceService(), stateShares); + folder2 = await Folder.loadByTitle('folder 2'); + expect(folder2.share_id).toBe(''); + })); + it('should apply the share ID to all notes', (async () => { const folder1 = await createFolderTree('', [ { @@ -179,7 +221,7 @@ describe('models/Folder.sharing', () => { await Folder.save({ id: folder1.id, share_id: 'abcd1234' }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); const note1: NoteEntity = await Note.loadByTitle('note 1'); const note2: NoteEntity = await Note.loadByTitle('note 2'); @@ -209,7 +251,7 @@ describe('models/Folder.sharing', () => { ]); await Folder.save({ id: folder1.id, share_id: 'abcd1234' }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); const note1: NoteEntity = await Note.loadByTitle('note 1'); const folder2: FolderEntity = await Folder.loadByTitle('folder 2'); expect(note1.share_id).toBe('abcd1234'); @@ -217,7 +259,7 @@ describe('models/Folder.sharing', () => { // Move the note outside of the shared folder await Note.save({ id: note1.id, parent_id: folder2.id }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); { const note1: NoteEntity = await Note.loadByTitle('note 1'); @@ -227,7 +269,7 @@ describe('models/Folder.sharing', () => { // Move the note back inside the shared folder await Note.save({ id: note1.id, parent_id: folder1.id }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); { const note1: NoteEntity = await Note.loadByTitle('note 1'); @@ -255,7 +297,7 @@ describe('models/Folder.sharing', () => { ]); await Folder.save({ id: folder1.id, share_id: 'abcd1234' }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); let note1: NoteEntity = await Note.loadByTitle('note 1'); let note2: NoteEntity = await Note.loadByTitle('note 2'); @@ -265,7 +307,7 @@ describe('models/Folder.sharing', () => { expect(note2.share_id).toBe('abcd1234'); await Note.save({ id: note1.id, parent_id: folder2.id }); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); note1 = await Note.loadByTitle('note 1'); note2 = await Note.loadByTitle('note 2'); @@ -295,7 +337,7 @@ describe('models/Folder.sharing', () => { ]); await Folder.save({ id: folder.id, share_id: 'abcd1234' }); - await Folder.updateAllShareIds(resourceService); + await Folder.updateAllShareIds(resourceService, []); const folder2: FolderEntity = await Folder.loadByTitle('folder 2'); const note1: NoteEntity = await Note.loadByTitle('note 1'); @@ -313,7 +355,7 @@ describe('models/Folder.sharing', () => { const previousBlobUpdatedTime = (await Resource.load(resourceId)).blob_updated_time; await msleep(1); - await Folder.updateAllShareIds(resourceService); + await Folder.updateAllShareIds(resourceService, []); { const resource: ResourceEntity = await Resource.load(resourceId); @@ -324,7 +366,7 @@ describe('models/Folder.sharing', () => { await Note.save({ id: note1.id, parent_id: folder2.id }); await resourceService.indexNoteResources(); - await Folder.updateAllShareIds(resourceService); + await Folder.updateAllShareIds(resourceService, []); { const resource: ResourceEntity = await Resource.load(resourceId); @@ -392,7 +434,7 @@ describe('models/Folder.sharing', () => { // We need to index the resources to populate the note_resources table await resourceService.indexNoteResources(); - await Folder.updateAllShareIds(resourceService); + await Folder.updateAllShareIds(resourceService, []); // BEFORE: // @@ -464,7 +506,7 @@ describe('models/Folder.sharing', () => { await resourceService.indexNoteResources(); - await Folder.updateAllShareIds(resourceService); + await Folder.updateAllShareIds(resourceService, []); await Folder.updateNoLongerSharedItems(['1']); diff --git a/packages/lib/models/Folder.ts b/packages/lib/models/Folder.ts index 55c2bbf309..12cbf2d5d2 100644 --- a/packages/lib/models/Folder.ts +++ b/packages/lib/models/Folder.ts @@ -7,7 +7,7 @@ import Note from './Note'; import Database from '../database'; import BaseItem from './BaseItem'; import Resource from './Resource'; -import { isRootSharedFolder } from '../services/share/reducer'; +import { isRootSharedFolder, StateShare } from '../services/share/reducer'; import Logger from '@joplin/utils/Logger'; import syncDebugLog from '../services/synchronizer/syncDebugLog'; import ResourceService from '../services/ResourceService'; @@ -417,18 +417,74 @@ export default class Folder extends BaseItem { return this.db().selectAll(sql, [folderId]); } - public static async rootSharedFolders(): Promise { - return this.db().selectAll('SELECT id, share_id FROM folders WHERE parent_id = \'\' AND share_id != \'\''); + public static async rootSharedFolders(activeShares: StateShare[]): Promise { + return this.removeDuplicateRootFolders(await this.db().selectAll('SELECT id, share_id FROM folders WHERE parent_id = \'\' AND share_id != \'\''), activeShares); } public static async rootShareFoldersByKeyId(keyId: string): Promise { return this.db().selectAll('SELECT id, share_id FROM folders WHERE master_key_id = ?', [keyId]); } - public static async updateFolderShareIds(): Promise { + // We need this function for this situation: + // + // - Folder is shared + // - Subfolder is created in the shared folder + // - Subfolder is moved to the root + // + // In that situation the subfolder will have "parent_id" = "" and so will be considered a "root + // shared folder". However it is not - a "root shared folder" is one that has been explicitly + // shared by the user. + // + // So we have this function to check for root folders that have the same "shared_id" - it + // indicates that one of them was a child of the other. We remove the formerly children folders. + private static removeDuplicateRootFolders(rootFolders: FolderEntity[], activeShares: StateShare[]) { + const folderIdsToRemove: string[] = []; + + for (let i = 0; i < rootFolders.length - 1; i++) { + const f1 = rootFolders[i]; + for (let j = i + 1; j < rootFolders.length; j++) { + const f2 = rootFolders[j]; + + if (f1.share_id === f2.share_id) { + logger.info('Found two root folders with the same share_id:', f1, f2); + const share = activeShares.find(s => s.id === f1.share_id); + if (!share) { + logger.warn('Could not find matching share object'); + continue; + } + + if (share.folder_id === f1.id) { + folderIdsToRemove.push(f2.id); + } else if (share.folder_id === f2.id) { + folderIdsToRemove.push(f1.id); + } else { + logger.warn('Could not find folder associated with share:', share); + } + } + } + } + + if (folderIdsToRemove.length) { + logger.info('Removing folders from the list of root folders:', folderIdsToRemove); + + const newRootFolders: FolderEntity[] = []; + + for (const f of rootFolders) { + if (!folderIdsToRemove.includes(f.id)) { + newRootFolders.push(f); + } + } + + return newRootFolders; + } + + return rootFolders; + } + + public static async updateFolderShareIds(activeShares: StateShare[]): Promise { // Get all the sub-folders of the shared folders, and set the share_id // property. - const rootFolders = await this.rootSharedFolders(); + const rootFolders = await this.rootSharedFolders(activeShares); let sharedFolderIds: string[] = []; @@ -652,8 +708,8 @@ export default class Folder extends BaseItem { throw new Error('Failed to update resource share IDs'); } - public static async updateAllShareIds(resourceService: ResourceService) { - await this.updateFolderShareIds(); + public static async updateAllShareIds(resourceService: ResourceService, activeShares: StateShare[]) { + await this.updateFolderShareIds(activeShares); await this.updateNoteShareIds(); await this.updateResourceShareIds(resourceService); } diff --git a/packages/lib/services/share/ShareService.test.ts b/packages/lib/services/share/ShareService.test.ts index e552ccea48..a0286c39ca 100644 --- a/packages/lib/services/share/ShareService.test.ts +++ b/packages/lib/services/share/ShareService.test.ts @@ -95,7 +95,7 @@ describe('ShareService', () => { await service.shareNote(note.id, false); await msleep(1); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); await synchronizerStart(); @@ -187,7 +187,7 @@ describe('ShareService', () => { await shareService.shareFolder(folder.id); - await Folder.updateAllShareIds(resourceService()); + await Folder.updateAllShareIds(resourceService(), []); // The share service should automatically create a new encryption key // specifically for that shared folder @@ -313,7 +313,7 @@ describe('ShareService', () => { const resourceService = new ResourceService(); await Folder.save({ id: folder1.id, share_id: '123456789' }); - await Folder.updateAllShareIds(resourceService); + await Folder.updateAllShareIds(resourceService, []); const cleanup = simulateReadOnlyShareEnv('123456789'); diff --git a/packages/lib/services/share/ShareService.ts b/packages/lib/services/share/ShareService.ts index d7b084205a..0e4e2ad5f6 100644 --- a/packages/lib/services/share/ShareService.ts +++ b/packages/lib/services/share/ShareService.ts @@ -137,7 +137,7 @@ export default class ShareService { // Note: race condition if the share is created but the app crashes // before setting share_id on the folder. See unshareFolder() for info. await Folder.save({ id: folder.id, share_id: share.id }); - await Folder.updateAllShareIds(ResourceService.instance()); + await Folder.updateAllShareIds(ResourceService.instance(), this.shares); return share; } @@ -182,7 +182,7 @@ export default class ShareService { // It's ok if updateAllShareIds() doesn't run because it's executed on // each sync too. - await Folder.updateAllShareIds(ResourceService.instance()); + await Folder.updateAllShareIds(ResourceService.instance(), this.shares); } // This is when a share recipient decides to leave the shared folder. @@ -216,7 +216,7 @@ export default class ShareService { // We call this to make sure all items are correctly linked before we // call deleteAllByShareId() - await Folder.updateAllShareIds(ResourceService.instance()); + await Folder.updateAllShareIds(ResourceService.instance(), this.shares); const source = 'ShareService.leaveSharedFolder'; await Folder.delete(folderId, { deleteChildren: false, disableReadOnlyCheck: true, sourceDescription: source }); @@ -228,7 +228,7 @@ export default class ShareService { // necessary otherwise sync will try to update items that are not longer // accessible and will throw the error "Could not find share with ID: xxxx") public async checkShareConsistency() { - const rootSharedFolders = await Folder.rootSharedFolders(); + const rootSharedFolders = await Folder.rootSharedFolders(this.shares); let hasRefreshedShares = false; let shares = this.shares;