refactor(server): access checks (#2776)

* refactor(server): access checks

* chore: simply asset module

server/src/domain/access/access.repository.ts

@@ -2,6 +2,8 @@ export const IAccessRepository = 'IAccessRepository';
 
 export interface IAccessRepository {
   hasPartnerAccess(userId: string, partnerId: string): Promise<boolean>;
+  hasAlbumAssetAccess(userId: string, assetId: string): Promise<boolean>;
+  hasOwnerAssetAccess(userId: string, assetId: string): Promise<boolean>;
   hasPartnerAssetAccess(userId: string, assetId: string): Promise<boolean>;
   hasSharedLinkAssetAccess(userId: string, assetId: string): Promise<boolean>;
 }

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<number>;
   addAssets(album: AlbumEntity, addAssetsDto: AddAssetsDto): Promise<AddAssetsResponseDto>;
   updateThumbnails(): Promise<number | undefined>;
-  getSharedWithUserAlbumCount(userId: string, assetId: string): Promise<number>;
 }
 
 export const IAlbumRepository = 'IAlbumRepository';
@@ -130,25 +129,4 @@ export class AlbumRepository implements IAlbumRepository {
 
     return result.affected;
   }
-
-  async getSharedWithUserAlbumCount(userId: string, assetId: string): Promise<number> {
-    return this.albumRepository.count({
-      where: [
-        {
-          ownerId: userId,
-          assets: {
-            id: assetId,
-          },
-        },
-        {
-          sharedUsers: {
-            id: userId,
-          },
-          assets: {
-            id: assetId,
-          },
-        },
-      ],
-    });
-  }
 }

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 {}

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();

server/src/immich/api-v1/asset/asset-repository.ts

@@ -39,7 +39,6 @@ export interface IAssetRepository {
   getAssetByTimeBucket(userId: string, getAssetByTimeBucketDto: GetAssetByTimeBucketDto): Promise<AssetEntity[]>;
   getAssetsByChecksums(userId: string, checksums: Buffer[]): Promise<AssetCheck[]>;
   getExistingAssets(userId: string, checkDuplicateAssetDto: CheckExistingAssetsDto): Promise<string[]>;
-  countByIdAndUser(assetId: string, userId: string): Promise<number>;
 }
 
 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<number> {
-    return this.assetRepository.count({
-      where: {
-        id: assetId,
-        ownerId,
-      },
-    });
-  }
-
   private getAssetCount(items: any): AssetCountByUserIdResponseDto {
     const assetCountByUserId = new AssetCountByUserIdResponseDto();
 

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 {}

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<AssetEntity>; // TO BE DELETED AFTER FINISHED REFACTORING
   let accessMock: jest.Mocked<IAccessRepository>;
   let assetRepositoryMock: jest.Mocked<IAssetRepository>;
-  let albumRepositoryMock: jest.Mocked<IAlbumRepository>;
   let downloadServiceMock: jest.Mocked<Partial<DownloadService>>;
   let sharedLinkRepositoryMock: jest.Mocked<ISharedLinkRepository>;
   let cryptoMock: jest.Mocked<ICryptoRepository>;
@@ -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<AlbumRepository>;
-
     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();
+    });
+  });
 });

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<AssetEntity>,
     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();

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<AssetEntity>,
+    @InjectRepository(AlbumEntity) private albumRepository: Repository<AlbumEntity>,
     @InjectRepository(PartnerEntity) private partnerRepository: Repository<PartnerEntity>,
     @InjectRepository(SharedLinkEntity) private sharedLinkRepository: Repository<SharedLinkEntity>,
   ) {}
@@ -18,6 +20,36 @@ export class AccessRepository implements IAccessRepository {
     });
   }
 
+  hasAlbumAssetAccess(userId: string, assetId: string): Promise<boolean> {
+    return this.albumRepository.exist({
+      where: [
+        {
+          ownerId: userId,
+          assets: {
+            id: assetId,
+          },
+        },
+        {
+          sharedUsers: {
+            id: userId,
+          },
+          assets: {
+            id: assetId,
+          },
+        },
+      ],
+    });
+  }
+
+  hasOwnerAssetAccess(userId: string, assetId: string): Promise<boolean> {
+    return this.assetRepository.exist({
+      where: {
+        id: assetId,
+        ownerId: userId,
+      },
+    });
+  }
+
   hasPartnerAssetAccess(userId: string, assetId: string): Promise<boolean> {
     return this.partnerRepository.exist({
       where: {

server/src/infra/repositories/album.repository.ts

@@ -124,8 +124,8 @@ export class AlbumRepository implements IAlbumRepository {
     });
   }
 
-  async hasAsset(id: string, assetId: string): Promise<boolean> {
-    const count = await this.repository.count({
+  hasAsset(id: string, assetId: string): Promise<boolean> {
+    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<AlbumEntity>): Promise<AlbumEntity> {

server/test/repositories/access.repository.mock.ts

@@ -3,6 +3,8 @@ import { IAccessRepository } from '@app/domain';
 export const newAccessRepositoryMock = (): jest.Mocked<IAccessRepository> => {
   return {
     hasPartnerAccess: jest.fn(),
+    hasAlbumAssetAccess: jest.fn(),
+    hasOwnerAssetAccess: jest.fn(),
     hasPartnerAssetAccess: jest.fn(),
     hasSharedLinkAssetAccess: jest.fn(),
   };