From 34228b8f0599ba15b9541f4f2132de6f7b3e0491 Mon Sep 17 00:00:00 2001 From: tomast1337 Date: Fri, 15 Nov 2024 14:09:04 -0300 Subject: [PATCH 1/2] refactor: segregate user validation a single function, taking out the responsibility from the services --- server/src/GetRequestUser.ts | 22 +++++++++- .../src/song/my-songs/my-songs.controller.ts | 3 +- server/src/song/song.controller.ts | 11 ++++- server/src/song/song.service.ts | 42 +++++-------------- server/src/user/user.controller.ts | 3 +- server/src/user/user.service.ts | 2 +- 6 files changed, 46 insertions(+), 37 deletions(-) diff --git a/server/src/GetRequestUser.ts b/server/src/GetRequestUser.ts index 8f0c34c8..9823711f 100644 --- a/server/src/GetRequestUser.ts +++ b/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; +}; diff --git a/server/src/song/my-songs/my-songs.controller.ts b/server/src/song/my-songs/my-songs.controller.ts index 5b0783a8..666d85e3 100644 --- a/server/src/song/my-songs/my-songs.controller.ts +++ b/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 { + user = validateUser(user); return await this.songService.getMySongsPage({ query, user, diff --git a/server/src/song/song.controller.ts b/server/src/song/song.controller.ts index 09ae64a8..a55cac17 100644 --- a/server/src/song/song.controller.ts +++ b/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 { + 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 { + user = validateUser(user); return await this.songService.getSongEdit(id, user); } @@ -107,9 +109,9 @@ export class SongController { @Req() req: RawBodyRequest, @GetRequestToken() user: UserDocument | null, ): Promise { + 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 { + 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 { + user = validateUser(user); await this.songService.deleteSong(id, user); } @@ -181,6 +187,7 @@ export class SongController { @Body() body: UploadSongDto, @GetRequestToken() user: UserDocument | null, ): Promise { + user = validateUser(user); return await this.songService.uploadSong({ body, file, user }); } } diff --git a/server/src/song/song.service.ts b/server/src/song/song.service.ts index 8219ed84..bb8a2e45 100644 --- a/server/src/song/song.service.ts +++ b/server/src/song/song.service.ts @@ -40,19 +40,6 @@ 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, @@ -60,12 +47,8 @@ export class SongService { }: { body: UploadSongDto; file: Express.Multer.File; - user: UserDocument | null; + user: UserDocument; }): Promise { - // 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 { 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 { const foundSong = (await this.songModel .findOne({ @@ -258,7 +241,7 @@ export class SongService { public async getSong( publicId: string, - user: UserDocument | null, + user: UserDocument, ): Promise { 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 { @@ -339,16 +322,13 @@ export class SongService { } } - public async getMySongsPage(arg0: { + public async getMySongsPage({ + query, + user, + }: { query: PageQueryDTO; - user: UserDocument | null; + user: UserDocument; }): Promise { - 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 { const foundSong = await this.songModel .findOne({ publicId: publicId }) diff --git a/server/src/user/user.controller.ts b/server/src/user/user.controller.ts index 30fc9a97..2805ce42 100644 --- a/server/src/user/user.controller.ts +++ b/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); } } diff --git a/server/src/user/user.service.ts b/server/src/user/user.service.ts index e8912ff1..d9e4e7ed 100644 --- a/server/src/user/user.service.ts +++ b/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()); From 4f19a02c10a8e1bb0e9a79aa7666dd7d617d1740 Mon Sep 17 00:00:00 2001 From: tomast1337 Date: Fri, 15 Nov 2024 14:37:23 -0300 Subject: [PATCH 2/2] fix: handle user validation as nullable in song service methods --- server/src/song/song.controller.ts | 4 ---- server/src/song/song.service.ts | 4 ++-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/server/src/song/song.controller.ts b/server/src/song/song.controller.ts index a55cac17..d310c2f9 100644 --- a/server/src/song/song.controller.ts +++ b/server/src/song/song.controller.ts @@ -80,7 +80,6 @@ export class SongController { @Param('id') id: string, @GetRequestToken() user: UserDocument | null, ): Promise { - user = validateUser(user); return await this.songService.getSong(id, user); } @@ -130,7 +129,6 @@ 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); } @@ -142,8 +140,6 @@ export class SongController { @GetRequestToken() user: UserDocument | null, @Headers('src') src: string, ): Promise { - user = validateUser(user); - if (src != 'downloadButton') { throw new UnauthorizedException('Invalid source'); } diff --git a/server/src/song/song.service.ts b/server/src/song/song.service.ts index bb8a2e45..e0ca3dfc 100644 --- a/server/src/song/song.service.ts +++ b/server/src/song/song.service.ts @@ -241,7 +241,7 @@ export class SongService { public async getSong( publicId: string, - user: UserDocument, + user: UserDocument | null, ): Promise { const foundSong = await this.songModel .findOne({ publicId: publicId }) @@ -272,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, + user: UserDocument | null, src?: string, packed: boolean = false, ): Promise {