From f04e47803c3eb5b0d7b0bf7fc5c0a12fc74e7931 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Fri, 16 Jun 2023 15:01:34 -0400 Subject: [PATCH] refactor(server): access checks (#2776) * refactor(server): access checks * chore: simply asset module --- server/src/domain/access/access.repository.ts | 2 + .../immich/api-v1/album/album-repository.ts | 24 +----- .../src/immich/api-v1/album/album.module.ts | 8 +- .../immich/api-v1/album/album.service.spec.ts | 16 ++-- .../immich/api-v1/asset/asset-repository.ts | 10 --- .../src/immich/api-v1/asset/asset.module.ts | 10 +-- .../immich/api-v1/asset/asset.service.spec.ts | 80 +++++++++++++++---- .../src/immich/api-v1/asset/asset.service.ts | 39 +++++---- .../infra/repositories/access.repository.ts | 34 +++++++- .../infra/repositories/album.repository.ts | 6 +- .../repositories/access.repository.mock.ts | 2 + 11 files changed, 132 insertions(+), 99 deletions(-) diff --git a/server/src/domain/access/access.repository.ts b/server/src/domain/access/access.repository.ts index 06466e5cd2..628647c5e9 100644 --- a/server/src/domain/access/access.repository.ts +++ b/server/src/domain/access/access.repository.ts @@ -2,6 +2,8 @@ export const IAccessRepository = 'IAccessRepository'; export interface IAccessRepository { hasPartnerAccess(userId: string, partnerId: string): Promise; + hasAlbumAssetAccess(userId: string, assetId: string): Promise; + hasOwnerAssetAccess(userId: string, assetId: string): Promise; hasPartnerAssetAccess(userId: string, assetId: string): Promise; hasSharedLinkAssetAccess(userId: string, assetId: string): Promise; } diff --git a/server/src/immich/api-v1/album/album-repository.ts b/server/src/immich/api-v1/album/album-repository.ts index f73e36f20a..42cdbdfb55 100644 --- a/server/src/immich/api-v1/album/album-repository.ts +++ b/server/src/immich/api-v1/album/album-repository.ts @@ -1,5 +1,5 @@ -import { AlbumEntity, AssetEntity } from '@app/infra/entities'; import { dataSource } from '@app/infra/database.config'; +import { AlbumEntity, AssetEntity } from '@app/infra/entities'; import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; @@ -12,7 +12,6 @@ export interface IAlbumRepository { removeAssets(album: AlbumEntity, removeAssets: RemoveAssetsDto): Promise; addAssets(album: AlbumEntity, addAssetsDto: AddAssetsDto): Promise; updateThumbnails(): Promise; - getSharedWithUserAlbumCount(userId: string, assetId: string): Promise; } export const IAlbumRepository = 'IAlbumRepository'; @@ -130,25 +129,4 @@ export class AlbumRepository implements IAlbumRepository { return result.affected; } - - async getSharedWithUserAlbumCount(userId: string, assetId: string): Promise { - return this.albumRepository.count({ - where: [ - { - ownerId: userId, - assets: { - id: assetId, - }, - }, - { - sharedUsers: { - id: userId, - }, - assets: { - id: assetId, - }, - }, - ], - }); - } } diff --git a/server/src/immich/api-v1/album/album.module.ts b/server/src/immich/api-v1/album/album.module.ts index 414584243d..ad13cdcf3e 100644 --- a/server/src/immich/api-v1/album/album.module.ts +++ b/server/src/immich/api-v1/album/album.module.ts @@ -6,15 +6,9 @@ import { AlbumEntity, AssetEntity } from '@app/infra/entities'; import { AlbumRepository, IAlbumRepository } from './album-repository'; import { DownloadModule } from '../../modules/download/download.module'; -const ALBUM_REPOSITORY_PROVIDER = { - provide: IAlbumRepository, - useClass: AlbumRepository, -}; - @Module({ imports: [TypeOrmModule.forFeature([AlbumEntity, AssetEntity]), DownloadModule], controllers: [AlbumController], - providers: [AlbumService, ALBUM_REPOSITORY_PROVIDER], - exports: [ALBUM_REPOSITORY_PROVIDER], + providers: [AlbumService, { provide: IAlbumRepository, useClass: AlbumRepository }], }) export class AlbumModule {} diff --git a/server/src/immich/api-v1/album/album.service.spec.ts b/server/src/immich/api-v1/album/album.service.spec.ts index 81e66bfd49..69b38021b6 100644 --- a/server/src/immich/api-v1/album/album.service.spec.ts +++ b/server/src/immich/api-v1/album/album.service.spec.ts @@ -1,13 +1,12 @@ -import { AlbumService } from './album.service'; -import { AuthUserDto } from '../../decorators/auth-user.decorator'; -import { NotFoundException, ForbiddenException } from '@nestjs/common'; +import { AlbumResponseDto, ICryptoRepository, ISharedLinkRepository, mapUser } from '@app/domain'; import { AlbumEntity, UserEntity } from '@app/infra/entities'; -import { AlbumResponseDto, ICryptoRepository, mapUser } from '@app/domain'; -import { AddAssetsResponseDto } from './response-dto/add-assets-response.dto'; -import { IAlbumRepository } from './album-repository'; -import { DownloadService } from '../../modules/download/download.service'; -import { ISharedLinkRepository } from '@app/domain'; +import { ForbiddenException, NotFoundException } from '@nestjs/common'; import { newCryptoRepositoryMock, newSharedLinkRepositoryMock, userEntityStub } from '@test'; +import { AuthUserDto } from '../../decorators/auth-user.decorator'; +import { DownloadService } from '../../modules/download/download.service'; +import { IAlbumRepository } from './album-repository'; +import { AlbumService } from './album.service'; +import { AddAssetsResponseDto } from './response-dto/add-assets-response.dto'; describe('Album service', () => { let sut: AlbumService; @@ -98,7 +97,6 @@ describe('Album service', () => { get: jest.fn(), removeAssets: jest.fn(), updateThumbnails: jest.fn(), - getSharedWithUserAlbumCount: jest.fn(), }; sharedLinkRepositoryMock = newSharedLinkRepositoryMock(); diff --git a/server/src/immich/api-v1/asset/asset-repository.ts b/server/src/immich/api-v1/asset/asset-repository.ts index 2170d20684..e896e196de 100644 --- a/server/src/immich/api-v1/asset/asset-repository.ts +++ b/server/src/immich/api-v1/asset/asset-repository.ts @@ -39,7 +39,6 @@ export interface IAssetRepository { getAssetByTimeBucket(userId: string, getAssetByTimeBucketDto: GetAssetByTimeBucketDto): Promise; getAssetsByChecksums(userId: string, checksums: Buffer[]): Promise; getExistingAssets(userId: string, checkDuplicateAssetDto: CheckExistingAssetsDto): Promise; - countByIdAndUser(assetId: string, userId: string): Promise; } export const IAssetRepository = 'IAssetRepository'; @@ -329,15 +328,6 @@ export class AssetRepository implements IAssetRepository { return assets.map((asset) => asset.deviceAssetId); } - countByIdAndUser(assetId: string, ownerId: string): Promise { - return this.assetRepository.count({ - where: { - id: assetId, - ownerId, - }, - }); - } - private getAssetCount(items: any): AssetCountByUserIdResponseDto { const assetCountByUserId = new AssetCountByUserIdResponseDto(); diff --git a/server/src/immich/api-v1/asset/asset.module.ts b/server/src/immich/api-v1/asset/asset.module.ts index 6ec9c603a6..5dcadc52ee 100644 --- a/server/src/immich/api-v1/asset/asset.module.ts +++ b/server/src/immich/api-v1/asset/asset.module.ts @@ -5,22 +5,14 @@ import { TypeOrmModule } from '@nestjs/typeorm'; import { AssetEntity, ExifEntity } from '@app/infra/entities'; import { AssetRepository, IAssetRepository } from './asset-repository'; import { DownloadModule } from '../../modules/download/download.module'; -import { AlbumModule } from '../album/album.module'; - -const ASSET_REPOSITORY_PROVIDER = { - provide: IAssetRepository, - useClass: AssetRepository, -}; @Module({ imports: [ // TypeOrmModule.forFeature([AssetEntity, ExifEntity]), DownloadModule, - AlbumModule, ], controllers: [AssetController], - providers: [AssetService, ASSET_REPOSITORY_PROVIDER], - exports: [ASSET_REPOSITORY_PROVIDER], + providers: [AssetService, { provide: IAssetRepository, useClass: AssetRepository }], }) export class AssetModule {} diff --git a/server/src/immich/api-v1/asset/asset.service.spec.ts b/server/src/immich/api-v1/asset/asset.service.spec.ts index acea758b4b..670c58f0db 100644 --- a/server/src/immich/api-v1/asset/asset.service.spec.ts +++ b/server/src/immich/api-v1/asset/asset.service.spec.ts @@ -7,7 +7,6 @@ import { AssetCountByTimeBucket } from './response-dto/asset-count-by-time-group import { TimeGroupEnum } from './dto/get-asset-count-by-time-bucket.dto'; import { AssetCountByUserIdResponseDto } from './response-dto/asset-count-by-user-id-response.dto'; import { DownloadService } from '../../modules/download/download.service'; -import { AlbumRepository, IAlbumRepository } from '../album/album-repository'; import { IAccessRepository, ICryptoRepository, @@ -29,7 +28,7 @@ import { sharedLinkStub, } from '@test'; import { CreateAssetsShareLinkDto } from './dto/create-asset-shared-link.dto'; -import { BadRequestException } from '@nestjs/common'; +import { BadRequestException, ForbiddenException } from '@nestjs/common'; import { when } from 'jest-when'; import { AssetRejectReason, AssetUploadAction } from './response-dto/asset-check-response.dto'; @@ -134,7 +133,6 @@ describe('AssetService', () => { let a: Repository; // TO BE DELETED AFTER FINISHED REFACTORING let accessMock: jest.Mocked; let assetRepositoryMock: jest.Mocked; - let albumRepositoryMock: jest.Mocked; let downloadServiceMock: jest.Mocked>; let sharedLinkRepositoryMock: jest.Mocked; let cryptoMock: jest.Mocked; @@ -160,13 +158,8 @@ describe('AssetService', () => { getAssetCountByUserId: jest.fn(), getArchivedAssetCountByUserId: jest.fn(), getExistingAssets: jest.fn(), - countByIdAndUser: jest.fn(), }; - albumRepositoryMock = { - getSharedWithUserAlbumCount: jest.fn(), - } as unknown as jest.Mocked; - downloadServiceMock = { downloadArchive: jest.fn(), }; @@ -180,7 +173,6 @@ describe('AssetService', () => { sut = new AssetService( accessMock, assetRepositoryMock, - albumRepositoryMock, a, downloadServiceMock as DownloadService, sharedLinkRepositoryMock, @@ -203,13 +195,13 @@ describe('AssetService', () => { const dto: CreateAssetsShareLinkDto = { assetIds: [asset1.id] }; assetRepositoryMock.getById.mockResolvedValue(asset1); - assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); + accessMock.hasOwnerAssetAccess.mockResolvedValue(true); sharedLinkRepositoryMock.create.mockResolvedValue(sharedLinkStub.valid); await expect(sut.createAssetsSharedLink(authStub.user1, dto)).resolves.toEqual(sharedLinkResponseStub.valid); expect(assetRepositoryMock.getById).toHaveBeenCalledWith(asset1.id); - expect(assetRepositoryMock.countByIdAndUser).toHaveBeenCalledWith(asset1.id, authStub.user1.id); + expect(accessMock.hasOwnerAssetAccess).toHaveBeenCalledWith(authStub.user1.id, asset1.id); }); }); @@ -383,7 +375,7 @@ describe('AssetService', () => { describe('deleteAll', () => { it('should return failed status when an asset is missing', async () => { assetRepositoryMock.get.mockResolvedValue(null); - assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); + accessMock.hasOwnerAssetAccess.mockResolvedValue(true); await expect(sut.deleteAll(authStub.user1, { ids: ['asset1'] })).resolves.toEqual([ { id: 'asset1', status: 'FAILED' }, @@ -395,7 +387,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); + accessMock.hasOwnerAssetAccess.mockResolvedValue(true); await expect(sut.deleteAll(authStub.user1, { ids: ['asset1'] })).resolves.toEqual([ { id: 'asset1', status: 'FAILED' }, @@ -405,7 +397,7 @@ describe('AssetService', () => { }); it('should delete a live photo', async () => { - assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); + accessMock.hasOwnerAssetAccess.mockResolvedValue(true); await expect(sut.deleteAll(authStub.user1, { ids: [assetEntityStub.livePhotoStillAsset.id] })).resolves.toEqual([ { id: assetEntityStub.livePhotoStillAsset.id, status: 'SUCCESS' }, @@ -454,7 +446,7 @@ describe('AssetService', () => { .calledWith(asset2.id) .mockResolvedValue(asset2 as AssetEntity); - assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); + accessMock.hasOwnerAssetAccess.mockResolvedValue(true); await expect(sut.deleteAll(authStub.user1, { ids: ['asset1', 'asset2'] })).resolves.toEqual([ { id: 'asset1', status: 'SUCCESS' }, @@ -499,7 +491,7 @@ describe('AssetService', () => { describe('downloadFile', () => { it('should download a single file', async () => { - assetRepositoryMock.countByIdAndUser.mockResolvedValue(1); + accessMock.hasOwnerAssetAccess.mockResolvedValue(true); assetRepositoryMock.get.mockResolvedValue(_getAsset_1()); await sut.downloadFile(authStub.admin, 'id_1'); @@ -535,4 +527,60 @@ describe('AssetService', () => { expect(assetRepositoryMock.getAssetsByChecksums).toHaveBeenCalledWith(authStub.admin.id, [file1, file2]); }); }); + + describe('getAssetById', () => { + it('should allow owner access', async () => { + accessMock.hasOwnerAssetAccess.mockResolvedValue(true); + assetRepositoryMock.getById.mockResolvedValue(assetEntityStub.image); + await sut.getAssetById(authStub.admin, assetEntityStub.image.id); + expect(accessMock.hasOwnerAssetAccess).toHaveBeenCalledWith(authStub.admin.id, assetEntityStub.image.id); + }); + + it('should allow shared link access', async () => { + accessMock.hasSharedLinkAssetAccess.mockResolvedValue(true); + assetRepositoryMock.getById.mockResolvedValue(assetEntityStub.image); + await sut.getAssetById(authStub.adminSharedLink, assetEntityStub.image.id); + expect(accessMock.hasSharedLinkAssetAccess).toHaveBeenCalledWith( + authStub.adminSharedLink.sharedLinkId, + assetEntityStub.image.id, + ); + }); + + it('should allow partner sharing access', async () => { + accessMock.hasOwnerAssetAccess.mockResolvedValue(false); + accessMock.hasPartnerAssetAccess.mockResolvedValue(true); + assetRepositoryMock.getById.mockResolvedValue(assetEntityStub.image); + await sut.getAssetById(authStub.admin, assetEntityStub.image.id); + expect(accessMock.hasPartnerAssetAccess).toHaveBeenCalledWith(authStub.admin.id, assetEntityStub.image.id); + }); + + it('should allow shared album access', async () => { + accessMock.hasOwnerAssetAccess.mockResolvedValue(false); + accessMock.hasPartnerAssetAccess.mockResolvedValue(false); + accessMock.hasAlbumAssetAccess.mockResolvedValue(true); + assetRepositoryMock.getById.mockResolvedValue(assetEntityStub.image); + await sut.getAssetById(authStub.admin, assetEntityStub.image.id); + expect(accessMock.hasAlbumAssetAccess).toHaveBeenCalledWith(authStub.admin.id, assetEntityStub.image.id); + }); + + it('should throw an error for no access', async () => { + accessMock.hasOwnerAssetAccess.mockResolvedValue(false); + accessMock.hasPartnerAssetAccess.mockResolvedValue(false); + accessMock.hasSharedLinkAssetAccess.mockResolvedValue(false); + accessMock.hasAlbumAssetAccess.mockResolvedValue(false); + await expect(sut.getAssetById(authStub.admin, assetEntityStub.image.id)).rejects.toBeInstanceOf( + ForbiddenException, + ); + expect(assetRepositoryMock.getById).not.toHaveBeenCalled(); + }); + + it('should throw an error for an invalid shared link', async () => { + accessMock.hasSharedLinkAssetAccess.mockResolvedValue(false); + await expect(sut.getAssetById(authStub.adminSharedLink, assetEntityStub.image.id)).rejects.toBeInstanceOf( + ForbiddenException, + ); + expect(accessMock.hasOwnerAssetAccess).not.toHaveBeenCalled(); + expect(assetRepositoryMock.getById).not.toHaveBeenCalled(); + }); + }); }); diff --git a/server/src/immich/api-v1/asset/asset.service.ts b/server/src/immich/api-v1/asset/asset.service.ts index 15c7a172e6..6305a48f31 100644 --- a/server/src/immich/api-v1/asset/asset.service.ts +++ b/server/src/immich/api-v1/asset/asset.service.ts @@ -53,7 +53,6 @@ import { AssetFileUploadResponseDto } from './response-dto/asset-file-upload-res import { ICryptoRepository, IJobRepository } from '@app/domain'; import { DownloadService } from '../../modules/download/download.service'; import { DownloadDto } from './dto/download-library.dto'; -import { IAlbumRepository } from '../album/album-repository'; import { SharedLinkCore } from '@app/domain'; import { ISharedLinkRepository } from '@app/domain'; import { DownloadFilesDto } from './dto/download-files.dto'; @@ -85,7 +84,6 @@ export class AssetService { constructor( @Inject(IAccessRepository) private accessRepository: IAccessRepository, @Inject(IAssetRepository) private _assetRepository: IAssetRepository, - @Inject(IAlbumRepository) private _albumRepository: IAlbumRepository, @InjectRepository(AssetEntity) private assetRepository: Repository, private downloadService: DownloadService, @@ -567,31 +565,32 @@ export class AssetService { const sharedLinkId = authUser.sharedLinkId; for (const assetId of assetIds) { - // Step 1: Check if asset is part of a public shared if (sharedLinkId) { const canAccess = await this.accessRepository.hasSharedLinkAssetAccess(sharedLinkId, assetId); if (canAccess) { continue; } - } else { - // Step 2: Check if user owns asset - if ((await this._assetRepository.countByIdAndUser(assetId, authUser.id)) == 1) { - continue; - } - // Step 3: Check if any partner owns the asset - const canAccess = await this.accessRepository.hasPartnerAssetAccess(authUser.id, assetId); - if (canAccess) { - continue; - } + throw new ForbiddenException(); + } - // Avoid additional checks if ownership is required - if (!mustBeOwner) { - // Step 2: Check if asset is part of an album shared with me - if ((await this._albumRepository.getSharedWithUserAlbumCount(authUser.id, assetId)) > 0) { - continue; - } - } + const isOwner = await this.accessRepository.hasOwnerAssetAccess(authUser.id, assetId); + if (isOwner) { + continue; + } + + if (mustBeOwner) { + throw new ForbiddenException(); + } + + const isPartnerShared = await this.accessRepository.hasPartnerAssetAccess(authUser.id, assetId); + if (isPartnerShared) { + continue; + } + + const isAlbumShared = await this.accessRepository.hasAlbumAssetAccess(authUser.id, assetId); + if (isAlbumShared) { + continue; } throw new ForbiddenException(); diff --git a/server/src/infra/repositories/access.repository.ts b/server/src/infra/repositories/access.repository.ts index 0937f985a4..c0d54e2396 100644 --- a/server/src/infra/repositories/access.repository.ts +++ b/server/src/infra/repositories/access.repository.ts @@ -1,10 +1,12 @@ import { IAccessRepository } from '@app/domain'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; -import { PartnerEntity, SharedLinkEntity } from '../entities'; +import { AlbumEntity, AssetEntity, PartnerEntity, SharedLinkEntity } from '../entities'; export class AccessRepository implements IAccessRepository { constructor( + @InjectRepository(AssetEntity) private assetRepository: Repository, + @InjectRepository(AlbumEntity) private albumRepository: Repository, @InjectRepository(PartnerEntity) private partnerRepository: Repository, @InjectRepository(SharedLinkEntity) private sharedLinkRepository: Repository, ) {} @@ -18,6 +20,36 @@ export class AccessRepository implements IAccessRepository { }); } + hasAlbumAssetAccess(userId: string, assetId: string): Promise { + return this.albumRepository.exist({ + where: [ + { + ownerId: userId, + assets: { + id: assetId, + }, + }, + { + sharedUsers: { + id: userId, + }, + assets: { + id: assetId, + }, + }, + ], + }); + } + + hasOwnerAssetAccess(userId: string, assetId: string): Promise { + return this.assetRepository.exist({ + where: { + id: assetId, + ownerId: userId, + }, + }); + } + hasPartnerAssetAccess(userId: string, assetId: string): Promise { return this.partnerRepository.exist({ where: { diff --git a/server/src/infra/repositories/album.repository.ts b/server/src/infra/repositories/album.repository.ts index 66ef239a90..bc3cc8adc2 100644 --- a/server/src/infra/repositories/album.repository.ts +++ b/server/src/infra/repositories/album.repository.ts @@ -124,8 +124,8 @@ export class AlbumRepository implements IAlbumRepository { }); } - async hasAsset(id: string, assetId: string): Promise { - const count = await this.repository.count({ + hasAsset(id: string, assetId: string): Promise { + return this.repository.exist({ where: { id, assets: { @@ -136,8 +136,6 @@ export class AlbumRepository implements IAlbumRepository { assets: true, }, }); - - return Boolean(count); } async create(album: Partial): Promise { diff --git a/server/test/repositories/access.repository.mock.ts b/server/test/repositories/access.repository.mock.ts index da00474db0..6bbf047176 100644 --- a/server/test/repositories/access.repository.mock.ts +++ b/server/test/repositories/access.repository.mock.ts @@ -3,6 +3,8 @@ import { IAccessRepository } from '@app/domain'; export const newAccessRepositoryMock = (): jest.Mocked => { return { hasPartnerAccess: jest.fn(), + hasAlbumAssetAccess: jest.fn(), + hasOwnerAssetAccess: jest.fn(), hasPartnerAssetAccess: jest.fn(), hasSharedLinkAssetAccess: jest.fn(), };