refactor(auth): Refactor cookie auth implementation and update attachments controller

LucienShui · LucienShui · commit 3ed6da356098 · 2025-08-08T18:08:57.000+08:00
diff --git a/src/attachments/attachments.controller.ts b/src/attachments/attachments.controller.ts
@@ -16,9 +16,8 @@ import { FilesInterceptor } from '@nestjs/platform-express';
 import { AttachmentsService } from 'omniboxd/attachments/attachments.service';
 import { Request, Response } from 'express';
 import { UserId } from 'omniboxd/decorators/user-id.decorator';
-import { Public } from 'omniboxd/auth/decorators/public.auth.decorator';
-import { Cookies } from 'omniboxd/decorators/cookie.decorators';
 import { AuthService } from 'omniboxd/auth/auth.service';
+import { CookieAuth } from 'omniboxd/auth';
 
 @Controller('api/v1/attachments')
 export class AttachmentsController {
@@ -62,65 +61,47 @@ export class AttachmentsController {
     );
   }
 
-  @Public()
+  setRedirect(req: Request, res: Response) {
+    res
+      .setHeader('Cache-Control', 'no-cache, no-store, must-revalidate')
+      .status(HttpStatus.FOUND)
+      .redirect(`/user/login?redirect=${encodeURIComponent(req.url)}`);
+  }
+
+  @CookieAuth({ onAuthFail: 'continue' })
   @Get('images/:attachmentId')
   async displayImage(
+    @UserId({ optional: true }) userId: string | undefined,
     @Req() req: Request,
     @Res() res: Response,
-    @Cookies('token') token: string,
     @Param('attachmentId') attachmentId: string,
   ) {
-    let userId = '';
-
-    if (token) {
-      const payload = this.authService.jwtVerify(token);
-      if (payload && payload.sub) {
-        userId = payload.sub;
-      }
-    }
-
-    this.logger.debug({ userId, token, cookies: req.cookies });
     if (userId) {
-      return await this.attachmentsService.displayImage(
+      return await this.attachmentsService.displayMedia(
         attachmentId,
         userId,
         res,
       );
     }
-    res
-      .setHeader('Cache-Control', 'no-cache, no-store, must-revalidate')
-      .status(HttpStatus.FOUND)
-      .redirect(`/user/login?redirect=${encodeURIComponent(req.url)}`);
+    this.setRedirect(req, res);
   }
 
-  @Public()
+  @CookieAuth({ onAuthFail: 'continue' })
   @Get('media/:attachmentId')
   async displayMedia(
+    @UserId({ optional: true }) userId: string | undefined,
     @Req() req: Request,
     @Res() res: Response,
-    @Cookies('token') token: string,
     @Param('attachmentId') attachmentId: string,
   ) {
-    let userId = '';
-
-    if (token) {
-      const payload = this.authService.jwtVerify(token);
-      if (payload && payload.sub) {
-        userId = payload.sub;
-      }
-    }
-
     if (userId) {
       return await this.attachmentsService.displayMedia(
         attachmentId,
         userId,
         res,
       );
     }
-    res
-      .setHeader('Cache-Control', 'no-cache, no-store, must-revalidate')
-      .status(HttpStatus.FOUND)
-      .redirect(`/user/login?redirect=${encodeURIComponent(req.url)}`);
+    this.setRedirect(req, res);
   }
 
   @Delete(':attachmentId')
diff --git a/src/attachments/attachments.e2e-spec.ts b/src/attachments/attachments.e2e-spec.ts
@@ -339,7 +339,7 @@ describe('AttachmentsController (e2e)', () => {
   describe('GET /api/v1/attachments/media/:attachmentId (Public)', () => {
     let mediaAttachmentId: string;
     const mediaContent = Buffer.from('fake-media-content');
-    const mediaFilename = 'test-media.mp4';
+    const mediaFilename = 'test-media.mp3';
 
     beforeAll(async () => {
       // Upload a media file to test display
diff --git a/src/attachments/attachments.service.ts b/src/attachments/attachments.service.ts
@@ -162,6 +162,15 @@ export class AttachmentsService {
     return { id: attachmentId, success: true };
   }
 
+  isMedia(mimetype: string): boolean {
+    for (const type of ['image/', 'audio/']) {
+      if (mimetype.startsWith(type)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   async displayMedia(
     attachmentId: string,
     userId: string,
@@ -179,33 +188,11 @@ export class AttachmentsService {
       ResourcePermission.CAN_VIEW,
     );
     await this.checkAttachment(namespaceId, resourceId, attachmentId);
+    if (!this.isMedia(objectResponse.mimetype)) {
+      throw new BadRequestException(attachmentId);
+    }
     return objectStreamResponse(objectResponse, httpResponse, {
       forceDownload: false,
     });
   }
-
-  async displayImage(
-    attachmentId: string,
-    userId: string,
-    httpResponse: Response,
-  ) {
-    const objectResponse = await this.minioService.get(
-      this.minioPath(attachmentId),
-    );
-    const { namespaceId, resourceId } = objectResponse.metadata;
-
-    await this.checkPermission(
-      namespaceId,
-      resourceId,
-      userId,
-      ResourcePermission.CAN_VIEW,
-    );
-    await this.checkAttachment(namespaceId, resourceId, attachmentId);
-    if (objectResponse.mimetype.startsWith('image/')) {
-      return objectStreamResponse(objectResponse, httpResponse, {
-        forceDownload: false,
-      });
-    }
-    throw new BadRequestException(attachmentId);
-  }
 }
diff --git a/src/auth/cookie/cookie-auth.guard.spec.ts b/src/auth/cookie/cookie-auth.guard.spec.ts
@@ -62,7 +62,7 @@ describe('CookieAuthGuard', () => {
   it('should return true for non-cookie auth routes', () => {
     reflector.getAllAndOverride
       .mockReturnValueOnce(false) // isPublic = false
-      .mockReturnValueOnce(false); // isCookieAuth = false
+      .mockReturnValueOnce(null); // cookieAuthOptions = null
     const context = createMockExecutionContext();
 
     const result = guard.canActivate(context);
@@ -73,7 +73,7 @@ describe('CookieAuthGuard', () => {
   it('should throw UnauthorizedException when no token cookie is provided', () => {
     reflector.getAllAndOverride
       .mockReturnValueOnce(false) // isPublic = false
-      .mockReturnValueOnce(true); // isCookieAuth = true
+      .mockReturnValueOnce({ enabled: true }); // cookieAuthOptions with default onAuthFail = 'reject'
     const context = createMockExecutionContext();
 
     expect(() => guard.canActivate(context)).toThrow(
@@ -84,7 +84,7 @@ describe('CookieAuthGuard', () => {
   it('should throw UnauthorizedException when token is invalid', () => {
     reflector.getAllAndOverride
       .mockReturnValueOnce(false) // isPublic = false
-      .mockReturnValueOnce(true); // isCookieAuth = true
+      .mockReturnValueOnce({ enabled: true }); // cookieAuthOptions with default onAuthFail = 'reject'
     const context = createMockExecutionContext({ token: 'invalid-token' });
     authService.jwtVerify.mockImplementation(() => {
       throw new Error('Invalid token');
@@ -98,7 +98,7 @@ describe('CookieAuthGuard', () => {
   it('should throw UnauthorizedException when token payload is invalid', () => {
     reflector.getAllAndOverride
       .mockReturnValueOnce(false) // isPublic = false
-      .mockReturnValueOnce(true); // isCookieAuth = true
+      .mockReturnValueOnce({ enabled: true }); // cookieAuthOptions with default onAuthFail = 'reject'
     const context = createMockExecutionContext({ token: 'valid-token' });
     authService.jwtVerify.mockReturnValue({}); // No sub field
 
@@ -110,7 +110,7 @@ describe('CookieAuthGuard', () => {
   it('should successfully authenticate with valid token and set cookie auth data', () => {
     reflector.getAllAndOverride
       .mockReturnValueOnce(false) // isPublic = false
-      .mockReturnValueOnce(true); // isCookieAuth = true
+      .mockReturnValueOnce({ enabled: true }); // cookieAuthOptions with default onAuthFail = 'reject'
     const context = createMockExecutionContext({ token: 'valid-token' });
     authService.jwtVerify.mockReturnValue({
       sub: 'user-123',
@@ -133,7 +133,7 @@ describe('CookieAuthGuard', () => {
   it('should handle token with missing optional fields', () => {
     reflector.getAllAndOverride
       .mockReturnValueOnce(false) // isPublic = false
-      .mockReturnValueOnce(true); // isCookieAuth = true
+      .mockReturnValueOnce({ enabled: true }); // cookieAuthOptions with default onAuthFail = 'reject'
     const context = createMockExecutionContext({ token: 'valid-token' });
     authService.jwtVerify.mockReturnValue({
       sub: 'user-123',
@@ -150,4 +150,41 @@ describe('CookieAuthGuard', () => {
       username: undefined,
     });
   });
+
+  it('should continue without authentication when onAuthFail is continue and no token', () => {
+    reflector.getAllAndOverride
+      .mockReturnValueOnce(false) // isPublic = false
+      .mockReturnValueOnce({ enabled: true, onAuthFail: 'continue' }); // cookieAuthOptions with onAuthFail = 'continue'
+    const context = createMockExecutionContext();
+
+    const result = guard.canActivate(context);
+
+    expect(result).toBe(true);
+  });
+
+  it('should continue without authentication when onAuthFail is continue and token is invalid', () => {
+    reflector.getAllAndOverride
+      .mockReturnValueOnce(false) // isPublic = false
+      .mockReturnValueOnce({ enabled: true, onAuthFail: 'continue' }); // cookieAuthOptions with onAuthFail = 'continue'
+    const context = createMockExecutionContext({ token: 'invalid-token' });
+    authService.jwtVerify.mockImplementation(() => {
+      throw new Error('Invalid token');
+    });
+
+    const result = guard.canActivate(context);
+
+    expect(result).toBe(true);
+  });
+
+  it('should continue without authentication when onAuthFail is continue and token payload is invalid', () => {
+    reflector.getAllAndOverride
+      .mockReturnValueOnce(false) // isPublic = false
+      .mockReturnValueOnce({ enabled: true, onAuthFail: 'continue' }); // cookieAuthOptions with onAuthFail = 'continue'
+    const context = createMockExecutionContext({ token: 'valid-token' });
+    authService.jwtVerify.mockReturnValue({}); // No sub field
+
+    const result = guard.canActivate(context);
+
+    expect(result).toBe(true);
+  });
 });
diff --git a/src/auth/cookie/cookie-auth.guard.ts b/src/auth/cookie/cookie-auth.guard.ts
@@ -1,12 +1,13 @@
 import {
-  Injectable,
   CanActivate,
   ExecutionContext,
+  Injectable,
   UnauthorizedException,
 } from '@nestjs/common';
 import { Reflector } from '@nestjs/core';
 import { AuthService } from 'omniboxd/auth/auth.service';
 import { IS_COOKIE_AUTH, IS_PUBLIC_KEY } from 'omniboxd/auth/decorators';
+import { CookieAuthOptions } from 'omniboxd/auth/cookie/cookie.auth.decorator';
 
 @Injectable()
 export class CookieAuthGuard implements CanActivate {
@@ -25,19 +26,22 @@ export class CookieAuthGuard implements CanActivate {
       return true;
     }
 
-    const isCookieAuth = this.reflector.getAllAndOverride<boolean>(
-      IS_COOKIE_AUTH,
-      [context.getHandler(), context.getClass()],
-    );
+    const cookieAuthOptions = this.reflector.getAllAndOverride<
+      CookieAuthOptions & { enabled: boolean }
+    >(IS_COOKIE_AUTH, [context.getHandler(), context.getClass()]);
 
-    if (!isCookieAuth) {
+    if (!cookieAuthOptions?.enabled) {
       return true; // Let other guards handle non-cookie routes
     }
 
     const request = context.switchToHttp().getRequest();
     const token = request.cookies?.token;
+    const onAuthFail = cookieAuthOptions.onAuthFail || 'reject';
 
     if (!token) {
+      if (onAuthFail === 'continue') {
+        return true; // Continue without authentication
+      }
       throw new UnauthorizedException(
         'Authentication token cookie is required',
       );
@@ -47,6 +51,9 @@ export class CookieAuthGuard implements CanActivate {
       const payload = this.authService.jwtVerify(token);
 
       if (!payload.sub) {
+        if (onAuthFail === 'continue') {
+          return true; // Continue without authentication
+        }
         throw new UnauthorizedException('Invalid token payload');
       }
 
@@ -59,6 +66,10 @@ export class CookieAuthGuard implements CanActivate {
 
       return true;
     } catch (error) {
+      if (onAuthFail === 'continue') {
+        return true; // Continue without authentication
+      }
+
       // Re-throw UnauthorizedException with original message
       if (error instanceof UnauthorizedException) {
         throw error;
diff --git a/src/auth/cookie/cookie.auth.decorator.ts b/src/auth/cookie/cookie.auth.decorator.ts
@@ -1,4 +1,9 @@
 import { SetMetadata } from '@nestjs/common';
 
+export interface CookieAuthOptions {
+  onAuthFail?: 'reject' | 'continue';
+}
+
 export const IS_COOKIE_AUTH = 'isCookieAuth';
-export const CookieAuth = () => SetMetadata(IS_COOKIE_AUTH, true);
+export const CookieAuth = (options: CookieAuthOptions = {}) =>
+  SetMetadata(IS_COOKIE_AUTH, { enabled: true, ...options });
diff --git a/src/decorators/user-id.decorator.spec.ts b/src/decorators/user-id.decorator.spec.ts
diff --git a/src/decorators/user-id.decorator.ts b/src/decorators/user-id.decorator.ts
