From d1db479727edbee419a79d147298db3fe5eb6cc6 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Tue, 6 Jun 2023 15:17:15 -0400 Subject: [PATCH] refactor(server): move asset checks to service (#2640) --- .../src/api-v1/asset/asset.controller.ts | 101 ++++++++---------- .../src/api-v1/asset/asset.service.spec.ts | 24 +++-- .../immich/src/api-v1/asset/asset.service.ts | 31 ++++-- 3 files changed, 87 insertions(+), 69 deletions(-) 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 496b567ea4..5fc837e68d 100644 --- a/server/apps/immich/src/api-v1/asset/asset.controller.ts +++ b/server/apps/immich/src/api-v1/asset/asset.controller.ts @@ -62,6 +62,12 @@ function asStreamableFile({ stream, type, length }: ImmichReadStream) { return new StreamableFile(stream, { type, length }); } +interface UploadFiles { + assetData: ImmichFile[]; + livePhotoData?: ImmichFile[]; + sidecarData: ImmichFile[]; +} + @ApiTags('Asset') @Controller('asset') @Authenticated() @@ -87,8 +93,7 @@ export class AssetController { }) async uploadFile( @GetAuthUser() authUser: AuthUserDto, - @UploadedFiles(new ParseFilePipe({ validators: [new FileNotEmptyValidator(['assetData'])] })) - files: { assetData: ImmichFile[]; livePhotoData?: ImmichFile[]; sidecarData: ImmichFile[] }, + @UploadedFiles(new ParseFilePipe({ validators: [new FileNotEmptyValidator(['assetData'])] })) files: UploadFiles, @Body(new ValidationPipe()) dto: CreateAssetDto, @Response({ passthrough: true }) res: Res, ): Promise { @@ -116,25 +121,19 @@ export class AssetController { @SharedLinkRoute() @Get('/download/:id') @ApiOkResponse({ content: { 'application/octet-stream': { schema: { type: 'string', format: 'binary' } } } }) - async downloadFile( - @GetAuthUser() authUser: AuthUserDto, - @Response({ passthrough: true }) res: Res, - @Param() { id }: UUIDParamDto, - ) { + downloadFile(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto) { return this.assetService.downloadFile(authUser, id).then(asStreamableFile); } @SharedLinkRoute() @Post('/download-files') @ApiOkResponse({ content: { 'application/octet-stream': { schema: { type: 'string', format: 'binary' } } } }) - async downloadFiles( + downloadFiles( @GetAuthUser() authUser: AuthUserDto, @Response({ passthrough: true }) res: Res, @Body(new ValidationPipe()) dto: DownloadFilesDto, ) { - this.assetService.checkDownloadAccess(authUser); - await this.assetService.checkAssetsAccess(authUser, [...dto.assetIds]); - return this.assetService.downloadFiles(dto).then((download) => handleDownload(download, res)); + return this.assetService.downloadFiles(authUser, dto).then((download) => handleDownload(download, res)); } /** @@ -143,12 +142,11 @@ export class AssetController { @SharedLinkRoute() @Get('/download-library') @ApiOkResponse({ content: { 'application/octet-stream': { schema: { type: 'string', format: 'binary' } } } }) - async downloadLibrary( + downloadLibrary( @GetAuthUser() authUser: AuthUserDto, @Query(new ValidationPipe({ transform: true })) dto: DownloadDto, @Response({ passthrough: true }) res: Res, ) { - this.assetService.checkDownloadAccess(authUser); return this.assetService.downloadLibrary(authUser, dto).then((download) => handleDownload(download, res)); } @@ -156,14 +154,13 @@ export class AssetController { @Get('/file/:id') @Header('Cache-Control', 'max-age=31536000') @ApiOkResponse({ content: { 'application/octet-stream': { schema: { type: 'string', format: 'binary' } } } }) - async serveFile( + serveFile( @GetAuthUser() authUser: AuthUserDto, @Headers() headers: Record, @Response({ passthrough: true }) res: Res, @Query(new ValidationPipe({ transform: true })) query: ServeFileDto, @Param() { id }: UUIDParamDto, ) { - await this.assetService.checkAssetsAccess(authUser, [id]); return this.assetService.serveFile(authUser, id, query, res, headers); } @@ -171,55 +168,54 @@ export class AssetController { @Get('/thumbnail/:id') @Header('Cache-Control', 'max-age=31536000') @ApiOkResponse({ content: { 'application/octet-stream': { schema: { type: 'string', format: 'binary' } } } }) - async getAssetThumbnail( + getAssetThumbnail( @GetAuthUser() authUser: AuthUserDto, @Headers() headers: Record, @Response({ passthrough: true }) res: Res, @Param() { id }: UUIDParamDto, @Query(new ValidationPipe({ transform: true })) query: GetAssetThumbnailDto, ) { - await this.assetService.checkAssetsAccess(authUser, [id]); - return this.assetService.getAssetThumbnail(id, query, res, headers); + return this.assetService.getAssetThumbnail(authUser, id, query, res, headers); } @Get('/curated-objects') - async getCuratedObjects(@GetAuthUser() authUser: AuthUserDto): Promise { + getCuratedObjects(@GetAuthUser() authUser: AuthUserDto): Promise { return this.assetService.getCuratedObject(authUser); } @Get('/curated-locations') - async getCuratedLocations(@GetAuthUser() authUser: AuthUserDto): Promise { + getCuratedLocations(@GetAuthUser() authUser: AuthUserDto): Promise { return this.assetService.getCuratedLocation(authUser); } @Get('/search-terms') - async getAssetSearchTerms(@GetAuthUser() authUser: AuthUserDto): Promise { + getAssetSearchTerms(@GetAuthUser() authUser: AuthUserDto): Promise { return this.assetService.getAssetSearchTerm(authUser); } @Post('/search') - async searchAsset( + searchAsset( @GetAuthUser() authUser: AuthUserDto, - @Body(ValidationPipe) searchAssetDto: SearchAssetDto, + @Body(ValidationPipe) dto: SearchAssetDto, ): Promise { - return this.assetService.searchAsset(authUser, searchAssetDto); + return this.assetService.searchAsset(authUser, dto); } @Post('/count-by-time-bucket') - async getAssetCountByTimeBucket( + getAssetCountByTimeBucket( @GetAuthUser() authUser: AuthUserDto, - @Body(ValidationPipe) getAssetCountByTimeGroupDto: GetAssetCountByTimeBucketDto, + @Body(ValidationPipe) dto: GetAssetCountByTimeBucketDto, ): Promise { - return this.assetService.getAssetCountByTimeBucket(authUser, getAssetCountByTimeGroupDto); + return this.assetService.getAssetCountByTimeBucket(authUser, dto); } @Get('/count-by-user-id') - async getAssetCountByUserId(@GetAuthUser() authUser: AuthUserDto): Promise { + getAssetCountByUserId(@GetAuthUser() authUser: AuthUserDto): Promise { return this.assetService.getAssetCountByUserId(authUser); } @Get('/stat/archive') - async getArchivedAssetCountByUserId(@GetAuthUser() authUser: AuthUserDto): Promise { + getArchivedAssetCountByUserId(@GetAuthUser() authUser: AuthUserDto): Promise { return this.assetService.getArchivedAssetCountByUserId(authUser); } /** @@ -240,19 +236,19 @@ export class AssetController { } @Post('/time-bucket') - async getAssetByTimeBucket( + getAssetByTimeBucket( @GetAuthUser() authUser: AuthUserDto, - @Body(ValidationPipe) getAssetByTimeBucketDto: GetAssetByTimeBucketDto, + @Body(ValidationPipe) dto: GetAssetByTimeBucketDto, ): Promise { - return await this.assetService.getAssetByTimeBucket(authUser, getAssetByTimeBucketDto); + return this.assetService.getAssetByTimeBucket(authUser, dto); } /** * Get all asset of a device that are in the database, ID only. */ @Get('/:deviceId') - async getUserAssetsByDeviceId(@GetAuthUser() authUser: AuthUserDto, @Param() { deviceId }: DeviceIdDto) { - return await this.assetService.getUserAssetsByDeviceId(authUser, deviceId); + getUserAssetsByDeviceId(@GetAuthUser() authUser: AuthUserDto, @Param() { deviceId }: DeviceIdDto) { + return this.assetService.getUserAssetsByDeviceId(authUser, deviceId); } /** @@ -260,30 +256,27 @@ export class AssetController { */ @SharedLinkRoute() @Get('/assetById/:id') - async getAssetById(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto): Promise { - await this.assetService.checkAssetsAccess(authUser, [id]); - return await this.assetService.getAssetById(authUser, id); + getAssetById(@GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto): Promise { + return this.assetService.getAssetById(authUser, id); } /** * Update an asset */ @Put('/:id') - async updateAsset( + updateAsset( @GetAuthUser() authUser: AuthUserDto, @Param() { id }: UUIDParamDto, @Body(ValidationPipe) dto: UpdateAssetDto, ): Promise { - await this.assetService.checkAssetsAccess(authUser, [id], true); - return await this.assetService.updateAsset(authUser, id, dto); + return this.assetService.updateAsset(authUser, id, dto); } @Delete('/') - async deleteAsset( + deleteAsset( @GetAuthUser() authUser: AuthUserDto, @Body(ValidationPipe) dto: DeleteAssetDto, ): Promise { - await this.assetService.checkAssetsAccess(authUser, dto.ids, true); return this.assetService.deleteAll(authUser, dto); } @@ -293,11 +286,11 @@ export class AssetController { @SharedLinkRoute() @Post('/check') @HttpCode(200) - async checkDuplicateAsset( + checkDuplicateAsset( @GetAuthUser() authUser: AuthUserDto, - @Body(ValidationPipe) checkDuplicateAssetDto: CheckDuplicateAssetDto, + @Body(ValidationPipe) dto: CheckDuplicateAssetDto, ): Promise { - return await this.assetService.checkDuplicatedAsset(authUser, checkDuplicateAssetDto); + return this.assetService.checkDuplicatedAsset(authUser, dto); } /** @@ -305,11 +298,11 @@ export class AssetController { */ @Post('/exist') @HttpCode(200) - async checkExistingAssets( + checkExistingAssets( @GetAuthUser() authUser: AuthUserDto, - @Body(ValidationPipe) checkExistingAssetsDto: CheckExistingAssetsDto, + @Body(ValidationPipe) dto: CheckExistingAssetsDto, ): Promise { - return await this.assetService.checkExistingAssets(authUser, checkExistingAssetsDto); + return this.assetService.checkExistingAssets(authUser, dto); } /** @@ -325,28 +318,28 @@ export class AssetController { } @Post('/shared-link') - async createAssetsSharedLink( + createAssetsSharedLink( @GetAuthUser() authUser: AuthUserDto, @Body(ValidationPipe) dto: CreateAssetsShareLinkDto, ): Promise { - return await this.assetService.createAssetsSharedLink(authUser, dto); + return this.assetService.createAssetsSharedLink(authUser, dto); } @SharedLinkRoute() @Patch('/shared-link/add') - async addAssetsToSharedLink( + addAssetsToSharedLink( @GetAuthUser() authUser: AuthUserDto, @Body(ValidationPipe) dto: AddAssetsDto, ): Promise { - return await this.assetService.addAssetsToSharedLink(authUser, dto); + return this.assetService.addAssetsToSharedLink(authUser, dto); } @SharedLinkRoute() @Patch('/shared-link/remove') - async removeAssetsFromSharedLink( + removeAssetsFromSharedLink( @GetAuthUser() authUser: AuthUserDto, @Body(ValidationPipe) dto: RemoveAssetsDto, ): Promise { - return await this.assetService.removeAssetsFromSharedLink(authUser, dto); + return this.assetService.removeAssetsFromSharedLink(authUser, dto); } } diff --git a/server/apps/immich/src/api-v1/asset/asset.service.spec.ts b/server/apps/immich/src/api-v1/asset/asset.service.spec.ts index a5e8ebe576..7a54b4de79 100644 --- a/server/apps/immich/src/api-v1/asset/asset.service.spec.ts +++ b/server/apps/immich/src/api-v1/asset/asset.service.spec.ts @@ -28,7 +28,7 @@ import { sharedLinkStub, } from '@app/domain/../test'; import { CreateAssetsShareLinkDto } from './dto/create-asset-shared-link.dto'; -import { BadRequestException, ForbiddenException } from '@nestjs/common'; +import { BadRequestException } from '@nestjs/common'; import { when } from 'jest-when'; import { AssetRejectReason, AssetUploadAction } from './response-dto/asset-check-response.dto'; @@ -387,6 +387,7 @@ describe('AssetService', () => { describe('deleteAll', () => { it('should return failed status when an asset is missing', async () => { assetRepositoryMock.get.mockResolvedValue(null); + assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); await expect(sut.deleteAll(authStub.user1, { ids: ['asset1'] })).resolves.toEqual([ { id: 'asset1', status: 'FAILED' }, @@ -398,6 +399,7 @@ describe('AssetService', () => { it('should return failed status a delete fails', async () => { assetRepositoryMock.get.mockResolvedValue({ id: 'asset1' } as AssetEntity); assetRepositoryMock.remove.mockRejectedValue('delete failed'); + assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); await expect(sut.deleteAll(authStub.user1, { ids: ['asset1'] })).resolves.toEqual([ { id: 'asset1', status: 'FAILED' }, @@ -407,6 +409,8 @@ describe('AssetService', () => { }); it('should delete a live photo', async () => { + assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); + await expect(sut.deleteAll(authStub.user1, { ids: [assetEntityStub.livePhotoStillAsset.id] })).resolves.toEqual([ { id: assetEntityStub.livePhotoStillAsset.id, status: 'SUCCESS' }, { id: assetEntityStub.livePhotoMotionAsset.id, status: 'SUCCESS' }, @@ -454,6 +458,8 @@ describe('AssetService', () => { .calledWith(asset2.id) .mockResolvedValue(asset2 as AssetEntity); + assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); + await expect(sut.deleteAll(authStub.user1, { ids: ['asset1', 'asset2'] })).resolves.toEqual([ { id: 'asset1', status: 'SUCCESS' }, { id: 'asset2', status: 'SUCCESS' }, @@ -485,15 +491,15 @@ describe('AssetService', () => { }); }); - describe('checkDownloadAccess', () => { - it('should validate download access', async () => { - await sut.checkDownloadAccess(authStub.adminSharedLink); - }); + // describe('checkDownloadAccess', () => { + // it('should validate download access', async () => { + // await sut.checkDownloadAccess(authStub.adminSharedLink); + // }); - it('should not allow when user is not allowed to download', async () => { - expect(() => sut.checkDownloadAccess(authStub.readonlySharedLink)).toThrow(ForbiddenException); - }); - }); + // it('should not allow when user is not allowed to download', async () => { + // expect(() => sut.checkDownloadAccess(authStub.readonlySharedLink)).toThrow(ForbiddenException); + // }); + // }); describe('downloadFile', () => { it('should download a single file', async () => { 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 f0fe84d717..57fbfcda17 100644 --- a/server/apps/immich/src/api-v1/asset/asset.service.ts +++ b/server/apps/immich/src/api-v1/asset/asset.service.ts @@ -175,6 +175,8 @@ export class AssetService { } public async getAssetById(authUser: AuthUserDto, assetId: string): Promise { + await this.checkAssetsAccess(authUser, [assetId]); + const allowExif = this.getExifPermission(authUser); const asset = await this._assetRepository.getById(assetId); @@ -186,6 +188,8 @@ export class AssetService { } public async updateAsset(authUser: AuthUserDto, assetId: string, dto: UpdateAssetDto): Promise { + await this.checkAssetsAccess(authUser, [assetId], true); + const asset = await this._assetRepository.getById(assetId); if (!asset) { throw new BadRequestException('Asset not found'); @@ -198,13 +202,17 @@ export class AssetService { return mapAsset(updatedAsset); } - public async downloadLibrary(user: AuthUserDto, dto: DownloadDto) { - const assets = await this._assetRepository.getAllByUserId(user.id, dto); + public async downloadLibrary(authUser: AuthUserDto, dto: DownloadDto) { + this.checkDownloadAccess(authUser); + const assets = await this._assetRepository.getAllByUserId(authUser.id, dto); return this.downloadService.downloadArchive(dto.name || `library`, assets); } - public async downloadFiles(dto: DownloadFilesDto) { + public async downloadFiles(authUser: AuthUserDto, dto: DownloadFilesDto) { + this.checkDownloadAccess(authUser); + await this.checkAssetsAccess(authUser, [...dto.assetIds]); + const assetToDownload = []; for (const assetId of dto.assetIds) { @@ -239,7 +247,14 @@ export class AssetService { throw new NotFoundException(); } - async getAssetThumbnail(assetId: string, query: GetAssetThumbnailDto, res: Res, headers: Record) { + async getAssetThumbnail( + authUser: AuthUserDto, + assetId: string, + query: GetAssetThumbnailDto, + res: Res, + headers: Record, + ) { + await this.checkAssetsAccess(authUser, [assetId]); const asset = await this._assetRepository.get(assetId); if (!asset) { throw new NotFoundException('Asset not found'); @@ -265,6 +280,8 @@ export class AssetService { res: Res, headers: Record, ) { + await this.checkAssetsAccess(authUser, [assetId]); + const allowOriginalFile = !!(!authUser.isPublicUser || authUser.isAllowDownload); const asset = await this._assetRepository.getById(assetId); @@ -346,6 +363,8 @@ export class AssetService { } public async deleteAll(authUser: AuthUserDto, dto: DeleteAssetDto): Promise { + await this.checkAssetsAccess(authUser, dto.ids, true); + const deleteQueue: Array = []; const result: DeleteAssetResponseDto[] = []; @@ -547,7 +566,7 @@ export class AssetService { return this._assetRepository.getArchivedAssetCountByUserId(authUser.id); } - async checkAssetsAccess(authUser: AuthUserDto, assetIds: string[], mustBeOwner = false) { + private async checkAssetsAccess(authUser: AuthUserDto, assetIds: string[], mustBeOwner = false) { for (const assetId of assetIds) { // Step 1: Check if asset is part of a public shared if (authUser.sharedLinkId) { @@ -587,7 +606,7 @@ export class AssetService { } } - checkDownloadAccess(authUser: AuthUserDto) { + private checkDownloadAccess(authUser: AuthUserDto) { this.shareCore.checkDownloadAccess(authUser); }