From 1f422ea4c4b60e1bfb2c8e551faead02d276f247 Mon Sep 17 00:00:00 2001 From: Yaros Date: Tue, 27 Oct 2026 19:23:10 +0100 Subject: [PATCH] fix(server): add album-shared assets to albums (#32609) Co-authored-by: Jason Rasmussen (cherry picked from commit 37c34343e1b98edf07dc3ff34c5da461de03c746) --- e2e/src/specs/server/api/album.e2e-spec.ts | 12 +--- server/src/services/album.service.spec.ts | 67 ++++++++++++++++++++++ server/src/utils/access.ts | 8 ++- 3 files changed, 76 insertions(+), 11 deletions(-) diff --git a/e2e/src/specs/server/api/album.e2e-spec.ts b/e2e/src/specs/server/api/album.e2e-spec.ts index db1d00f6a..7bb62ff38 100644 --- a/e2e/src/specs/server/api/album.e2e-spec.ts +++ b/e2e/src/specs/server/api/album.e2e-spec.ts @@ -628,25 +628,19 @@ describe('/albums', () => { ]); }); - it('should not be able to remove foreign asset from foreign album', async () => { + it('should be able to remove foreign asset from shared album as an editor', async () => { const { status, body } = await request(app) .delete(`/albums/${user1Albums[0].id}/assets`) .set('Authorization', `Bearer ${user2.accessToken}`) .send({ ids: [user1Asset1.id] }); expect(status).toBe(200); - expect(body).toEqual([ - expect.objectContaining({ - id: user1Asset1.id, - success: false, - error: 'no_permission', - }), - ]); + expect(body).toEqual([expect.objectContaining({ id: user1Asset1.id, success: true })]); }); it('should be able to remove own asset from own album', async () => { const { status, body } = await request(app) - .delete(`/albums/${user1Albums[0].id}/assets`) + .delete(`/albums/${user1Albums[2].id}/assets`) .set('Authorization', `Bearer ${user1.accessToken}`) .send({ ids: [user1Asset1.id] }); diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index 92ca72f01..10ab5c0c8 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -888,6 +888,24 @@ describe(AlbumService.name, () => { expect(mocks.access.asset.checkPartnerAccess).toHaveBeenCalledWith(owner.id, new Set([asset.id])); }); + it('should allow adding assets shared via album sharing', async () => { + const album = AlbumFactory.create(); + const { user: owner } = album.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; + const asset = AssetFactory.create(); + mocks.access.album.checkOwnerAccess.mockResolvedValue(new Set([album.id])); + mocks.access.asset.checkAlbumAccess.mockResolvedValue(new Set([asset.id])); + mocks.album.getById.mockResolvedValue(getForAlbum(album)); + mocks.album.getAssetIds.mockResolvedValueOnce(new Set()); + + await expect(sut.addAssets(AuthFactory.create(owner), album.id, { ids: [asset.id] })).resolves.toEqual([ + { success: true, id: asset.id }, + ]); + + expect(mocks.album.addAssetIds).toHaveBeenCalledWith(album.id, [asset.id]); + expect(mocks.access.asset.checkOwnerAccess).toHaveBeenCalledWith(owner.id, new Set([asset.id]), false); + expect(mocks.access.asset.checkAlbumAccess).toHaveBeenCalledWith(owner.id, new Set([asset.id])); + }); + it('should skip duplicate assets', async () => { const asset = AssetFactory.create(); const album = AlbumFactory.create(); @@ -917,6 +935,7 @@ describe(AlbumService.name, () => { ]); expect(mocks.access.asset.checkOwnerAccess).toHaveBeenCalledWith(owner.id, new Set([asset.id]), false); + expect(mocks.access.asset.checkAlbumAccess).toHaveBeenCalledWith(owner.id, new Set([asset.id])); expect(mocks.access.asset.checkPartnerAccess).toHaveBeenCalledWith(owner.id, new Set([asset.id])); }); @@ -1185,6 +1204,33 @@ describe(AlbumService.name, () => { ); }); + it('should allow adding assets shared via album sharing', async () => { + const user = UserFactory.create(); + const album1 = AlbumFactory.create(); + const { user: owner } = album1.albumUsers.find(({ role }) => role === AlbumUserRole.Owner)!; + const album2 = AlbumFactory.create(); + const [asset1, asset2] = [AssetFactory.create({ ownerId: user.id }), AssetFactory.create({ ownerId: user.id })]; + mocks.access.album.checkOwnerAccess.mockResolvedValueOnce(new Set([album1.id, album2.id])); + mocks.access.asset.checkAlbumAccess.mockResolvedValue(new Set([asset1.id, asset2.id])); + mocks.album.getById.mockResolvedValueOnce(getForAlbum(album1)).mockResolvedValueOnce(getForAlbum(album2)); + mocks.album.getAssetIds.mockResolvedValueOnce(new Set()).mockResolvedValueOnce(new Set()); + + await expect( + sut.addAssetsToAlbums(AuthFactory.create(owner), { + albumIds: [album1.id, album2.id], + assetIds: [asset1.id, asset2.id], + }), + ).resolves.toEqual({ success: true, error: undefined }); + + expect(mocks.album.addAssetIdsToAlbums).toHaveBeenCalledWith([ + { albumId: album1.id, assetId: asset1.id }, + { albumId: album1.id, assetId: asset2.id }, + { albumId: album2.id, assetId: asset1.id }, + { albumId: album2.id, assetId: asset2.id }, + ]); + expect(mocks.access.asset.checkAlbumAccess).toHaveBeenCalledWith(owner.id, new Set([asset1.id, asset2.id])); + }); + it('should skip some duplicate assets', async () => { const [asset1, asset2, asset3] = [AssetFactory.create(), AssetFactory.create(), AssetFactory.create()]; const album1 = AlbumFactory.create(); @@ -1282,6 +1328,10 @@ describe(AlbumService.name, () => { new Set([asset1.id, asset2.id, asset3.id]), false, ); + expect(mocks.access.asset.checkAlbumAccess).toHaveBeenCalledWith( + owner.id, + new Set([asset1.id, asset2.id, asset3.id]), + ); expect(mocks.access.asset.checkPartnerAccess).toHaveBeenCalledWith( owner.id, new Set([asset1.id, asset2.id, asset3.id]), @@ -1348,6 +1398,23 @@ describe(AlbumService.name, () => { expect(mocks.album.removeAssetIds).toHaveBeenCalledWith(album.id, [asset.id]); }); + it('should allow a shared user to remove assets shared via album sharing', async () => { + const user = UserFactory.create(); + const album = AlbumFactory.from().albumUser({ userId: user.id, role: AlbumUserRole.Editor }).build(); + const asset = AssetFactory.create(); + mocks.access.album.checkSharedAlbumAccess.mockResolvedValue(new Set([album.id])); + mocks.access.asset.checkAlbumAccess.mockResolvedValue(new Set([asset.id])); + mocks.album.getById.mockResolvedValue(getForAlbum(album)); + mocks.album.getAssetIds.mockResolvedValue(new Set([asset.id])); + + await expect(sut.removeAssets(AuthFactory.create(user), album.id, { ids: [asset.id] })).resolves.toEqual([ + { success: true, id: asset.id }, + ]); + + expect(mocks.album.removeAssetIds).toHaveBeenCalledWith(album.id, [asset.id]); + expect(mocks.access.asset.checkAlbumAccess).toHaveBeenCalledWith(user.id, new Set([asset.id])); + }); + it('should skip assets not in the album', async () => { const asset = AssetFactory.create(); const album = AlbumFactory.create(); diff --git a/server/src/utils/access.ts b/server/src/utils/access.ts index f54c90461..b328b18c6 100644 --- a/server/src/utils/access.ts +++ b/server/src/utils/access.ts @@ -173,8 +173,12 @@ const checkOtherAccess = async (access: AccessRepository, request: OtherAccessRe case Permission.AssetShare: { const isOwner = await access.asset.checkOwnerAccess(auth.user.id, ids, false); - const isPartner = await access.asset.checkPartnerAccess(auth.user.id, setDifference(ids, isOwner)); - return setUnion(isOwner, isPartner); + const isAlbum = await access.asset.checkAlbumAccess(auth.user.id, setDifference(ids, isOwner)); + const isPartner = await access.asset.checkPartnerAccess( + auth.user.id, + setDifference(setDifference(ids, isOwner), isAlbum), + ); + return setUnion(setUnion(isOwner, isAlbum), isPartner); } case Permission.AssetFileDownload: {