fix: check admin permission on recycled items for deletion (#1949)

pyphilia · kim · web-flow · commit 63ce44f059ac · 2025-07-21T14:52:34.000Z
* fix: check admin permission on recycled items for deletion

* refactor: fix PR requested changes

---------

Co-authored-by: kim &lt;kim.phanhoang@epfl.ch&gt;
diff --git a/src/services/item/item.controller.update.test.ts b/src/services/item/item.controller.update.test.ts
@@ -1168,6 +1168,87 @@ describe('Item routes tests', () => {
           // ws should not fail
         }, MULTIPLE_ITEMS_LOADING_TIME);
       });
+      it('Delete recycled items', async () => {
+        const {
+          actor,
+          items: [item1, item2],
+          itemMemberships: [im1, im2],
+        } = await seedFromJson({
+          items: [
+            {
+              isDeleted: true,
+              memberships: [{ account: 'actor', permission: PermissionLevel.Admin }],
+            },
+            {
+              isDeleted: true,
+              memberships: [{ account: 'actor', permission: PermissionLevel.Admin }],
+            },
+          ],
+        });
+        assertIsDefined(actor);
+        assertIsMemberForTest(actor);
+        mockAuthenticate(actor);
+
+        const items = [item1, item2];
+        const response = await app.inject({
+          method: HttpMethod.Delete,
+          url: '/items',
+          query: { id: items.map(({ id }) => id) },
+        });
+        expect(response.json()).toEqual(items.map(({ id }) => id));
+        expect(response.statusCode).toBe(StatusCodes.ACCEPTED);
+        await waitForExpect(async () => {
+          const remaining = await db.query.itemsRawTable.findMany({
+            where: inArray(itemsRawTable.id, [item1.id, item2.id]),
+          });
+          expect(remaining).toHaveLength(0);
+          const memberships = await db.query.itemMembershipsTable.findMany({
+            where: inArray(itemMembershipsTable.id, [im1.id, im2.id]),
+          });
+          expect(memberships).toHaveLength(0);
+        }, MULTIPLE_ITEMS_LOADING_TIME);
+      });
+      it('Do not delete items with write permission', async () => {
+        const {
+          actor,
+          items: [item1, item2],
+        } = await seedFromJson({
+          items: [
+            {
+              isDeleted: true,
+              memberships: [{ account: 'actor', permission: PermissionLevel.Write }],
+            },
+            {
+              isDeleted: true,
+              memberships: [{ account: 'actor', permission: PermissionLevel.Admin }],
+            },
+          ],
+        });
+        assertIsDefined(actor);
+        assertIsMemberForTest(actor);
+        mockAuthenticate(actor);
+
+        const items = [item1, item2];
+        const response = await app.inject({
+          method: HttpMethod.Delete,
+          url: '/items',
+          query: { id: items.map(({ id }) => id) },
+        });
+        expect(response.json()).toEqual(items.map(({ id }) => id));
+        expect(response.statusCode).toBe(StatusCodes.ACCEPTED);
+
+        await new Promise((done) => {
+          // wait one second for the operation to apply, because this is already the start state
+          setTimeout(async () => {
+            const remaining = await db.query.itemsRawTable.findMany({
+              where: inArray(itemsRawTable.id, [item1.id, item2.id]),
+            });
+            expect(remaining).toHaveLength(2);
+
+            done(true);
+          }, 1000);
+        });
+      });
       it('Bad request if one id is invalid', async () => {
         const { actor, items } = await seedFromJson({
           items: [
diff --git a/src/services/item/plugins/recycled/recycled.repository.test.ts b/src/services/item/plugins/recycled/recycled.repository.test.ts
@@ -0,0 +1,83 @@
+import { PermissionLevel } from '@graasp/sdk';
+
+import { seedFromJson } from '../../../../../test/mocks/seed';
+import { db } from '../../../../drizzle/db';
+import { assertIsDefined } from '../../../../utils/assertions';
+import { MemberCannotAdminItem } from '../../../../utils/errors';
+import { RecycledItemDataRepository } from './recycled.repository';
+
+const recycledRepository = new RecycledItemDataRepository();
+
+describe('RecycledItemDataRepository', () => {
+  describe('assertAdminAccessForItemIds', () => {
+    it('resolve for item with admin permission', async () => {
+      const { items, actor } = await seedFromJson({
+        items: [{ memberships: [{ permission: PermissionLevel.Admin, account: 'actor' }] }],
+      });
+      assertIsDefined(actor);
+
+      // eslint-disable-next-line @typescript-eslint/no-unused-expressions
+      await expect(
+        recycledRepository.assertAdminAccessForItemIds(
+          db,
+          actor.id,
+          items.map((i) => i.id),
+        ),
+      ).resolves;
+    });
+
+    it('resolve for item with parent with admin permission', async () => {
+      const {
+        items: [_parent, child],
+        actor,
+      } = await seedFromJson({
+        items: [
+          {
+            memberships: [{ permission: PermissionLevel.Admin, account: 'actor' }],
+            children: [{}],
+          },
+        ],
+      });
+      assertIsDefined(actor);
+
+      // eslint-disable-next-line @typescript-eslint/no-unused-expressions
+      await expect(recycledRepository.assertAdminAccessForItemIds(db, actor.id, [child.id]))
+        .resolves;
+    });
+
+    it('reject for item with write permission', async () => {
+      const { items, actor } = await seedFromJson({
+        items: [{ memberships: [{ permission: PermissionLevel.Write, account: 'actor' }] }],
+      });
+      assertIsDefined(actor);
+
+      // eslint-disable-next-line @typescript-eslint/no-unused-expressions
+      await expect(
+        recycledRepository.assertAdminAccessForItemIds(
+          db,
+          actor.id,
+          items.map((i) => i.id),
+        ),
+      ).rejects.toBeInstanceOf(MemberCannotAdminItem);
+    });
+
+    it('reject for one item with write permission', async () => {
+      const { items, actor } = await seedFromJson({
+        items: [
+          { memberships: [{ permission: PermissionLevel.Admin, account: 'actor' }] },
+          { memberships: [{ permission: PermissionLevel.Write, account: 'actor' }] },
+        ],
+      });
+      assertIsDefined(actor);
+
+      // eslint-disable-next-line @typescript-eslint/no-unused-expressions
+      await expect(
+        recycledRepository.assertAdminAccessForItemIds(
+          db,
+          actor.id,
+          items.map((i) => i.id),
+        ),
+      ).rejects.toBeInstanceOf(MemberCannotAdminItem);
+    });
+  });
+});
diff --git a/src/services/item/plugins/recycled/recycled.repository.ts b/src/services/item/plugins/recycled/recycled.repository.ts
@@ -5,16 +5,17 @@ import { and, asc, desc, isNotNull, ne } from 'drizzle-orm/sql';
 import { type Paginated, type Pagination, PermissionLevel } from '@graasp/sdk';
 
 import { type DBConnection } from '../../../../drizzle/db';
-import { isDescendantOrSelf } from '../../../../drizzle/operations';
+import { isAncestorOrSelf, isDescendantOrSelf } from '../../../../drizzle/operations';
 import {
   itemMembershipsTable,
   itemsRawTable,
   membersView,
   recycledItemDatasTable,
 } from '../../../../drizzle/schema';
-import type { ItemRaw } from '../../../../drizzle/types';
+import type { ItemRaw, MemberRaw } from '../../../../drizzle/types';
 import { throwsIfParamIsInvalid } from '../../../../repositories/utils';
 import type { MinimalMember } from '../../../../types';
+import { MemberCannotAdminItem } from '../../../../utils/errors';
 import { ITEMS_PAGE_SIZE_MAX } from '../../constants';
 import type { FolderItem } from '../../discrimination';
 
@@ -148,4 +149,37 @@ export class RecycledItemDataRepository {
 
     return trees.map(({ descendants }) => descendants);
   }
+
+  /**
+   * Returns whether the member has an admin access on all the items
+   * @param dbConnection database connection
+   * @param memberId id of member performing the task
+   * @param ids item ids
+   * @returns true if the member has access to all items
+   */
+  async assertAdminAccessForItemIds(
+    dbConnection: DBConnection,
+    memberId: MemberRaw['id'],
+    ids: ItemRaw['id'][],
+  ) {
+    const im = await dbConnection
+      .select({ itemId: itemsRawTable.id })
+      .from(itemsRawTable)
+      .innerJoin(
+        itemMembershipsTable,
+        and(
+          eq(itemMembershipsTable.accountId, memberId),
+          eq(itemMembershipsTable.permission, PermissionLevel.Admin),
+          inArray(itemsRawTable.id, ids),
+          isAncestorOrSelf(itemMembershipsTable.itemPath, itemsRawTable.path),
+        ),
+      );
+
+    const imIds = im.map(({ itemId }) => itemId);
+    const idsWithoutAccess = ids.filter((m) => !imIds.includes(m));
+    if (idsWithoutAccess.length) {
+      // return first id lacking access
+      throw new MemberCannotAdminItem(idsWithoutAccess[0]);
+    }
+  }
 }
diff --git a/src/services/item/plugins/recycled/recycled.service.ts b/src/services/item/plugins/recycled/recycled.service.ts
@@ -53,14 +53,7 @@ export class RecycledBinService {
     ids: ItemRaw['id'][],
   ) {
     // validate permission on parents
-    // TODO: optimize!!!!!!
-    for (const id of ids) {
-      await this.authorizedItemService.assertAccessForItemId(dbConnection, {
-        permission: PermissionLevel.Admin,
-        accountId: member.id,
-        itemId: id,
-      });
-    }
+    await this.recycledItemRepository.assertAdminAccessForItemIds(dbConnection, member.id, ids);
 
     const items = await this.recycledItemRepository.getDeletedTreesById(dbConnection, ids);
 
