From 0e02bbed8568a3c11277265e3f082035deccf4e4 Mon Sep 17 00:00:00 2001 From: Matthias Rupp Date: Sun, 20 Nov 2022 18:14:40 +0100 Subject: [PATCH] Limit asset access to owner --- .../src/api-v1/asset/asset-repository.ts | 10 +++++++ .../src/api-v1/asset/asset.controller.ts | 22 +++++++++++---- .../immich/src/api-v1/asset/asset.service.ts | 27 +++++++++++++------ 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/server/apps/immich/src/api-v1/asset/asset-repository.ts b/server/apps/immich/src/api-v1/asset/asset-repository.ts index 6d0aa2e41d..2aa5fc6418 100644 --- a/server/apps/immich/src/api-v1/asset/asset-repository.ts +++ b/server/apps/immich/src/api-v1/asset/asset-repository.ts @@ -43,6 +43,7 @@ export interface IAssetRepository { userId: string, checkDuplicateAssetDto: CheckExistingAssetsDto, ): Promise; + countByIdAndUser(assetId: string, userId: string): Promise; } export const ASSET_REPOSITORY = 'ASSET_REPOSITORY'; @@ -343,4 +344,13 @@ export class AssetRepository implements IAssetRepository { }); return new CheckExistingAssetsResponseDto(existingAssets.map((a) => a.deviceAssetId)); } + + async countByIdAndUser(assetId: string, userId: string): Promise { + return await this.assetRepository.count({ + where: { + id: assetId, + userId + } + }); + } } diff --git a/server/apps/immich/src/api-v1/asset/asset.controller.ts b/server/apps/immich/src/api-v1/asset/asset.controller.ts index 6a3bf2d16f..03280d88df 100644 --- a/server/apps/immich/src/api-v1/asset/asset.controller.ts +++ b/server/apps/immich/src/api-v1/asset/asset.controller.ts @@ -14,6 +14,8 @@ import { Header, Put, UploadedFiles, + HttpException, + HttpStatus } from '@nestjs/common'; import { Authenticated } from '../../decorators/authenticated.decorator'; import { AssetService } from './asset.service'; @@ -86,10 +88,12 @@ export class AssetController { @Get('/download/:assetId') async downloadFile( + @GetAuthUser() authUser: AuthUserDto, @Response({ passthrough: true }) res: Res, @Query(new ValidationPipe({ transform: true })) query: ServeFileDto, @Param('assetId') assetId: string, ): Promise { + await this.assetService.checkAssetsAccess(authUser, [assetId]); return this.assetService.downloadFile(query, assetId, res); } @@ -110,21 +114,25 @@ export class AssetController { @Get('/file/:assetId') @Header('Cache-Control', 'max-age=300') async serveFile( + @GetAuthUser() authUser: AuthUserDto, @Headers() headers: Record, @Response({ passthrough: true }) res: Res, @Query(new ValidationPipe({ transform: true })) query: ServeFileDto, @Param('assetId') assetId: string, ): Promise { + await this.assetService.checkAssetsAccess(authUser, [assetId]); return this.assetService.serveFile(assetId, query, res, headers); } @Get('/thumbnail/:assetId') @Header('Cache-Control', 'max-age=300') async getAssetThumbnail( + @GetAuthUser() authUser: AuthUserDto, @Response({ passthrough: true }) res: Res, @Param('assetId') assetId: string, @Query(new ValidationPipe({ transform: true })) query: GetAssetThumbnailDto, ): Promise { + await this.assetService.checkAssetsAccess(authUser, [assetId]); return this.assetService.getAssetThumbnail(assetId, query, res); } @@ -195,7 +203,8 @@ export class AssetController { @GetAuthUser() authUser: AuthUserDto, @Param('assetId') assetId: string, ): Promise { - return await this.assetService.getAssetById(authUser, assetId); + await this.assetService.checkAssetsAccess(authUser, [assetId]); + return await this.assetService.getAssetById(assetId); } /** @@ -207,7 +216,8 @@ export class AssetController { @Param('assetId') assetId: string, @Body() dto: UpdateAssetDto, ): Promise { - return await this.assetService.updateAssetById(authUser, assetId, dto); + await this.assetService.checkAssetsAccess(authUser, [assetId], true); + return await this.assetService.updateAssetById(assetId, dto); } @Delete('/') @@ -215,17 +225,19 @@ export class AssetController { @GetAuthUser() authUser: AuthUserDto, @Body(ValidationPipe) assetIds: DeleteAssetDto, ): Promise { + await this.assetService.checkAssetsAccess(authUser, assetIds.ids, true); + const deleteAssetList: AssetResponseDto[] = []; for (const id of assetIds.ids) { - const assets = await this.assetService.getAssetById(authUser, id); + const assets = await this.assetService.getAssetById(id); if (!assets) { continue; } deleteAssetList.push(assets); if (assets.livePhotoVideoId) { - const livePhotoVideo = await this.assetService.getAssetById(authUser, assets.livePhotoVideoId); + const livePhotoVideo = await this.assetService.getAssetById(assets.livePhotoVideoId); if (livePhotoVideo) { deleteAssetList.push(livePhotoVideo); assetIds.ids = [...assetIds.ids, livePhotoVideo.id]; @@ -233,7 +245,7 @@ export class AssetController { } } - const result = await this.assetService.deleteAssetById(authUser, assetIds); + const result = await this.assetService.deleteAssetById(assetIds); result.forEach((res) => { deleteAssetList.filter((a) => a.id == res.id && res.status == DeleteAssetStatusEnum.SUCCESS); diff --git a/server/apps/immich/src/api-v1/asset/asset.service.ts b/server/apps/immich/src/api-v1/asset/asset.service.ts index 74217abf5b..9f69bf45b6 100644 --- a/server/apps/immich/src/api-v1/asset/asset.service.ts +++ b/server/apps/immich/src/api-v1/asset/asset.service.ts @@ -221,22 +221,18 @@ export class AssetService { return assets.map((asset) => mapAsset(asset)); } - public async getAssetById(authUser: AuthUserDto, assetId: string): Promise { + public async getAssetById(assetId: string): Promise { const asset = await this._assetRepository.getById(assetId); return mapAsset(asset); } - public async updateAssetById(authUser: AuthUserDto, assetId: string, dto: UpdateAssetDto): Promise { + public async updateAssetById(assetId: string, dto: UpdateAssetDto): Promise { const asset = await this._assetRepository.getById(assetId); if (!asset) { throw new BadRequestException('Asset not found'); } - if (authUser.id !== asset.userId) { - throw new ForbiddenException('Not the owner'); - } - const updatedAsset = await this._assetRepository.update(asset, dto); return mapAsset(updatedAsset); @@ -485,14 +481,13 @@ export class AssetService { } } - public async deleteAssetById(authUser: AuthUserDto, assetIds: DeleteAssetDto): Promise { + public async deleteAssetById(assetIds: DeleteAssetDto): Promise { const result: DeleteAssetResponseDto[] = []; const target = assetIds.ids; for (const assetId of target) { const res = await this.assetRepository.delete({ id: assetId, - userId: authUser.id, }); if (res.affected) { @@ -631,4 +626,20 @@ export class AssetService { getAssetCountByUserId(authUser: AuthUserDto): Promise { return this._assetRepository.getAssetCountByUserId(authUser.id); } + + async checkAssetsAccess(authUser: AuthUserDto, assetIds: string[], mustBeOwner: boolean = false) { + for (let assetId of assetIds) { + // Step 1: Check if user owns asset + if (await this._assetRepository.countByIdAndUser(assetId, authUser.id) == 1) { + continue; + } + + // Avoid additional checks if ownership is required + if (!mustBeOwner) { + + } + + throw new ForbiddenException(); + } + } }