fix: album remove asset bug (#10687)

* fix: album remove asset bug

* trigger GH Action

---------

Co-authored-by: Alex Tran <alex.tran1502@gmail.com>

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,
+          success: true,
-          error: 'no_permission',
         }),
       ]);
     });
@@ -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 () => {

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);
       }

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 () => {
+    it('should allow owner to remove all assets from the album', async () => {
-      accessMock.album.checkSharedAlbumAccess.mockResolvedValue(new Set(['album-123']));
+      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: true, id: 'asset-id' },
-          success: false,
-          id: 'asset-id',
-          error: BulkIdErrorReason.NO_PERMISSION,
-        },
       ]);
 
-      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 () => {

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<BulkIdResponseDto[]> {
-    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);

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']));

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, {
-    const results = await removeAssets(auth, repos, { id, assetIds: dto.ids, permissions });
+      parentId: id,
+      assetIds: dto.ids,
+      canAlwaysRemove: Permission.MEMORY_DELETE,
+    });
 
     const hasSuccess = results.find(({ success }) => success);
     if (hasSuccess) {

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<Set<string>>;
@@ -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);
+  // check if the user can always remove from the parent album, memory, etc.
-  let allowedAssetIds = new Set<string>();
+  const canAlwaysRemove = await access.checkAccess(auth, dto.canAlwaysRemove, [dto.parentId]);
-  let remainingAssetIds = existingAssetIds;
+  const existingAssetIds = await repository.getAssetIds(dto.parentId, dto.assetIds);
-
+  const allowedAssetIds = canAlwaysRemove.has(dto.parentId)
-  for (const permission of dto.permissions) {
+    ? existingAssetIds
-    const newAssetIds = await access.checkAccess(auth, permission, setDifference(remainingAssetIds, allowedAssetIds));
+    : await access.checkAccess(auth, Permission.ASSET_SHARE, existingAssetIds);
-    remainingAssetIds = setDifference(remainingAssetIds, newAssetIds);
-    allowedAssetIds = setUnion(allowedAssetIds, newAssetIds);
-  }
 
   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;