updated upload unit tests for s3 image handling

2025-12-19 18:58:30 -05:00
parent 4b4584bc0f
commit 4e0a4ef019
2 changed files with 435 additions and 53 deletions
--- a/backend/tests/unit/routes/upload.test.js
+++ b/backend/tests/unit/routes/upload.test.js
@@ -446,15 +446,137 @@ describe('Upload Routes', () => {
    });
  });

-  // Note: The GET /upload/signed-url/*key route uses Express 5 wildcard syntax
-  // which is not fully compatible with the test environment when mocking.
-  // The S3OwnershipService functionality is tested separately in s3OwnershipService.test.js
-  // The route integration is verified in integration tests.
-  describe('GET /upload/signed-url/*key (wildcard route)', () => {
-    it('should be defined as a route', () => {
-      // The route exists and is properly configured
-      // Full integration testing of wildcard routes is done in integration tests
-      expect(true).toBe(true);
+  describe('GET /upload/signed-url/:key(*)', () => {
+    const mockSignedUrl = 'https://bucket.s3.amazonaws.com/messages/uuid.jpg?signature=abc';
+
+    beforeEach(() => {
+      mockGetPresignedDownloadUrl.mockResolvedValue(mockSignedUrl);
+      mockCanAccessFile.mockResolvedValue({ authorized: true });
+    });
+
+    it('should return signed URL for authorized private content (messages)', async () => {
+      const response = await request(app)
+        .get('/upload/signed-url/messages/550e8400-e29b-41d4-a716-446655440000.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(200);
+      expect(response.body.url).toBe(mockSignedUrl);
+      expect(response.body.expiresIn).toBe(3600);
+
+      expect(mockCanAccessFile).toHaveBeenCalledWith(
+        'messages/550e8400-e29b-41d4-a716-446655440000.jpg',
+        'user-123'
+      );
+      expect(mockGetPresignedDownloadUrl).toHaveBeenCalledWith(
+        'messages/550e8400-e29b-41d4-a716-446655440000.jpg'
+      );
+    });
+
+    it('should return signed URL for authorized condition-check content', async () => {
+      const response = await request(app)
+        .get('/upload/signed-url/condition-checks/550e8400-e29b-41d4-a716-446655440000.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(200);
+      expect(response.body.url).toBe(mockSignedUrl);
+
+      expect(mockCanAccessFile).toHaveBeenCalledWith(
+        'condition-checks/550e8400-e29b-41d4-a716-446655440000.jpg',
+        'user-123'
+      );
+    });
+
+    it('should require authentication', async () => {
+      const response = await request(app)
+        .get('/upload/signed-url/messages/uuid.jpg');
+
+      expect(response.status).toBe(401);
+      expect(mockGetPresignedDownloadUrl).not.toHaveBeenCalled();
+    });
+
+    it('should return 503 when S3 is disabled', async () => {
+      mockIsEnabled.mockReturnValue(false);
+
+      const response = await request(app)
+        .get('/upload/signed-url/messages/uuid.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(503);
+    });
+
+    it('should return 400 for public folder paths (items)', async () => {
+      const response = await request(app)
+        .get('/upload/signed-url/items/uuid.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(400);
+      expect(response.body.error).toBe('Signed URLs only for private content');
+      expect(mockGetPresignedDownloadUrl).not.toHaveBeenCalled();
+    });
+
+    it('should return 400 for public folder paths (profiles)', async () => {
+      const response = await request(app)
+        .get('/upload/signed-url/profiles/uuid.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(400);
+      expect(response.body.error).toBe('Signed URLs only for private content');
+    });
+
+    it('should return 400 for public folder paths (forum)', async () => {
+      const response = await request(app)
+        .get('/upload/signed-url/forum/uuid.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(400);
+      expect(response.body.error).toBe('Signed URLs only for private content');
+    });
+
+    it('should return 403 when user is not authorized to access file', async () => {
+      mockCanAccessFile.mockResolvedValue({
+        authorized: false,
+        reason: 'Not a participant in this message'
+      });
+
+      const response = await request(app)
+        .get('/upload/signed-url/messages/uuid.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(403);
+      expect(response.body.error).toBe('Access denied');
+      expect(mockGetPresignedDownloadUrl).not.toHaveBeenCalled();
+    });
+
+    it('should handle URL-encoded keys', async () => {
+      const response = await request(app)
+        .get('/upload/signed-url/messages%2Fuuid.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      // The key should be decoded
+      expect(mockCanAccessFile).toHaveBeenCalledWith(
+        expect.stringContaining('messages'),
+        'user-123'
+      );
+    });
+
+    it('should handle S3 service errors gracefully', async () => {
+      mockGetPresignedDownloadUrl.mockRejectedValue(new Error('S3 error'));
+
+      const response = await request(app)
+        .get('/upload/signed-url/messages/uuid.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(500);
+    });
+
+    it('should handle ownership service errors gracefully', async () => {
+      mockCanAccessFile.mockRejectedValue(new Error('Database error'));
+
+      const response = await request(app)
+        .get('/upload/signed-url/messages/uuid.jpg')
+        .set('Authorization', 'Bearer valid-token');
+
+      expect(response.status).toBe(500);
    });
  });
 });
--- a/frontend/src/tests/services/uploadService.test.ts
+++ b/frontend/src/tests/services/uploadService.test.ts
@@ -23,12 +23,88 @@ jest.mock('../../services/api');

 const mockedApi = api as jest.Mocked<typeof api>;

+// Mock XMLHttpRequest for uploadToS3 tests
+class MockXMLHttpRequest {
+  static instances: MockXMLHttpRequest[] = [];
+
+  status = 200;
+  readyState = 4;
+  responseText = '';
+  upload = {
+    onprogress: null as ((e: { lengthComputable: boolean; loaded: number; total: number }) => void) | null,
+  };
+  onload: (() => void) | null = null;
+  onerror: (() => void) | null = null;
+
+  private headers: Record<string, string> = {};
+  private method = '';
+  private url = '';
+
+  constructor() {
+    MockXMLHttpRequest.instances.push(this);
+  }
+
+  open(method: string, url: string) {
+    this.method = method;
+    this.url = url;
+  }
+
+  setRequestHeader(key: string, value: string) {
+    this.headers[key] = value;
+  }
+
+  send(_data: unknown) {
+    // Use Promise.resolve().then for async behavior in tests
+    // This allows promises to resolve without real delays
+    Promise.resolve().then(() => {
+      if (this.upload.onprogress) {
+        this.upload.onprogress({ lengthComputable: true, loaded: 50, total: 100 });
+        this.upload.onprogress({ lengthComputable: true, loaded: 100, total: 100 });
+      }
+      if (this.onload) {
+        this.onload();
+      }
+    });
+  }
+
+  getHeaders() {
+    return this.headers;
+  }
+
+  getMethod() {
+    return this.method;
+  }
+
+  getUrl() {
+    return this.url;
+  }
+
+  static reset() {
+    MockXMLHttpRequest.instances = [];
+  }
+
+  static getLastInstance() {
+    return MockXMLHttpRequest.instances[MockXMLHttpRequest.instances.length - 1];
+  }
+}
+
+// Store original XMLHttpRequest
+const originalXMLHttpRequest = global.XMLHttpRequest;
+
 describe('Upload Service', () => {
  beforeEach(() => {
    jest.clearAllMocks();
+    MockXMLHttpRequest.reset();
    // Reset environment variables
    process.env.REACT_APP_S3_BUCKET = 'test-bucket';
    process.env.REACT_APP_AWS_REGION = 'us-east-1';
+    // Mock XMLHttpRequest globally
+    (global as unknown as { XMLHttpRequest: typeof MockXMLHttpRequest }).XMLHttpRequest = MockXMLHttpRequest;
+  });
+
+  afterEach(() => {
+    // Restore original XMLHttpRequest
+    (global as unknown as { XMLHttpRequest: typeof XMLHttpRequest }).XMLHttpRequest = originalXMLHttpRequest;
  });

  describe('getPublicImageUrl', () => {
@@ -173,18 +249,42 @@ describe('Upload Service', () => {
  });

  describe('uploadToS3', () => {
-    // Note: XMLHttpRequest mocking is complex and can cause timeouts.
-    // The uploadToS3 function is a thin wrapper around XHR.
-    // Testing focuses on verifying the function signature and basic behavior.
+    const mockFile = new File(['test content'], 'photo.jpg', { type: 'image/jpeg' });
+    const mockUploadUrl = 'https://presigned-url.s3.amazonaws.com/items/uuid.jpg?signature=abc';

-    it('should export uploadToS3 function', () => {
-      expect(typeof uploadToS3).toBe('function');
+    it('should upload file successfully', async () => {
+      await uploadToS3(mockFile, mockUploadUrl);
+
+      const instance = MockXMLHttpRequest.getLastInstance();
+      expect(instance.getMethod()).toBe('PUT');
+      expect(instance.getUrl()).toBe(mockUploadUrl);
+      expect(instance.getHeaders()['Content-Type']).toBe('image/jpeg');
    });

-    it('should accept file, url, and options parameters', () => {
-      // Verify function signature
+    it('should call onProgress callback during upload', async () => {
+      const onProgress = jest.fn();
+
+      await uploadToS3(mockFile, mockUploadUrl, { onProgress });
+
+      // Progress should be called at least once
+      expect(onProgress).toHaveBeenCalled();
+      // Should receive percentage values
+      expect(onProgress).toHaveBeenCalledWith(expect.any(Number));
+    });
+
+    it('should export uploadToS3 function with correct signature', () => {
+      expect(typeof uploadToS3).toBe('function');
+      // Function accepts file, url, and optional options
      expect(uploadToS3.length).toBeGreaterThanOrEqual(2);
    });
+
+    it('should set correct content-type header', async () => {
+      const pngFile = new File(['test'], 'image.png', { type: 'image/png' });
+      await uploadToS3(pngFile, mockUploadUrl);
+
+      const instance = MockXMLHttpRequest.getLastInstance();
+      expect(instance.getHeaders()['Content-Type']).toBe('image/png');
+    });
  });

  describe('confirmUploads', () => {
@@ -214,70 +314,230 @@ describe('Upload Service', () => {
  });

  describe('uploadFile', () => {
-    it('should call getPresignedUrl and confirmUploads in sequence', async () => {
-      // Test the flow without mocking XMLHttpRequest (which is complex)
-      // Instead test that the functions are called with correct parameters
-      const file = new File(['test'], 'photo.jpg', { type: 'image/jpeg' });
+    const mockFile = new File(['test content'], 'photo.jpg', { type: 'image/jpeg' });
    const presignResponse: PresignedUrlResponse = {
-        uploadUrl: 'https://presigned.s3.amazonaws.com',
+      uploadUrl: 'https://presigned.s3.amazonaws.com/items/uuid.jpg',
      key: 'items/uuid.jpg',
      publicUrl: 'https://bucket.s3.amazonaws.com/items/uuid.jpg',
      expiresAt: new Date().toISOString(),
    };

+    it('should complete full upload flow successfully', async () => {
+      // Mock presign response
      mockedApi.post.mockResolvedValueOnce({ data: presignResponse });
+      // Mock confirm response
+      mockedApi.post.mockResolvedValueOnce({
+        data: { confirmed: [presignResponse.key], total: 1 },
+      });

-      // Just test getPresignedUrl is called correctly
-      await getPresignedUrl('item', file);
+      const result = await uploadFile('item', mockFile);

+      expect(result).toEqual({
+        key: presignResponse.key,
+        publicUrl: presignResponse.publicUrl,
+      });
+
+      // Verify presign was called
      expect(mockedApi.post).toHaveBeenCalledWith('/upload/presign', {
        uploadType: 'item',
        contentType: 'image/jpeg',
        fileName: 'photo.jpg',
-        fileSize: file.size,
+        fileSize: mockFile.size,
      });
+
+      // Verify confirm was called
+      expect(mockedApi.post).toHaveBeenCalledWith('/upload/confirm', {
+        keys: [presignResponse.key],
+      });
+    });
+
+    it('should throw error when upload verification fails', async () => {
+      mockedApi.post.mockResolvedValueOnce({ data: presignResponse });
+      // Mock confirm returning empty confirmed array
+      mockedApi.post.mockResolvedValueOnce({
+        data: { confirmed: [], total: 1 },
+      });
+
+      await expect(uploadFile('item', mockFile)).rejects.toThrow('Upload verification failed');
+    });
+
+    it('should pass onProgress to uploadToS3', async () => {
+      const onProgress = jest.fn();
+
+      mockedApi.post.mockResolvedValueOnce({ data: presignResponse });
+      mockedApi.post.mockResolvedValueOnce({
+        data: { confirmed: [presignResponse.key], total: 1 },
+      });
+
+      await uploadFile('item', mockFile, { onProgress });
+
+      // onProgress should have been called during XHR upload
+      expect(onProgress).toHaveBeenCalled();
+    });
+
+    it('should work with different upload types', async () => {
+      const messagePresignResponse = {
+        ...presignResponse,
+        key: 'messages/uuid.jpg',
+        publicUrl: null, // Messages are private
+      };
+
+      mockedApi.post.mockResolvedValueOnce({ data: messagePresignResponse });
+      mockedApi.post.mockResolvedValueOnce({
+        data: { confirmed: [messagePresignResponse.key], total: 1 },
+      });
+
+      const result = await uploadFile('message', mockFile);
+
+      expect(result.key).toBe('messages/uuid.jpg');
+      expect(mockedApi.post).toHaveBeenCalledWith('/upload/presign', expect.objectContaining({
+        uploadType: 'message',
+      }));
    });
  });

  describe('uploadFiles', () => {
-    it('should return empty array for empty files array', async () => {
-      const result = await uploadFiles('item', []);
-      expect(result).toEqual([]);
-      expect(mockedApi.post).not.toHaveBeenCalled();
-    });
-
-    it('should call getPresignedUrls with correct parameters', async () => {
-      const files = [
+    const mockFiles = [
      new File(['test1'], 'photo1.jpg', { type: 'image/jpeg' }),
      new File(['test2'], 'photo2.png', { type: 'image/png' }),
    ];

    const presignResponses: PresignedUrlResponse[] = [
      {
-          uploadUrl: 'https://presigned1.s3.amazonaws.com',
+        uploadUrl: 'https://presigned1.s3.amazonaws.com/items/uuid1.jpg',
        key: 'items/uuid1.jpg',
        publicUrl: 'https://bucket.s3.amazonaws.com/items/uuid1.jpg',
        expiresAt: new Date().toISOString(),
      },
      {
-          uploadUrl: 'https://presigned2.s3.amazonaws.com',
+        uploadUrl: 'https://presigned2.s3.amazonaws.com/items/uuid2.png',
        key: 'items/uuid2.png',
        publicUrl: 'https://bucket.s3.amazonaws.com/items/uuid2.png',
        expiresAt: new Date().toISOString(),
      },
    ];

+    it('should return empty array for empty files array', async () => {
+      const result = await uploadFiles('item', []);
+      expect(result).toEqual([]);
+      expect(mockedApi.post).not.toHaveBeenCalled();
+    });
+
+    it('should complete full batch upload flow successfully', async () => {
      mockedApi.post.mockResolvedValueOnce({ data: { uploads: presignResponses } });
+      mockedApi.post.mockResolvedValueOnce({
+        data: {
+          confirmed: presignResponses.map((p) => p.key),
+          total: 2,
+        },
+      });

-      await getPresignedUrls('item', files);
+      const result = await uploadFiles('item', mockFiles);

+      expect(result).toHaveLength(2);
+      expect(result[0]).toEqual({
+        key: 'items/uuid1.jpg',
+        publicUrl: 'https://bucket.s3.amazonaws.com/items/uuid1.jpg',
+      });
+      expect(result[1]).toEqual({
+        key: 'items/uuid2.png',
+        publicUrl: 'https://bucket.s3.amazonaws.com/items/uuid2.png',
+      });
+
+      // Verify batch presign was called
      expect(mockedApi.post).toHaveBeenCalledWith('/upload/presign-batch', {
        uploadType: 'item',
        files: [
-          { contentType: 'image/jpeg', fileName: 'photo1.jpg', fileSize: files[0].size },
-          { contentType: 'image/png', fileName: 'photo2.png', fileSize: files[1].size },
+          { contentType: 'image/jpeg', fileName: 'photo1.jpg', fileSize: mockFiles[0].size },
+          { contentType: 'image/png', fileName: 'photo2.png', fileSize: mockFiles[1].size },
        ],
      });
+
+      // Verify confirm was called with all keys
+      expect(mockedApi.post).toHaveBeenCalledWith('/upload/confirm', {
+        keys: ['items/uuid1.jpg', 'items/uuid2.png'],
+      });
+    });
+
+    it('should filter out unconfirmed uploads', async () => {
+      mockedApi.post.mockResolvedValueOnce({ data: { uploads: presignResponses } });
+      // Only first file confirmed
+      mockedApi.post.mockResolvedValueOnce({
+        data: {
+          confirmed: ['items/uuid1.jpg'],
+          total: 2,
+        },
+      });
+
+      const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+      const result = await uploadFiles('item', mockFiles);
+
+      // Only confirmed uploads should be returned
+      expect(result).toHaveLength(1);
+      expect(result[0].key).toBe('items/uuid1.jpg');
+
+      // Should log warning about failed verification
+      expect(consoleSpy).toHaveBeenCalledWith('1 uploads failed verification');
+
+      consoleSpy.mockRestore();
+    });
+
+    it('should handle all uploads failing verification', async () => {
+      mockedApi.post.mockResolvedValueOnce({ data: { uploads: presignResponses } });
+      mockedApi.post.mockResolvedValueOnce({
+        data: {
+          confirmed: [],
+          total: 2,
+        },
+      });
+
+      const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+      const result = await uploadFiles('item', mockFiles);
+
+      expect(result).toHaveLength(0);
+      expect(consoleSpy).toHaveBeenCalledWith('2 uploads failed verification');
+
+      consoleSpy.mockRestore();
+    });
+
+    it('should upload all files in parallel', async () => {
+      mockedApi.post.mockResolvedValueOnce({ data: { uploads: presignResponses } });
+      mockedApi.post.mockResolvedValueOnce({
+        data: {
+          confirmed: presignResponses.map((p) => p.key),
+          total: 2,
+        },
+      });
+
+      await uploadFiles('item', mockFiles);
+
+      // Should have created 2 XHR instances for parallel uploads
+      expect(MockXMLHttpRequest.instances.length).toBe(2);
+    });
+
+    it('should work with different upload types', async () => {
+      const forumResponses = presignResponses.map((r) => ({
+        ...r,
+        key: r.key.replace('items/', 'forum/'),
+        publicUrl: r.publicUrl.replace('items/', 'forum/'),
+      }));
+
+      mockedApi.post.mockResolvedValueOnce({ data: { uploads: forumResponses } });
+      mockedApi.post.mockResolvedValueOnce({
+        data: {
+          confirmed: forumResponses.map((p) => p.key),
+          total: 2,
+        },
+      });
+
+      const result = await uploadFiles('forum', mockFiles);
+
+      expect(result[0].key).toBe('forum/uuid1.jpg');
+      expect(mockedApi.post).toHaveBeenCalledWith('/upload/presign-batch', expect.objectContaining({
+        uploadType: 'forum',
+      }));
    });
  });