fix: Allow viewers to upload avatar (#4349)

* fix: Allow viewers to upload avatar

* DeleteAttachmentTask

* fix: Previous avatar should be deleted on change, if possible

* fix: Also cleanup team logo on change

server/models/Team.ts

@@ -18,11 +18,15 @@ import {
   IsUUID,
   IsUrl,
   AllowNull,
+  AfterUpdate,
 } from "sequelize-typescript";
 import { CollectionPermission, TeamPreference } from "@shared/types";
 import { getBaseDomain, RESERVED_SUBDOMAINS } from "@shared/utils/domains";
 import env from "@server/env";
+import DeleteAttachmentTask from "@server/queues/tasks/DeleteAttachmentTask";
 import { generateAvatarUrl } from "@server/utils/avatars";
+import parseAttachmentIds from "@server/utils/parseAttachmentIds";
+import Attachment from "./Attachment";
 import AuthenticationProvider from "./AuthenticationProvider";
 import Collection from "./Collection";
 import Document from "./Document";
@@ -288,6 +292,37 @@ class Team extends ParanoidModel {
 
   @HasMany(() => TeamDomain)
   allowedDomains: TeamDomain[];
+
+  // hooks
+
+  @AfterUpdate
+  static deletePreviousAvatar = async (model: Team) => {
+    if (
+      model.previous("avatarUrl") &&
+      model.previous("avatarUrl") !== model.avatarUrl
+    ) {
+      const attachmentIds = parseAttachmentIds(
+        model.previous("avatarUrl"),
+        true
+      );
+      if (!attachmentIds.length) {
+        return;
+      }
+
+      const attachment = await Attachment.findOne({
+        where: {
+          id: attachmentIds[0],
+          teamId: model.id,
+        },
+      });
+
+      if (attachment) {
+        await DeleteAttachmentTask.schedule({
+          attachmentId: attachment.id,
+        });
+      }
+    }
+  };
 }
 
 export default Team;

server/models/User.ts

@@ -21,6 +21,7 @@ import {
   IsDate,
   IsUrl,
   AllowNull,
+  AfterUpdate,
 } from "sequelize-typescript";
 import { languages } from "@shared/i18n";
 import {
@@ -30,8 +31,11 @@ import {
 } from "@shared/types";
 import { stringToColor } from "@shared/utils/color";
 import env from "@server/env";
+import DeleteAttachmentTask from "@server/queues/tasks/DeleteAttachmentTask";
+import parseAttachmentIds from "@server/utils/parseAttachmentIds";
 import { ValidationError } from "../errors";
 import ApiKey from "./ApiKey";
+import Attachment from "./Attachment";
 import Collection from "./Collection";
 import CollectionUser from "./CollectionUser";
 import NotificationSetting from "./NotificationSetting";
@@ -580,6 +584,36 @@ class User extends ParanoidModel {
     model.jwtSecret = crypto.randomBytes(64).toString("hex");
   };
 
+  @AfterUpdate
+  static deletePreviousAvatar = async (model: User) => {
+    if (
+      model.previous("avatarUrl") &&
+      model.previous("avatarUrl") !== model.avatarUrl
+    ) {
+      const attachmentIds = parseAttachmentIds(
+        model.previous("avatarUrl"),
+        true
+      );
+      if (!attachmentIds.length) {
+        return;
+      }
+
+      const attachment = await Attachment.findOne({
+        where: {
+          id: attachmentIds[0],
+          teamId: model.teamId,
+          userId: model.id,
+        },
+      });
+
+      if (attachment) {
+        await DeleteAttachmentTask.schedule({
+          attachmentId: attachment.id,
+        });
+      }
+    }
+  };
+
   // By default when a user signs up we subscribe them to email notifications
   // when documents they created are edited by other team members and onboarding.
   // If the user is an admin, they will also be subscribed to export_completed

server/queues/tasks/DeleteAttachmentTask.ts

@@ -0,0 +1,17 @@
+import { Attachment } from "@server/models";
+import BaseTask from "./BaseTask";
+
+type Props = {
+  attachmentId: string;
+};
+
+export default class DeleteAttachmentTask extends BaseTask<Props> {
+  public async perform({ attachmentId }: Props) {
+    const attachment = await Attachment.findByPk(attachmentId);
+    if (!attachment) {
+      return;
+    }
+
+    await attachment.destroy();
+  }
+}

server/routes/api/attachments.test.ts

@@ -5,6 +5,7 @@ import {
   buildCollection,
   buildAttachment,
   buildDocument,
+  buildViewer,
 } from "@server/test/factories";
 import { getTestServer } from "@server/test/support";
 
@@ -18,32 +19,50 @@ describe("#attachments.create", () => {
     expect(res.status).toEqual(401);
   });
 
-  it("should allow simple image upload for public attachments", async () => {
-    const user = await buildUser();
-    const res = await server.post("/api/attachments.create", {
-      body: {
-        name: "test.png",
-        contentType: "image/png",
-        size: 1000,
-        public: true,
-        token: user.getJwtToken(),
-      },
+  describe("member", () => {
+    it("should allow simple image upload for public attachments", async () => {
+      const user = await buildUser();
+      const res = await server.post("/api/attachments.create", {
+        body: {
+          name: "test.png",
+          contentType: "image/png",
+          size: 1000,
+          public: true,
+          token: user.getJwtToken(),
+        },
+      });
+      expect(res.status).toEqual(200);
+    });
+
+    it("should not allow file upload for public attachments", async () => {
+      const user = await buildUser();
+      const res = await server.post("/api/attachments.create", {
+        body: {
+          name: "test.pdf",
+          contentType: "application/pdf",
+          size: 1000,
+          public: true,
+          token: user.getJwtToken(),
+        },
+      });
+      expect(res.status).toEqual(400);
     });
-    expect(res.status).toEqual(200);
   });
 
-  it("should not allow file upload for public attachments", async () => {
-    const user = await buildUser();
-    const res = await server.post("/api/attachments.create", {
-      body: {
-        name: "test.pdf",
-        contentType: "application/pdf",
-        size: 1000,
-        public: true,
-        token: user.getJwtToken(),
-      },
+  describe("viewer", () => {
+    it("should allow simple image upload for public attachments", async () => {
+      const user = await buildViewer();
+      const res = await server.post("/api/attachments.create", {
+        body: {
+          name: "test.png",
+          contentType: "image/png",
+          size: 1000,
+          public: true,
+          token: user.getJwtToken(),
+        },
+      });
+      expect(res.status).toEqual(200);
     });
-    expect(res.status).toEqual(400);
   });
 });
 

server/routes/api/attachments.ts

@@ -19,22 +19,24 @@ const router = new Router();
 const AWS_S3_ACL = process.env.AWS_S3_ACL || "private";
 
 router.post("attachments.create", auth(), async (ctx) => {
-  const isPublic = ctx.request.body.public;
   const {
     name,
     documentId,
     contentType = "application/octet-stream",
     size,
+    public: isPublic,
   } = ctx.request.body;
   assertPresent(name, "name is required");
   assertPresent(size, "size is required");
 
   const { user } = ctx.state;
-  authorize(user, "createAttachment", user.team);
 
-  // Public attachments are only used for avatars, so this is loosely coupled.
+  // Public attachments are only used for avatars, so this is loosely coupled –
+  // all user types can upload an avatar so no additional authorization is needed.
   if (isPublic) {
     assertIn(contentType, AttachmentValidation.avatarContentTypes);
+  } else {
+    authorize(user, "createAttachment", user.team);
   }
 
   if (
@@ -48,11 +50,11 @@ router.post("attachments.create", auth(), async (ctx) => {
     );
   }
 
-  const s3Key = uuidv4();
+  const modelId = uuidv4();
   const acl =
     isPublic === undefined ? AWS_S3_ACL : isPublic ? "public-read" : "private";
   const bucket = acl === "public-read" ? "public" : "uploads";
-  const keyPrefix = `${bucket}/${user.id}/${s3Key}`;
+  const keyPrefix = `${bucket}/${user.id}/${modelId}`;
   const key = `${keyPrefix}/${name}`;
   const presignedPost = await getPresignedPost(key, acl, contentType);
   const endpoint = publicS3Endpoint();
@@ -69,6 +71,7 @@ router.post("attachments.create", auth(), async (ctx) => {
   const attachment = await sequelize.transaction(async (transaction) => {
     const attachment = await Attachment.create(
       {
+        id: modelId,
         key,
         acl,
         size,
@@ -86,6 +89,7 @@ router.post("attachments.create", auth(), async (ctx) => {
         data: {
           name,
         },
+        modelId,
         teamId: user.teamId,
         actorId: user.id,
         ip: ctx.request.ip,

server/routes/api/users.ts

@@ -18,6 +18,10 @@ import { ValidationError } from "@server/errors";
 import logger from "@server/logging/Logger";
 import auth from "@server/middlewares/authentication";
 import { rateLimiter } from "@server/middlewares/rateLimiter";
+import {
+  transaction,
+  TransactionContext,
+} from "@server/middlewares/transaction";
 import { Event, User, Team } from "@server/models";
 import { UserFlag, UserRole } from "@server/models/User";
 import { can, authorize } from "@server/policies";
@@ -176,43 +180,51 @@ router.post("users.info", auth(), async (ctx) => {
   };
 });
 
-router.post("users.update", auth(), async (ctx) => {
-  const { user } = ctx.state;
-  const { name, avatarUrl, language, preferences } = ctx.request.body;
-  if (name) {
-    user.name = name;
-  }
-  if (avatarUrl) {
-    user.avatarUrl = avatarUrl;
-  }
-  if (language) {
-    user.language = language;
-  }
-  if (preferences) {
-    assertKeysIn(preferences, UserPreference);
+router.post(
+  "users.update",
+  auth(),
+  transaction(),
+  async (ctx: TransactionContext) => {
+    const { user, transaction } = ctx.state;
+    const { name, avatarUrl, language, preferences } = ctx.request.body;
+    if (name) {
+      user.name = name;
+    }
+    if (avatarUrl) {
+      user.avatarUrl = avatarUrl;
+    }
+    if (language) {
+      user.language = language;
+    }
+    if (preferences) {
+      assertKeysIn(preferences, UserPreference);
 
-    for (const value of Object.values(UserPreference)) {
-      if (has(preferences, value)) {
-        assertBoolean(preferences[value]);
-        user.setPreference(value, preferences[value]);
+      for (const value of Object.values(UserPreference)) {
+        if (has(preferences, value)) {
+          assertBoolean(preferences[value]);
+          user.setPreference(value, preferences[value]);
+        }
       }
     }
-  }
-  await user.save();
-  await Event.create({
-    name: "users.update",
-    actorId: user.id,
-    userId: user.id,
-    teamId: user.teamId,
-    ip: ctx.request.ip,
-  });
+    await user.save({ transaction });
+    await Event.create(
+      {
+        name: "users.update",
+        actorId: user.id,
+        userId: user.id,
+        teamId: user.teamId,
+        ip: ctx.request.ip,
+      },
+      { transaction }
+    );
 
-  ctx.body = {
-    data: presentUser(user, {
-      includeDetails: true,
-    }),
-  };
-});
+    ctx.body = {
+      data: presentUser(user, {
+        includeDetails: true,
+      }),
+    };
+  }
+);
 
 // Admin specific
 router.post("users.promote", auth(), async (ctx) => {

server/utils/parseAttachmentIds.ts

@@ -1,13 +1,18 @@
 import { uniq, compact } from "lodash";
 
-const attachmentRegex = /\/api\/attachments\.redirect\?id=(?<id>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})/gi;
+const attachmentRedirectRegex = /\/api\/attachments\.redirect\?id=(?<id>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})/gi;
+const attachmentPublicRegex = /public\/([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})\/(?<id>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})/gi;
 
-export default function parseAttachmentIds(text: string): string[] {
+export default function parseAttachmentIds(
+  text: string,
+  includePublic = false
+): string[] {
   return uniq(
     compact(
-      [...text.matchAll(attachmentRegex)].map(
-        (match) => match.groups && match.groups.id
-      )
+      [
+        ...text.matchAll(attachmentRedirectRegex),
+        ...(includePublic ? text.matchAll(attachmentPublicRegex) : []),
+      ].map((match) => match.groups && match.groups.id)
     )
   );
 }