From 79cbe304da6d38a185b864cebddefabdf2f3061e Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 29 Oct 2022 09:08:20 -0400 Subject: [PATCH] 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 | 35 +++++++++ server/models/User.ts | 34 +++++++++ server/queues/tasks/DeleteAttachmentTask.ts | 17 +++++ server/routes/api/attachments.test.ts | 63 +++++++++++------ server/routes/api/attachments.ts | 14 ++-- server/routes/api/users.ts | 78 ++++++++++++--------- server/utils/parseAttachmentIds.ts | 15 ++-- 7 files changed, 191 insertions(+), 65 deletions(-) create mode 100644 server/queues/tasks/DeleteAttachmentTask.ts diff --git a/server/models/Team.ts b/server/models/Team.ts index 037628463..1e7fd700c 100644 --- a/server/models/Team.ts +++ b/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; diff --git a/server/models/User.ts b/server/models/User.ts index 2c9dd5a0f..1cf03eb47 100644 --- a/server/models/User.ts +++ b/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 diff --git a/server/queues/tasks/DeleteAttachmentTask.ts b/server/queues/tasks/DeleteAttachmentTask.ts new file mode 100644 index 000000000..e080f9f6d --- /dev/null +++ b/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 { + public async perform({ attachmentId }: Props) { + const attachment = await Attachment.findByPk(attachmentId); + if (!attachment) { + return; + } + + await attachment.destroy(); + } +} diff --git a/server/routes/api/attachments.test.ts b/server/routes/api/attachments.test.ts index dd9e3541c..7deab6300 100644 --- a/server/routes/api/attachments.test.ts +++ b/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); }); }); diff --git a/server/routes/api/attachments.ts b/server/routes/api/attachments.ts index 4c92ea15b..b447279ed 100644 --- a/server/routes/api/attachments.ts +++ b/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, diff --git a/server/routes/api/users.ts b/server/routes/api/users.ts index acd0f8cdb..85c79ee5c 100644 --- a/server/routes/api/users.ts +++ b/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) => { diff --git a/server/utils/parseAttachmentIds.ts b/server/utils/parseAttachmentIds.ts index c1f8b86e3..b646ea0e3 100644 --- a/server/utils/parseAttachmentIds.ts +++ b/server/utils/parseAttachmentIds.ts @@ -1,13 +1,18 @@ import { uniq, compact } from "lodash"; -const attachmentRegex = /\/api\/attachments\.redirect\?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=(?[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})\/(?[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) ) ); }