From 24c1855899961463d132e85b2446d05b76472bc1 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Sat, 29 Jun 2024 00:17:58 -0400 Subject: [PATCH] fix: album remove asset bug (#10687) * fix: album remove asset bug * trigger GH Action --------- Co-authored-by: Alex Tran --- e2e/src/api/specs/album.e2e-spec.ts | 23 +++++++++++++------ server/src/cores/access.core.ts | 4 ++++ server/src/services/album.service.spec.ts | 12 ++++------ server/src/services/album.service.ts | 7 +++--- server/src/services/memory.service.spec.ts | 9 -------- server/src/services/memory.service.ts | 9 +++++--- server/src/utils/asset.util.ts | 26 +++++++++------------- 7 files changed, 44 insertions(+), 46 deletions(-) diff --git a/e2e/src/api/specs/album.e2e-spec.ts b/e2e/src/api/specs/album.e2e-spec.ts index 7d260d2547..2a35eb3c92 100644 --- a/e2e/src/api/specs/album.e2e-spec.ts +++ b/e2e/src/api/specs/album.e2e-spec.ts @@ -88,7 +88,7 @@ describe('/albums', () => { }); await addAssetsToAlbum( - { id: user2Albums[0].id, bulkIdsDto: { ids: [user1Asset1.id] } }, + { id: user2Albums[0].id, bulkIdsDto: { ids: [user1Asset1.id, user1Asset2.id] } }, { headers: asBearerAuth(user1.accessToken) }, ); @@ -261,7 +261,7 @@ describe('/albums', () => { .get(`/albums?assetId=${user1Asset2.id}`) .set('Authorization', `Bearer ${user1.accessToken}`); expect(status).toBe(200); - expect(body).toHaveLength(1); + expect(body).toHaveLength(2); }); it('should return the album collection filtered by assetId and ignores shared=true', async () => { @@ -509,7 +509,17 @@ describe('/albums', () => { expect(body).toEqual(errorDto.unauthorized); }); - it('should not be able to remove foreign asset from own album', async () => { + it('should require authorization', async () => { + const { status, body } = await request(app) + .delete(`/albums/${user1Albums[1].id}/assets`) + .set('Authorization', `Bearer ${user2.accessToken}`) + .send({ ids: [user1Asset1.id] }); + + expect(status).toBe(400); + expect(body).toEqual(errorDto.noPermission); + }); + + it('should be able to remove foreign asset from owned album', async () => { const { status, body } = await request(app) .delete(`/albums/${user2Albums[0].id}/assets`) .set('Authorization', `Bearer ${user2.accessToken}`) @@ -519,8 +529,7 @@ describe('/albums', () => { expect(body).toEqual([ expect.objectContaining({ id: user1Asset1.id, - success: false, - error: 'no_permission', + success: true, }), ]); }); @@ -555,10 +564,10 @@ describe('/albums', () => { const { status, body } = await request(app) .delete(`/albums/${user2Albums[0].id}/assets`) .set('Authorization', `Bearer ${user1.accessToken}`) - .send({ ids: [user1Asset1.id] }); + .send({ ids: [user1Asset2.id] }); expect(status).toBe(200); - expect(body).toEqual([expect.objectContaining({ id: user1Asset1.id, success: true })]); + expect(body).toEqual([expect.objectContaining({ id: user1Asset2.id, success: true })]); }); it('should not be able to remove assets from album as a viewer', async () => { diff --git a/server/src/cores/access.core.ts b/server/src/cores/access.core.ts index ae666562cf..e857e9b5cc 100644 --- a/server/src/cores/access.core.ts +++ b/server/src/cores/access.core.ts @@ -307,6 +307,10 @@ export class AccessCore { return this.repository.memory.checkOwnerAccess(auth.user.id, ids); } + case Permission.MEMORY_DELETE: { + return this.repository.memory.checkOwnerAccess(auth.user.id, ids); + } + case Permission.PERSON_READ: { return await this.repository.person.checkOwnerAccess(auth.user.id, ids); } diff --git a/server/src/services/album.service.spec.ts b/server/src/services/album.service.spec.ts index 7a2df77710..c14dfe445e 100644 --- a/server/src/services/album.service.spec.ts +++ b/server/src/services/album.service.spec.ts @@ -762,20 +762,16 @@ describe(AlbumService.name, () => { expect(albumMock.update).not.toHaveBeenCalled(); }); - it('should skip assets when user has remove permission on album but not on asset', async () => { - accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123'])); + it('should allow owner to remove all assets from the album', async () => { + accessMock.album.checkOwnerAccess.mockResolvedValue(new Set(['album-123'])); albumMock.getById.mockResolvedValue(_.cloneDeep(albumStub.oneAsset)); albumMock.getAssetIds.mockResolvedValue(new Set(['asset-id'])); await expect(sut.removeAssets(authStub.admin, 'album-123', { ids: ['asset-id'] })).resolves.toEqual([ - { - success: false, - id: 'asset-id', - error: BulkIdErrorReason.NO_PERMISSION, - }, + { success: true, id: 'asset-id' }, ]); - expect(albumMock.update).not.toHaveBeenCalled(); + expect(albumMock.update).toHaveBeenCalledWith({ id: 'album-123', updatedAt: expect.any(Date) }); }); it('should reset the thumbnail if it is removed', async () => { diff --git a/server/src/services/album.service.ts b/server/src/services/album.service.ts index cf179bf289..69cbd71e43 100644 --- a/server/src/services/album.service.ts +++ b/server/src/services/album.service.ts @@ -178,7 +178,7 @@ export class AlbumService { const results = await addAssets( auth, { accessRepository: this.accessRepository, repository: this.albumRepository }, - { id, assetIds: dto.ids }, + { parentId: id, assetIds: dto.ids }, ); const { id: firstNewAssetId } = results.find(({ success }) => success) || {}; @@ -199,14 +199,13 @@ export class AlbumService { } async removeAssets(auth: AuthDto, id: string, dto: BulkIdsDto): Promise { - const album = await this.findOrFail(id, { withAssets: false }); - await this.access.requirePermission(auth, Permission.ALBUM_REMOVE_ASSET, id); + const album = await this.findOrFail(id, { withAssets: false }); const results = await removeAssets( auth, { accessRepository: this.accessRepository, repository: this.albumRepository }, - { id, assetIds: dto.ids, permissions: [Permission.ASSET_SHARE, Permission.ALBUM_REMOVE_ASSET] }, + { parentId: id, assetIds: dto.ids, canAlwaysRemove: Permission.ALBUM_DELETE }, ); const removedIds = results.filter(({ success }) => success).map(({ id }) => id); diff --git a/server/src/services/memory.service.spec.ts b/server/src/services/memory.service.spec.ts index b4dd4bd2ad..cee3113f00 100644 --- a/server/src/services/memory.service.spec.ts +++ b/server/src/services/memory.service.spec.ts @@ -193,15 +193,6 @@ describe(MemoryService.name, () => { expect(memoryMock.removeAssetIds).not.toHaveBeenCalled(); }); - it('should require asset access', async () => { - accessMock.memory.checkOwnerAccess.mockResolvedValue(new Set(['memory1'])); - memoryMock.getAssetIds.mockResolvedValue(new Set(['asset1'])); - await expect(sut.removeAssets(authStub.admin, 'memory1', { ids: ['asset1'] })).resolves.toEqual([ - { error: 'no_permission', id: 'asset1', success: false }, - ]); - expect(memoryMock.removeAssetIds).not.toHaveBeenCalled(); - }); - it('should remove assets', async () => { accessMock.memory.checkOwnerAccess.mockResolvedValue(new Set(['memory1'])); accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset1'])); diff --git a/server/src/services/memory.service.ts b/server/src/services/memory.service.ts index a73eb3ec04..0164dd0b96 100644 --- a/server/src/services/memory.service.ts +++ b/server/src/services/memory.service.ts @@ -70,7 +70,7 @@ export class MemoryService { await this.access.requirePermission(auth, Permission.MEMORY_READ, id); const repos = { accessRepository: this.accessRepository, repository: this.repository }; - const results = await addAssets(auth, repos, { id, assetIds: dto.ids }); + const results = await addAssets(auth, repos, { parentId: id, assetIds: dto.ids }); const hasSuccess = results.find(({ success }) => success); if (hasSuccess) { @@ -84,8 +84,11 @@ export class MemoryService { await this.access.requirePermission(auth, Permission.MEMORY_WRITE, id); const repos = { accessRepository: this.accessRepository, repository: this.repository }; - const permissions = [Permission.ASSET_SHARE]; - const results = await removeAssets(auth, repos, { id, assetIds: dto.ids, permissions }); + const results = await removeAssets(auth, repos, { + parentId: id, + assetIds: dto.ids, + canAlwaysRemove: Permission.MEMORY_DELETE, + }); const hasSuccess = results.find(({ success }) => success); if (hasSuccess) { diff --git a/server/src/utils/asset.util.ts b/server/src/utils/asset.util.ts index d556c905b1..76a8dc06b0 100644 --- a/server/src/utils/asset.util.ts +++ b/server/src/utils/asset.util.ts @@ -3,7 +3,6 @@ import { BulkIdErrorReason, BulkIdResponseDto } from 'src/dtos/asset-ids.respons import { AuthDto } from 'src/dtos/auth.dto'; import { IAccessRepository } from 'src/interfaces/access.interface'; import { IPartnerRepository } from 'src/interfaces/partner.interface'; -import { setDifference, setUnion } from 'src/utils/set'; export interface IBulkAsset { getAssetIds: (id: string, assetIds: string[]) => Promise>; @@ -14,12 +13,12 @@ export interface IBulkAsset { export const addAssets = async ( auth: AuthDto, repositories: { accessRepository: IAccessRepository; repository: IBulkAsset }, - dto: { id: string; assetIds: string[] }, + dto: { parentId: string; assetIds: string[] }, ) => { const { accessRepository, repository } = repositories; const access = AccessCore.create(accessRepository); - const existingAssetIds = await repository.getAssetIds(dto.id, dto.assetIds); + const existingAssetIds = await repository.getAssetIds(dto.parentId, dto.assetIds); const notPresentAssetIds = dto.assetIds.filter((id) => !existingAssetIds.has(id)); const allowedAssetIds = await access.checkAccess(auth, Permission.ASSET_SHARE, notPresentAssetIds); @@ -43,7 +42,7 @@ export const addAssets = async ( const newAssetIds = results.filter(({ success }) => success).map(({ id }) => id); if (newAssetIds.length > 0) { - await repository.addAssetIds(dto.id, newAssetIds); + await repository.addAssetIds(dto.parentId, newAssetIds); } return results; @@ -52,20 +51,17 @@ export const addAssets = async ( export const removeAssets = async ( auth: AuthDto, repositories: { accessRepository: IAccessRepository; repository: IBulkAsset }, - dto: { id: string; assetIds: string[]; permissions: Permission[] }, + dto: { parentId: string; assetIds: string[]; canAlwaysRemove: Permission }, ) => { const { accessRepository, repository } = repositories; const access = AccessCore.create(accessRepository); - const existingAssetIds = await repository.getAssetIds(dto.id, dto.assetIds); - let allowedAssetIds = new Set(); - let remainingAssetIds = existingAssetIds; - - for (const permission of dto.permissions) { - const newAssetIds = await access.checkAccess(auth, permission, setDifference(remainingAssetIds, allowedAssetIds)); - remainingAssetIds = setDifference(remainingAssetIds, newAssetIds); - allowedAssetIds = setUnion(allowedAssetIds, newAssetIds); - } + // check if the user can always remove from the parent album, memory, etc. + const canAlwaysRemove = await access.checkAccess(auth, dto.canAlwaysRemove, [dto.parentId]); + const existingAssetIds = await repository.getAssetIds(dto.parentId, dto.assetIds); + const allowedAssetIds = canAlwaysRemove.has(dto.parentId) + ? existingAssetIds + : await access.checkAccess(auth, Permission.ASSET_SHARE, existingAssetIds); const results: BulkIdResponseDto[] = []; for (const assetId of dto.assetIds) { @@ -87,7 +83,7 @@ export const removeAssets = async ( const removedIds = results.filter(({ success }) => success).map(({ id }) => id); if (removedIds.length > 0) { - await repository.removeAssetIds(dto.id, removedIds); + await repository.removeAssetIds(dto.parentId, removedIds); } return results;