From 5aa658de595ac456cdc4e26206afe0e91cdea776 Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Fri, 1 Dec 2023 21:56:41 -0500 Subject: [PATCH] chore(server): Check asset permissions in bulk (#5329) Modify Access repository, to evaluate `asset` permissions in bulk. Queries have been validated to match what they currently generate for single ids. Queries: * `asset` album access: ```sql -- Before SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS ( SELECT 1 FROM "albums" "AlbumEntity" LEFT JOIN "albums_assets_assets" "AlbumEntity_AlbumEntity__AlbumEntity_assets" ON "AlbumEntity_AlbumEntity__AlbumEntity_assets"."albumsId"="AlbumEntity"."id" LEFT JOIN "assets" "AlbumEntity__AlbumEntity_assets" ON "AlbumEntity__AlbumEntity_assets"."id"="AlbumEntity_AlbumEntity__AlbumEntity_assets"."assetsId" AND "AlbumEntity__AlbumEntity_assets"."deletedAt" IS NULL LEFT JOIN "albums_shared_users_users" "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers" ON "AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."albumsId"="AlbumEntity"."id" LEFT JOIN "users" "AlbumEntity__AlbumEntity_sharedUsers" ON "AlbumEntity__AlbumEntity_sharedUsers"."id"="AlbumEntity_AlbumEntity__AlbumEntity_sharedUsers"."usersId" AND "AlbumEntity__AlbumEntity_sharedUsers"."deletedAt" IS NULL WHERE ( ("AlbumEntity"."ownerId" = $1 AND "AlbumEntity__AlbumEntity_assets"."id" = $2) OR ("AlbumEntity__AlbumEntity_sharedUsers"."id" = $3 AND "AlbumEntity__AlbumEntity_assets"."id" = $4) OR ("AlbumEntity"."ownerId" = $5 AND "AlbumEntity__AlbumEntity_assets"."livePhotoVideoId" = $6) OR ("AlbumEntity__AlbumEntity_sharedUsers"."id" = $7 AND "AlbumEntity__AlbumEntity_assets"."livePhotoVideoId" = $8) ) AND "AlbumEntity"."deletedAt" IS NULL ) LIMIT 1 -- After SELECT "asset"."id" AS "assetId", "asset"."livePhotoVideoId" AS "livePhotoVideoId" FROM "albums" "album" INNER JOIN "albums_assets_assets" "album_asset" ON "album_asset"."albumsId"="album"."id" INNER JOIN "assets" "asset" ON "asset"."id"="album_asset"."assetsId" AND "asset"."deletedAt" IS NULL LEFT JOIN "albums_shared_users_users" "album_sharedUsers" ON "album_sharedUsers"."albumsId"="album"."id" LEFT JOIN "users" "sharedUsers" ON "sharedUsers"."id"="album_sharedUsers"."usersId" AND "sharedUsers"."deletedAt" IS NULL WHERE ( "album"."ownerId" = $1 OR "sharedUsers"."id" = $2 ) AND ( "asset"."id" IN ($3, $4) OR "asset"."livePhotoVideoId" IN ($5, $6) ) AND "album"."deletedAt" IS NULL ``` * `asset` owner access: ```sql -- Before SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS ( SELECT 1 FROM "assets" "AssetEntity" WHERE "AssetEntity"."id" = $1 AND "AssetEntity"."ownerId" = $2 ) LIMIT 1 -- After SELECT "AssetEntity"."id" AS "AssetEntity_id" FROM "assets" "AssetEntity" WHERE "AssetEntity"."id" IN ($1, $2) AND "AssetEntity"."ownerId" = $3 ``` * `asset` partner access: ```sql -- Before SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS ( SELECT 1 FROM "partners" "PartnerEntity" LEFT JOIN "users" "PartnerEntity__PartnerEntity_sharedWith" ON "PartnerEntity__PartnerEntity_sharedWith"."id"="PartnerEntity"."sharedWithId" AND "PartnerEntity__PartnerEntity_sharedWith"."deletedAt" IS NULL LEFT JOIN "users" "PartnerEntity__PartnerEntity_sharedBy" ON "PartnerEntity__PartnerEntity_sharedBy"."id"="PartnerEntity"."sharedById" AND "PartnerEntity__PartnerEntity_sharedBy"."deletedAt" IS NULL LEFT JOIN "assets" "0aabe9f4a62b794e2c24a074297e534f51a4ac6c" ON "0aabe9f4a62b794e2c24a074297e534f51a4ac6c"."ownerId"="PartnerEntity__PartnerEntity_sharedBy"."id" AND "0aabe9f4a62b794e2c24a074297e534f51a4ac6c"."deletedAt" IS NULL LEFT JOIN "users" "PartnerEntity__sharedBy" ON "PartnerEntity__sharedBy"."id"="PartnerEntity"."sharedById" AND "PartnerEntity__sharedBy"."deletedAt" IS NULL LEFT JOIN "users" "PartnerEntity__sharedWith" ON "PartnerEntity__sharedWith"."id"="PartnerEntity"."sharedWithId" AND "PartnerEntity__sharedWith"."deletedAt" IS NULL WHERE "PartnerEntity__PartnerEntity_sharedWith"."id" = $1 AND "0aabe9f4a62b794e2c24a074297e534f51a4ac6c"."id" = $2 ) LIMIT 1 -- After SELECT "asset"."id" AS "assetId" FROM "partners" "partner" INNER JOIN "users" "sharedBy" ON "sharedBy"."id"="partner"."sharedById" AND "sharedBy"."deletedAt" IS NULL INNER JOIN "assets" "asset" ON "asset"."ownerId"="sharedBy"."id" AND "asset"."deletedAt" IS NULL WHERE "partner"."sharedWithId" = $1 AND "asset"."id" IN ($2, $3) ``` * `asset` shared link access: ```sql -- Before SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS ( SELECT 1 FROM "shared_links" "SharedLinkEntity" LEFT JOIN "albums" "SharedLinkEntity__SharedLinkEntity_album" ON "SharedLinkEntity__SharedLinkEntity_album"."id"="SharedLinkEntity"."albumId" AND "SharedLinkEntity__SharedLinkEntity_album"."deletedAt" IS NULL LEFT JOIN "albums_assets_assets" "760f12c00d97bdcec1ce224d1e3bf449859942b6" ON "760f12c00d97bdcec1ce224d1e3bf449859942b6"."albumsId"="SharedLinkEntity__SharedLinkEntity_album"."id" LEFT JOIN "assets" "4a35f463ae8c5544ede95c4b6d9ce8c686b6bfe6" ON "4a35f463ae8c5544ede95c4b6d9ce8c686b6bfe6"."id"="760f12c00d97bdcec1ce224d1e3bf449859942b6"."assetsId" AND "4a35f463ae8c5544ede95c4b6d9ce8c686b6bfe6"."deletedAt" IS NULL LEFT JOIN "shared_link__asset" "SharedLinkEntity__SharedLinkEntity_assets_SharedLinkEntity" ON "SharedLinkEntity__SharedLinkEntity_assets_SharedLinkEntity"."sharedLinksId"="SharedLinkEntity"."id" LEFT JOIN "assets" "SharedLinkEntity__SharedLinkEntity_assets" ON "SharedLinkEntity__SharedLinkEntity_assets"."id"="SharedLinkEntity__SharedLinkEntity_assets_SharedLinkEntity"."assetsId" AND "SharedLinkEntity__SharedLinkEntity_assets"."deletedAt" IS NULL WHERE ( ("SharedLinkEntity"."id" = $1 AND "4a35f463ae8c5544ede95c4b6d9ce8c686b6bfe6"."id" = $2) OR ("SharedLinkEntity"."id" = $3 AND "SharedLinkEntity__SharedLinkEntity_assets"."id" = $4) OR ("SharedLinkEntity"."id" = $5 AND "4a35f463ae8c5544ede95c4b6d9ce8c686b6bfe6"."livePhotoVideoId" = $6) OR ("SharedLinkEntity"."id" = $7 AND "SharedLinkEntity__SharedLinkEntity_assets"."livePhotoVideoId" = $8) ) ) LIMIT 1 -- After SELECT "assets"."id" AS "assetId", "assets"."livePhotoVideoId" AS "assetLivePhotoVideoId", "albumAssets"."id" AS "albumAssetId", "albumAssets"."livePhotoVideoId" AS "albumAssetLivePhotoVideoId" FROM "shared_links" "sharedLink" LEFT JOIN "albums" "album" ON "album"."id"="sharedLink"."albumId" AND "album"."deletedAt" IS NULL LEFT JOIN "shared_link__asset" "assets_sharedLink" ON "assets_sharedLink"."sharedLinksId"="sharedLink"."id" LEFT JOIN "assets" "assets" ON "assets"."id"="assets_sharedLink"."assetsId" AND "assets"."deletedAt" IS NULL LEFT JOIN "albums_assets_assets" "album_albumAssets" ON "album_albumAssets"."albumsId"="album"."id" LEFT JOIN "assets" "albumAssets" ON "albumAssets"."id"="album_albumAssets"."assetsId" AND "albumAssets"."deletedAt" IS NULL WHERE "sharedLink"."id" = $1 AND ( "assets"."id" IN ($2, $3) OR "albumAssets"."id" IN ($4, $5) OR "assets"."livePhotoVideoId" IN ($6, $7) OR "albumAssets"."livePhotoVideoId" IN ($8, $9) ) ``` --- server/src/domain/access/access.core.ts | 157 +++++++------- server/src/domain/album/album.service.spec.ts | 21 +- server/src/domain/asset/asset.service.spec.ts | 84 ++++---- server/src/domain/domain.util.ts | 31 ++- .../domain/library/library.service.spec.ts | 3 +- .../src/domain/person/person.service.spec.ts | 2 +- .../domain/repositories/access.repository.ts | 9 +- .../shared-link/shared-link.service.spec.ts | 12 +- .../immich/api-v1/asset/asset.service.spec.ts | 31 ++- .../infra/repositories/access.repository.ts | 203 ++++++++++-------- .../repositories/access.repository.mock.ts | 9 +- 11 files changed, 287 insertions(+), 275 deletions(-) diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts index c47b2acd2..862fafc32 100644 --- a/server/src/domain/access/access.core.ts +++ b/server/src/domain/access/access.core.ts @@ -1,6 +1,6 @@ import { BadRequestException, UnauthorizedException } from '@nestjs/common'; import { AuthUserDto } from '../auth'; -import { setDifference, setUnion } from '../domain.util'; +import { setDifference, setIsEqual, setUnion } from '../domain.util'; import { IAccessRepository } from '../repositories'; export enum Permission { @@ -76,7 +76,7 @@ export class AccessCore { async requirePermission(authUser: AuthUserDto, permission: Permission, ids: string[] | string) { ids = Array.isArray(ids) ? ids : [ids]; const allowedIds = await this.checkAccess(authUser, permission, ids); - if (new Set(ids).size !== allowedIds.size) { + if (!setIsEqual(new Set(ids), allowedIds)) { throw new BadRequestException(`Not found or no ${permission} access`); } } @@ -106,9 +106,24 @@ export class AccessCore { } switch (permission) { + case Permission.ASSET_READ: + return await this.repository.asset.checkSharedLinkAccess(sharedLinkId, ids); + + case Permission.ASSET_VIEW: + return await this.repository.asset.checkSharedLinkAccess(sharedLinkId, ids); + + case Permission.ASSET_DOWNLOAD: + return !!authUser.isAllowDownload + ? await this.repository.asset.checkSharedLinkAccess(sharedLinkId, ids) + : new Set(); + case Permission.ASSET_UPLOAD: return authUser.isAllowUpload ? ids : new Set(); + case Permission.ASSET_SHARE: + // TODO: fix this to not use authUser.id for shared link access control + return await this.repository.asset.checkOwnerAccess(authUser.id, ids); + case Permission.ALBUM_READ: return await this.repository.album.checkSharedLinkAccess(sharedLinkId, ids); @@ -116,46 +131,59 @@ export class AccessCore { 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); - if (hasAccess) { - allowedIds.add(id); - } - } - return allowedIds; - } - - // TODO: Migrate logic to checkAccessSharedLink to evaluate permissions in bulk. - 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); default: - return false; + return new Set(); } } private async checkAccessOther(authUser: AuthUserDto, permission: Permission, ids: Set) { switch (permission) { + case Permission.ASSET_READ: { + const isOwner = await this.repository.asset.checkOwnerAccess(authUser.id, ids); + const isAlbum = await this.repository.asset.checkAlbumAccess(authUser.id, setDifference(ids, isOwner)); + const isPartner = await this.repository.asset.checkPartnerAccess( + authUser.id, + setDifference(ids, isOwner, isAlbum), + ); + return setUnion(isOwner, isAlbum, isPartner); + } + + case Permission.ASSET_SHARE: { + const isOwner = await this.repository.asset.checkOwnerAccess(authUser.id, ids); + const isPartner = await this.repository.asset.checkPartnerAccess(authUser.id, setDifference(ids, isOwner)); + return setUnion(isOwner, isPartner); + } + + case Permission.ASSET_VIEW: { + const isOwner = await this.repository.asset.checkOwnerAccess(authUser.id, ids); + const isAlbum = await this.repository.asset.checkAlbumAccess(authUser.id, setDifference(ids, isOwner)); + const isPartner = await this.repository.asset.checkPartnerAccess( + authUser.id, + setDifference(ids, isOwner, isAlbum), + ); + return setUnion(isOwner, isAlbum, isPartner); + } + + case Permission.ASSET_DOWNLOAD: { + const isOwner = await this.repository.asset.checkOwnerAccess(authUser.id, ids); + const isAlbum = await this.repository.asset.checkAlbumAccess(authUser.id, setDifference(ids, isOwner)); + const isPartner = await this.repository.asset.checkPartnerAccess( + authUser.id, + setDifference(ids, isOwner, isAlbum), + ); + return setUnion(isOwner, isAlbum, isPartner); + } + + case Permission.ASSET_UPDATE: + return await this.repository.asset.checkOwnerAccess(authUser.id, ids); + + case Permission.ASSET_DELETE: + return await this.repository.asset.checkOwnerAccess(authUser.id, ids); + + case Permission.ASSET_RESTORE: + return await this.repository.asset.checkOwnerAccess(authUser.id, ids); + 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)); @@ -163,13 +191,13 @@ export class AccessCore { } case Permission.ALBUM_UPDATE: - return this.repository.album.checkOwnerAccess(authUser.id, ids); + return await this.repository.album.checkOwnerAccess(authUser.id, ids); case Permission.ALBUM_DELETE: - return this.repository.album.checkOwnerAccess(authUser.id, ids); + return await this.repository.album.checkOwnerAccess(authUser.id, ids); case Permission.ALBUM_SHARE: - return this.repository.album.checkOwnerAccess(authUser.id, ids); + return await this.repository.album.checkOwnerAccess(authUser.id, ids); case Permission.ALBUM_DOWNLOAD: { const isOwner = await this.repository.album.checkOwnerAccess(authUser.id, ids); @@ -178,16 +206,16 @@ export class AccessCore { } case Permission.ALBUM_REMOVE_ASSET: - return this.repository.album.checkOwnerAccess(authUser.id, ids); + return await this.repository.album.checkOwnerAccess(authUser.id, ids); case Permission.ASSET_UPLOAD: - return this.repository.library.checkOwnerAccess(authUser.id, ids); + return await this.repository.library.checkOwnerAccess(authUser.id, ids); case Permission.ARCHIVE_READ: return ids.has(authUser.id) ? new Set([authUser.id]) : new Set(); case Permission.AUTH_DEVICE_DELETE: - return this.repository.authDevice.checkOwnerAccess(authUser.id, ids); + return await this.repository.authDevice.checkOwnerAccess(authUser.id, ids); case Permission.TIMELINE_READ: { const isOwner = ids.has(authUser.id) ? new Set([authUser.id]) : new Set(); @@ -205,22 +233,22 @@ export class AccessCore { } case Permission.LIBRARY_UPDATE: - return this.repository.library.checkOwnerAccess(authUser.id, ids); + return await this.repository.library.checkOwnerAccess(authUser.id, ids); case Permission.LIBRARY_DELETE: - return this.repository.library.checkOwnerAccess(authUser.id, ids); + return await this.repository.library.checkOwnerAccess(authUser.id, ids); case Permission.PERSON_READ: - return this.repository.person.checkOwnerAccess(authUser.id, ids); + return await this.repository.person.checkOwnerAccess(authUser.id, ids); case Permission.PERSON_WRITE: - return this.repository.person.checkOwnerAccess(authUser.id, ids); + return await this.repository.person.checkOwnerAccess(authUser.id, ids); case Permission.PERSON_MERGE: - return this.repository.person.checkOwnerAccess(authUser.id, ids); + return await this.repository.person.checkOwnerAccess(authUser.id, ids); case Permission.PARTNER_UPDATE: - return this.repository.partner.checkUpdateAccess(authUser.id, ids); + return await this.repository.partner.checkUpdateAccess(authUser.id, ids); } const allowedIds = new Set(); @@ -247,41 +275,6 @@ export class AccessCore { (await this.repository.activity.hasAlbumOwnerAccess(authUser.id, id)) ); - 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_RESTORE: - 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)) - ); - default: return false; } diff --git a/server/src/domain/album/album.service.spec.ts b/server/src/domain/album/album.service.spec.ts index e89030538..9a4614c79 100644 --- a/server/src/domain/album/album.service.spec.ts +++ b/server/src/domain/album/album.service.spec.ts @@ -509,7 +509,7 @@ describe(AlbumService.name, () => { describe('addAssets', () => { it('should allow the owner to add assets', async () => { accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -534,7 +534,7 @@ describe(AlbumService.name, () => { it('should not set the thumbnail if the album has one already', async () => { accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); albumMock.getById.mockResolvedValue(_.cloneDeep({ ...albumStub.empty, albumThumbnailAssetId: 'asset-id' })); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -552,7 +552,7 @@ describe(AlbumService.name, () => { it('should allow a shared user to add assets', async () => { accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123'])); - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.sharedWithUser)); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -577,7 +577,7 @@ describe(AlbumService.name, () => { it('should allow a shared link user to add assets', async () => { accessMock.album.checkSharedLinkAccess.mockResolvedValue(new Set(['album-123'])); - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2', 'asset-3'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -607,8 +607,7 @@ describe(AlbumService.name, () => { it('should allow adding assets shared via partner sharing', async () => { accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); - accessMock.asset.hasPartnerAccess.mockResolvedValue(true); + accessMock.asset.checkPartnerAccess.mockResolvedValue(new Set(['asset-1'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -621,12 +620,12 @@ describe(AlbumService.name, () => { updatedAt: expect.any(Date), albumThumbnailAssetId: 'asset-1', }); - expect(accessMock.asset.hasPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1'); + expect(accessMock.asset.checkPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1'])); }); it('should skip duplicate assets', async () => { accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-id'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValueOnce(new Set(['asset-id'])); @@ -639,8 +638,6 @@ describe(AlbumService.name, () => { it('should skip assets not shared with user', async () => { accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); - accessMock.asset.hasPartnerAccess.mockResolvedValue(false); albumMock.getById.mockResolvedValue(albumStub.oneAsset); albumMock.getAssetIds.mockResolvedValueOnce(new Set()); @@ -648,8 +645,8 @@ describe(AlbumService.name, () => { { success: false, id: 'asset-1', error: BulkIdErrorReason.NO_PERMISSION }, ]); - expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1'); - expect(accessMock.asset.hasPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1'); + expect(accessMock.asset.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1'])); + expect(accessMock.asset.checkPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1'])); }); it('should not allow unauthorized access to the album', async () => { diff --git a/server/src/domain/asset/asset.service.spec.ts b/server/src/domain/asset/asset.service.spec.ts index 28a138254..e4052fb34 100644 --- a/server/src/domain/asset/asset.service.spec.ts +++ b/server/src/domain/asset/asset.service.spec.ts @@ -457,19 +457,15 @@ describe(AssetService.name, () => { describe('downloadFile', () => { it('should require the asset.download permission', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); - accessMock.asset.hasAlbumAccess.mockResolvedValue(false); - accessMock.asset.hasPartnerAccess.mockResolvedValue(false); - await expect(sut.downloadFile(authStub.admin, 'asset-1')).rejects.toBeInstanceOf(BadRequestException); - expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1'); - expect(accessMock.asset.hasAlbumAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1'); - expect(accessMock.asset.hasPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, 'asset-1'); + expect(accessMock.asset.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1'])); + expect(accessMock.asset.checkAlbumAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1'])); + expect(accessMock.asset.checkPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set(['asset-1'])); }); it('should throw an error if the asset is not found', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); assetMock.getByIds.mockResolvedValue([]); await expect(sut.downloadFile(authStub.admin, 'asset-1')).rejects.toBeInstanceOf(BadRequestException); @@ -480,7 +476,7 @@ describe(AssetService.name, () => { it('should download a file', async () => { const stream = new Readable(); - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); assetMock.getByIds.mockResolvedValue([assetStub.image]); storageMock.createReadStream.mockResolvedValue({ stream }); @@ -496,7 +492,7 @@ describe(AssetService.name, () => { stream: new Readable(), }; - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2'])); assetMock.getByIds.mockResolvedValue([assetStub.noResizePath, assetStub.noWebpPath]); storageMock.createZipStream.mockReturnValue(archiveMock); @@ -516,7 +512,7 @@ describe(AssetService.name, () => { stream: new Readable(), }; - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2'])); assetMock.getByIds.mockResolvedValue([assetStub.noResizePath, assetStub.noResizePath]); storageMock.createZipStream.mockReturnValue(archiveMock); @@ -536,7 +532,7 @@ describe(AssetService.name, () => { }); it('should return a list of archives (assetIds)', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2'])); assetMock.getByIds.mockResolvedValue([assetStub.image, assetStub.video]); const assetIds = ['asset-1', 'asset-2']; @@ -602,7 +598,9 @@ describe(AssetService.name, () => { }); it('should include the video portion of a live photo', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + const assetIds = [assetStub.livePhotoStillAsset.id]; + + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(assetIds)); when(assetMock.getByIds) .calledWith([assetStub.livePhotoStillAsset.id]) .mockResolvedValue([assetStub.livePhotoStillAsset]); @@ -610,7 +608,6 @@ describe(AssetService.name, () => { .calledWith([assetStub.livePhotoMotionAsset.id]) .mockResolvedValue([assetStub.livePhotoMotionAsset]); - const assetIds = [assetStub.livePhotoStillAsset.id]; await expect(sut.getDownloadInfo(authStub.admin, { assetIds })).resolves.toEqual({ totalSize: 125_000, archives: [ @@ -651,7 +648,6 @@ describe(AssetService.name, () => { describe('update', () => { it('should require asset write access for the id', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); await expect(sut.update(authStub.admin, 'asset-1', { isArchived: false })).rejects.toBeInstanceOf( BadRequestException, ); @@ -659,14 +655,14 @@ describe(AssetService.name, () => { }); it('should update the asset', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); assetMock.save.mockResolvedValue(assetStub.image); await sut.update(authStub.admin, 'asset-1', { isFavorite: true }); expect(assetMock.save).toHaveBeenCalledWith({ id: 'asset-1', isFavorite: true }); }); it('should update the exif description', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); assetMock.save.mockResolvedValue(assetStub.image); await sut.update(authStub.admin, 'asset-1', { description: 'Test description' }); expect(assetMock.upsertExif).toHaveBeenCalledWith({ assetId: 'asset-1', description: 'Test description' }); @@ -675,7 +671,6 @@ describe(AssetService.name, () => { describe('updateAll', () => { it('should require asset write access for all ids', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); await expect( sut.updateAll(authStub.admin, { ids: ['asset-1'], @@ -685,7 +680,7 @@ describe(AssetService.name, () => { }); it('should update all assets', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2'])); await sut.updateAll(authStub.admin, { ids: ['asset-1', 'asset-2'], isArchived: true }); expect(assetMock.updateAll).toHaveBeenCalledWith(['asset-1', 'asset-2'], { isArchived: true }); }); @@ -693,8 +688,7 @@ describe(AssetService.name, () => { /// Stack related it('should require asset update access for parent', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); - when(accessMock.asset.hasOwnerAccess).calledWith(authStub.user1.id, 'parent').mockResolvedValue(false); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); await expect( sut.updateAll(authStub.user1, { ids: ['asset-1'], @@ -704,7 +698,7 @@ describe(AssetService.name, () => { }); it('should update parent asset when children are added', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['parent'])); await sut.updateAll(authStub.user1, { ids: [], stackParentId: 'parent', @@ -713,7 +707,7 @@ describe(AssetService.name, () => { }); it('should update parent asset when children are removed', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['child-1'])); assetMock.getByIds.mockResolvedValue([{ id: 'child-1', stackParentId: 'parent' } as AssetEntity]); await sut.updateAll(authStub.user1, { @@ -724,7 +718,8 @@ describe(AssetService.name, () => { }); it('update parentId for new children', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['child-1', 'child-2'])); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['parent'])); await sut.updateAll(authStub.user1, { stackParentId: 'parent', ids: ['child-1', 'child-2'], @@ -734,7 +729,7 @@ describe(AssetService.name, () => { }); it('nullify parentId for remove children', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['child-1', 'child-2'])); await sut.updateAll(authStub.user1, { removeParent: true, ids: ['child-1', 'child-2'], @@ -744,7 +739,8 @@ describe(AssetService.name, () => { }); it('merge stacks if new child has children', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['child-1'])); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['parent'])); assetMock.getByIds.mockResolvedValue([ { id: 'child-1', stack: [{ id: 'child-2' } as AssetEntity] } as AssetEntity, ]); @@ -758,7 +754,9 @@ describe(AssetService.name, () => { }); it('should send ws asset update event', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['asset-1'])); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['parent'])); + await sut.updateAll(authStub.user1, { ids: ['asset-1'], stackParentId: 'parent', @@ -772,7 +770,6 @@ describe(AssetService.name, () => { describe('deleteAll', () => { it('should require asset delete access for all ids', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); await expect( sut.deleteAll(authStub.user1, { ids: ['asset-1'], @@ -781,7 +778,7 @@ describe(AssetService.name, () => { }); it('should force delete a batch of assets', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset1', 'asset2'])); await sut.deleteAll(authStub.user1, { ids: ['asset1', 'asset2'], force: true }); @@ -792,7 +789,7 @@ describe(AssetService.name, () => { }); it('should soft delete a batch of assets', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset1', 'asset2'])); await sut.deleteAll(authStub.user1, { ids: ['asset1', 'asset2'], force: false }); @@ -810,7 +807,6 @@ describe(AssetService.name, () => { describe('restoreAll', () => { it('should require asset restore access for all ids', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); await expect( sut.deleteAll(authStub.user1, { ids: ['asset-1'], @@ -819,7 +815,7 @@ describe(AssetService.name, () => { }); it('should restore a batch of assets', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset1', 'asset2'])); await sut.restoreAll(authStub.user1, { ids: ['asset1', 'asset2'] }); @@ -984,19 +980,19 @@ describe(AssetService.name, () => { describe('run', () => { it('should run the refresh metadata job', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); await sut.run(authStub.admin, { assetIds: ['asset-1'], name: AssetJobName.REFRESH_METADATA }), expect(jobMock.queue).toHaveBeenCalledWith({ name: JobName.METADATA_EXTRACTION, data: { id: 'asset-1' } }); }); it('should run the refresh thumbnails job', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); await sut.run(authStub.admin, { assetIds: ['asset-1'], name: AssetJobName.REGENERATE_THUMBNAIL }), expect(jobMock.queue).toHaveBeenCalledWith({ name: JobName.GENERATE_JPEG_THUMBNAIL, data: { id: 'asset-1' } }); }); it('should run the transcode video', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); await sut.run(authStub.admin, { assetIds: ['asset-1'], name: AssetJobName.TRANSCODE_VIDEO }), expect(jobMock.queue).toHaveBeenCalledWith({ name: JobName.VIDEO_CONVERSION, data: { id: 'asset-1' } }); }); @@ -1004,9 +1000,7 @@ describe(AssetService.name, () => { describe('updateStackParent', () => { it('should require asset update access for new parent', async () => { - when(accessMock.asset.hasOwnerAccess).calledWith(authStub.user1.id, 'old').mockResolvedValue(true); - when(accessMock.asset.hasOwnerAccess).calledWith(authStub.user1.id, 'new').mockResolvedValue(false); - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['old'])); await expect( sut.updateStackParent(authStub.user1, { oldParentId: 'old', @@ -1016,8 +1010,7 @@ describe(AssetService.name, () => { }); it('should require asset read access for old parent', async () => { - when(accessMock.asset.hasOwnerAccess).calledWith(authStub.user1.id, 'old').mockResolvedValue(false); - when(accessMock.asset.hasOwnerAccess).calledWith(authStub.user1.id, 'new').mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['new'])); await expect( sut.updateStackParent(authStub.user1, { oldParentId: 'old', @@ -1027,7 +1020,9 @@ describe(AssetService.name, () => { }); it('make old parent the child of new parent', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set([assetStub.image.id])); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['new'])); + when(assetMock.getById) .calledWith(assetStub.image.id) .mockResolvedValue(assetStub.image as AssetEntity); @@ -1041,7 +1036,9 @@ describe(AssetService.name, () => { }); it('remove stackParentId of new parent', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set([assetStub.primaryImage.id])); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['new'])); + await sut.updateStackParent(authStub.user1, { oldParentId: assetStub.primaryImage.id, newParentId: 'new', @@ -1051,7 +1048,8 @@ describe(AssetService.name, () => { }); it('update stackParentId of old parents children to new parent', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set([assetStub.primaryImage.id])); + accessMock.asset.checkOwnerAccess.mockResolvedValueOnce(new Set(['new'])); when(assetMock.getById) .calledWith(assetStub.primaryImage.id) .mockResolvedValue(assetStub.primaryImage as AssetEntity); diff --git a/server/src/domain/domain.util.ts b/server/src/domain/domain.util.ts index 00ad27bc7..1b1c0a3e2 100644 --- a/server/src/domain/domain.util.ts +++ b/server/src/domain/domain.util.ts @@ -155,18 +155,35 @@ export function Optional({ nullable, ...validationOptions }: OptionalOptions = { // 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); +export const setUnion = (...sets: Set[]): Set => { + const union = new Set(sets[0]); + for (const set of sets.slice(1)) { + for (const elem of set) { + union.add(elem); + } } return union; }; -export const setDifference = (setA: Set, setB: Set): Set => { +export const setDifference = (setA: Set, ...sets: Set[]): Set => { const difference = new Set(setA); - for (const elem of setB) { - difference.delete(elem); + for (const set of sets) { + for (const elem of set) { + difference.delete(elem); + } } return difference; }; + +export const setIsSuperset = (set: Set, subset: Set): boolean => { + for (const elem of subset) { + if (!set.has(elem)) { + return false; + } + } + return true; +}; + +export const setIsEqual = (setA: Set, setB: Set): boolean => { + return setA.size === setB.size && setIsSuperset(setA, setB); +}; diff --git a/server/src/domain/library/library.service.spec.ts b/server/src/domain/library/library.service.spec.ts index c7e15e960..f49600a36 100644 --- a/server/src/domain/library/library.service.spec.ts +++ b/server/src/domain/library/library.service.spec.ts @@ -58,7 +58,8 @@ describe(LibraryService.name, () => { ctime: new Date('2023-01-01'), } as Stats); - accessMock.library.checkOwnerAccess.mockResolvedValue(new Set([authStub.admin.id])); + // Always validate owner access for library. + accessMock.library.checkOwnerAccess.mockImplementation(async (_, libraryIds) => libraryIds); sut = new LibraryService( accessMock, diff --git a/server/src/domain/person/person.service.spec.ts b/server/src/domain/person/person.service.spec.ts index b210a9165..44c20712b 100644 --- a/server/src/domain/person/person.service.spec.ts +++ b/server/src/domain/person/person.service.spec.ts @@ -331,7 +331,7 @@ describe(PersonService.name, () => { personMock.getById.mockResolvedValue(personStub.withName); personMock.update.mockResolvedValue(personStub.withName); personMock.getFacesByIds.mockResolvedValue([faceStub.face1]); - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); accessMock.person.checkOwnerAccess.mockResolvedValue(new Set(['person-1'])); await expect( diff --git a/server/src/domain/repositories/access.repository.ts b/server/src/domain/repositories/access.repository.ts index 7736fd890..b6a71daf8 100644 --- a/server/src/domain/repositories/access.repository.ts +++ b/server/src/domain/repositories/access.repository.ts @@ -6,11 +6,12 @@ export interface IAccessRepository { hasAlbumOwnerAccess(userId: string, activityId: string): Promise; hasCreateAccess(userId: string, albumId: 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; + checkOwnerAccess(userId: string, assetIds: Set): Promise>; + checkAlbumAccess(userId: string, assetIds: Set): Promise>; + checkPartnerAccess(userId: string, assetIds: Set): Promise>; + checkSharedLinkAccess(sharedLinkId: string, assetIds: Set): Promise>; }; authDevice: { 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 abf8128c4..bfc74e824 100644 --- a/server/src/domain/shared-link/shared-link.service.spec.ts +++ b/server/src/domain/shared-link/shared-link.service.spec.ts @@ -11,7 +11,6 @@ import { sharedLinkResponseStub, sharedLinkStub, } from '@test'; -import { when } from 'jest-when'; import _ from 'lodash'; import { AssetIdErrorReason } from '../asset'; import { ICryptoRepository, ISharedLinkRepository } from '../repositories'; @@ -109,7 +108,6 @@ describe(SharedLinkService.name, () => { }); it('should require asset ownership to make an individual shared link', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); await expect( sut.create(authStub.admin, { type: SharedLinkType.INDIVIDUAL, assetIds: ['asset-1'] }), ).rejects.toBeInstanceOf(BadRequestException); @@ -140,7 +138,7 @@ describe(SharedLinkService.name, () => { }); it('should create an individual shared link', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); shareMock.create.mockResolvedValue(sharedLinkStub.individual); await sut.create(authStub.admin, { @@ -151,7 +149,7 @@ describe(SharedLinkService.name, () => { allowUpload: true, }); - expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, assetStub.image.id); + expect(accessMock.asset.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set([assetStub.image.id])); expect(shareMock.create).toHaveBeenCalledWith({ type: SharedLinkType.INDIVIDUAL, userId: authStub.admin.id, @@ -215,9 +213,7 @@ describe(SharedLinkService.name, () => { it('should add assets to a shared link', async () => { shareMock.get.mockResolvedValue(_.cloneDeep(sharedLinkStub.individual)); shareMock.create.mockResolvedValue(sharedLinkStub.individual); - - when(accessMock.asset.hasOwnerAccess).calledWith(authStub.admin.id, 'asset-2').mockResolvedValue(false); - when(accessMock.asset.hasOwnerAccess).calledWith(authStub.admin.id, 'asset-3').mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-3'])); await expect( sut.addAssets(authStub.admin, 'link-1', { assetIds: [assetStub.image.id, 'asset-2', 'asset-3'] }), @@ -227,7 +223,7 @@ describe(SharedLinkService.name, () => { { assetId: 'asset-3', success: true }, ]); - expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledTimes(2); + expect(accessMock.asset.checkOwnerAccess).toHaveBeenCalledTimes(1); expect(shareMock.update).toHaveBeenCalledWith({ ...sharedLinkStub.individual, assets: [assetStub.image, { id: 'asset-3' }], 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 11173b55f..9b1e02b9c 100644 --- a/server/src/immich/api-v1/asset/asset.service.spec.ts +++ b/server/src/immich/api-v1/asset/asset.service.spec.ts @@ -232,54 +232,49 @@ describe('AssetService', () => { describe('getAssetById', () => { it('should allow owner access', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(true); + accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); assetRepositoryMock.getById.mockResolvedValue(assetStub.image); await sut.getAssetById(authStub.admin, assetStub.image.id); - expect(accessMock.asset.hasOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, assetStub.image.id); + expect(accessMock.asset.checkOwnerAccess).toHaveBeenCalledWith(authStub.admin.id, new Set([assetStub.image.id])); }); it('should allow shared link access', async () => { - accessMock.asset.hasSharedLinkAccess.mockResolvedValue(true); + accessMock.asset.checkSharedLinkAccess.mockResolvedValue(new Set([assetStub.image.id])); assetRepositoryMock.getById.mockResolvedValue(assetStub.image); await sut.getAssetById(authStub.adminSharedLink, assetStub.image.id); - expect(accessMock.asset.hasSharedLinkAccess).toHaveBeenCalledWith( + expect(accessMock.asset.checkSharedLinkAccess).toHaveBeenCalledWith( authStub.adminSharedLink.sharedLinkId, - assetStub.image.id, + new Set([assetStub.image.id]), ); }); it('should allow partner sharing access', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); - accessMock.asset.hasPartnerAccess.mockResolvedValue(true); + accessMock.asset.checkPartnerAccess.mockResolvedValue(new Set([assetStub.image.id])); assetRepositoryMock.getById.mockResolvedValue(assetStub.image); await sut.getAssetById(authStub.admin, assetStub.image.id); - expect(accessMock.asset.hasPartnerAccess).toHaveBeenCalledWith(authStub.admin.id, assetStub.image.id); + expect(accessMock.asset.checkPartnerAccess).toHaveBeenCalledWith( + authStub.admin.id, + new Set([assetStub.image.id]), + ); }); it('should allow shared album access', async () => { - accessMock.asset.hasOwnerAccess.mockResolvedValue(false); - accessMock.asset.hasPartnerAccess.mockResolvedValue(false); - accessMock.asset.hasAlbumAccess.mockResolvedValue(true); + accessMock.asset.checkAlbumAccess.mockResolvedValue(new Set([assetStub.image.id])); assetRepositoryMock.getById.mockResolvedValue(assetStub.image); await sut.getAssetById(authStub.admin, assetStub.image.id); - expect(accessMock.asset.hasAlbumAccess).toHaveBeenCalledWith(authStub.admin.id, assetStub.image.id); + expect(accessMock.asset.checkAlbumAccess).toHaveBeenCalledWith(authStub.admin.id, new Set([assetStub.image.id])); }); it('should throw an error for no access', async () => { - 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, assetStub.image.id)).rejects.toBeInstanceOf(BadRequestException); expect(assetRepositoryMock.getById).not.toHaveBeenCalled(); }); it('should throw an error for an invalid shared link', async () => { - accessMock.asset.hasSharedLinkAccess.mockResolvedValue(false); await expect(sut.getAssetById(authStub.adminSharedLink, assetStub.image.id)).rejects.toBeInstanceOf( BadRequestException, ); - expect(accessMock.asset.hasOwnerAccess).not.toHaveBeenCalled(); + expect(accessMock.asset.checkOwnerAccess).not.toHaveBeenCalled(); expect(assetRepositoryMock.getById).not.toHaveBeenCalled(); }); }); diff --git a/server/src/infra/repositories/access.repository.ts b/server/src/infra/repositories/access.repository.ts index b23c559a6..208b7095c 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 { In, Repository } from 'typeorm'; +import { Brackets, In, Repository } from 'typeorm'; import { ActivityEntity, AlbumEntity, @@ -112,107 +112,120 @@ export class AccessRepository implements IAccessRepository { }; asset = { - hasAlbumAccess: (userId: string, assetId: string): Promise => { - return this.albumRepository.exist({ - where: [ - { + checkAlbumAccess: async (userId: string, assetIds: Set): Promise> => { + if (assetIds.size === 0) { + return new Set(); + } + + return this.albumRepository + .createQueryBuilder('album') + .innerJoin('album.assets', 'asset') + .leftJoin('album.sharedUsers', 'sharedUsers') + .select('asset.id', 'assetId') + .addSelect('asset.livePhotoVideoId', 'livePhotoVideoId') + .where( + new Brackets((qb) => { + qb.where('album.ownerId = :userId', { userId }).orWhere('sharedUsers.id = :userId', { userId }); + }), + ) + .andWhere( + new Brackets((qb) => { + qb.where('asset.id IN (:...assetIds)', { assetIds: [...assetIds] }) + // still part of a live photo is in an album + .orWhere('asset.livePhotoVideoId IN (:...assetIds)', { assetIds: [...assetIds] }); + }), + ) + .getRawMany() + .then((rows) => { + const allowedIds = new Set(); + for (const row of rows) { + if (row.assetId && assetIds.has(row.assetId)) { + allowedIds.add(row.assetId); + } + if (row.livePhotoVideoId && assetIds.has(row.livePhotoVideoId)) { + allowedIds.add(row.livePhotoVideoId); + } + } + return allowedIds; + }); + }, + + checkOwnerAccess: async (userId: string, assetIds: Set): Promise> => { + if (assetIds.size === 0) { + return new Set(); + } + + return this.assetRepository + .find({ + select: { id: true }, + where: { + id: In([...assetIds]), ownerId: userId, - assets: { - id: assetId, - }, }, - { - sharedUsers: { - id: userId, - }, - assets: { - id: assetId, - }, - }, - // still part of a live photo is in an album - { - ownerId: userId, - assets: { - livePhotoVideoId: assetId, - }, - }, - { - sharedUsers: { - id: userId, - }, - assets: { - livePhotoVideoId: assetId, - }, - }, - ], - }); + withDeleted: true, + }) + .then((assets) => new Set(assets.map((asset) => asset.id))); }, - hasOwnerAccess: (userId: string, assetId: string): Promise => { - return this.assetRepository.exist({ - where: { - id: assetId, - ownerId: userId, - }, - withDeleted: true, - }); + checkPartnerAccess: async (userId: string, assetIds: Set): Promise> => { + if (assetIds.size === 0) { + return new Set(); + } + + return this.partnerRepository + .createQueryBuilder('partner') + .innerJoin('partner.sharedBy', 'sharedBy') + .innerJoin('sharedBy.assets', 'asset') + .select('asset.id', 'assetId') + .where('partner.sharedWithId = :userId', { userId }) + .andWhere('asset.id IN (:...assetIds)', { assetIds: [...assetIds] }) + .getRawMany() + .then((rows) => new Set(rows.map((row) => row.assetId))); }, - hasPartnerAccess: (userId: string, assetId: string): Promise => { - return this.partnerRepository.exist({ - where: { - sharedWith: { - id: userId, - }, - sharedBy: { - assets: { - id: assetId, - }, - }, - }, - relations: { - sharedWith: true, - sharedBy: { - assets: true, - }, - }, - }); - }, + checkSharedLinkAccess: async (sharedLinkId: string, assetIds: Set): Promise> => { + if (assetIds.size === 0) { + return new Set(); + } - hasSharedLinkAccess: async (sharedLinkId: string, assetId: string): Promise => { - return this.sharedLinkRepository.exist({ - where: [ - { - id: sharedLinkId, - album: { - assets: { - id: assetId, - }, - }, - }, - { - id: sharedLinkId, - assets: { - id: assetId, - }, - }, - // still part of a live photo is in a shared link - { - id: sharedLinkId, - album: { - assets: { - livePhotoVideoId: assetId, - }, - }, - }, - { - id: sharedLinkId, - assets: { - livePhotoVideoId: assetId, - }, - }, - ], - }); + return this.sharedLinkRepository + .createQueryBuilder('sharedLink') + .leftJoin('sharedLink.album', 'album') + .leftJoin('sharedLink.assets', 'assets') + .leftJoin('album.assets', 'albumAssets') + .select('assets.id', 'assetId') + .addSelect('albumAssets.id', 'albumAssetId') + .addSelect('assets.livePhotoVideoId', 'assetLivePhotoVideoId') + .addSelect('albumAssets.livePhotoVideoId', 'albumAssetLivePhotoVideoId') + .where('sharedLink.id = :sharedLinkId', { sharedLinkId }) + .andWhere( + new Brackets((qb) => { + qb.where('assets.id IN (:...assetIds)', { assetIds: [...assetIds] }) + .orWhere('albumAssets.id IN (:...assetIds)', { assetIds: [...assetIds] }) + // still part of a live photo is in a shared link + .orWhere('assets.livePhotoVideoId IN (:...assetIds)', { assetIds: [...assetIds] }) + .orWhere('albumAssets.livePhotoVideoId IN (:...assetIds)', { assetIds: [...assetIds] }); + }), + ) + .getRawMany() + .then((rows) => { + const allowedIds = new Set(); + for (const row of rows) { + if (row.assetId && assetIds.has(row.assetId)) { + allowedIds.add(row.assetId); + } + if (row.assetLivePhotoVideoId && assetIds.has(row.assetLivePhotoVideoId)) { + allowedIds.add(row.assetLivePhotoVideoId); + } + if (row.albumAssetId && assetIds.has(row.albumAssetId)) { + allowedIds.add(row.albumAssetId); + } + if (row.albumAssetLivePhotoVideoId && assetIds.has(row.albumAssetLivePhotoVideoId)) { + allowedIds.add(row.albumAssetLivePhotoVideoId); + } + } + return allowedIds; + }); }, }; diff --git a/server/test/repositories/access.repository.mock.ts b/server/test/repositories/access.repository.mock.ts index f495d800e..4c2a5ed8d 100644 --- a/server/test/repositories/access.repository.mock.ts +++ b/server/test/repositories/access.repository.mock.ts @@ -22,11 +22,12 @@ export const newAccessRepositoryMock = (reset = true): IAccessRepositoryMock => hasAlbumOwnerAccess: jest.fn(), hasCreateAccess: jest.fn(), }, + asset: { - hasOwnerAccess: jest.fn(), - hasAlbumAccess: jest.fn(), - hasPartnerAccess: jest.fn(), - hasSharedLinkAccess: jest.fn(), + checkOwnerAccess: jest.fn().mockResolvedValue(new Set()), + checkAlbumAccess: jest.fn().mockResolvedValue(new Set()), + checkPartnerAccess: jest.fn().mockResolvedValue(new Set()), + checkSharedLinkAccess: jest.fn().mockResolvedValue(new Set()), }, album: {