refactor(attachments): unify get api

Author: ycdzj

src/attachments/attachments.controller.ts

@@ -38,13 +38,19 @@ export class AttachmentsController {
   }
 
   @Get(':attachmentId')
+  @CookieAuth({ onAuthFail: 'continue' })
   async downloadAttachment(
-    @UserId() userId: string,
+    @Req() req: Request,
+    @UserId({ optional: true }) userId: string | undefined,
     @Param('namespaceId') namespaceId: string,
     @Param('resourceId') resourceId: string,
     @Param('attachmentId') attachmentId: string,
     @Res() res: Response,
   ) {
+    if (!userId) {
+      this.setRedirect(req, res);
+      return;
+    }
     return await this.attachmentsService.downloadAttachment(
       namespaceId,
       resourceId,
@@ -61,50 +67,6 @@ export class AttachmentsController {
       .redirect(`/user/login?redirect=${encodeURIComponent(req.url)}`);
   }
 
-  @CookieAuth({ onAuthFail: 'continue' })
-  @Get(':attachmentId/images')
-  async displayImage(
-    @Req() req: Request,
-    @UserId({ optional: true }) userId: string | undefined,
-    @Param('namespaceId') namespaceId: string,
-    @Param('resourceId') resourceId: string,
-    @Param('attachmentId') attachmentId: string,
-    @Res() res: Response,
-  ) {
-    if (userId) {
-      return await this.attachmentsService.displayMedia(
-        namespaceId,
-        resourceId,
-        attachmentId,
-        userId,
-        res,
-      );
-    }
-    this.setRedirect(req, res);
-  }
-
-  @CookieAuth({ onAuthFail: 'continue' })
-  @Get(':attachmentId/media')
-  async displayMedia(
-    @Req() req: Request,
-    @UserId({ optional: true }) userId: string | undefined,
-    @Param('namespaceId') namespaceId: string,
-    @Param('resourceId') resourceId: string,
-    @Param('attachmentId') attachmentId: string,
-    @Res() res: Response,
-  ) {
-    if (userId) {
-      return await this.attachmentsService.displayMedia(
-        namespaceId,
-        resourceId,
-        attachmentId,
-        userId,
-        res,
-      );
-    }
-    this.setRedirect(req, res);
-  }
-
   @Delete(':attachmentId')
   async deleteAttachment(
     @UserId() userId: string,

src/attachments/attachments.e2e-spec.ts

@@ -146,13 +146,15 @@ describe('AttachmentsController (e2e)', () => {
       expect(response.headers['content-disposition']).toContain(testFilename);
     });
 
-    it('should reject download without authentication', async () => {
-      await client
+    it('should redirect to login when no authentication provided', async () => {
+      const response = await client
         .request()
         .get(
           `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${attachmentId}`,
         )
-        .expect(401);
+        .expect(302);
+
+      expect(response.headers.location).toContain('/user/login');
     });
 
     it('should reject download with invalid attachment ID', async () => {
@@ -257,7 +259,7 @@ describe('AttachmentsController (e2e)', () => {
     });
   });
 
-  describe('GET /api/v1/namespaces/:namespaceId/resources/:resourceId/attachments/:attachmentId/images (Public)', () => {
+  describe('GET /api/v1/namespaces/:namespaceId/resources/:resourceId/attachments/:attachmentId (Public)', () => {
     let imageAttachmentId: string;
     const imageContent = Buffer.from('fake-image-content');
     const imageFilename = 'test-image.png';
@@ -278,7 +280,7 @@ describe('AttachmentsController (e2e)', () => {
       const response = await client
         .request()
         .get(
-          `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${imageAttachmentId}/images`,
+          `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${imageAttachmentId}`,
         )
         .expect(HttpStatus.FOUND);
 
@@ -290,7 +292,7 @@ describe('AttachmentsController (e2e)', () => {
       const response = await client
         .request()
         .get(
-          `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${imageAttachmentId}/images`,
+          `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${imageAttachmentId}`,
         )
         .set('Cookie', `token=${client.user.token}`)
         .expect(200);
@@ -299,7 +301,7 @@ describe('AttachmentsController (e2e)', () => {
     });
   });
 
-  describe('GET /api/v1/namespaces/:namespaceId/resources/:resourceId/attachments/:attachmentId/media (Public)', () => {
+  describe('GET /api/v1/namespaces/:namespaceId/resources/:resourceId/attachments/:attachmentId (Public)', () => {
     let mediaAttachmentId: string;
     const mediaContent = Buffer.from('fake-media-content');
     const mediaFilename = 'test-media.mp3';
@@ -320,7 +322,7 @@ describe('AttachmentsController (e2e)', () => {
       const response = await client
         .request()
         .get(
-          `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${mediaAttachmentId}/media`,
+          `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${mediaAttachmentId}`,
         )
         .expect(HttpStatus.FOUND);
 
@@ -332,7 +334,7 @@ describe('AttachmentsController (e2e)', () => {
       const response = await client
         .request()
         .get(
-          `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${mediaAttachmentId}/media`,
+          `/api/v1/namespaces/${client.namespace.id}/resources/${testResourceId}/attachments/${mediaAttachmentId}`,
         )
         .set('Cookie', `token=${client.user.token}`)
         .expect(200);

src/attachments/attachments.service.ts

@@ -3,7 +3,6 @@ import {
   getOriginalFileName,
 } from 'omniboxd/utils/encode-filename';
 import {
-  BadRequestException,
   ForbiddenException,
   Injectable,
   Logger,
@@ -166,7 +165,13 @@ export class AttachmentsService {
     const objectResponse = await this.minioService.get(
       this.minioPath(attachmentId),
     );
-    return objectStreamResponse(objectResponse, httpResponse);
+
+    // Display media files inline, download other files as attachments
+    const forceDownload = !this.isMedia(objectResponse.mimetype);
+
+    return objectStreamResponse(objectResponse, httpResponse, {
+      forceDownload,
+    });
   }
 
   async deleteAttachment(
@@ -199,35 +204,6 @@ export class AttachmentsService {
     return false;
   }
 
-  async displayMedia(
-    namespaceId: string,
-    resourceId: string,
-    attachmentId: string,
-    userId: string,
-    httpResponse: Response,
-  ) {
-    await this.checkPermission(
-      namespaceId,
-      resourceId,
-      userId,
-      ResourcePermission.CAN_VIEW,
-    );
-
-    await this.getRelationOrFail(namespaceId, resourceId, attachmentId);
-
-    const objectResponse = await this.minioService.get(
-      this.minioPath(attachmentId),
-    );
-
-    if (!this.isMedia(objectResponse.mimetype)) {
-      throw new BadRequestException(attachmentId);
-    }
-
-    return objectStreamResponse(objectResponse, httpResponse, {
-      forceDownload: false,
-    });
-  }
-
   async copyAttachmentsToResource(
     namespaceId: string,
     sourceResourceId: string,

src/resources/resources.controller.ts

@@ -3,7 +3,18 @@ import { CreateResourceDto } from 'omniboxd/resources/dto/create-resource.dto';
 import { UpdateResourceDto } from 'omniboxd/resources/dto/update-resource.dto';
 import { PermissionsService } from 'omniboxd/permissions/permissions.service';
 import { ResourcePermission } from 'omniboxd/permissions/resource-permission.enum';
-import { Body, Controller, Delete, ForbiddenException, Get, Param, Patch, Post, Query, Req, } from '@nestjs/common';
+import {
+  Body,
+  Controller,
+  Delete,
+  ForbiddenException,
+  Get,
+  Param,
+  Patch,
+  Post,
+  Query,
+  Req,
+} from '@nestjs/common';
 import { UserId } from 'omniboxd/decorators/user-id.decorator';
 import { Request } from 'express';
 import { ResourceMetaDto } from 'omniboxd/resources/dto/resource.dto';

test/test-client.ts

@@ -92,31 +92,36 @@ export class TestClient {
   get(url: string) {
     return this.request()
       .get(url)
-      .set('Authorization', `Bearer ${this.user.token}`);
+      .set('Authorization', `Bearer ${this.user.token}`)
+      .set('Cookie', `token=${this.user.token}`);
   }
 
   post(url: string) {
     return this.request()
       .post(url)
-      .set('Authorization', `Bearer ${this.user.token}`);
+      .set('Authorization', `Bearer ${this.user.token}`)
+      .set('Cookie', `token=${this.user.token}`);
   }
 
   patch(url: string) {
     return this.request()
       .patch(url)
-      .set('Authorization', `Bearer ${this.user.token}`);
+      .set('Authorization', `Bearer ${this.user.token}`)
+      .set('Cookie', `token=${this.user.token}`);
   }
 
   put(url: string) {
     return this.request()
       .put(url)
-      .set('Authorization', `Bearer ${this.user.token}`);
+      .set('Authorization', `Bearer ${this.user.token}`)
+      .set('Cookie', `token=${this.user.token}`);
   }
 
   delete(url: string) {
     return this.request()
       .delete(url)
-      .set('Authorization', `Bearer ${this.user.token}`);
+      .set('Authorization', `Bearer ${this.user.token}`)
+      .set('Cookie', `token=${this.user.token}`);
   }
 
   public static async create(): Promise<TestClient> {