From 5de01d2a52c30e2db075b7d07fbae78d757e2c0e Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Fri, 24 Nov 2023 00:38:52 -0300 Subject: [PATCH 1/3] chore(server): Check album permissions in bulk Modify Access repository, to evaluate `album` permissions in bulk. Queries have been validated to match what they currently generate for single ids. Queries: * Owner access: ```sql -- Before SELECT 1 AS "row_exists" FROM (SELECT 1 AS dummy_column) "dummy_table" WHERE EXISTS ( SELECT 1 FROM "albums" "AlbumEntity" WHERE "AlbumEntity"."id" = $1 AND "AlbumEntity"."ownerId" = $2 AND "AlbumEntity"."deletedAt" IS NULL ) LIMIT 1 -- After SELECT "AlbumEntity"."id" AS "AlbumEntity_id" FROM "albums" "AlbumEntity" WHERE "AlbumEntity"."id" IN ($1, $2) AND "AlbumEntity"."ownerId" = $3 AND "AlbumEntity"."deletedAt" IS NULL ``` * 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" WHERE "SharedLinkEntity"."id" = $1 AND "SharedLinkEntity"."albumId" = $2 ) LIMIT 1 -- After SELECT "SharedLinkEntity"."albumId" AS "SharedLinkEntity_albumId", "SharedLinkEntity"."id" AS "SharedLinkEntity_id" FROM "shared_links" "SharedLinkEntity" WHERE "SharedLinkEntity"."id" = $1 AND "SharedLinkEntity"."albumId" IN ($2, $3) ``` * Shared 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_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"."id" = $1 AND "AlbumEntity__AlbumEntity_sharedUsers"."id" = $2 AND "AlbumEntity"."deletedAt" IS NULL ) LIMIT 1 -- After SELECT "AlbumEntity"."id" AS "AlbumEntity_id" FROM "albums" "AlbumEntity" 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"."id" IN ($1, $2) AND "AlbumEntity__AlbumEntity_sharedUsers"."id" = $3 AND "AlbumEntity"."deletedAt" IS NULL ``` --- server/src/domain/access/access.core.ts | 77 +++++++++------- server/src/domain/activity/activity.spec.ts | 13 ++- server/src/domain/album/album.service.spec.ts | 91 +++++++++---------- server/src/domain/asset/asset.service.spec.ts | 8 +- .../domain/repositories/access.repository.ts | 6 +- .../shared-link/shared-link.service.spec.ts | 8 +- .../infra/repositories/access.repository.ts | 73 ++++++++++----- .../repositories/access.repository.mock.ts | 6 +- 8 files changed, 158 insertions(+), 124 deletions(-) diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts index 30086f5d26b6c..87787c78aaae1 100644 --- a/server/src/domain/access/access.core.ts +++ b/server/src/domain/access/access.core.ts @@ -99,6 +99,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 +144,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, ids); + return new Set([...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, ids); + return new Set([...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 +239,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/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/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..58df5a461f950 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,60 @@ 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) => { + return 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) => { + return 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) => { + return 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: { From d2103159195f7be61ea26dfc8cdd7c23d92cafbc Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Fri, 24 Nov 2023 22:27:29 -0300 Subject: [PATCH 2/3] chore(server): Add set utils, avoid double queries for same ids --- server/src/domain/access/access.core.ts | 9 +++++---- server/src/domain/album/album.service.ts | 3 ++- server/src/domain/domain.util.ts | 12 ++++++++++++ 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts index 87787c78aaae1..977c3274c5a7d 100644 --- a/server/src/domain/access/access.core.ts +++ b/server/src/domain/access/access.core.ts @@ -1,4 +1,5 @@ import { BadRequestException, UnauthorizedException } from '@nestjs/common'; +import { setDifference, setUnion } from '..'; import { AuthUserDto } from '../auth'; import { IAccessRepository } from '../repositories'; @@ -157,8 +158,8 @@ export class AccessCore { 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, ids); - return new Set([...isOwner, ...isShared]); + const isShared = await this.repository.album.checkSharedAlbumAccess(authUser.id, setDifference(ids, isOwner)); + return setUnion(isOwner, isShared); } case Permission.ALBUM_UPDATE: @@ -172,8 +173,8 @@ export class AccessCore { case Permission.ALBUM_DOWNLOAD: { const isOwner = await this.repository.album.checkOwnerAccess(authUser.id, ids); - const isShared = await this.repository.album.checkSharedAlbumAccess(authUser.id, ids); - return new Set([...isOwner, ...isShared]); + const isShared = await this.repository.album.checkSharedAlbumAccess(authUser.id, setDifference(ids, isOwner)); + return setUnion(isOwner, isShared); } case Permission.ALBUM_REMOVE_ASSET: diff --git a/server/src/domain/album/album.service.ts b/server/src/domain/album/album.service.ts index 986cdb891138c..0c1d5a5bdd0e0 100644 --- a/server/src/domain/album/album.service.ts +++ b/server/src/domain/album/album.service.ts @@ -1,5 +1,6 @@ import { AlbumEntity, AssetEntity, UserEntity } from '@app/infra/entities'; import { BadRequestException, Inject, Injectable } from '@nestjs/common'; +import { setUnion } from '..'; import { AccessCore, Permission } from '../access'; import { BulkIdErrorReason, BulkIdResponseDto, BulkIdsDto } from '../asset'; import { AuthUserDto } from '../auth'; @@ -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/domain.util.ts b/server/src/domain/domain.util.ts index 53e674bf30f61..7e01fe0408138 100644 --- a/server/src/domain/domain.util.ts +++ b/server/src/domain/domain.util.ts @@ -150,3 +150,15 @@ 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 => { + return new Set([...setA, ...setB]); +}; + +export const setDifference = (setA: Set, setB: Set): Set => { + return new Set([...setA].filter((x) => !setB.has(x))); +}; From a6c5c5c12453addc40d8e7c7c01094a666d3857d Mon Sep 17 00:00:00 2001 From: Michael Manganiello Date: Sat, 25 Nov 2023 15:55:43 -0300 Subject: [PATCH 3/3] chore(server): Review feedback --- server/src/domain/access/access.core.ts | 2 +- server/src/domain/album/album.service.ts | 2 +- server/src/domain/domain.util.ts | 12 ++++++++++-- .../src/infra/repositories/access.repository.ts | 15 ++++++--------- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/server/src/domain/access/access.core.ts b/server/src/domain/access/access.core.ts index 977c3274c5a7d..d829139a9542c 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 { setDifference, setUnion } from '..'; import { AuthUserDto } from '../auth'; +import { setDifference, setUnion } from '../domain.util'; import { IAccessRepository } from '../repositories'; export enum Permission { diff --git a/server/src/domain/album/album.service.ts b/server/src/domain/album/album.service.ts index 0c1d5a5bdd0e0..0d92dae04deed 100644 --- a/server/src/domain/album/album.service.ts +++ b/server/src/domain/album/album.service.ts @@ -1,9 +1,9 @@ import { AlbumEntity, AssetEntity, UserEntity } from '@app/infra/entities'; import { BadRequestException, Inject, Injectable } from '@nestjs/common'; -import { setUnion } from '..'; 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, diff --git a/server/src/domain/domain.util.ts b/server/src/domain/domain.util.ts index 7e01fe0408138..00ad27bc77ad7 100644 --- a/server/src/domain/domain.util.ts +++ b/server/src/domain/domain.util.ts @@ -156,9 +156,17 @@ export function Optional({ nullable, ...validationOptions }: OptionalOptions = { // Proposal reference: https://github.com/tc39/proposal-set-methods export const setUnion = (setA: Set, setB: Set): Set => { - return new Set([...setA, ...setB]); + const union = new Set(setA); + for (const elem of setB) { + union.add(elem); + } + return union; }; export const setDifference = (setA: Set, setB: Set): Set => { - return new Set([...setA].filter((x) => !setB.has(x))); + const difference = new Set(setA); + for (const elem of setB) { + difference.delete(elem); + } + return difference; }; diff --git a/server/src/infra/repositories/access.repository.ts b/server/src/infra/repositories/access.repository.ts index 58df5a461f950..fb0b865fb641f 100644 --- a/server/src/infra/repositories/access.repository.ts +++ b/server/src/infra/repositories/access.repository.ts @@ -222,9 +222,7 @@ export class AccessRepository implements IAccessRepository { ownerId: userId, }, }) - .then((albums) => { - return new Set(albums.map((album) => album.id)); - }); + .then((albums) => new Set(albums.map((album) => album.id))); }, checkSharedAlbumAccess: async (userId: string, albumIds: Set): Promise> => { @@ -242,9 +240,7 @@ export class AccessRepository implements IAccessRepository { }, }, }) - .then((albums) => { - return new Set(albums.map((album) => album.id)); - }); + .then((albums) => new Set(albums.map((album) => album.id))); }, checkSharedLinkAccess: async (sharedLinkId: string, albumIds: Set): Promise> => { @@ -260,9 +256,10 @@ export class AccessRepository implements IAccessRepository { albumId: In([...albumIds]), }, }) - .then((sharedLinks) => { - return new Set(sharedLinks.flatMap((sharedLink) => (!!sharedLink.albumId ? [sharedLink.albumId] : []))); - }); + .then( + (sharedLinks) => + new Set(sharedLinks.flatMap((sharedLink) => (!!sharedLink.albumId ? [sharedLink.albumId] : []))), + ); }, };