diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts index 30086f5d26b6c..d829139a9542c 100644 --- a/server/src/domain/access/access.core.ts +++ b/server/src/domain/access/access.core.ts @@ -1,5 +1,6 @@ import { BadRequestException, UnauthorizedException } from '@nestjs/common'; import { AuthUserDto } from '../auth'; +import { setDifference, setUnion } from '../domain.util'; import { IAccessRepository } from '../repositories'; export enum Permission { @@ -99,6 +100,24 @@ export class AccessCore { } private async checkAccessSharedLink(authUser: AuthUserDto, permission: Permission, ids: Set) { + const sharedLinkId = authUser.sharedLinkId; + if (!sharedLinkId) { + return new Set(); + } + + switch (permission) { + case Permission.ASSET_UPLOAD: + return authUser.isAllowUpload ? ids : new Set(); + + case Permission.ALBUM_READ: + return await this.repository.album.checkSharedLinkAccess(sharedLinkId, ids); + + case Permission.ALBUM_DOWNLOAD: + return !!authUser.isAllowDownload + ? await this.repository.album.checkSharedLinkAccess(sharedLinkId, ids) + : new Set(); + } + const allowedIds = new Set(); for (const id of ids) { const hasAccess = await this.hasSharedLinkAccess(authUser, permission, id); @@ -126,25 +145,42 @@ export class AccessCore { case Permission.ASSET_DOWNLOAD: return !!authUser.isAllowDownload && (await this.repository.asset.hasSharedLinkAccess(sharedLinkId, id)); - case Permission.ASSET_UPLOAD: - return authUser.isAllowUpload; - 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); - - case Permission.ALBUM_DOWNLOAD: - return !!authUser.isAllowDownload && (await this.repository.album.hasSharedLinkAccess(sharedLinkId, id)); - default: return false; } } private async checkAccessOther(authUser: AuthUserDto, permission: Permission, ids: Set) { + switch (permission) { + case Permission.ALBUM_READ: { + const isOwner = await this.repository.album.checkOwnerAccess(authUser.id, ids); + const isShared = await this.repository.album.checkSharedAlbumAccess(authUser.id, setDifference(ids, isOwner)); + return setUnion(isOwner, isShared); + } + + case Permission.ALBUM_UPDATE: + return this.repository.album.checkOwnerAccess(authUser.id, ids); + + case Permission.ALBUM_DELETE: + return this.repository.album.checkOwnerAccess(authUser.id, ids); + + case Permission.ALBUM_SHARE: + return this.repository.album.checkOwnerAccess(authUser.id, ids); + + case Permission.ALBUM_DOWNLOAD: { + const isOwner = await this.repository.album.checkOwnerAccess(authUser.id, ids); + const isShared = await this.repository.album.checkSharedAlbumAccess(authUser.id, setDifference(ids, isOwner)); + return setUnion(isOwner, isShared); + } + + case Permission.ALBUM_REMOVE_ASSET: + return this.repository.album.checkOwnerAccess(authUser.id, ids); + } + const allowedIds = new Set(); for (const id of ids) { const hasAccess = await this.hasOtherAccess(authUser, permission, id); @@ -204,33 +240,9 @@ export class AccessCore { (await this.repository.asset.hasPartnerAccess(authUser.id, id)) ); - case Permission.ALBUM_READ: - return ( - (await this.repository.album.hasOwnerAccess(authUser.id, id)) || - (await this.repository.album.hasSharedAlbumAccess(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.ALBUM_DOWNLOAD: - return ( - (await this.repository.album.hasOwnerAccess(authUser.id, id)) || - (await this.repository.album.hasSharedAlbumAccess(authUser.id, id)) - ); - case Permission.ASSET_UPLOAD: return this.repository.library.hasOwnerAccess(authUser.id, id); - case Permission.ALBUM_REMOVE_ASSET: - return this.repository.album.hasOwnerAccess(authUser.id, id); - case Permission.ARCHIVE_READ: return authUser.id === id; diff --git a/server/src/domain/activity/activity.spec.ts b/server/src/domain/activity/activity.spec.ts index 496d8978b7d24..659718bede7b9 100644 --- a/server/src/domain/activity/activity.spec.ts +++ b/server/src/domain/activity/activity.spec.ts @@ -24,7 +24,7 @@ describe(ActivityService.name, () => { describe('getAll', () => { it('should get all', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id'])); activityMock.search.mockResolvedValue([]); await expect(sut.getAll(authStub.admin, { assetId: 'asset-id', albumId: 'album-id' })).resolves.toEqual([]); @@ -37,7 +37,7 @@ describe(ActivityService.name, () => { }); it('should filter by type=like', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id'])); activityMock.search.mockResolvedValue([]); await expect( @@ -52,7 +52,7 @@ describe(ActivityService.name, () => { }); it('should filter by type=comment', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id'])); activityMock.search.mockResolvedValue([]); await expect( @@ -70,7 +70,7 @@ describe(ActivityService.name, () => { describe('getStatistics', () => { it('should get the comment count', async () => { activityMock.getStatistics.mockResolvedValue(1); - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([activityStub.oneComment.albumId])); await expect( sut.getStatistics(authStub.admin, { assetId: 'asset-id', @@ -82,7 +82,6 @@ describe(ActivityService.name, () => { describe('addComment', () => { it('should require access to the album', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(false); await expect( sut.create(authStub.admin, { albumId: 'album-id', @@ -114,7 +113,7 @@ describe(ActivityService.name, () => { }); it('should fail because activity is disabled for the album', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id'])); accessMock.activity.hasCreateAccess.mockResolvedValue(false); activityMock.create.mockResolvedValue(activityStub.oneComment); @@ -148,7 +147,7 @@ describe(ActivityService.name, () => { }); it('should skip if like exists', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id'])); accessMock.activity.hasCreateAccess.mockResolvedValue(true); activityMock.search.mockResolvedValue([activityStub.liked]); diff --git a/server/src/domain/album/album.service.spec.ts b/server/src/domain/album/album.service.spec.ts index a93cb0ad17937..414826cb2c88d 100644 --- a/server/src/domain/album/album.service.spec.ts +++ b/server/src/domain/album/album.service.spec.ts @@ -204,7 +204,6 @@ describe(AlbumService.name, () => { }); it('should prevent updating a not owned album (shared with auth user)', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(false); await expect( sut.update(authStub.admin, albumStub.sharedWithAdmin.id, { albumName: 'new album name', @@ -213,7 +212,7 @@ describe(AlbumService.name, () => { }); it('should require a valid thumbnail asset id', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-4'])); albumMock.getById.mockResolvedValue(albumStub.oneAsset); albumMock.update.mockResolvedValue(albumStub.oneAsset); albumMock.hasAsset.mockResolvedValue(false); @@ -229,7 +228,7 @@ describe(AlbumService.name, () => { }); it('should allow the owner to update the album', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-4'])); albumMock.getById.mockResolvedValue(albumStub.oneAsset); albumMock.update.mockResolvedValue(albumStub.oneAsset); @@ -252,7 +251,7 @@ describe(AlbumService.name, () => { describe('delete', () => { it('should throw an error for an album not found', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.sharedWithAdmin.id])); albumMock.getById.mockResolvedValue(null); await expect(sut.delete(authStub.admin, albumStub.sharedWithAdmin.id)).rejects.toBeInstanceOf( @@ -263,7 +262,6 @@ describe(AlbumService.name, () => { }); it('should not let a shared user delete the album', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(false); albumMock.getById.mockResolvedValue(albumStub.sharedWithAdmin); await expect(sut.delete(authStub.admin, albumStub.sharedWithAdmin.id)).rejects.toBeInstanceOf( @@ -274,7 +272,7 @@ describe(AlbumService.name, () => { }); it('should let the owner delete an album', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.empty.id])); albumMock.getById.mockResolvedValue(albumStub.empty); await sut.delete(authStub.admin, albumStub.empty.id); @@ -286,7 +284,6 @@ describe(AlbumService.name, () => { describe('addUsers', () => { 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(BadRequestException); @@ -294,7 +291,7 @@ describe(AlbumService.name, () => { }); it('should throw an error if the userId is already added', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.sharedWithAdmin.id])); albumMock.getById.mockResolvedValue(albumStub.sharedWithAdmin); await expect( sut.addUsers(authStub.user1, albumStub.sharedWithAdmin.id, { sharedUserIds: [authStub.admin.id] }), @@ -303,7 +300,7 @@ describe(AlbumService.name, () => { }); it('should throw an error if the userId does not exist', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.sharedWithAdmin.id])); albumMock.getById.mockResolvedValue(albumStub.sharedWithAdmin); userMock.get.mockResolvedValue(null); await expect( @@ -313,7 +310,7 @@ describe(AlbumService.name, () => { }); it('should add valid shared users', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.sharedWithAdmin.id])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.sharedWithAdmin)); albumMock.update.mockResolvedValue(albumStub.sharedWithAdmin); userMock.get.mockResolvedValue(userStub.user2); @@ -328,14 +325,14 @@ describe(AlbumService.name, () => { describe('removeUser', () => { it('should require a valid album id', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-1'])); albumMock.getById.mockResolvedValue(null); 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); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.sharedWithUser.id])); albumMock.getById.mockResolvedValue(albumStub.sharedWithUser); await expect( @@ -352,7 +349,6 @@ 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.getById.mockResolvedValue(albumStub.sharedWithMultiple); await expect( @@ -360,7 +356,10 @@ describe(AlbumService.name, () => { ).rejects.toBeInstanceOf(BadRequestException); expect(albumMock.update).not.toHaveBeenCalled(); - expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.user1.id, albumStub.sharedWithMultiple.id); + expect(accessMock.album.checkOwnerAccess).toHaveBeenCalledWith( + authStub.user1.id, + new Set([albumStub.sharedWithMultiple.id]), + ); }); it('should allow a shared user to remove themselves', async () => { @@ -413,51 +412,51 @@ describe(AlbumService.name, () => { describe('getAlbumInfo', () => { it('should get a shared album', async () => { albumMock.getById.mockResolvedValue(albumStub.oneAsset); - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.oneAsset.id])); await sut.get(authStub.admin, albumStub.oneAsset.id, {}); expect(albumMock.getById).toHaveBeenCalledWith(albumStub.oneAsset.id, { withAssets: true }); - expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, albumStub.oneAsset.id); + expect(accessMock.album.checkOwnerAccess).toHaveBeenCalledWith( + authStub.admin.id, + new Set([albumStub.oneAsset.id]), + ); }); it('should get a shared album via a shared link', async () => { albumMock.getById.mockResolvedValue(albumStub.oneAsset); - accessMock.album.hasSharedLinkAccess.mockResolvedValue(true); + accessMock.album.checkSharedLinkAccess.mockResolvedValue(new Set(['album-123'])); await sut.get(authStub.adminSharedLink, 'album-123', {}); expect(albumMock.getById).toHaveBeenCalledWith('album-123', { withAssets: true }); - expect(accessMock.album.hasSharedLinkAccess).toHaveBeenCalledWith( + expect(accessMock.album.checkSharedLinkAccess).toHaveBeenCalledWith( authStub.adminSharedLink.sharedLinkId, - 'album-123', + new Set(['album-123']), ); }); it('should get a shared album via shared with user', async () => { albumMock.getById.mockResolvedValue(albumStub.oneAsset); - accessMock.album.hasSharedAlbumAccess.mockResolvedValue(true); + accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123'])); await sut.get(authStub.user1, 'album-123', {}); expect(albumMock.getById).toHaveBeenCalledWith('album-123', { withAssets: true }); - expect(accessMock.album.hasSharedAlbumAccess).toHaveBeenCalledWith(authStub.user1.id, 'album-123'); + expect(accessMock.album.checkSharedAlbumAccess).toHaveBeenCalledWith(authStub.user1.id, new Set(['album-123'])); }); it('should throw an error for no access', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(false); - accessMock.album.hasSharedAlbumAccess.mockResolvedValue(false); - await expect(sut.get(authStub.admin, 'album-123', {})).rejects.toBeInstanceOf(BadRequestException); - expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'album-123'); - expect(accessMock.album.hasSharedAlbumAccess).toHaveBeenCalledWith(authStub.admin.id, 'album-123'); + expect(accessMock.album.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['album-123'])); + expect(accessMock.album.checkSharedAlbumAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['album-123'])); }); }); describe('addAssets', () => { it('should allow the owner to add assets', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -482,7 +481,7 @@ describe(AlbumService.name, () => { }); it('should not set the thumbnail if the album has one already', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep({ ...albumStub.empty, albumThumbnailAssetId: 'asset-id' })); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -500,8 +499,7 @@ describe(AlbumService.name, () => { }); it('should allow a shared user to add assets', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(false); - accessMock.album.hasSharedAlbumAccess.mockResolvedValue(true); + accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123'])); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.sharedWithUser)); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -526,9 +524,7 @@ describe(AlbumService.name, () => { }); it('should allow a shared link user to add assets', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(false); - accessMock.album.hasSharedAlbumAccess.mockResolvedValue(false); - accessMock.album.hasSharedLinkAccess.mockResolvedValue(true); + accessMock.album.checkSharedLinkAccess.mockResolvedValue(new Set(['album-123'])); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -551,14 +547,14 @@ describe(AlbumService.name, () => { assetIds: ['asset-1', 'asset-2', 'asset-3'], }); - expect(accessMock.album.hasSharedLinkAccess).toHaveBeenCalledWith( + expect(accessMock.album.checkSharedLinkAccess).toHaveBeenCalledWith( authStub.adminSharedLink.sharedLinkId, - 'album-123', + new Set(['album-123']), ); }); it('should allow adding assets shared via partner sharing', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); accessMock.asset.hasOwnerAccess.mockResolvedValue(false); accessMock.asset.hasPartnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); @@ -577,7 +573,7 @@ describe(AlbumService.name, () => { }); it('should skip duplicate assets', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); accessMock.asset.hasOwnerAccess.mockResolvedValue(true); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); @@ -590,7 +586,7 @@ describe(AlbumService.name, () => { }); it('should skip assets not shared with user', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); accessMock.asset.hasOwnerAccess.mockResolvedValue(false); accessMock.asset.hasPartnerAccess.mockResolvedValue(false); albumMock.getById.mockResolvedValue(albumStub.oneAsset); @@ -605,33 +601,31 @@ describe(AlbumService.name, () => { }); it('should not allow unauthorized access to the album', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(false); - accessMock.album.hasSharedAlbumAccess.mockResolvedValue(false); albumMock.getById.mockResolvedValue(albumStub.oneAsset); await expect( sut.addAssets(authStub.admin, 'album-123', { ids: ['asset-1', 'asset-2', 'asset-3'] }), ).rejects.toBeInstanceOf(BadRequestException); - expect(accessMock.album.hasOwnerAccess).toHaveBeenCalled(); - expect(accessMock.album.hasSharedAlbumAccess).toHaveBeenCalled(); + expect(accessMock.album.checkOwnerAccess).toHaveBeenCalled(); + expect(accessMock.album.checkSharedAlbumAccess).toHaveBeenCalled(); }); it('should not allow unauthorized shared link access to the album', async () => { - accessMock.album.hasSharedLinkAccess.mockResolvedValue(false); albumMock.getById.mockResolvedValue(albumStub.oneAsset); await expect( sut.addAssets(authStub.adminSharedLink, 'album-123', { ids: ['asset-1', 'asset-2', 'asset-3'] }), ).rejects.toBeInstanceOf(BadRequestException); - expect(accessMock.album.hasSharedLinkAccess).toHaveBeenCalled(); + expect(accessMock.album.checkSharedLinkAccess).toHaveBeenCalled(); }); }); describe('removeAssets', () => { it('should allow the owner to remove assets', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123'])); + accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['asset-id'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); @@ -644,7 +638,7 @@ describe(AlbumService.name, () => { }); it('should skip assets not in the album', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.empty)); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -656,7 +650,7 @@ describe(AlbumService.name, () => { }); it('should skip assets without user permission to remove', async () => { - accessMock.album.hasSharedAlbumAccess.mockResolvedValue(true); + accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); @@ -672,7 +666,8 @@ describe(AlbumService.name, () => { }); it('should reset the thumbnail if it is removed', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['album-123'])); + accessMock.album.checkOwnerAccess.mockResolvedValueOnce(new Set(['asset-id'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.twoAssets)); albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); diff --git a/server/src/domain/album/album.service.ts b/server/src/domain/album/album.service.ts index 986cdb891138c..0d92dae04deed 100644 --- a/server/src/domain/album/album.service.ts +++ b/server/src/domain/album/album.service.ts @@ -3,6 +3,7 @@ import { BadRequestException, Inject, Injectable } from '@nestjs/common'; import { AccessCore, Permission } from '../access'; import { BulkIdErrorReason, BulkIdResponseDto, BulkIdsDto } from '../asset'; import { AuthUserDto } from '../auth'; +import { setUnion } from '../domain.util'; import { JobName } from '../job'; import { AlbumInfoOptions, @@ -194,7 +195,7 @@ export class AlbumService { const existingAssetIds = await this.albumRepository.getAssetIds(id, dto.ids); const canRemove = await this.access.checkAccess(authUser, Permission.ALBUM_REMOVE_ASSET, existingAssetIds); const canShare = await this.access.checkAccess(authUser, Permission.ASSET_SHARE, existingAssetIds); - const allowedAssetIds = new Set([...canRemove, ...canShare]); + const allowedAssetIds = setUnion(canRemove, canShare); const results: BulkIdResponseDto[] = []; for (const assetId of dto.ids) { diff --git a/server/src/domain/asset/asset.service.spec.ts b/server/src/domain/asset/asset.service.spec.ts index 45687282f8993..932c3b364341a 100644 --- a/server/src/domain/asset/asset.service.spec.ts +++ b/server/src/domain/asset/asset.service.spec.ts @@ -347,14 +347,14 @@ describe(AssetService.name, () => { describe('getTimeBucket', () => { it('should return the assets for a album time bucket if user has album.read', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-id'])); assetMock.getTimeBucket.mockResolvedValue([assetStub.image]); await expect( sut.getTimeBucket(authStub.admin, { size: TimeBucketSize.DAY, timeBucket: 'bucket', albumId: 'album-id' }), ).resolves.toEqual(expect.arrayContaining([expect.objectContaining({ id: 'asset-id' })])); - expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'album-id'); + expect(accessMock.album.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['album-id'])); expect(assetMock.getTimeBucket).toBeCalledWith('bucket', { size: TimeBucketSize.DAY, timeBucket: 'bucket', @@ -546,7 +546,7 @@ describe(AssetService.name, () => { }); it('should return a list of archives (albumId)', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-1'])); assetMock.getByAlbumId.mockResolvedValue({ items: [assetStub.image, assetStub.video], hasNextPage: false, @@ -554,7 +554,7 @@ describe(AssetService.name, () => { await expect(sut.getDownloadInfo(authStub.admin, { albumId: 'album-1' })).resolves.toEqual(downloadResponse); - expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'album-1'); + expect(accessMock.album.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['album-1'])); expect(assetMock.getByAlbumId).toHaveBeenCalledWith({ take: 2500, skip: 0 }, 'album-1'); }); diff --git a/server/src/domain/domain.util.ts b/server/src/domain/domain.util.ts index 53e674bf30f61..00ad27bc77ad7 100644 --- a/server/src/domain/domain.util.ts +++ b/server/src/domain/domain.util.ts @@ -150,3 +150,23 @@ export function Optional({ nullable, ...validationOptions }: OptionalOptions = { return ValidateIf((obj: any, v: any) => v !== undefined, validationOptions); } + +// NOTE: The following Set utils have been added here, to easily determine where they are used. +// They should be replaced with native Set operations, when they are added to the language. +// Proposal reference: https://github.com/tc39/proposal-set-methods + +export const setUnion = (setA: Set, setB: Set): Set => { + const union = new Set(setA); + for (const elem of setB) { + union.add(elem); + } + return union; +}; + +export const setDifference = (setA: Set, setB: Set): Set => { + const difference = new Set(setA); + for (const elem of setB) { + difference.delete(elem); + } + return difference; +}; diff --git a/server/src/domain/repositories/access.repository.ts b/server/src/domain/repositories/access.repository.ts index 9c009719d362c..e1ea27ce6dba6 100644 --- a/server/src/domain/repositories/access.repository.ts +++ b/server/src/domain/repositories/access.repository.ts @@ -18,9 +18,9 @@ export interface IAccessRepository { }; album: { - hasOwnerAccess(userId: string, albumId: string): Promise; - hasSharedAlbumAccess(userId: string, albumId: string): Promise; - hasSharedLinkAccess(sharedLinkId: string, albumId: string): Promise; + checkOwnerAccess(userId: string, albumIds: Set): Promise>; + checkSharedAlbumAccess(userId: string, albumIds: Set): Promise>; + checkSharedLinkAccess(sharedLinkId: string, albumIds: Set): Promise>; }; library: { 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 863e3a3534c79..abf8128c4820e 100644 --- a/server/src/domain/shared-link/shared-link.service.spec.ts +++ b/server/src/domain/shared-link/shared-link.service.spec.ts @@ -97,7 +97,6 @@ describe(SharedLinkService.name, () => { }); it('should not allow non-owners to create album shared links', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(false); await expect( sut.create(authStub.admin, { type: SharedLinkType.ALBUM, assetIds: [], albumId: 'album-1' }), ).rejects.toBeInstanceOf(BadRequestException); @@ -117,12 +116,15 @@ describe(SharedLinkService.name, () => { }); it('should create an album shared link', async () => { - accessMock.album.hasOwnerAccess.mockResolvedValue(true); + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set([albumStub.oneAsset.id])); shareMock.create.mockResolvedValue(sharedLinkStub.valid); await sut.create(authStub.admin, { type: SharedLinkType.ALBUM, albumId: albumStub.oneAsset.id }); - expect(accessMock.album.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, albumStub.oneAsset.id); + expect(accessMock.album.checkOwnerAccess).toHaveBeenCalledWith( + authStub.admin.id, + new Set([albumStub.oneAsset.id]), + ); expect(shareMock.create).toHaveBeenCalledWith({ type: SharedLinkType.ALBUM, userId: authStub.admin.id, diff --git a/server/src/infra/repositories/access.repository.ts b/server/src/infra/repositories/access.repository.ts index fa58628851164..fb0b865fb641f 100644 --- a/server/src/infra/repositories/access.repository.ts +++ b/server/src/infra/repositories/access.repository.ts @@ -1,6 +1,6 @@ import { IAccessRepository } from '@app/domain'; import { InjectRepository } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { In, Repository } from 'typeorm'; import { ActivityEntity, AlbumEntity, @@ -209,33 +209,57 @@ export class AccessRepository implements IAccessRepository { }; album = { - hasOwnerAccess: (userId: string, albumId: string): Promise => { - return this.albumRepository.exist({ - where: { - id: albumId, - ownerId: userId, - }, - }); + checkOwnerAccess: async (userId: string, albumIds: Set): Promise> => { + if (albumIds.size === 0) { + return new Set(); + } + + return this.albumRepository + .find({ + select: { id: true }, + where: { + id: In([...albumIds]), + ownerId: userId, + }, + }) + .then((albums) => new Set(albums.map((album) => album.id))); }, - hasSharedAlbumAccess: (userId: string, albumId: string): Promise => { - return this.albumRepository.exist({ - where: { - id: albumId, - sharedUsers: { - id: userId, + checkSharedAlbumAccess: async (userId: string, albumIds: Set): Promise> => { + if (albumIds.size === 0) { + return new Set(); + } + + return this.albumRepository + .find({ + select: { id: true }, + where: { + id: In([...albumIds]), + sharedUsers: { + id: userId, + }, }, - }, - }); + }) + .then((albums) => new Set(albums.map((album) => album.id))); }, - hasSharedLinkAccess: (sharedLinkId: string, albumId: string): Promise => { - return this.sharedLinkRepository.exist({ - where: { - id: sharedLinkId, - albumId, - }, - }); + checkSharedLinkAccess: async (sharedLinkId: string, albumIds: Set): Promise> => { + if (albumIds.size === 0) { + return new Set(); + } + + return this.sharedLinkRepository + .find({ + select: { albumId: true }, + where: { + id: sharedLinkId, + albumId: In([...albumIds]), + }, + }) + .then( + (sharedLinks) => + new Set(sharedLinks.flatMap((sharedLink) => (!!sharedLink.albumId ? [sharedLink.albumId] : []))), + ); }, }; diff --git a/server/test/repositories/access.repository.mock.ts b/server/test/repositories/access.repository.mock.ts index 8f1e9355d8584..eceb25812e3da 100644 --- a/server/test/repositories/access.repository.mock.ts +++ b/server/test/repositories/access.repository.mock.ts @@ -30,9 +30,9 @@ export const newAccessRepositoryMock = (reset = true): IAccessRepositoryMock => }, album: { - hasOwnerAccess: jest.fn(), - hasSharedAlbumAccess: jest.fn(), - hasSharedLinkAccess: jest.fn(), + checkOwnerAccess: jest.fn().mockResolvedValue(new Set()), + checkSharedAlbumAccess: jest.fn().mockResolvedValue(new Set()), + checkSharedLinkAccess: jest.fn().mockResolvedValue(new Set()), }, authDevice: {