From e98398cab8ffe140e912884ce13a1a1e8ff47e31 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 28 Jun 2023 09:56:24 -0400 Subject: [PATCH] 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 --- server/src/domain/access/access.core.ts | 134 +++++++++++++ server/src/domain/access/access.repository.ts | 20 +- server/src/domain/access/index.ts | 1 + server/src/domain/album/album.service.spec.ts | 46 +++-- server/src/domain/album/album.service.ts | 29 ++- .../shared-link/shared-link.service.spec.ts | 23 ++- .../domain/shared-link/shared-link.service.ts | 26 +-- .../immich/api-v1/asset/asset.service.spec.ts | 55 ++--- .../src/immich/api-v1/asset/asset.service.ts | 123 +++--------- .../infra/repositories/access.repository.ts | 188 ++++++++++-------- .../repositories/access.repository.mock.ts | 28 ++- 11 files changed, 400 insertions(+), 273 deletions(-) create mode 100644 server/src/domain/access/access.core.ts diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts new file mode 100644 index 0000000000..f730e1be9c --- /dev/null +++ b/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; + } +} diff --git a/server/src/domain/access/access.repository.ts b/server/src/domain/access/access.repository.ts index f9949e46c8..3d99fecb07 100644 --- a/server/src/domain/access/access.repository.ts +++ b/server/src/domain/access/access.repository.ts @@ -1,12 +1,20 @@ export const IAccessRepository = 'IAccessRepository'; export interface IAccessRepository { - hasPartnerAccess(userId: string, partnerId: string): Promise; + asset: { + hasOwnerAccess(userId: string, assetId: string): Promise; + hasAlbumAccess(userId: string, assetId: string): Promise; + hasPartnerAccess(userId: string, assetId: string): Promise; + hasSharedLinkAccess(sharedLinkId: string, assetId: 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; + album: { + hasOwnerAccess(userId: string, albumId: string): Promise; + hasSharedAlbumAccess(userId: string, albumId: string): Promise; + hasSharedLinkAccess(sharedLinkId: string, albumId: string): Promise; + }; - hasAlbumOwnerAccess(userId: string, albumId: string): Promise; + library: { + hasPartnerAccess(userId: string, partnerId: string): Promise; + }; } diff --git a/server/src/domain/access/index.ts b/server/src/domain/access/index.ts index d864cd8dae..24f84d67e0 100644 --- a/server/src/domain/access/index.ts +++ b/server/src/domain/access/index.ts @@ -1 +1,2 @@ +export * from './access.core'; export * from './access.repository'; diff --git a/server/src/domain/album/album.service.spec.ts b/server/src/domain/album/album.service.spec.ts index 0b4f42a43a..7d2304d443 100644 --- a/server/src/domain/album/album.service.spec.ts +++ b/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; let assetMock: jest.Mocked; let jobMock: jest.Mocked; let userMock: jest.Mocked; 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 () => { - albumMock.getByIds.mockResolvedValue([]); - 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]); + it('should throw an error if the auth user is not the owner', async () => { + accessMock.album.hasOwnerAccess.mockResolvedValue(false); 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 () => { diff --git a/server/src/domain/album/album.service.ts b/server/src/domain/album/album.service.ts index ba350db2ca..945516e2a7 100644 --- a/server/src/domain/album/album.service.ts +++ b/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 { const [owned, shared, notShared] = await Promise.all([ @@ -100,8 +105,9 @@ export class AlbumService { } async update(authUser: AuthUserDto, id: string, dto: UpdateAlbumDto): Promise { + 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 { + 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'); - } - } } diff --git a/server/src/domain/shared-link/shared-link.service.spec.ts b/server/src/domain/shared-link/shared-link.service.spec.ts index 74fe479bfa..507305e8d7 100644 --- a/server/src/domain/shared-link/shared-link.service.spec.ts +++ b/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; + let accessMock: IAccessRepositoryMock; let cryptoMock: jest.Mocked; let shareMock: jest.Mocked; @@ -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.hasOwnerAssetAccess).calledWith(authStub.admin.id, 'asset-3').mockResolvedValue(true); + when(accessMock.asset.hasOwnerAccess).calledWith(authStub.admin.id, 'asset-2').mockResolvedValue(false); + 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' }], diff --git a/server/src/domain/shared-link/shared-link.service.ts b/server/src/domain/shared-link/shared-link.service.ts index 2e30b629e6..ba51589c77 100644 --- a/server/src/domain/shared-link/shared-link.service.ts +++ b/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 { 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'); } - - const isAlbumOwner = await this.accessRepository.hasAlbumOwnerAccess(authUser.id, dto.albumId); - if (!isAlbumOwner) { - throw new BadRequestException('Invalid albumId'); - } - + await this.access.requirePermission(authUser, Permission.ALBUM_SHARE, dto.albumId); break; case SharedLinkType.INDIVIDUAL: @@ -56,12 +55,7 @@ export class SharedLinkService { throw new BadRequestException('Invalid assetIds'); } - for (const assetId of dto.assetIds) { - const hasAccess = await this.accessRepository.hasOwnerAssetAccess(authUser.id, assetId); - if (!hasAccess) { - throw new BadRequestException(`No access to assetId: ${assetId}`); - } - } + await this.access.requirePermission(authUser, Permission.ASSET_SHARE, dto.assetIds); 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; 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 4f51ad23aa..5963aa0a61 100644 --- a/server/src/immich/api-v1/asset/asset.service.spec.ts +++ b/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; // TO BE DELETED AFTER FINISHED REFACTORING - let accessMock: jest.Mocked; + let accessMock: IAccessRepositoryMock; let assetRepositoryMock: jest.Mocked; let cryptoMock: jest.Mocked; let downloadServiceMock: jest.Mocked>; @@ -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.hasPartnerAssetAccess.mockResolvedValue(true); + accessMock.asset.hasOwnerAccess.mockResolvedValue(false); + 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.hasPartnerAssetAccess.mockResolvedValue(false); - accessMock.hasAlbumAssetAccess.mockResolvedValue(true); + accessMock.asset.hasOwnerAccess.mockResolvedValue(false); + accessMock.asset.hasPartnerAccess.mockResolvedValue(false); + 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.hasPartnerAssetAccess.mockResolvedValue(false); - accessMock.hasSharedLinkAssetAccess.mockResolvedValue(false); - accessMock.hasAlbumAssetAccess.mockResolvedValue(false); + accessMock.asset.hasOwnerAccess.mockResolvedValue(false); + accessMock.asset.hasPartnerAccess.mockResolvedValue(false); + accessMock.asset.hasSharedLinkAccess.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(); }); }); diff --git a/server/src/immich/api-v1/asset/asset.service.ts b/server/src/immich/api-v1/asset/asset.service.ts index 671ab74b1e..6ce6c907b3 100644 --- a/server/src/immich/api-v1/asset/asset.service.ts +++ b/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, @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 { - if (dto.userId && dto.userId !== authUser.id) { - await this.checkUserAccess(authUser, dto.userId); - } - const assets = await this._assetRepository.getAllByUserId(dto.userId || authUser.id, dto); - + const userId = dto.userId || authUser.id; + await this.access.requirePermission(authUser, Permission.LIBRARY_READ, userId); + const assets = await this._assetRepository.getAllByUserId(userId, dto); return assets.map((asset) => mapAsset(asset)); } - public async getAssetByTimeBucket( - authUser: AuthUserDto, - getAssetByTimeBucketDto: GetAssetByTimeBucketDto, - ): Promise { - if (getAssetByTimeBucketDto.userId) { - await this.checkUserAccess(authUser, getAssetByTimeBucketDto.userId); - } - - const assets = await this._assetRepository.getAssetByTimeBucket( - getAssetByTimeBucketDto.userId || authUser.id, - getAssetByTimeBucketDto, - ); - + public async getAssetByTimeBucket(authUser: AuthUserDto, dto: GetAssetByTimeBucketDto): Promise { + const userId = dto.userId || authUser.id; + await this.access.requirePermission(authUser, Permission.LIBRARY_READ, userId); + const assets = await this._assetRepository.getAssetByTimeBucket(userId, dto); return assets.map((asset) => mapAsset(asset)); } public async getAssetById(authUser: AuthUserDto, assetId: string): Promise { - 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 { - 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.checkAssetsAccess(authUser, [...dto.assetIds]); + await this.access.requirePermission(authUser, Permission.ASSET_DOWNLOAD, dto.assetIds); const assetToDownload = []; @@ -289,8 +281,7 @@ export class AssetService { } public async downloadFile(authUser: AuthUserDto, assetId: string): Promise { - this.checkDownloadAccess(authUser); - await this.checkAssetsAccess(authUser, [assetId]); + await this.access.requirePermission(authUser, Permission.ASSET_DOWNLOAD, assetId); try { const asset = await this._assetRepository.get(assetId); @@ -312,7 +303,8 @@ export class AssetService { res: Res, headers: Record, ) { - 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, ) { - 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 { - await this.checkAssetsAccess(authUser, dto.ids, true); - const deleteQueue: Array = []; 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 { - if (getAssetCountByTimeBucketDto.userId !== undefined) { - await this.checkUserAccess(authUser, getAssetCountByTimeBucketDto.userId); - } - - const result = await this._assetRepository.getAssetCountByTimeBucket( - getAssetCountByTimeBucketDto.userId || authUser.id, - getAssetCountByTimeBucketDto, - ); - + const userId = dto.userId || authUser.id; + await this.access.requirePermission(authUser, Permission.LIBRARY_READ, userId); + const result = await this._assetRepository.getAssetCountByTimeBucket(userId, dto); 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; } diff --git a/server/src/infra/repositories/access.repository.ts b/server/src/infra/repositories/access.repository.ts index 95dfdadfb3..11f520993c 100644 --- a/server/src/infra/repositories/access.repository.ts +++ b/server/src/infra/repositories/access.repository.ts @@ -11,97 +11,121 @@ export class AccessRepository implements IAccessRepository { @InjectRepository(SharedLinkEntity) private sharedLinkRepository: Repository, ) {} - hasPartnerAccess(userId: string, partnerId: string): Promise { - return this.partnerRepository.exist({ - where: { - sharedWithId: userId, - sharedById: partnerId, - }, - }); - } - - hasAlbumAssetAccess(userId: string, assetId: string): Promise { - return this.albumRepository.exist({ - where: [ - { - ownerId: userId, - assets: { - id: assetId, - }, + library = { + hasPartnerAccess: (userId: string, partnerId: string): Promise => { + return this.partnerRepository.exist({ + where: { + sharedWithId: userId, + sharedById: partnerId, }, - { - sharedUsers: { + }); + }, + }; + + asset = { + hasAlbumAccess: (userId: string, assetId: string): Promise => { + return this.albumRepository.exist({ + where: [ + { + ownerId: userId, + assets: { + id: assetId, + }, + }, + { + sharedUsers: { + id: userId, + }, + assets: { + id: assetId, + }, + }, + ], + }); + }, + + hasOwnerAccess: (userId: string, assetId: string): Promise => { + return this.assetRepository.exist({ + where: { + id: assetId, + ownerId: userId, + }, + }); + }, + + hasPartnerAccess: (userId: string, assetId: string): Promise => { + return this.partnerRepository.exist({ + where: { + sharedWith: { 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: { - sharedWith: { - id: userId, - }, - sharedBy: { - assets: { - id: assetId, - }, - }, - }, - relations: { - sharedWith: true, - sharedBy: { - assets: true, - }, - }, - }); - } - - async hasSharedLinkAssetAccess(sharedLinkId: string, assetId: string): Promise { - return ( - // album asset - (await this.sharedLinkRepository.exist({ - where: { - id: sharedLinkId, - album: { + sharedBy: { assets: { id: assetId, }, }, }, - })) || - // individual asset - (await this.sharedLinkRepository.exist({ - where: { - id: sharedLinkId, - assets: { - id: assetId, + relations: { + sharedWith: true, + sharedBy: { + assets: true, }, }, - })) - ); - } + }); + }, - hasAlbumOwnerAccess(userId: string, albumId: string): Promise { - return this.albumRepository.exist({ - where: { - id: albumId, - ownerId: userId, - }, - }); - } + hasSharedLinkAccess: async (sharedLinkId: string, assetId: string): Promise => { + return ( + // album asset + (await this.sharedLinkRepository.exist({ + where: { + id: sharedLinkId, + album: { + assets: { + id: assetId, + }, + }, + }, + })) || + // individual asset + (await this.sharedLinkRepository.exist({ + where: { + id: sharedLinkId, + assets: { + id: assetId, + }, + }, + })) + ); + }, + }; + + album = { + hasOwnerAccess: (userId: string, albumId: string): Promise => { + return this.albumRepository.exist({ + where: { + id: albumId, + ownerId: userId, + }, + }); + }, + + hasSharedAlbumAccess: (userId: string, albumId: string): Promise => { + return this.albumRepository.exist({ + where: { + id: albumId, + ownerId: userId, + }, + }); + }, + + hasSharedLinkAccess: (sharedLinkId: string, albumId: string): Promise => { + return this.sharedLinkRepository.exist({ + where: { + id: sharedLinkId, + albumId, + }, + }); + }, + }; } diff --git a/server/test/repositories/access.repository.mock.ts b/server/test/repositories/access.repository.mock.ts index 055cc226e7..1c8d09c507 100644 --- a/server/test/repositories/access.repository.mock.ts +++ b/server/test/repositories/access.repository.mock.ts @@ -1,14 +1,28 @@ import { IAccessRepository } from '@app/domain'; -export const newAccessRepositoryMock = (): jest.Mocked => { +export type IAccessRepositoryMock = { + asset: jest.Mocked; + album: jest.Mocked; + library: jest.Mocked; +}; + +export const newAccessRepositoryMock = (): IAccessRepositoryMock => { return { - hasPartnerAccess: jest.fn(), + asset: { + hasOwnerAccess: jest.fn(), + hasAlbumAccess: jest.fn(), + hasPartnerAccess: jest.fn(), + hasSharedLinkAccess: jest.fn(), + }, - hasAlbumAssetAccess: jest.fn(), - hasOwnerAssetAccess: jest.fn(), - hasPartnerAssetAccess: jest.fn(), - hasSharedLinkAssetAccess: jest.fn(), + album: { + hasOwnerAccess: jest.fn(), + hasSharedAlbumAccess: jest.fn(), + hasSharedLinkAccess: jest.fn(), + }, - hasAlbumOwnerAccess: jest.fn(), + library: { + hasPartnerAccess: jest.fn(), + }, }; };