refactor: segregate user validation a single function, taking out the responsibility from the services

Author: tomast1337

server/src/GetRequestUser.ts

@@ -1,4 +1,9 @@
-import { ExecutionContext, createParamDecorator } from '@nestjs/common';
+import {
+  ExecutionContext,
+  HttpException,
+  HttpStatus,
+  createParamDecorator,
+} from '@nestjs/common';
 
 import { UserDocument } from './user/entity/user.entity';
 
@@ -10,3 +15,18 @@ export const GetRequestToken = createParamDecorator(
     return user;
   },
 );
+
+export const validateUser = (user: UserDocument | null) => {
+  if (!user) {
+    throw new HttpException(
+      {
+        error: {
+          user: 'User not found',
+        },
+      },
+      HttpStatus.UNAUTHORIZED,
+    );
+  }
+
+  return user;
+};

server/src/song/my-songs/my-songs.controller.ts

@@ -4,7 +4,7 @@ import { ApiBearerAuth, ApiOperation } from '@nestjs/swagger';
 import { PageQueryDTO } from '@shared/validation/common/dto/PageQuery.dto';
 import { SongPageDto } from '@shared/validation/song/dto/SongPageDto';
 
-import { GetRequestToken } from '@server/GetRequestUser';
+import { GetRequestToken, validateUser } from '@server/GetRequestUser';
 import { UserDocument } from '@server/user/entity/user.entity';
 
 import { SongService } from '../song.service';
@@ -24,6 +24,7 @@ export class MySongsController {
     @Query() query: PageQueryDTO,
     @GetRequestToken() user: UserDocument | null,
   ): Promise<SongPageDto> {
+    user = validateUser(user);
     return await this.songService.getMySongsPage({
       query,
       user,

server/src/song/song.controller.ts

@@ -36,7 +36,7 @@ import { UploadSongResponseDto } from '@shared/validation/song/dto/UploadSongRes
 import type { Response } from 'express';
 
 import { FileService } from '@server/file/file.service';
-import { GetRequestToken } from '@server/GetRequestUser';
+import { GetRequestToken, validateUser } from '@server/GetRequestUser';
 import { UserDocument } from '@server/user/entity/user.entity';
 
 import { SongService } from './song.service';
@@ -80,6 +80,7 @@ export class SongController {
     @Param('id') id: string,
     @GetRequestToken() user: UserDocument | null,
   ): Promise<SongViewDto> {
+    user = validateUser(user);
     return await this.songService.getSong(id, user);
   }
 
@@ -91,6 +92,7 @@ export class SongController {
     @Param('id') id: string,
     @GetRequestToken() user: UserDocument | null,
   ): Promise<UploadSongDto> {
+    user = validateUser(user);
     return await this.songService.getSongEdit(id, user);
   }
 
@@ -107,9 +109,9 @@ export class SongController {
     @Req() req: RawBodyRequest<Request>,
     @GetRequestToken() user: UserDocument | null,
   ): Promise<UploadSongResponseDto> {
+    user = validateUser(user);
     //TODO: Fix this weird type casting and raw body access
     const body = req.body as unknown as UploadSongDto;
-
     return await this.songService.patchSong(id, body, user);
   }
 
@@ -128,6 +130,7 @@ export class SongController {
       'Access-Control-Expose-Headers': 'Content-Disposition',
     });
 
+    user = validateUser(user);
     const url = await this.songService.getSongDownloadUrl(id, user, src, false);
     res.redirect(HttpStatus.FOUND, url);
   }
@@ -139,6 +142,8 @@ export class SongController {
     @GetRequestToken() user: UserDocument | null,
     @Headers('src') src: string,
   ): Promise<string> {
+    user = validateUser(user);
+
     if (src != 'downloadButton') {
       throw new UnauthorizedException('Invalid source');
     }
@@ -161,6 +166,7 @@ export class SongController {
     @Param('id') id: string,
     @GetRequestToken() user: UserDocument | null,
   ): Promise<void> {
+    user = validateUser(user);
     await this.songService.deleteSong(id, user);
   }
 
@@ -181,6 +187,7 @@ export class SongController {
     @Body() body: UploadSongDto,
     @GetRequestToken() user: UserDocument | null,
   ): Promise<UploadSongResponseDto> {
+    user = validateUser(user);
     return await this.songService.uploadSong({ body, file, user });
   }
 }

server/src/song/song.service.ts

@@ -40,32 +40,15 @@ export class SongService {
     private songUploadService: SongUploadService,
   ) {}
 
-  private isUserValid(user: UserDocument | null) {
-    if (!user) {
-      throw new HttpException(
-        {
-          error: {
-            user: 'User not found',
-          },
-        },
-        HttpStatus.UNAUTHORIZED,
-      );
-    }
-  }
-
   public async uploadSong({
     file,
     user,
     body,
   }: {
     body: UploadSongDto;
     file: Express.Multer.File;
-    user: UserDocument | null;
+    user: UserDocument;
   }): Promise<UploadSongResponseDto> {
-    // Is user valid?
-    this.isUserValid(user);
-    user = user as UserDocument;
-
     const song = await this.songUploadService.processUploadedSong({
       file,
       user,
@@ -86,7 +69,7 @@ export class SongService {
 
   public async deleteSong(
     publicId: string,
-    user: UserDocument | null,
+    user: UserDocument,
   ): Promise<UploadSongResponseDto> {
     const foundSong = await this.songModel
       .findOne({ publicId: publicId })
@@ -115,7 +98,7 @@ export class SongService {
   public async patchSong(
     publicId: string,
     body: UploadSongDto,
-    user: UserDocument | null,
+    user: UserDocument,
   ): Promise<UploadSongResponseDto> {
     const foundSong = (await this.songModel
       .findOne({
@@ -258,7 +241,7 @@ export class SongService {
 
   public async getSong(
     publicId: string,
-    user: UserDocument | null,
+    user: UserDocument,
   ): Promise<SongViewDto> {
     const foundSong = await this.songModel
       .findOne({ publicId: publicId })
@@ -289,7 +272,7 @@ export class SongService {
   // TODO: service should not handle HTTP -> https://www.reddit.com/r/node/comments/uoicw1/should_i_return_status_code_from_service_layer/
   public async getSongDownloadUrl(
     publicId: string,
-    user: UserDocument | null,
+    user: UserDocument,
     src?: string,
     packed: boolean = false,
   ): Promise<string> {
@@ -339,16 +322,13 @@ export class SongService {
     }
   }
 
-  public async getMySongsPage(arg0: {
+  public async getMySongsPage({
+    query,
+    user,
+  }: {
     query: PageQueryDTO;
-    user: UserDocument | null;
+    user: UserDocument;
   }): Promise<SongPageDto> {
-    const { query, user } = arg0;
-
-    if (!user) {
-      throw new HttpException('User not found', HttpStatus.UNAUTHORIZED);
-    }
-
     const page = parseInt(query.page?.toString() ?? '1');
     const limit = parseInt(query.limit?.toString() ?? '10');
     const order = query.order ? query.order : false;
@@ -383,7 +363,7 @@ export class SongService {
 
   public async getSongEdit(
     publicId: string,
-    user: UserDocument | null,
+    user: UserDocument,
   ): Promise<UploadSongDto> {
     const foundSong = await this.songModel
       .findOne({ publicId: publicId })

server/src/user/user.controller.ts

@@ -3,7 +3,7 @@ import { ApiBearerAuth, ApiOperation, ApiTags } from '@nestjs/swagger';
 import { PageQueryDTO } from '@shared/validation/common/dto/PageQuery.dto';
 import { GetUser } from '@shared/validation/user/dto/GetUser.dto';
 
-import { GetRequestToken } from '@server/GetRequestUser';
+import { GetRequestToken, validateUser } from '@server/GetRequestUser';
 
 import { UserDocument } from './entity/user.entity';
 import { UserService } from './user.service';
@@ -34,6 +34,7 @@ export class UserController {
   @ApiBearerAuth()
   @ApiOperation({ summary: 'Get the token owner data' })
   async getMe(@GetRequestToken() user: UserDocument | null) {
+    user = validateUser(user);
     return await this.userService.getSelfUserData(user);
   }
 }

server/src/user/user.service.ts

@@ -79,7 +79,7 @@ export class UserService {
     return hydratedUser;
   }
 
-  public async getSelfUserData(user: UserDocument | null) {
+  public async getSelfUserData(user: UserDocument) {
     if (!user)
       throw new HttpException('not logged in', HttpStatus.UNAUTHORIZED);
     const usedData = await this.findByID(user._id.toString());