chore: Remove future public bucket usage (#10977)

* No longer upload avatars to public bucket

* Public redirect

* tests

* test

* test

plugins/linear/server/tasks/UploadLinearWorkspaceLogoTask.ts

@@ -1,9 +1,9 @@
 import type { IntegrationType } from "@shared/types";
-import { IntegrationService } from "@shared/types";
-import { Integration } from "@server/models";
-import { Buckets } from "@server/models/helpers/AttachmentHelper";
+import { IntegrationService, AttachmentPreset } from "@shared/types";
+import attachmentCreator from "@server/commands/attachmentCreator";
+import { createContext } from "@server/context";
+import { Integration, User } from "@server/models";
 import { BaseTask, TaskPriority } from "@server/queues/tasks/base/BaseTask";
-import FileStorage from "@server/storage/files";
 
 type Props = {
   /** The integrationId to operate on */
@@ -25,19 +25,26 @@ export default class UploadLinearWorkspaceLogoTask extends BaseTask<Props> {
       return;
     }
 
-    const res = await FileStorage.storeFromUrl(
-      props.logoUrl,
-      `${Buckets.avatars}/${integration.teamId}/${crypto.randomUUID()}`,
-      "public-read",
-      {
+    const user = await User.findByPk(integration.userId);
+    if (!user) {
+      return;
+    }
+
+    const attachment = await attachmentCreator({
+      name: "logo",
+      url: props.logoUrl,
+      user,
+      preset: AttachmentPreset.Avatar,
+      ctx: createContext({ user }),
+      fetchOptions: {
         headers: {
           Authorization: `Bearer ${integration.authentication.token}`,
         },
-      }
-    );
+      },
+    });
 
-    if (res?.url) {
-      integration.settings.linear!.workspace.logoUrl = res.url;
+    if (attachment) {
+      integration.settings.linear!.workspace.logoUrl = attachment.url;
       integration.changed("settings", true);
       await integration.save();
     }

plugins/storage/server/api/files.test.ts

@@ -5,7 +5,9 @@ import FormData from "form-data";
 import { ensureDirSync } from "fs-extra";
 import { FileOperationState, FileOperationType } from "@shared/types";
 import env from "@server/env";
-import { Buckets } from "@server/models/helpers/AttachmentHelper";
+import AttachmentHelper, {
+  Buckets,
+} from "@server/models/helpers/AttachmentHelper";
 import FileStorage from "@server/storage/files";
 import {
   buildAttachment,
@@ -13,6 +15,7 @@ import {
   buildUser,
 } from "@server/test/factories";
 import { getTestServer } from "@server/test/support";
+import { randomUUID } from "crypto";
 
 const server = getTestServer();
 
@@ -175,6 +178,12 @@ describe("#files.get", () => {
 
     const attachment = await buildAttachment(
       {
+        acl: "public-read",
+        key: AttachmentHelper.getKey({
+          id: randomUUID(),
+          name: fileName,
+          userId: user.id,
+        }).replace(Buckets.uploads, Buckets.public),
         teamId: user.teamId,
         userId: user.id,
         contentType:

server/commands/attachmentCreator.ts

@@ -47,7 +47,6 @@ export default async function attachmentCreator({
 }: Props): Promise<Attachment | undefined> {
   const acl = AttachmentHelper.presetToAcl(preset);
   const key = AttachmentHelper.getKey({
-    acl,
     id: randomUUID(),
     name,
     userId: user.id,

server/models/Attachment.ts

@@ -32,6 +32,7 @@ import { SkipChangeset } from "./decorators/Changeset";
 import Fix from "./decorators/Fix";
 import Length from "./validators/Length";
 import Logger from "@server/logging/Logger";
+import { Buckets } from "./helpers/AttachmentHelper";
 
 @Table({ tableName: "attachments", modelName: "attachment" })
 @Fix
@@ -78,6 +79,17 @@ class Attachment extends IdModel<
     return path.parse(this.key).base;
   }
 
+  /**
+   * Whether the attachment is stored in a public bucket. This does not relate
+   * to the ACL of the attachment itself. Previously "public" attachments were
+   * stored in a separate bucket – now all attachments are stored in a private
+   * bucket and ACL is checked per attachment.
+   */
+  get isStoredInPublicBucket() {
+    const bucket = this.key.split("/")[0];
+    return [Buckets.avatars, Buckets.public].includes(bucket as Buckets);
+  }
+
   /**
    * Whether the attachment is private or not.
    */

server/models/helpers/AttachmentHelper.test.ts

@@ -2,20 +2,8 @@ import AttachmentHelper from "./AttachmentHelper";
 
 describe("AttachmentHelper", () => {
   describe("getKey", () => {
-    it("should return the correct key for a public attachment", () => {
-      const key = AttachmentHelper.getKey({
-        acl: "public-read",
-        id: "123",
-        name: "test.png",
-        userId: "456",
-      });
-
-      expect(key).toEqual("public/456/123/test.png");
-    });
-
     it("should return the correct key for a private attachment", () => {
       const key = AttachmentHelper.getKey({
-        acl: "private",
         id: "123",
         name: "test.png",
         userId: "456",
@@ -26,26 +14,24 @@ describe("AttachmentHelper", () => {
 
     it("should return the correct key for a long file name", () => {
       const key = AttachmentHelper.getKey({
-        acl: "public-read",
         id: "123",
         name: "a".repeat(300),
         userId: "456",
       });
 
       expect(key).toEqual(
-        `public/456/123/${"a".repeat(AttachmentHelper.maximumFileNameLength)}`
+        `uploads/456/123/${"a".repeat(AttachmentHelper.maximumFileNameLength)}`
       );
     });
 
     it("should remove invalid characters from the key", () => {
       const key = AttachmentHelper.getKey({
-        acl: "public-read",
         id: "123",
         name: "test/../one.png",
         userId: "456",
       });
 
-      expect(key).toEqual("public/456/123/test/one.png");
+      expect(key).toEqual("uploads/456/123/test/one.png");
     });
   });
 });

server/models/helpers/AttachmentHelper.ts

@@ -16,24 +16,20 @@ export default class AttachmentHelper {
   /**
    * Get the upload location for the given upload details
    *
-   * @param acl The ACL to use
    * @param id The ID of the attachment
    * @param name The name of the attachment
    * @param userId The ID of the user uploading the attachment
    */
   static getKey({
-    acl,
     id,
     name,
     userId,
   }: {
-    acl: string;
     id: string;
     name: string;
     userId: string;
   }) {
-    const bucket = acl === "public-read" ? Buckets.public : Buckets.uploads;
-    const keyPrefix = `${bucket}/${userId}/${id}`;
+    const keyPrefix = `${Buckets.uploads}/${userId}/${id}`;
     return ValidateKey.sanitize(
       `${keyPrefix}/${name.slice(0, this.maximumFileNameLength)}`
     );

server/queues/tasks/APIImportTask.ts

@@ -295,7 +295,6 @@ export default abstract class APIImportTask<
           AttachmentPreset.DocumentAttachment
         );
         const key = AttachmentHelper.getKey({
-          acl,
           id: modelId,
           name: item.name,
           userId: createdBy.id,

server/queues/tasks/UploadTeamAvatarTask.ts

@@ -1,7 +1,7 @@
-import { randomUUID } from "crypto";
-import { Team } from "@server/models";
-import { Buckets } from "@server/models/helpers/AttachmentHelper";
-import FileStorage from "@server/storage/files";
+import { AttachmentPreset } from "@shared/types";
+import attachmentCreator from "@server/commands/attachmentCreator";
+import { createContext } from "@server/context";
+import { Team, User } from "@server/models";
 import { BaseTask, TaskPriority } from "./base/BaseTask";
 
 type Props = {
@@ -21,14 +21,26 @@ export default class UploadTeamAvatarTask extends BaseTask<Props> {
       rejectOnEmpty: true,
     });
 
-    const res = await FileStorage.storeFromUrl(
-      props.avatarUrl,
-      `${Buckets.avatars}/${team.id}/${randomUUID()}`,
-      "public-read"
-    );
+    const user = await User.findOne({
+      where: {
+        teamId: team.id,
+      },
+    });
 
-    if (res?.url) {
-      await team.update({ avatarUrl: res.url });
+    if (!user) {
+      return;
+    }
+
+    const attachment = await attachmentCreator({
+      name: "avatar",
+      url: props.avatarUrl,
+      user,
+      preset: AttachmentPreset.Avatar,
+      ctx: createContext({ user }),
+    });
+
+    if (attachment) {
+      await team.update({ avatarUrl: attachment.url });
     }
   }
 

server/queues/tasks/UploadUserAvatarTask.ts

@@ -1,7 +1,8 @@
-import { createHash, randomUUID } from "crypto";
+import { createHash } from "crypto";
+import { AttachmentPreset } from "@shared/types";
+import attachmentCreator from "@server/commands/attachmentCreator";
+import { createContext } from "@server/context";
 import { User } from "@server/models";
-import { Buckets } from "@server/models/helpers/AttachmentHelper";
-import FileStorage from "@server/storage/files";
 import { BaseTask, TaskPriority } from "./base/BaseTask";
 
 type Props = {
@@ -28,14 +29,16 @@ export default class UploadUserAvatarTask extends BaseTask<Props> {
       return;
     }
 
-    const res = await FileStorage.storeFromUrl(
-      props.avatarUrl,
-      `${Buckets.avatars}/${user.id}/${randomUUID()}/${hash}`,
-      "public-read"
-    );
+    const attachment = await attachmentCreator({
+      name: hash,
+      url: props.avatarUrl,
+      user,
+      preset: AttachmentPreset.Avatar,
+      ctx: createContext({ user }),
+    });
 
-    if (res?.url) {
-      await user.update({ avatarUrl: res.url });
+    if (attachment) {
+      await user.update({ avatarUrl: attachment.url });
     }
   }
 

server/routes/api/attachments/attachments.test.ts

@@ -1,3 +1,4 @@
+import { randomUUID } from "crypto";
 import { AttachmentPreset, CollectionPermission } from "@shared/types";
 import { UserMembership } from "@server/models";
 import Attachment from "@server/models/Attachment";
@@ -425,11 +426,6 @@ describe("#attachments.delete", () => {
 });
 
 describe("#attachments.redirect", () => {
-  it("should require authentication", async () => {
-    const res = await server.post("/api/attachments.redirect");
-    expect(res.status).toEqual(401);
-  });
-
   it("should return a redirect for an attachment belonging to a document user has access to", async () => {
     const user = await buildUser();
     const attachment = await buildAttachment({
@@ -518,6 +514,35 @@ describe("#attachments.redirect", () => {
     expect(res.status).toEqual(302);
   });
 
+  it("should return a redirect for an attachment in a public bucket without authentication", async () => {
+    const attachment = await buildAttachment({
+      key: `public/${randomUUID()}/test.png`,
+      acl: "public-read",
+    });
+    const res = await server.post("/api/attachments.redirect", {
+      body: {
+        id: attachment.id,
+      },
+      redirect: "manual",
+    });
+    expect(res.status).toEqual(302);
+    expect(res.headers.get("location")).toContain(attachment.canonicalUrl);
+  });
+
+  it("should return a redirect for a public-read attachment without authentication (not in public bucket)", async () => {
+    const attachment = await buildAttachment({
+      acl: "public-read",
+    });
+    const res = await server.post("/api/attachments.redirect", {
+      body: {
+        id: attachment.id,
+      },
+      redirect: "manual",
+    });
+    expect(res.status).toEqual(302);
+    expect(res.headers.get("location")).toContain(await attachment.signedUrl);
+  });
+
   it("should not return a redirect for a private attachment belonging to a document user does not have access to", async () => {
     const user = await buildUser();
     const collection = await buildCollection({

server/routes/api/attachments/attachments.ts

@@ -121,7 +121,6 @@ router.post(
     const modelId = id ?? randomUUID();
     const acl = AttachmentHelper.presetToAcl(preset);
     const key = AttachmentHelper.getKey({
-      acl,
       id: modelId,
       name,
       userId: user.id,
@@ -157,12 +156,7 @@ router.post(
         },
         attachment: {
           ...presentAttachment(attachment),
-          // always use the redirect url for document attachments, as the serializer
-          // depends on it to detect attachment vs link
-          url:
-            preset === AttachmentPreset.DocumentAttachment
-              ? attachment.redirectUrl
-              : attachment.url,
+          url: attachment.redirectUrl,
         },
       },
     };
@@ -193,7 +187,6 @@ router.post(
     const modelId = id ?? randomUUID();
     const acl = AttachmentHelper.presetToAcl(preset);
     const key = AttachmentHelper.getKey({
-      acl,
       id: modelId,
       name,
       userId: user.id,
@@ -278,12 +271,12 @@ const handleAttachmentsRedirect = async (
 ) => {
   const id = (ctx.input.body.id ?? ctx.input.query.id) as string;
 
-  const { user } = ctx.state.auth;
+  const user = ctx.state.auth?.user;
   const attachment = await Attachment.findByPk(id, {
     rejectOnEmpty: true,
   });
 
-  if (attachment.isPrivate && attachment.teamId !== user.teamId) {
+  if (attachment.isPrivate && attachment.teamId !== user?.teamId) {
     throw AuthorizationError();
   }
 
@@ -296,27 +289,27 @@ const handleAttachmentsRedirect = async (
     }
   );
 
-  if (attachment.isPrivate) {
+  if (attachment.isStoredInPublicBucket) {
+    ctx.set("Cache-Control", `max-age=604800, immutable`);
+    ctx.redirect(attachment.canonicalUrl);
+  } else {
     ctx.set(
       "Cache-Control",
       `max-age=${BaseStorage.defaultSignedUrlExpires}, immutable`
     );
     ctx.redirect(await attachment.signedUrl);
-  } else {
-    ctx.set("Cache-Control", `max-age=604800, immutable`);
-    ctx.redirect(attachment.canonicalUrl);
   }
 };
 
 router.get(
   "attachments.redirect",
-  auth(),
+  auth({ optional: true }),
   validate(T.AttachmentsRedirectSchema),
   handleAttachmentsRedirect
 );
 router.post(
   "attachments.redirect",
-  auth(),
+  auth({ optional: true }),
   validate(T.AttachmentsRedirectSchema),
   handleAttachmentsRedirect
 );

server/routes/api/documents/documents.ts

@@ -1514,7 +1514,6 @@ router.post(
     const acl = "private";
 
     const key = AttachmentHelper.getKey({
-      acl,
       id: randomUUID(),
       name: fileName,
       userId: user.id,

server/test/factories.ts

@@ -564,7 +564,7 @@ export async function buildAttachment(
   const name = fileName || faker.system.fileName();
   return Attachment.create({
     id,
-    key: AttachmentHelper.getKey({ acl, id, name, userId: overrides.userId }),
+    key: AttachmentHelper.getKey({ id, name, userId: overrides.userId }),
     contentType: "image/png",
     size: 100,
     acl,