Bugfix(modeling-commons): use public ACL image previews for uploads

omargfh · omargfh · commit e1cfab4b946a · 2026-06-18T15:58:59.000-05:00
diff --git a/apps/modeling-commons-backend/src/modules/model-draft/model-draft.service.spec.ts b/apps/modeling-commons-backend/src/modules/model-draft/model-draft.service.spec.ts
@@ -255,6 +255,75 @@ describe('modelDraftService', () => {
       expect(next.attachments).toHaveLength(1);
       expect(next.attachments![0]!.s3Key).toBe('staging/user-1/draft-1/att.csv');
     });
+
+    it('stages a preview image as a public object and persists it', async () => {
+      const { service, modelDraftRepository, modelDraftStorage } = buildService();
+      modelDraftStorage.putStaged.mockResolvedValue(
+        'files/public/staging/user-1/draft-1/abc-preview.png',
+      );
+      const draft = makeDraft();
+
+      const result = await service.addFile(draft, 'preview', {
+        buffer: Buffer.from('img') as Buffer<ArrayBuffer>,
+        filename: 'preview.png',
+        contentType: 'image/png',
+      });
+
+      expect(result.role).toBe('preview');
+      expect(modelDraftStorage.putStaged).toHaveBeenCalledWith(
+        expect.objectContaining({ public: true }),
+      );
+      const next = modelDraftRepository.updateDataTx.mock.calls[0]![3] as DraftDataV1;
+      expect(next.previewImage?.s3Key).toBe('files/public/staging/user-1/draft-1/abc-preview.png');
+    });
+
+    it('stages non-preview files as private objects', async () => {
+      const { service, modelDraftStorage } = buildService();
+      modelDraftStorage.putStaged.mockResolvedValue('staging/user-1/draft-1/abc-new.nlogo');
+
+      await service.addFile(makeDraft(), 'primary', {
+        buffer: Buffer.from('x') as Buffer<ArrayBuffer>,
+        filename: 'new.nlogo',
+        contentType: 'text/plain',
+      });
+
+      expect(modelDraftStorage.putStaged).toHaveBeenCalledWith(
+        expect.objectContaining({ public: false }),
+      );
+    });
+  });
+
+  describe('generatePreviewImage', () => {
+    function buildWithPreview() {
+      const previewImageService = {
+        generatePreviewFromNetlogoFile: vi
+          .fn()
+          .mockResolvedValue({ buffer: new Uint8Array([1, 2, 3]).buffer, contentType: 'image/png' }),
+      };
+      return { previewImageService, ...buildService({ previewImageService }) };
+    }
+
+    it('stages the generated preview as a public object', async () => {
+      const { service, modelDraftStorage } = buildWithPreview();
+      modelDraftStorage.putStaged.mockResolvedValue(
+        'files/public/staging/user-1/draft-1/uuid-preview.png',
+      );
+      const draft = makeDraft({ primaryFile: validPrimary });
+
+      const result = await service.generatePreviewImage(draft);
+
+      expect(result.s3Key).toBe('files/public/staging/user-1/draft-1/uuid-preview.png');
+      expect(modelDraftStorage.putStaged).toHaveBeenCalledWith(
+        expect.objectContaining({ public: true, filename: 'preview.png', contentType: 'image/png' }),
+      );
+    });
+
+    it('throws when the draft has no primary file', async () => {
+      const { service } = buildWithPreview();
+      await expect(service.generatePreviewImage(makeDraft({}))).rejects.toThrow(
+        ModelDraftFileNotFoundError,
+      );
+    });
   });
 
   describe('abandon', () => {
diff --git a/apps/modeling-commons-backend/src/modules/model-draft/model-draft.service.ts b/apps/modeling-commons-backend/src/modules/model-draft/model-draft.service.ts
@@ -23,6 +23,7 @@ import {
   type DraftPreviewImageV1,
   type DraftPrimaryFileV1,
 } from '#src/modules/model-draft/schemas/index.ts';
+import { PUBLIC_PREFIX } from '#src/modules/file/domain/file.types.ts';
 import { UnauthorizedException } from '#src/shared/exceptions/exceptions.ts';
 import { canWrite } from '#src/shared/permissions/model-access.policy.ts';
 import { loadModelAccessContext } from '#src/shared/permissions/model-access.viewer.ts';
@@ -79,8 +80,16 @@ export default function makeModelDraftService({
     });
   }
 
-  function stagingKey(userId: string, draftId: string, filename: string): string {
-    return `staging/${userId}/${draftId}/${randomUUID()}-${sanitizeFilename(filename)}`;
+  function stagingKey(
+    userId: string,
+    draftId: string,
+    filename: string,
+    isPublic = false,
+  ): string {
+    const prefix = isPublic
+      ? `${PUBLIC_PREFIX}/staging/${userId}/${draftId}/`
+      : `staging/${userId}/${draftId}/`;
+    return `${prefix}${randomUUID()}-${sanitizeFilename(filename)}`;
   }
 
   function filenameFromKey(key: string): string {
@@ -134,13 +143,15 @@ export default function makeModelDraftService({
     userId: string;
     draftId: string;
     filename: string;
+    public?: boolean;
   }): Promise<{ s3Key: string; sizeBytes: number; mimeType: string }> {
-    const destKey = stagingKey(params.userId, params.draftId, params.filename);
+    const destKey = stagingKey(params.userId, params.draftId, params.filename, params.public);
     await storage.send(
       new CopyObjectCommand({
         Bucket: bucket.Name,
         Key: destKey,
         CopySource: `${bucket.Name}/${params.sourceKey}`,
+        ...(params.public ? { ACL: 'public-read' } : {}),
       }),
     );
     const head = await storage.send(new HeadObjectCommand({ Bucket: bucket.Name, Key: destKey }));
@@ -219,6 +230,7 @@ export default function makeModelDraftService({
           userId,
           draftId,
           filename: filenameFromKey(version.previewImageFileKey),
+          public: true,
         })
       : undefined;
 
@@ -287,6 +299,7 @@ export default function makeModelDraftService({
         buffer: file.buffer,
         filename: file.filename,
         contentType: file.contentType,
+        public: role === 'preview',
       });
 
       const data = currentData(draft);
@@ -344,6 +357,7 @@ export default function makeModelDraftService({
         buffer: previewBuffer,
         filename: 'preview.png',
         contentType: 'image/png',
+        public: true,
       });
 
       if (data.previewImage) {
diff --git a/apps/modeling-commons-backend/src/modules/model-draft/model-draft.storage.spec.ts b/apps/modeling-commons-backend/src/modules/model-draft/model-draft.storage.spec.ts
@@ -0,0 +1,88 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import makeModelDraftStorage from '#src/modules/model-draft/model-draft.storage.ts';
+import { PUBLIC_PREFIX } from '#src/modules/file/domain/file.types.ts';
+
+describe('modelDraftStorage', () => {
+  const storage = { send: vi.fn() };
+  const bucket = { Name: 'test-bucket' };
+  const fileDomain = { createFile: vi.fn() };
+
+  const draftStorage = makeModelDraftStorage({ storage, bucket, fileDomain } as never);
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  describe('putStaged', () => {
+    it('stages private files under the staging prefix without an ACL', async () => {
+      storage.send.mockResolvedValue({});
+
+      const key = await draftStorage.putStaged({
+        userId: 'u1',
+        draftId: 'd1',
+        buffer: Buffer.from('model') as Buffer<ArrayBuffer>,
+        filename: 'model.nlogox',
+        contentType: 'text/plain',
+      });
+
+      expect(key.startsWith('staging/u1/d1/')).toBe(true);
+      expect(key.startsWith(`${PUBLIC_PREFIX}/`)).toBe(false);
+      const cmd = storage.send.mock.calls[0]![0];
+      expect(cmd.input.Key).toBe(key);
+      expect(cmd.input.ACL).toBeUndefined();
+    });
+
+    it('stages public files under the public prefix with a public-read ACL', async () => {
+      storage.send.mockResolvedValue({});
+
+      const key = await draftStorage.putStaged({
+        userId: 'u1',
+        draftId: 'd1',
+        buffer: Buffer.from('img') as Buffer<ArrayBuffer>,
+        filename: 'preview.png',
+        contentType: 'image/png',
+        public: true,
+      });
+
+      expect(key.startsWith(`${PUBLIC_PREFIX}/staging/u1/d1/`)).toBe(true);
+      const cmd = storage.send.mock.calls[0]![0];
+      expect(cmd.input.Key).toBe(key);
+      expect(cmd.input.ACL).toBe('public-read');
+    });
+  });
+
+  describe('deleteStagingPrefix', () => {
+    it('sweeps both the private and public staging prefixes', async () => {
+      storage.send.mockImplementation(async (cmd: { constructor: { name: string }; input: { Prefix?: string } }) => {
+        if (cmd.constructor.name === 'ListObjectsV2Command') {
+          return { Contents: [{ Key: `${cmd.input.Prefix}leftover.bin` }], IsTruncated: false };
+        }
+        return {};
+      });
+
+      await draftStorage.deleteStagingPrefix('u1', 'd1');
+
+      const sent = storage.send.mock.calls.map((c) => c[0]);
+      const listedPrefixes = sent
+        .filter((cmd) => cmd.constructor.name === 'ListObjectsV2Command')
+        .map((cmd) => cmd.input.Prefix);
+      expect(listedPrefixes).toContain('staging/u1/d1/');
+      expect(listedPrefixes).toContain(`${PUBLIC_PREFIX}/staging/u1/d1/`);
+
+      const deletedKeys = sent
+        .filter((cmd) => cmd.constructor.name === 'DeleteObjectsCommand')
+        .flatMap((cmd) => cmd.input.Delete.Objects.map((o: { Key: string }) => o.Key));
+      expect(deletedKeys).toContain('staging/u1/d1/leftover.bin');
+      expect(deletedKeys).toContain(`${PUBLIC_PREFIX}/staging/u1/d1/leftover.bin`);
+    });
+
+    it('skips deletion when a prefix is empty', async () => {
+      storage.send.mockResolvedValue({ Contents: [], IsTruncated: false });
+
+      await draftStorage.deleteStagingPrefix('u1', 'd1');
+
+      const sentNames = storage.send.mock.calls.map((c) => c[0].constructor.name);
+      expect(sentNames).not.toContain('DeleteObjectsCommand');
+    });
+  });
+});
diff --git a/apps/modeling-commons-backend/src/modules/model-draft/model-draft.storage.ts b/apps/modeling-commons-backend/src/modules/model-draft/model-draft.storage.ts
@@ -1,3 +1,4 @@
+import { PUBLIC_PREFIX } from '#src/modules/file/domain/file.types.ts';
 import {
   CopyObjectCommand,
   DeleteObjectCommand,
@@ -13,8 +14,45 @@ export default function makeModelDraftStorage({ storage, bucket, fileDomain }: D
     return `staging/${userId}/${draftId}/`;
   }
 
-  function stagingKey(userId: string, draftId: string, filename: string): string {
-    return `${stagingPrefix(userId, draftId)}${randomUUID()}-${sanitizeFilename(filename)}`;
+  function publicStagingPrefix(userId: string, draftId: string): string {
+    return `${PUBLIC_PREFIX}/staging/${userId}/${draftId}/`;
+  }
+
+  function stagingKey(
+    userId: string,
+    draftId: string,
+    filename: string,
+    isPublic = false,
+  ): string {
+    const prefix = isPublic
+      ? publicStagingPrefix(userId, draftId)
+      : stagingPrefix(userId, draftId);
+    return `${prefix}${randomUUID()}-${sanitizeFilename(filename)}`;
+  }
+
+  async function deletePrefix(prefix: string): Promise<void> {
+    let continuationToken: string | undefined;
+    do {
+      const listed = await storage.send(
+        new ListObjectsV2Command({
+          Bucket: bucket.Name,
+          Prefix: prefix,
+          ContinuationToken: continuationToken,
+        }),
+      );
+      const keys = (listed.Contents ?? [])
+        .map((o) => o.Key)
+        .filter((k): k is string => Boolean(k));
+      if (keys.length > 0) {
+        await storage.send(
+          new DeleteObjectsCommand({
+            Bucket: bucket.Name,
+            Delete: { Objects: keys.map((Key) => ({ Key })) },
+          }),
+        );
+      }
+      continuationToken = listed.IsTruncated ? listed.NextContinuationToken : undefined;
+    } while (continuationToken);
   }
 
   return {
@@ -24,14 +62,16 @@ export default function makeModelDraftStorage({ storage, bucket, fileDomain }: D
       buffer: Buffer<ArrayBuffer>;
       filename: string;
       contentType: string;
+      public?: boolean;
     }): Promise<string> {
-      const key = stagingKey(params.userId, params.draftId, params.filename);
+      const key = stagingKey(params.userId, params.draftId, params.filename, params.public);
       await storage.send(
         new PutObjectCommand({
           Bucket: bucket.Name,
           Key: key,
           Body: params.buffer,
           ContentType: params.contentType,
+          ...(params.public ? { ACL: 'public-read' } : {}),
           Metadata: {
             filename: sanitizeFilename(params.filename),
             createdat: new Date().toISOString(),
@@ -77,29 +117,8 @@ export default function makeModelDraftStorage({ storage, bucket, fileDomain }: D
     },
 
     async deleteStagingPrefix(userId: string, draftId: string): Promise<void> {
-      const prefix = stagingPrefix(userId, draftId);
-      let continuationToken: string | undefined;
-      do {
-        const listed = await storage.send(
-          new ListObjectsV2Command({
-            Bucket: bucket.Name,
-            Prefix: prefix,
-            ContinuationToken: continuationToken,
-          }),
-        );
-        const keys = (listed.Contents ?? [])
-          .map((o) => o.Key)
-          .filter((k): k is string => Boolean(k));
-        if (keys.length > 0) {
-          await storage.send(
-            new DeleteObjectsCommand({
-              Bucket: bucket.Name,
-              Delete: { Objects: keys.map((Key) => ({ Key })) },
-            }),
-          );
-        }
-        continuationToken = listed.IsTruncated ? listed.NextContinuationToken : undefined;
-      } while (continuationToken);
+      await deletePrefix(stagingPrefix(userId, draftId));
+      await deletePrefix(publicStagingPrefix(userId, draftId));
     },
   };
 }
diff --git a/apps/modeling-commons-backend/tests/api/model-draft.feature b/apps/modeling-commons-backend/tests/api/model-draft.feature
@@ -69,6 +69,29 @@ Feature: Model Drafts
     And the response body should have property "modelId"
     And the response body should have property "versionNumber"
 
+  Scenario: Uploading a preview image returns a public unsigned URL
+    Given an authenticated user
+    And an empty draft
+    When I upload a preview image to the draft
+    Then the response status should be 201
+    And the response body should have property "previewImageUrl"
+    And the response body property "previewImageUrl" should contain "files/public/"
+    And the response body property "previewImageUrl" should not contain "X-Amz-Signature"
+
+  Scenario: A draft seeded from a model with a preview keeps it public and unsigned
+    Given an authenticated user
+    And an empty draft
+    When I patch the draft with title "Seeded Preview Model" and visibility "public"
+    And I upload a primary file to the draft
+    And I upload a preview image to the draft
+    And I publish the draft
+    Then the response status should be 201
+    When I seed a new draft from the published model
+    And I get the draft
+    Then the response status should be 200
+    And the response body property "previewImageUrl" should contain "files/public/"
+    And the response body property "previewImageUrl" should not contain "X-Amz-Signature"
+
   @smoke
   Scenario: Publishing a draft seeded from an existing model creates a new version
     Given an authenticated user
diff --git a/apps/modeling-commons-backend/tests/api/model-draft.steps.ts b/apps/modeling-commons-backend/tests/api/model-draft.steps.ts
