fix: Moving documents between collections does not update attachment permissions (#2136)

* fix: Copy attachments when neccessary and moving between collections

* test: regression

server/commands/documentMover.js

@@ -1,6 +1,42 @@
 // @flow
-import { Document, Collection, User, Event } from "../models";
+import { Document, Attachment, Collection, User, Event } from "../models";
 import { sequelize } from "../sequelize";
+import parseAttachmentIds from "../utils/parseAttachmentIds";
+
+async function copyAttachments(document: Document, options) {
+  let text = document.text;
+  const documentId = document.id;
+
+  // find any image attachments that are in this documents text
+  const attachmentIds = parseAttachmentIds(text);
+
+  for (const id of attachmentIds) {
+    const existing = await Attachment.findOne({
+      where: {
+        teamId: document.teamId,
+        id,
+      },
+    });
+
+    // if the image attachment was originally uploaded to another document
+    // (this can happen in various ways, copy/paste, or duplicate for example)
+    // then create a new attachment pointed to this doc and update the reference
+    // in the text so that it gets the moved documents permissions
+    if (existing && existing.documentId !== documentId) {
+      const { id, ...rest } = existing.dataValues;
+      const attachment = await Attachment.create(
+        {
+          ...rest,
+          documentId,
+        },
+        options
+      );
+      text = text.replace(existing.redirectUrl, attachment.redirectUrl);
+    }
+  }
+
+  return text;
+}
 
 export default async function documentMover({
   user,
@@ -63,7 +99,11 @@ export default async function documentMover({
 
       // if the collection is the same then it will get saved below, this
       // line prevents a pointless intermediate save from occurring.
-      if (collectionChanged) await collection.save({ transaction });
+      if (collectionChanged) {
+        await collection.save({ transaction });
+
+        document.text = await copyAttachments(document, { transaction });
+      }
 
       // add to new collection (may be the same)
       document.collectionId = collectionId;
@@ -82,8 +122,8 @@ export default async function documentMover({
       result.collections.push(collection);
 
       // if collection does not remain the same loop through children and change their
-      // collectionId too. This includes archived children, otherwise their collection
-      // would be wrong once restored.
+      // collectionId and move any attachments they may have too. This includes
+      // archived children, otherwise their collection would be wrong once restored.
       if (collectionChanged) {
         result.collections.push(newCollection);
 
@@ -95,7 +135,9 @@ export default async function documentMover({
           await Promise.all(
             childDocuments.map(async (child) => {
               await loopChildren(child.id);
-              await child.update({ collectionId }, { transaction });
+              child.text = await copyAttachments(child, { transaction });
+              child.collectionId = collectionId;
+              await child.save();
               child.collection = newCollection;
               result.documents.push(child);
             })

server/commands/documentMover.test.js

@@ -1,6 +1,13 @@
 // @flow
-import { buildDocument, buildCollection, buildUser } from "../test/factories";
+import { Attachment } from "../models";
+import {
+  buildDocument,
+  buildAttachment,
+  buildCollection,
+  buildUser,
+} from "../test/factories";
 import { flushdb, seed } from "../test/support";
+import parseAttachmentIds from "../utils/parseAttachmentIds";
 import documentMover from "./documentMover";
 
 beforeEach(() => flushdb());
@@ -110,4 +117,46 @@ describe("documentMover", () => {
     expect(response.documents[0].collection.id).toEqual(newCollection.id);
     expect(response.documents[1].collection.id).toEqual(newCollection.id);
   });
+
+  it("should move attachments in children to another collection", async () => {
+    const { document, user, collection } = await seed();
+    const newCollection = await buildCollection({
+      teamId: collection.teamId,
+    });
+    const attachment = await buildAttachment({
+      teamId: user.teamId,
+      userId: user.id,
+    });
+    const newDocument = await buildDocument({
+      parentDocumentId: document.id,
+      collectionId: collection.id,
+      teamId: collection.teamId,
+      userId: collection.createdById,
+      title: "Child document",
+      text: `content ![attachment](${attachment.redirectUrl})`,
+    });
+    await collection.addDocumentToStructure(newDocument);
+
+    await documentMover({
+      user,
+      document,
+      collectionId: newCollection.id,
+      parentDocumentId: undefined,
+      index: 0,
+      ip,
+    });
+
+    // check document ids where updated
+    await newDocument.reload();
+    expect(newDocument.collectionId).toBe(newCollection.id);
+
+    // check new attachment was created pointint to same key
+    const attachmentIds = parseAttachmentIds(newDocument.text);
+    const newAttachment = await Attachment.findByPk(attachmentIds[0]);
+    expect(newAttachment.documentId).toBe(newDocument.id);
+    expect(newAttachment.key).toBe(attachment.key);
+
+    await document.reload();
+    expect(document.collectionId).toBe(newCollection.id);
+  });
 });

server/test/factories.js

@@ -242,11 +242,6 @@ export async function buildAttachment(overrides: Object = {}) {
     overrides.userId = user.id;
   }
 
-  if (!overrides.collectionId) {
-    const collection = await buildCollection(overrides);
-    overrides.collectionId = collection.id;
-  }
-
   if (!overrides.documentId) {
     const document = await buildDocument(overrides);
     overrides.documentId = document.id;