Merge branch 'dev' into sharing_bug_2

packages/app-desktop/runForTesting.sh

@@ -182,6 +182,12 @@ do
 	fi
 done
 
+echo '----------------------------------------------------'
+echo 'Running commands:'
+echo '';
+cat "$CMD_FILE"
+echo '----------------------------------------------------'
+
 cd "$ROOT_DIR/packages/app-cli"
 yarn start --profile "$PROFILE_DIR" batch "$CMD_FILE"
 

packages/lib/Synchronizer.ts

@@ -448,6 +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 {
+			if (this.shareService_) await this.shareService_.maintenance();
 			await Folder.updateAllShareIds(this.resourceService(), this.shareService_ ? this.shareService_.shares : []);
 			if (this.shareService_) await this.shareService_.checkShareConsistency();
 		} catch (error) {

packages/lib/models/Folder.sharing.test.ts

@@ -10,6 +10,25 @@ import { StateShare } from '../services/share/reducer';
 
 const testImagePath = `${supportDir}/photo.jpg`;
 
+const makeStateShares = (folderIds: string[] | string, shareIds: string|string[] = 'abcd1234'): StateShare[] => {
+	folderIds = (typeof folderIds === 'string') ? [folderIds] : folderIds;
+	shareIds = (typeof shareIds === 'string') ? [shareIds] : shareIds;
+
+	const output: StateShare[] = [];
+
+	for (let i = 0; i < folderIds.length; i++) {
+		output.push({
+			folder_id: folderIds[i],
+			master_key_id: '',
+			id: shareIds[i],
+			note_id: '',
+			type: 3,
+		});
+	}
+
+	return output;
+};
+
 describe('models/Folder.sharing', () => {
 
 	beforeEach(async () => {
@@ -41,7 +60,7 @@ describe('models/Folder.sharing', () => {
 		]);
 
 		await Folder.save({ id: folder.id, share_id: 'abcd1234' });
-		await Folder.updateAllShareIds(resourceService(), []);
+		await Folder.updateAllShareIds(resourceService(), makeStateShares(folder.id));
 
 		const allItems = await allNotesFolders();
 		for (const item of allItems) {
@@ -87,7 +106,7 @@ describe('models/Folder.sharing', () => {
 
 		await Folder.save({ id: folder1.id, share_id: 'abcd1234' });
 
-		await Folder.updateAllShareIds(resourceService(), []);
+		await Folder.updateAllShareIds(resourceService(), makeStateShares(folder1.id));
 
 		folder1 = await Folder.loadByTitle('folder 1');
 		const folder2 = await Folder.loadByTitle('folder 2');
@@ -121,7 +140,7 @@ describe('models/Folder.sharing', () => {
 
 		await Folder.save({ id: folder1.id, share_id: 'abcd1234' });
 
-		await Folder.updateAllShareIds(resourceService(), []);
+		await Folder.updateAllShareIds(resourceService(), makeStateShares(folder1.id));
 
 		folder1 = await Folder.loadByTitle('folder 1');
 		let folder2 = await Folder.loadByTitle('folder 2');
@@ -133,7 +152,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(), makeStateShares(folder1.id));
 		folder2 = await Folder.loadByTitle('folder 2');
 		expect(folder2.share_id).toBe('');
 
@@ -141,7 +160,7 @@ describe('models/Folder.sharing', () => {
 
 		{
 			await Folder.save({ id: folder2.id, parent_id: folder1.id });
-			await Folder.updateAllShareIds(resourceService(), []);
+			await Folder.updateAllShareIds(resourceService(), makeStateShares(folder1.id));
 			folder2 = await Folder.loadByTitle('folder 2');
 			expect(folder2.share_id).toBe('abcd1234');
 		}
@@ -162,15 +181,7 @@ describe('models/Folder.sharing', () => {
 
 		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,
-			},
-		];
+		const stateShares: StateShare[] = makeStateShares(folder1.id);
 
 		await Folder.updateAllShareIds(resourceService(), stateShares);
 
@@ -221,7 +232,7 @@ describe('models/Folder.sharing', () => {
 
 		await Folder.save({ id: folder1.id, share_id: 'abcd1234' });
 
-		await Folder.updateAllShareIds(resourceService(), []);
+		await Folder.updateAllShareIds(resourceService(), makeStateShares(folder1.id));
 
 		const note1: NoteEntity = await Note.loadByTitle('note 1');
 		const note2: NoteEntity = await Note.loadByTitle('note 2');
@@ -251,7 +262,7 @@ describe('models/Folder.sharing', () => {
 		]);
 
 		await Folder.save({ id: folder1.id, share_id: 'abcd1234' });
-		await Folder.updateAllShareIds(resourceService(), []);
+		await Folder.updateAllShareIds(resourceService(), makeStateShares(folder1.id));
 		const note1: NoteEntity = await Note.loadByTitle('note 1');
 		const folder2: FolderEntity = await Folder.loadByTitle('folder 2');
 		expect(note1.share_id).toBe('abcd1234');
@@ -259,7 +270,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(), makeStateShares(folder1.id));
 
 		{
 			const note1: NoteEntity = await Note.loadByTitle('note 1');
@@ -269,7 +280,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(), makeStateShares(folder1.id));
 
 		{
 			const note1: NoteEntity = await Note.loadByTitle('note 1');
@@ -297,7 +308,7 @@ describe('models/Folder.sharing', () => {
 		]);
 
 		await Folder.save({ id: folder1.id, share_id: 'abcd1234' });
-		await Folder.updateAllShareIds(resourceService(), []);
+		await Folder.updateAllShareIds(resourceService(), makeStateShares(folder1.id));
 
 		let note1: NoteEntity = await Note.loadByTitle('note 1');
 		let note2: NoteEntity = await Note.loadByTitle('note 2');
@@ -307,7 +318,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(), makeStateShares(folder1.id));
 
 		note1 = await Note.loadByTitle('note 1');
 		note2 = await Note.loadByTitle('note 2');
@@ -315,6 +326,60 @@ describe('models/Folder.sharing', () => {
 		expect(note2.share_id).toBe('abcd1234');
 	}));
 
+	it('should clear the share ID if that share no longer exists', (async () => {
+		const folder1 = await createFolderTree('', [
+			{
+				title: 'folder 1',
+				children: [
+					{
+						title: 'note 1',
+					},
+					{
+						title: 'note 2',
+					},
+					{
+						title: 'subfolder',
+						children: [],
+					},
+				],
+			},
+			{
+				title: 'folder 2',
+				children: [],
+			},
+		]);
+
+		let note1: NoteEntity = await Note.loadByTitle('note 1');
+		await shim.attachFileToNote(note1, testImagePath);
+		await resourceService().indexNoteResources();
+
+		await Folder.save({ id: folder1.id, share_id: 'abcd1234' });
+
+		await Folder.updateAllShareIds(resourceService(), makeStateShares(folder1.id));
+
+		note1 = await Note.loadByTitle('note 1');
+		let note2: NoteEntity = await Note.loadByTitle('note 2');
+		let resource: ResourceEntity = (await Resource.all())[0];
+		let subFolder: FolderEntity = await Folder.loadByTitle('subfolder');
+
+		expect(note1.share_id).toBe('abcd1234');
+		expect(note2.share_id).toBe('abcd1234');
+		expect(resource.share_id).toBe('abcd1234');
+		expect(subFolder.share_id).toBe('abcd1234');
+
+		await Folder.updateAllShareIds(resourceService(), []);
+
+		note1 = await Note.loadByTitle('note 1');
+		note2 = await Note.loadByTitle('note 2');
+		resource = (await Resource.all())[0];
+		subFolder = await Folder.loadByTitle('subfolder');
+
+		expect(note1.share_id).toBe('');
+		expect(note2.share_id).toBe('');
+		expect(resource.share_id).toBe('');
+		expect(subFolder.share_id).toBe('');
+	}));
+
 	it('should apply the note share ID to its resources', async () => {
 		const resourceService = new ResourceService();
 
@@ -337,7 +402,7 @@ describe('models/Folder.sharing', () => {
 		]);
 
 		await Folder.save({ id: folder.id, share_id: 'abcd1234' });
-		await Folder.updateAllShareIds(resourceService, []);
+		await Folder.updateAllShareIds(resourceService, makeStateShares(folder.id));
 
 		const folder2: FolderEntity = await Folder.loadByTitle('folder 2');
 		const note1: NoteEntity = await Note.loadByTitle('note 1');
@@ -355,7 +420,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, makeStateShares(folder.id));
 
 		{
 			const resource: ResourceEntity = await Resource.load(resourceId);
@@ -366,7 +431,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, makeStateShares(folder.id));
 
 		{
 			const resource: ResourceEntity = await Resource.load(resourceId);
@@ -434,7 +499,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, makeStateShares(folder1.id, 'share1'));
 
 		// BEFORE:
 		//
@@ -506,7 +571,7 @@ describe('models/Folder.sharing', () => {
 
 		await resourceService.indexNoteResources();
 
-		await Folder.updateAllShareIds(resourceService, []);
+		await Folder.updateAllShareIds(resourceService, makeStateShares([folder1.id, folder2.id], ['1', '2']));
 
 		await Folder.updateNoLongerSharedItems(['1']);
 

packages/lib/models/Folder.ts

@@ -484,7 +484,8 @@ export default class Folder extends BaseItem {
 	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(activeShares);
+		const activeShareIds = activeShares.map(s => s.id);
+		const rootFolders = (await this.rootSharedFolders(activeShares)).filter(f => activeShareIds.includes(f.share_id));
 
 		let sharedFolderIds: string[] = [];
 
@@ -538,19 +539,26 @@ export default class Folder extends BaseItem {
 		logger.debug('updateFolderShareIds:', report);
 	}
 
-	public static async updateNoteShareIds() {
+	public static async updateNoteShareIds(activeShares: StateShare[]) {
 		// Find all the notes where the share_id is not the same as the
 		// parent share_id because we only need to update those.
-		const rows = await this.db().selectAll(`
+		const rows1 = await this.db().selectAll(`
 			SELECT notes.id, folders.share_id, notes.parent_id
 			FROM notes
 			LEFT JOIN folders ON notes.parent_id = folders.id
 			WHERE notes.share_id != folders.share_id
 		`);
 
-		logger.debug('updateNoteShareIds: notes to update:', rows.length);
+		const rows2 = await this.db().selectAll(`
+			SELECT notes.id, notes.parent_id
+			FROM notes
+			WHERE notes.share_id != '' AND notes.share_id NOT IN (${BaseModel.escapeIdsForSql(activeShares.map(s => s.id))})
+		`);
 
-		for (const row of rows) {
+		logger.debug('updateNoteShareIds: notes to update (1)', rows1);
+		logger.debug('updateNoteShareIds: notes to update (2)', rows2);
+
+		for (const row of rows1.concat(rows2)) {
 			await Note.save({
 				id: row.id,
 				share_id: row.share_id || '',
@@ -710,7 +718,7 @@ export default class Folder extends BaseItem {
 
 	public static async updateAllShareIds(resourceService: ResourceService, activeShares: StateShare[]) {
 		await this.updateFolderShareIds(activeShares);
-		await this.updateNoteShareIds();
+		await this.updateNoteShareIds(activeShares);
 		await this.updateResourceShareIds(resourceService);
 	}
 

packages/lib/services/share/ShareService.test.ts

@@ -18,6 +18,17 @@ import Setting from '../../models/Setting';
 import { ModelType } from '../../BaseModel';
 import { remoteNotesFoldersResources } from '../../testing/test-utils-synchronizer';
 import mockShareService from '../../testing/share/mockShareService';
+import { StateShare } from './reducer';
+
+const makeStateShares = (folderId: string, shareId = 'abcd1234'): StateShare[] => {
+	return [{
+		folder_id: folderId,
+		master_key_id: '',
+		id: shareId,
+		note_id: '',
+		type: 3,
+	}];
+};
 
 interface TestShareFolderServiceOptions {
 	master_key_id?: string;
@@ -157,7 +168,7 @@ describe('ShareService', () => {
 
 	async function testShareFolder(service: ShareService) {
 		const { folder, note, resource } = await prepareNoteFolderResource();
-		const share = await service.shareFolder(folder.id);
+		const share = await service.shareFolder(folder.id, makeStateShares(folder.id, 'share_1'));
 		expect(share.id).toBe('share_1');
 		expect((await Folder.load(folder.id)).share_id).toBe('share_1');
 		expect((await Note.load(note.id)).share_id).toBe('share_1');
@@ -185,9 +196,9 @@ describe('ShareService', () => {
 		BaseItem.shareService_ = shareService;
 		Resource.shareService_ = shareService;
 
-		await shareService.shareFolder(folder.id);
+		const share = await shareService.shareFolder(folder.id, makeStateShares(folder.id, 'share_1'));
 
-		await Folder.updateAllShareIds(resourceService(), []);
+		await Folder.updateAllShareIds(resourceService(), makeStateShares(folder.id, share.id));
 
 		// The share service should automatically create a new encryption key
 		// specifically for that shared folder
@@ -318,7 +329,7 @@ describe('ShareService', () => {
 		const cleanup = simulateReadOnlyShareEnv('123456789');
 
 		const shareService = testShareFolderService();
-		await shareService.leaveSharedFolder(folder1.id, 'somethingrandom');
+		await shareService.leaveSharedFolder(folder1.id, 'somethingrandom', BaseItem.syncShareCache.shares);
 
 		expect(await Folder.count()).toBe(0);
 		expect(await Note.count()).toBe(0);

packages/lib/services/share/ShareService.ts

@@ -98,7 +98,9 @@ export default class ShareService {
 		return this.api_;
 	}
 
-	public async shareFolder(folderId: string): Promise<ApiShare> {
+	public async shareFolder(folderId: string, stateShares: StateShare[]|null = null): Promise<ApiShare> {
+		if (stateShares === null) stateShares = this.shares;
+
 		const folder = await Folder.load(folderId);
 		if (!folder) throw new Error(`No such folder: ${folderId}`);
 
@@ -137,7 +139,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(), this.shares);
+		await Folder.updateAllShareIds(ResourceService.instance(), stateShares);
 
 		return share;
 	}
@@ -206,17 +208,19 @@ export default class ShareService {
 	// If `folderShareUserId` is provided, the function will check that the user
 	// does not own the share. It would be an error to leave such a folder
 	// (instead "unshareFolder" should be called).
-	public async leaveSharedFolder(folderId: string, folderShareUserId: string = null): Promise<void> {
+	public async leaveSharedFolder(folderId: string, folderShareUserId: string = null, stateShares: StateShare[]|null = null): Promise<void> {
 		if (folderShareUserId !== null) {
 			const userId = Setting.value('sync.userId');
 			if (folderShareUserId === userId) throw new Error('Cannot leave own notebook');
 		}
 
+		if (stateShares === null) stateShares = this.shares;
+
 		const folder = await Folder.load(folderId);
 
 		// We call this to make sure all items are correctly linked before we
 		// call deleteAllByShareId()
-		await Folder.updateAllShareIds(ResourceService.instance(), this.shares);
+		await Folder.updateAllShareIds(ResourceService.instance(), stateShares);
 
 		const source = 'ShareService.leaveSharedFolder';
 		await Folder.delete(folderId, { deleteChildren: false, disableReadOnlyCheck: true, sourceDescription: source });