All: Fixes #12089: Moving sub-notebook of shared notebook should unshare it (#12647)

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
 	

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) {

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']);
 

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<FolderEntity[]> {
+	public static async rootSharedFolders(activeShares: StateShare[]): Promise<FolderEntity[]> {
-		return this.db().selectAll('SELECT id, share_id FROM folders WHERE parent_id = \'\' AND share_id != \'\'');
+		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<FolderEntity[]> {
 		return this.db().selectAll('SELECT id, share_id FROM folders WHERE master_key_id = ?', [keyId]);
 	}
 
-	public static async updateFolderShareIds(): Promise<void> {
+	// 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<void> {
 		// 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) {
+	public static async updateAllShareIds(resourceService: ResourceService, activeShares: StateShare[]) {
-		await this.updateFolderShareIds();
+		await this.updateFolderShareIds(activeShares);
 		await this.updateNoteShareIds();
 		await this.updateResourceShareIds(resourceService);
 	}

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');
 

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;