refactor(server): access permissions (#2910)

* refactor: access repo interface

* feat: access core

* fix: allow shared links to add to a shared link

* chore: comment out unused code

* fix: pr feedback

---------

Co-authored-by: Alex Tran <alex.tran1502@gmail.com>

server/src/domain/access/access.core.ts

@@ -0,0 +1,134 @@
+import { BadRequestException } from '@nestjs/common';
+import { AuthUserDto } from '../auth';
+import { IAccessRepository } from './access.repository';
+
+export enum Permission {
+  // ASSET_CREATE = 'asset.create',
+  ASSET_READ = 'asset.read',
+  ASSET_UPDATE = 'asset.update',
+  ASSET_DELETE = 'asset.delete',
+  ASSET_SHARE = 'asset.share',
+  ASSET_VIEW = 'asset.view',
+  ASSET_DOWNLOAD = 'asset.download',
+
+  // ALBUM_CREATE = 'album.create',
+  // ALBUM_READ = 'album.read',
+  ALBUM_UPDATE = 'album.update',
+  ALBUM_DELETE = 'album.delete',
+  ALBUM_SHARE = 'album.share',
+
+  LIBRARY_READ = 'library.read',
+  LIBRARY_DOWNLOAD = 'library.download',
+}
+
+export class AccessCore {
+  constructor(private repository: IAccessRepository) {}
+
+  async requirePermission(authUser: AuthUserDto, permission: Permission, ids: string[] | string) {
+    const hasAccess = await this.hasPermission(authUser, permission, ids);
+    if (!hasAccess) {
+      throw new BadRequestException(`Not found or no ${permission} access`);
+    }
+  }
+
+  async hasPermission(authUser: AuthUserDto, permission: Permission, ids: string[] | string) {
+    ids = Array.isArray(ids) ? ids : [ids];
+
+    const isSharedLink = authUser.isPublicUser ?? false;
+
+    for (const id of ids) {
+      const hasAccess = isSharedLink
+        ? await this.hasSharedLinkAccess(authUser, permission, id)
+        : await this.hasOtherAccess(authUser, permission, id);
+      if (!hasAccess) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  private async hasSharedLinkAccess(authUser: AuthUserDto, permission: Permission, id: string) {
+    const sharedLinkId = authUser.sharedLinkId;
+    if (!sharedLinkId) {
+      return false;
+    }
+
+    switch (permission) {
+      case Permission.ASSET_READ:
+        return this.repository.asset.hasSharedLinkAccess(sharedLinkId, id);
+
+      case Permission.ASSET_VIEW:
+        return await this.repository.asset.hasSharedLinkAccess(sharedLinkId, id);
+
+      case Permission.ASSET_DOWNLOAD:
+        return !!authUser.isAllowDownload && (await this.repository.asset.hasSharedLinkAccess(sharedLinkId, id));
+
+      case Permission.ASSET_SHARE:
+        // TODO: fix this to not use authUser.id for shared link access control
+        return this.repository.asset.hasOwnerAccess(authUser.id, id);
+
+      // case Permission.ALBUM_READ:
+      //   return this.repository.album.hasSharedLinkAccess(sharedLinkId, id);
+
+      default:
+        return false;
+    }
+  }
+
+  private async hasOtherAccess(authUser: AuthUserDto, permission: Permission, id: string) {
+    switch (permission) {
+      case Permission.ASSET_READ:
+        return (
+          (await this.repository.asset.hasOwnerAccess(authUser.id, id)) ||
+          (await this.repository.asset.hasAlbumAccess(authUser.id, id)) ||
+          (await this.repository.asset.hasPartnerAccess(authUser.id, id))
+        );
+      case Permission.ASSET_UPDATE:
+        return this.repository.asset.hasOwnerAccess(authUser.id, id);
+
+      case Permission.ASSET_DELETE:
+        return this.repository.asset.hasOwnerAccess(authUser.id, id);
+
+      case Permission.ASSET_SHARE:
+        return (
+          (await this.repository.asset.hasOwnerAccess(authUser.id, id)) ||
+          (await this.repository.asset.hasPartnerAccess(authUser.id, id))
+        );
+
+      case Permission.ASSET_VIEW:
+        return (
+          (await this.repository.asset.hasOwnerAccess(authUser.id, id)) ||
+          (await this.repository.asset.hasAlbumAccess(authUser.id, id)) ||
+          (await this.repository.asset.hasPartnerAccess(authUser.id, id))
+        );
+
+      case Permission.ASSET_DOWNLOAD:
+        return (
+          (await this.repository.asset.hasOwnerAccess(authUser.id, id)) ||
+          (await this.repository.asset.hasAlbumAccess(authUser.id, id)) ||
+          (await this.repository.asset.hasPartnerAccess(authUser.id, id))
+        );
+
+      // case Permission.ALBUM_READ:
+      //   return this.repository.album.hasOwnerAccess(authUser.id, id);
+
+      case Permission.ALBUM_UPDATE:
+        return this.repository.album.hasOwnerAccess(authUser.id, id);
+
+      case Permission.ALBUM_DELETE:
+        return this.repository.album.hasOwnerAccess(authUser.id, id);
+
+      case Permission.ALBUM_SHARE:
+        return this.repository.album.hasOwnerAccess(authUser.id, id);
+
+      case Permission.LIBRARY_READ:
+        return authUser.id === id || (await this.repository.library.hasPartnerAccess(authUser.id, id));
+
+      case Permission.LIBRARY_DOWNLOAD:
+        return authUser.id === id;
+    }
+
+    return false;
+  }
+}

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

@@ -1,12 +1,20 @@
 export const IAccessRepository = 'IAccessRepository';
 
 export interface IAccessRepository {
+  asset: {
+    hasOwnerAccess(userId: string, assetId: string): Promise<boolean>;
+    hasAlbumAccess(userId: string, assetId: string): Promise<boolean>;
+    hasPartnerAccess(userId: string, assetId: string): Promise<boolean>;
+    hasSharedLinkAccess(sharedLinkId: string, assetId: string): Promise<boolean>;
+  };
+
+  album: {
+    hasOwnerAccess(userId: string, albumId: string): Promise<boolean>;
+    hasSharedAlbumAccess(userId: string, albumId: string): Promise<boolean>;
+    hasSharedLinkAccess(sharedLinkId: string, albumId: string): Promise<boolean>;
+  };
+
+  library: {
     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>;
-
-  hasAlbumOwnerAccess(userId: string, albumId: string): Promise<boolean>;
 }

server/src/domain/access/index.ts

@@ -1 +1,2 @@
+export * from './access.core';
 export * from './access.repository';

server/src/domain/album/album.service.spec.ts

@@ -1,7 +1,9 @@
-import { BadRequestException, ForbiddenException } from '@nestjs/common';
+import { BadRequestException } from '@nestjs/common';
 import {
   albumStub,
   authStub,
+  IAccessRepositoryMock,
+  newAccessRepositoryMock,
   newAlbumRepositoryMock,
   newAssetRepositoryMock,
   newJobRepositoryMock,
@@ -17,18 +19,20 @@ import { AlbumService } from './album.service';
 
 describe(AlbumService.name, () => {
   let sut: AlbumService;
+  let accessMock: IAccessRepositoryMock;
   let albumMock: jest.Mocked<IAlbumRepository>;
   let assetMock: jest.Mocked<IAssetRepository>;
   let jobMock: jest.Mocked<IJobRepository>;
   let userMock: jest.Mocked<IUserRepository>;
 
   beforeEach(async () => {
+    accessMock = newAccessRepositoryMock();
     albumMock = newAlbumRepositoryMock();
     assetMock = newAssetRepositoryMock();
     jobMock = newJobRepositoryMock();
     userMock = newUserRepositoryMock();
 
-    sut = new AlbumService(albumMock, assetMock, jobMock, userMock);
+    sut = new AlbumService(accessMock, albumMock, assetMock, jobMock, userMock);
   });
 
   it('should work', () => {
@@ -210,16 +214,16 @@ describe(AlbumService.name, () => {
     });
 
     it('should prevent updating a not owned album (shared with auth user)', async () => {
-      albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]);
+      accessMock.album.hasOwnerAccess.mockResolvedValue(false);
-
       await expect(
         sut.update(authStub.admin, albumStub.sharedWithAdmin.id, {
           albumName: 'new album name',
         }),
-      ).rejects.toBeInstanceOf(ForbiddenException);
+      ).rejects.toBeInstanceOf(BadRequestException);
     });
 
     it('should require a valid thumbnail asset id', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getByIds.mockResolvedValue([albumStub.oneAsset]);
       albumMock.update.mockResolvedValue(albumStub.oneAsset);
       albumMock.hasAsset.mockResolvedValue(false);
@@ -235,6 +239,8 @@ describe(AlbumService.name, () => {
     });
 
     it('should allow the owner to update the album', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
+
       albumMock.getByIds.mockResolvedValue([albumStub.oneAsset]);
       albumMock.update.mockResolvedValue(albumStub.oneAsset);
 
@@ -256,6 +262,7 @@ describe(AlbumService.name, () => {
 
   describe('delete', () => {
     it('should throw an error for an album not found', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getByIds.mockResolvedValue([]);
 
       await expect(sut.delete(authStub.admin, albumStub.sharedWithAdmin.id)).rejects.toBeInstanceOf(
@@ -266,14 +273,18 @@ describe(AlbumService.name, () => {
     });
 
     it('should not let a shared user delete the album', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(false);
       albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]);
 
-      await expect(sut.delete(authStub.admin, albumStub.sharedWithAdmin.id)).rejects.toBeInstanceOf(ForbiddenException);
+      await expect(sut.delete(authStub.admin, albumStub.sharedWithAdmin.id)).rejects.toBeInstanceOf(
+        BadRequestException,
+      );
 
       expect(albumMock.delete).not.toHaveBeenCalled();
     });
 
     it('should let the owner delete an album', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getByIds.mockResolvedValue([albumStub.empty]);
 
       await sut.delete(authStub.admin, albumStub.empty.id);
@@ -284,23 +295,16 @@ describe(AlbumService.name, () => {
   });
 
   describe('addUsers', () => {
-    it('should require a valid album id', async () => {
+    it('should throw an error if the auth user is not the owner', async () => {
-      albumMock.getByIds.mockResolvedValue([]);
+      accessMock.album.hasOwnerAccess.mockResolvedValue(false);
-      await expect(sut.addUsers(authStub.admin, 'album-1', { sharedUserIds: ['user-1'] })).rejects.toBeInstanceOf(
-        BadRequestException,
-      );
-      expect(albumMock.update).not.toHaveBeenCalled();
-    });
-
-    it('should require the user to be the owner', async () => {
-      albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]);
       await expect(
         sut.addUsers(authStub.admin, albumStub.sharedWithAdmin.id, { sharedUserIds: ['user-1'] }),
-      ).rejects.toBeInstanceOf(ForbiddenException);
+      ).rejects.toBeInstanceOf(BadRequestException);
       expect(albumMock.update).not.toHaveBeenCalled();
     });
 
     it('should throw an error if the userId is already added', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]);
       await expect(
         sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { sharedUserIds: [authStub.admin.id] }),
@@ -309,6 +313,7 @@ describe(AlbumService.name, () => {
     });
 
     it('should throw an error if the userId does not exist', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getByIds.mockResolvedValue([albumStub.sharedWithAdmin]);
       userMock.get.mockResolvedValue(null);
       await expect(
@@ -318,6 +323,7 @@ describe(AlbumService.name, () => {
     });
 
     it('should add valid shared users', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getByIds.mockResolvedValue([_.cloneDeep(albumStub.sharedWithAdmin)]);
       albumMock.update.mockResolvedValue(albumStub.sharedWithAdmin);
       userMock.get.mockResolvedValue(userEntityStub.user2);
@@ -332,12 +338,14 @@ describe(AlbumService.name, () => {
 
   describe('removeUser', () => {
     it('should require a valid album id', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getByIds.mockResolvedValue([]);
       await expect(sut.removeUser(authStub.admin, 'album-1', 'user-1')).rejects.toBeInstanceOf(BadRequestException);
       expect(albumMock.update).not.toHaveBeenCalled();
     });
 
     it('should remove a shared user from an owned album', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       albumMock.getByIds.mockResolvedValue([albumStub.sharedWithUser]);
 
       await expect(
@@ -353,13 +361,15 @@ describe(AlbumService.name, () => {
     });
 
     it('should prevent removing a shared user from a not-owned album (shared with auth user)', async () => {
+      accessMock.album.hasOwnerAccess.mockResolvedValue(false);
       albumMock.getByIds.mockResolvedValue([albumStub.sharedWithMultiple]);
 
       await expect(
         sut.removeUser(authStub.user1, albumStub.sharedWithMultiple.id, authStub.user2.id),
-      ).rejects.toBeInstanceOf(ForbiddenException);
+      ).rejects.toBeInstanceOf(BadRequestException);
 
       expect(albumMock.update).not.toHaveBeenCalled();
+      expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.user1.id, albumStub.sharedWithMultiple.id);
     });
 
     it('should allow a shared user to remove themselves', async () => {

server/src/domain/album/album.service.ts

@@ -1,7 +1,8 @@
 import { AlbumEntity, AssetEntity, UserEntity } from '@app/infra/entities';
-import { BadRequestException, ForbiddenException, Inject, Injectable } from '@nestjs/common';
+import { BadRequestException, Inject, Injectable } from '@nestjs/common';
 import { IAssetRepository, mapAsset } from '../asset';
 import { AuthUserDto } from '../auth';
+import { AccessCore, IAccessRepository, Permission } from '../index';
 import { IJobRepository, JobName } from '../job';
 import { IUserRepository } from '../user';
 import { AlbumCountResponseDto, AlbumResponseDto, mapAlbum } from './album-response.dto';
@@ -10,12 +11,16 @@ import { AddUsersDto, CreateAlbumDto, GetAlbumsDto, UpdateAlbumDto } from './dto
 
 @Injectable()
 export class AlbumService {
+  private access: AccessCore;
   constructor(
+    @Inject(IAccessRepository) accessRepository: IAccessRepository,
     @Inject(IAlbumRepository) private albumRepository: IAlbumRepository,
     @Inject(IAssetRepository) private assetRepository: IAssetRepository,
     @Inject(IJobRepository) private jobRepository: IJobRepository,
     @Inject(IUserRepository) private userRepository: IUserRepository,
-  ) {}
+  ) {
+    this.access = new AccessCore(accessRepository);
+  }
 
   async getCount(authUser: AuthUserDto): Promise<AlbumCountResponseDto> {
     const [owned, shared, notShared] = await Promise.all([
@@ -100,8 +105,9 @@ export class AlbumService {
   }
 
   async update(authUser: AuthUserDto, id: string, dto: UpdateAlbumDto): Promise<AlbumResponseDto> {
+    await this.access.requirePermission(authUser, Permission.ALBUM_UPDATE, id);
+
     const album = await this.get(id);
-    this.assertOwner(authUser, album);
 
     if (dto.albumThumbnailAssetId) {
       const valid = await this.albumRepository.hasAsset(id, dto.albumThumbnailAssetId);
@@ -122,22 +128,21 @@ export class AlbumService {
   }
 
   async delete(authUser: AuthUserDto, id: string): Promise<void> {
+    await this.access.requirePermission(authUser, Permission.ALBUM_DELETE, id);
+
     const [album] = await this.albumRepository.getByIds([id]);
     if (!album) {
       throw new BadRequestException('Album not found');
     }
 
-    if (album.ownerId !== authUser.id) {
-      throw new ForbiddenException('Album not owned by user');
-    }
-
     await this.albumRepository.delete(album);
     await this.jobRepository.queue({ name: JobName.SEARCH_REMOVE_ALBUM, data: { ids: [id] } });
   }
 
   async addUsers(authUser: AuthUserDto, id: string, dto: AddUsersDto) {
+    await this.access.requirePermission(authUser, Permission.ALBUM_SHARE, id);
+
     const album = await this.get(id);
-    this.assertOwner(authUser, album);
 
     for (const userId of dto.sharedUserIds) {
       const exists = album.sharedUsers.find((user) => user.id === userId);
@@ -180,7 +185,7 @@ export class AlbumService {
 
     // non-admin can remove themselves
     if (authUser.id !== userId) {
-      this.assertOwner(authUser, album);
+      await this.access.requirePermission(authUser, Permission.ALBUM_SHARE, id);
     }
 
     await this.albumRepository.update({
@@ -197,10 +202,4 @@ export class AlbumService {
     }
     return album;
   }
-
-  private assertOwner(authUser: AuthUserDto, album: AlbumEntity) {
-    if (album.ownerId !== authUser.id) {
-      throw new ForbiddenException('Album not owned by user');
-    }
-  }
 }

server/src/domain/shared-link/shared-link.service.spec.ts

@@ -3,6 +3,7 @@ import {
   albumStub,
   assetEntityStub,
   authStub,
+  IAccessRepositoryMock,
   newAccessRepositoryMock,
   newCryptoRepositoryMock,
   newSharedLinkRepositoryMock,
@@ -12,13 +13,13 @@ import {
 import { when } from 'jest-when';
 import _ from 'lodash';
 import { SharedLinkType } from '../../infra/entities/shared-link.entity';
-import { AssetIdErrorReason, IAccessRepository, ICryptoRepository } from '../index';
+import { AssetIdErrorReason, ICryptoRepository } from '../index';
 import { ISharedLinkRepository } from './shared-link.repository';
 import { SharedLinkService } from './shared-link.service';
 
 describe(SharedLinkService.name, () => {
   let sut: SharedLinkService;
-  let accessMock: jest.Mocked<IAccessRepository>;
+  let accessMock: IAccessRepositoryMock;
   let cryptoMock: jest.Mocked<ICryptoRepository>;
   let shareMock: jest.Mocked<ISharedLinkRepository>;
 
@@ -89,7 +90,7 @@ describe(SharedLinkService.name, () => {
     });
 
     it('should not allow non-owners to create album shared links', async () => {
-      accessMock.hasAlbumOwnerAccess.mockResolvedValue(false);
+      accessMock.album.hasOwnerAccess.mockResolvedValue(false);
       await expect(
         sut.create(authStub.admin, { type: SharedLinkType.ALBUM, assetIds: [], albumId: 'album-1' }),
       ).rejects.toBeInstanceOf(BadRequestException);
@@ -102,19 +103,19 @@ describe(SharedLinkService.name, () => {
     });
 
     it('should require asset ownership to make an individual shared link', async () => {
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(false);
       await expect(
         sut.create(authStub.admin, { type: SharedLinkType.INDIVIDUAL, assetIds: ['asset-1'] }),
       ).rejects.toBeInstanceOf(BadRequestException);
     });
 
     it('should create an album shared link', async () => {
-      accessMock.hasAlbumOwnerAccess.mockResolvedValue(true);
+      accessMock.album.hasOwnerAccess.mockResolvedValue(true);
       shareMock.create.mockResolvedValue(sharedLinkStub.valid);
 
       await sut.create(authStub.admin, { type: SharedLinkType.ALBUM, albumId: albumStub.oneAsset.id });
 
-      expect(accessMock.hasAlbumOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, albumStub.oneAsset.id);
+      expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, albumStub.oneAsset.id);
       expect(shareMock.create).toHaveBeenCalledWith({
         type: SharedLinkType.ALBUM,
         userId: authStub.admin.id,
@@ -130,7 +131,7 @@ describe(SharedLinkService.name, () => {
     });
 
     it('should create an individual shared link', async () => {
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
       shareMock.create.mockResolvedValue(sharedLinkStub.individual);
 
       await sut.create(authStub.admin, {
@@ -141,7 +142,7 @@ describe(SharedLinkService.name, () => {
         allowUpload: true,
       });
 
-      expect(accessMock.hasOwnerAssetAccess).toHaveBeenCalledWith(authStub.admin.id, assetEntityStub.image.id);
+      expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, assetEntityStub.image.id);
       expect(shareMock.create).toHaveBeenCalledWith({
         type: SharedLinkType.INDIVIDUAL,
         userId: authStub.admin.id,
@@ -206,8 +207,8 @@ describe(SharedLinkService.name, () => {
       shareMock.get.mockResolvedValue(_.cloneDeep(sharedLinkStub.individual));
       shareMock.create.mockResolvedValue(sharedLinkStub.individual);
 
-      when(accessMock.hasOwnerAssetAccess).calledWith(authStub.admin.id, 'asset-2').mockResolvedValue(false);
+      when(accessMock.asset.hasOwnerAccess).calledWith(authStub.admin.id, 'asset-2').mockResolvedValue(false);
-      when(accessMock.hasOwnerAssetAccess).calledWith(authStub.admin.id, 'asset-3').mockResolvedValue(true);
+      when(accessMock.asset.hasOwnerAccess).calledWith(authStub.admin.id, 'asset-3').mockResolvedValue(true);
 
       await expect(
         sut.addAssets(authStub.admin, 'link-1', { assetIds: [assetEntityStub.image.id, 'asset-2', 'asset-3'] }),
@@ -217,7 +218,7 @@ describe(SharedLinkService.name, () => {
         { assetId: 'asset-3', success: true },
       ]);
 
-      expect(accessMock.hasOwnerAssetAccess).toHaveBeenCalledTimes(2);
+      expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledTimes(2);
       expect(shareMock.update).toHaveBeenCalledWith({
         ...sharedLinkStub.individual,
         assets: [assetEntityStub.image, { id: 'asset-3' }],

server/src/domain/shared-link/shared-link.service.ts

@@ -1,6 +1,6 @@
 import { AssetEntity, SharedLinkEntity, SharedLinkType } from '@app/infra/entities';
 import { BadRequestException, ForbiddenException, Inject, Injectable } from '@nestjs/common';
-import { IAccessRepository } from '../access';
+import { AccessCore, IAccessRepository, Permission } from '../access';
 import { AssetIdErrorReason, AssetIdsDto, AssetIdsResponseDto } from '../asset';
 import { AuthUserDto } from '../auth';
 import { ICryptoRepository } from '../crypto';
@@ -10,11 +10,15 @@ import { ISharedLinkRepository } from './shared-link.repository';
 
 @Injectable()
 export class SharedLinkService {
+  private access: AccessCore;
+
   constructor(
-    @Inject(IAccessRepository) private accessRepository: IAccessRepository,
+    @Inject(IAccessRepository) accessRepository: IAccessRepository,
     @Inject(ICryptoRepository) private cryptoRepository: ICryptoRepository,
     @Inject(ISharedLinkRepository) private repository: ISharedLinkRepository,
-  ) {}
+  ) {
+    this.access = new AccessCore(accessRepository);
+  }
 
   getAll(authUser: AuthUserDto): Promise<SharedLinkResponseDto[]> {
     return this.repository.getAll(authUser.id).then((links) => links.map(mapSharedLink));
@@ -43,12 +47,7 @@ export class SharedLinkService {
         if (!dto.albumId) {
           throw new BadRequestException('Invalid albumId');
         }
-
+        await this.access.requirePermission(authUser, Permission.ALBUM_SHARE, dto.albumId);
-        const isAlbumOwner = await this.accessRepository.hasAlbumOwnerAccess(authUser.id, dto.albumId);
-        if (!isAlbumOwner) {
-          throw new BadRequestException('Invalid albumId');
-        }
-
         break;
 
       case SharedLinkType.INDIVIDUAL:
@@ -56,12 +55,7 @@ export class SharedLinkService {
           throw new BadRequestException('Invalid assetIds');
         }
 
-        for (const assetId of dto.assetIds) {
+        await this.access.requirePermission(authUser, Permission.ASSET_SHARE, dto.assetIds);
-          const hasAccess = await this.accessRepository.hasOwnerAssetAccess(authUser.id, assetId);
-          if (!hasAccess) {
-            throw new BadRequestException(`No access to assetId: ${assetId}`);
-          }
-        }
 
         break;
     }
@@ -124,7 +118,7 @@ export class SharedLinkService {
         continue;
       }
 
-      const hasAccess = await this.accessRepository.hasOwnerAssetAccess(authUser.id, assetId);
+      const hasAccess = await this.access.hasPermission(authUser, Permission.ASSET_SHARE, assetId);
       if (!hasAccess) {
         results.push({ assetId, success: false, error: AssetIdErrorReason.NO_PERMISSION });
         continue;

server/src/immich/api-v1/asset/asset.service.spec.ts

@@ -1,10 +1,11 @@
-import { IAccessRepository, ICryptoRepository, IJobRepository, IStorageRepository, JobName } from '@app/domain';
+import { ICryptoRepository, IJobRepository, IStorageRepository, JobName } from '@app/domain';
 import { AssetEntity, AssetType, ExifEntity } from '@app/infra/entities';
-import { ForbiddenException } from '@nestjs/common';
+import { BadRequestException } from '@nestjs/common';
 import {
   assetEntityStub,
   authStub,
   fileStub,
+  IAccessRepositoryMock,
   newAccessRepositoryMock,
   newCryptoRepositoryMock,
   newJobRepositoryMock,
@@ -120,7 +121,7 @@ const _getArchivedAssetsCountByUserId = (): AssetCountByUserIdResponseDto => {
 describe('AssetService', () => {
   let sut: AssetService;
   let a: Repository<AssetEntity>; // TO BE DELETED AFTER FINISHED REFACTORING
-  let accessMock: jest.Mocked<IAccessRepository>;
+  let accessMock: IAccessRepositoryMock;
   let assetRepositoryMock: jest.Mocked<IAssetRepository>;
   let cryptoMock: jest.Mocked<ICryptoRepository>;
   let downloadServiceMock: jest.Mocked<Partial<DownloadService>>;
@@ -293,7 +294,7 @@ describe('AssetService', () => {
   describe('deleteAll', () => {
     it('should return failed status when an asset is missing', async () => {
       assetRepositoryMock.get.mockResolvedValue(null);
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
 
       await expect(sut.deleteAll(authStub.user1, { ids: ['asset1'] })).resolves.toEqual([
         { id: 'asset1', status: 'FAILED' },
@@ -305,7 +306,7 @@ describe('AssetService', () => {
     it('should return failed status a delete fails', async () => {
       assetRepositoryMock.get.mockResolvedValue({ id: 'asset1' } as AssetEntity);
       assetRepositoryMock.remove.mockRejectedValue('delete failed');
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
 
       await expect(sut.deleteAll(authStub.user1, { ids: ['asset1'] })).resolves.toEqual([
         { id: 'asset1', status: 'FAILED' },
@@ -315,7 +316,7 @@ describe('AssetService', () => {
     });
 
     it('should delete a live photo', async () => {
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
 
       await expect(sut.deleteAll(authStub.user1, { ids: [assetEntityStub.livePhotoStillAsset.id] })).resolves.toEqual([
         { id: assetEntityStub.livePhotoStillAsset.id, status: 'SUCCESS' },
@@ -364,7 +365,7 @@ describe('AssetService', () => {
         .calledWith(asset2.id)
         .mockResolvedValue(asset2 as AssetEntity);
 
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
 
       await expect(sut.deleteAll(authStub.user1, { ids: ['asset1', 'asset2'] })).resolves.toEqual([
         { id: 'asset1', status: 'SUCCESS' },
@@ -409,7 +410,7 @@ describe('AssetService', () => {
 
   describe('downloadFile', () => {
     it('should download a single file', async () => {
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(true);
       assetRepositoryMock.get.mockResolvedValue(_getAsset_1());
 
       await sut.downloadFile(authStub.admin, 'id_1');
@@ -485,56 +486,56 @@ describe('AssetService', () => {
 
   describe('getAssetById', () => {
     it('should allow owner access', async () => {
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasOwnerAccess.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);
+      expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, assetEntityStub.image.id);
     });
 
     it('should allow shared link access', async () => {
-      accessMock.hasSharedLinkAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasSharedLinkAccess.mockResolvedValue(true);
       assetRepositoryMock.getById.mockResolvedValue(assetEntityStub.image);
       await sut.getAssetById(authStub.adminSharedLink, assetEntityStub.image.id);
-      expect(accessMock.hasSharedLinkAssetAccess).toHaveBeenCalledWith(
+      expect(accessMock.asset.hasSharedLinkAccess).toHaveBeenCalledWith(
         authStub.adminSharedLink.sharedLinkId,
         assetEntityStub.image.id,
       );
     });
 
     it('should allow partner sharing access', async () => {
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(false);
-      accessMock.hasPartnerAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasPartnerAccess.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);
+      expect(accessMock.asset.hasPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, assetEntityStub.image.id);
     });
 
     it('should allow shared album access', async () => {
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(false);
-      accessMock.hasPartnerAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasPartnerAccess.mockResolvedValue(false);
-      accessMock.hasAlbumAssetAccess.mockResolvedValue(true);
+      accessMock.asset.hasAlbumAccess.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);
+      expect(accessMock.asset.hasAlbumAccess).toHaveBeenCalledWith(authStub.admin.id, assetEntityStub.image.id);
     });
 
     it('should throw an error for no access', async () => {
-      accessMock.hasOwnerAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasOwnerAccess.mockResolvedValue(false);
-      accessMock.hasPartnerAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasPartnerAccess.mockResolvedValue(false);
-      accessMock.hasSharedLinkAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasSharedLinkAccess.mockResolvedValue(false);
-      accessMock.hasAlbumAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasAlbumAccess.mockResolvedValue(false);
       await expect(sut.getAssetById(authStub.admin, assetEntityStub.image.id)).rejects.toBeInstanceOf(
-        ForbiddenException,
+        BadRequestException,
       );
       expect(assetRepositoryMock.getById).not.toHaveBeenCalled();
     });
 
     it('should throw an error for an invalid shared link', async () => {
-      accessMock.hasSharedLinkAssetAccess.mockResolvedValue(false);
+      accessMock.asset.hasSharedLinkAccess.mockResolvedValue(false);
       await expect(sut.getAssetById(authStub.adminSharedLink, assetEntityStub.image.id)).rejects.toBeInstanceOf(
-        ForbiddenException,
+        BadRequestException,
       );
-      expect(accessMock.hasOwnerAssetAccess).not.toHaveBeenCalled();
+      expect(accessMock.asset.hasOwnerAccess).not.toHaveBeenCalled();
       expect(assetRepositoryMock.getById).not.toHaveBeenCalled();
     });
   });

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

@@ -1,4 +1,5 @@
 import {
+  AccessCore,
   AssetResponseDto,
   AuthUserDto,
   getLivePhotoMotionFilename,
@@ -12,11 +13,11 @@ import {
   JobName,
   mapAsset,
   mapAssetWithoutExif,
+  Permission,
 } from '@app/domain';
 import { AssetEntity, AssetType } from '@app/infra/entities';
 import {
   BadRequestException,
-  ForbiddenException,
   Inject,
   Injectable,
   InternalServerErrorException,
@@ -79,9 +80,10 @@ interface ServableFile {
 export class AssetService {
   readonly logger = new Logger(AssetService.name);
   private assetCore: AssetCore;
+  private access: AccessCore;
 
   constructor(
-    @Inject(IAccessRepository) private accessRepository: IAccessRepository,
+    @Inject(IAccessRepository) accessRepository: IAccessRepository,
     @Inject(IAssetRepository) private _assetRepository: IAssetRepository,
     @InjectRepository(AssetEntity) private assetRepository: Repository<AssetEntity>,
     @Inject(ICryptoRepository) private cryptoRepository: ICryptoRepository,
@@ -90,6 +92,7 @@ export class AssetService {
     @Inject(IStorageRepository) private storageRepository: IStorageRepository,
   ) {
     this.assetCore = new AssetCore(_assetRepository, jobRepository);
+    this.access = new AccessCore(accessRepository);
   }
 
   public async uploadFile(
@@ -208,32 +211,21 @@ export class AssetService {
   }
 
   public async getAllAssets(authUser: AuthUserDto, dto: AssetSearchDto): Promise<AssetResponseDto[]> {
-    if (dto.userId && dto.userId !== authUser.id) {
+    const userId = dto.userId || authUser.id;
-      await this.checkUserAccess(authUser, dto.userId);
+    await this.access.requirePermission(authUser, Permission.LIBRARY_READ, userId);
-    }
+    const assets = await this._assetRepository.getAllByUserId(userId, dto);
-    const assets = await this._assetRepository.getAllByUserId(dto.userId || authUser.id, dto);
-
     return assets.map((asset) => mapAsset(asset));
   }
 
-  public async getAssetByTimeBucket(
+  public async getAssetByTimeBucket(authUser: AuthUserDto, dto: GetAssetByTimeBucketDto): Promise<AssetResponseDto[]> {
-    authUser: AuthUserDto,
+    const userId = dto.userId || authUser.id;
-    getAssetByTimeBucketDto: GetAssetByTimeBucketDto,
+    await this.access.requirePermission(authUser, Permission.LIBRARY_READ, userId);
-  ): Promise<AssetResponseDto[]> {
+    const assets = await this._assetRepository.getAssetByTimeBucket(userId, dto);
-    if (getAssetByTimeBucketDto.userId) {
-      await this.checkUserAccess(authUser, getAssetByTimeBucketDto.userId);
-    }
-
-    const assets = await this._assetRepository.getAssetByTimeBucket(
-      getAssetByTimeBucketDto.userId || authUser.id,
-      getAssetByTimeBucketDto,
-    );
-
     return assets.map((asset) => mapAsset(asset));
   }
 
   public async getAssetById(authUser: AuthUserDto, assetId: string): Promise<AssetResponseDto> {
-    await this.checkAssetsAccess(authUser, [assetId]);
+    await this.access.requirePermission(authUser, Permission.ASSET_READ, assetId);
 
     const allowExif = this.getExifPermission(authUser);
     const asset = await this._assetRepository.getById(assetId);
@@ -246,7 +238,7 @@ export class AssetService {
   }
 
   public async updateAsset(authUser: AuthUserDto, assetId: string, dto: UpdateAssetDto): Promise<AssetResponseDto> {
-    await this.checkAssetsAccess(authUser, [assetId], true);
+    await this.access.requirePermission(authUser, Permission.ASSET_UPDATE, assetId);
 
     const asset = await this._assetRepository.getById(assetId);
     if (!asset) {
@@ -261,15 +253,15 @@ export class AssetService {
   }
 
   public async downloadLibrary(authUser: AuthUserDto, dto: DownloadDto) {
-    this.checkDownloadAccess(authUser);
+    await this.access.requirePermission(authUser, Permission.LIBRARY_DOWNLOAD, authUser.id);
+
     const assets = await this._assetRepository.getAllByUserId(authUser.id, dto);
 
     return this.downloadService.downloadArchive(dto.name || `library`, assets);
   }
 
   public async downloadFiles(authUser: AuthUserDto, dto: DownloadFilesDto) {
-    this.checkDownloadAccess(authUser);
+    await this.access.requirePermission(authUser, Permission.ASSET_DOWNLOAD, dto.assetIds);
-    await this.checkAssetsAccess(authUser, [...dto.assetIds]);
 
     const assetToDownload = [];
 
@@ -289,8 +281,7 @@ export class AssetService {
   }
 
   public async downloadFile(authUser: AuthUserDto, assetId: string): Promise<ImmichReadStream> {
-    this.checkDownloadAccess(authUser);
+    await this.access.requirePermission(authUser, Permission.ASSET_DOWNLOAD, assetId);
-    await this.checkAssetsAccess(authUser, [assetId]);
 
     try {
       const asset = await this._assetRepository.get(assetId);
@@ -312,7 +303,8 @@ export class AssetService {
     res: Res,
     headers: Record<string, string>,
   ) {
-    await this.checkAssetsAccess(authUser, [assetId]);
+    await this.access.requirePermission(authUser, Permission.ASSET_VIEW, assetId);
+
     const asset = await this._assetRepository.get(assetId);
     if (!asset) {
       throw new NotFoundException('Asset not found');
@@ -338,7 +330,8 @@ export class AssetService {
     res: Res,
     headers: Record<string, string>,
   ) {
-    await this.checkAssetsAccess(authUser, [assetId]);
+    // this is not quite right as sometimes this returns the original still
+    await this.access.requirePermission(authUser, Permission.ASSET_VIEW, assetId);
 
     const allowOriginalFile = !!(!authUser.isPublicUser || authUser.isAllowDownload);
 
@@ -421,13 +414,17 @@ export class AssetService {
   }
 
   public async deleteAll(authUser: AuthUserDto, dto: DeleteAssetDto): Promise<DeleteAssetResponseDto[]> {
-    await this.checkAssetsAccess(authUser, dto.ids, true);
-
     const deleteQueue: Array<string | null> = [];
     const result: DeleteAssetResponseDto[] = [];
 
     const ids = dto.ids.slice();
     for (const id of ids) {
+      const hasAccess = await this.access.hasPermission(authUser, Permission.ASSET_DELETE, id);
+      if (!hasAccess) {
+        result.push({ id, status: DeleteAssetStatusEnum.FAILED });
+        continue;
+      }
+
       const asset = await this._assetRepository.get(id);
       if (!asset) {
         result.push({ id, status: DeleteAssetStatusEnum.FAILED });
@@ -605,17 +602,11 @@ export class AssetService {
 
   async getAssetCountByTimeBucket(
     authUser: AuthUserDto,
-    getAssetCountByTimeBucketDto: GetAssetCountByTimeBucketDto,
+    dto: GetAssetCountByTimeBucketDto,
   ): Promise<AssetCountByTimeBucketResponseDto> {
-    if (getAssetCountByTimeBucketDto.userId !== undefined) {
+    const userId = dto.userId || authUser.id;
-      await this.checkUserAccess(authUser, getAssetCountByTimeBucketDto.userId);
+    await this.access.requirePermission(authUser, Permission.LIBRARY_READ, userId);
-    }
+    const result = await this._assetRepository.getAssetCountByTimeBucket(userId, dto);
-
-    const result = await this._assetRepository.getAssetCountByTimeBucket(
-      getAssetCountByTimeBucketDto.userId || authUser.id,
-      getAssetCountByTimeBucketDto,
-    );
-
     return mapAssetCountByTimeBucket(result);
   }
 
@@ -627,56 +618,6 @@ export class AssetService {
     return this._assetRepository.getArchivedAssetCountByUserId(authUser.id);
   }
 
-  private async checkAssetsAccess(authUser: AuthUserDto, assetIds: string[], mustBeOwner = false) {
-    const sharedLinkId = authUser.sharedLinkId;
-
-    for (const assetId of assetIds) {
-      if (sharedLinkId) {
-        const canAccess = await this.accessRepository.hasSharedLinkAssetAccess(sharedLinkId, assetId);
-        if (canAccess) {
-          continue;
-        }
-
-        throw new ForbiddenException();
-      }
-
-      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();
-    }
-  }
-
-  private async checkUserAccess(authUser: AuthUserDto, userId: string) {
-    // Check if userId shares assets with authUser
-    const canAccess = await this.accessRepository.hasPartnerAccess(authUser.id, userId);
-    if (!canAccess) {
-      throw new ForbiddenException();
-    }
-  }
-
-  private checkDownloadAccess(authUser: AuthUserDto) {
-    if (authUser.isPublicUser && !authUser.isAllowDownload) {
-      throw new ForbiddenException();
-    }
-  }
-
   getExifPermission(authUser: AuthUserDto) {
     return !authUser.isPublicUser || authUser.isShowExif;
   }

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

@@ -11,16 +11,19 @@ export class AccessRepository implements IAccessRepository {
     @InjectRepository(SharedLinkEntity) private sharedLinkRepository: Repository<SharedLinkEntity>,
   ) {}
 
-  hasPartnerAccess(userId: string, partnerId: string): Promise<boolean> {
+  library = {
+    hasPartnerAccess: (userId: string, partnerId: string): Promise<boolean> => {
       return this.partnerRepository.exist({
         where: {
           sharedWithId: userId,
           sharedById: partnerId,
         },
       });
-  }
+    },
+  };
 
-  hasAlbumAssetAccess(userId: string, assetId: string): Promise<boolean> {
+  asset = {
+    hasAlbumAccess: (userId: string, assetId: string): Promise<boolean> => {
       return this.albumRepository.exist({
         where: [
           {
@@ -39,18 +42,18 @@ export class AccessRepository implements IAccessRepository {
           },
         ],
       });
-  }
+    },
 
-  hasOwnerAssetAccess(userId: string, assetId: string): Promise<boolean> {
+    hasOwnerAccess: (userId: string, assetId: string): Promise<boolean> => {
       return this.assetRepository.exist({
         where: {
           id: assetId,
           ownerId: userId,
         },
       });
-  }
+    },
 
-  hasPartnerAssetAccess(userId: string, assetId: string): Promise<boolean> {
+    hasPartnerAccess: (userId: string, assetId: string): Promise<boolean> => {
       return this.partnerRepository.exist({
         where: {
           sharedWith: {
@@ -69,9 +72,9 @@ export class AccessRepository implements IAccessRepository {
           },
         },
       });
-  }
+    },
 
-  async hasSharedLinkAssetAccess(sharedLinkId: string, assetId: string): Promise<boolean> {
+    hasSharedLinkAccess: async (sharedLinkId: string, assetId: string): Promise<boolean> => {
       return (
         // album asset
         (await this.sharedLinkRepository.exist({
@@ -94,14 +97,35 @@ export class AccessRepository implements IAccessRepository {
           },
         }))
       );
-  }
+    },
+  };
 
-  hasAlbumOwnerAccess(userId: string, albumId: string): Promise<boolean> {
+  album = {
+    hasOwnerAccess: (userId: string, albumId: string): Promise<boolean> => {
       return this.albumRepository.exist({
         where: {
           id: albumId,
           ownerId: userId,
         },
       });
-  }
+    },
+
+    hasSharedAlbumAccess: (userId: string, albumId: string): Promise<boolean> => {
+      return this.albumRepository.exist({
+        where: {
+          id: albumId,
+          ownerId: userId,
+        },
+      });
+    },
+
+    hasSharedLinkAccess: (sharedLinkId: string, albumId: string): Promise<boolean> => {
+      return this.sharedLinkRepository.exist({
+        where: {
+          id: sharedLinkId,
+          albumId,
+        },
+      });
+    },
+  };
 }

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

@@ -1,14 +1,28 @@
 import { IAccessRepository } from '@app/domain';
 
-export const newAccessRepositoryMock = (): jest.Mocked<IAccessRepository> => {
+export type IAccessRepositoryMock = {
+  asset: jest.Mocked<IAccessRepository['asset']>;
+  album: jest.Mocked<IAccessRepository['album']>;
+  library: jest.Mocked<IAccessRepository['library']>;
+};
+
+export const newAccessRepositoryMock = (): IAccessRepositoryMock => {
   return {
+    asset: {
+      hasOwnerAccess: jest.fn(),
+      hasAlbumAccess: jest.fn(),
       hasPartnerAccess: jest.fn(),
+      hasSharedLinkAccess: jest.fn(),
+    },
 
-    hasAlbumAssetAccess: jest.fn(),
+    album: {
-    hasOwnerAssetAccess: jest.fn(),
+      hasOwnerAccess: jest.fn(),
-    hasPartnerAssetAccess: jest.fn(),
+      hasSharedAlbumAccess: jest.fn(),
-    hasSharedLinkAssetAccess: jest.fn(),
+      hasSharedLinkAccess: jest.fn(),
+    },
 
-    hasAlbumOwnerAccess: jest.fn(),
+    library: {
+      hasPartnerAccess: jest.fn(),
+    },
   };
 };