Review deletion checks for reviewers

Author: jmgasper

src/api/review/review.controller.ts

@@ -267,11 +267,12 @@ export class ReviewController {
   }
 
   @Delete('/items/:itemId')
-  @Roles(UserRole.Copilot, UserRole.Admin)
+  @Roles(UserRole.Copilot, UserRole.Reviewer, UserRole.Admin)
   @Scopes(Scope.DeleteReviewItem)
   @ApiOperation({
     summary: 'Delete a review item',
-    description: 'Roles: Copilot, Admin | Scopes: delete:review-item',
+    description:
+      'Roles: Copilot, Reviewers (their own review items), Admin | Scopes: delete:review-item',
   })
   @ApiParam({
     name: 'itemId',

src/api/review/review.service.spec.ts

@@ -204,6 +204,11 @@ describe('ReviewService.createReview authorization checks', () => {
     } as any;
 
     prismaMock.review.create.mockResolvedValue(reviewCreateResult);
+    prismaMock.review.update.mockResolvedValue({
+      ...reviewCreateResult,
+      initialScore: 0,
+      finalScore: 0,
+    });
 
     await expect(
       service.createReview(baseAuthUser, request),
@@ -263,6 +268,11 @@ describe('ReviewService.createReview authorization checks', () => {
     ]);
 
     prismaMock.review.create.mockResolvedValue(reviewCreateResult);
+    prismaMock.review.update.mockResolvedValue({
+      ...reviewCreateResult,
+      initialScore: 0,
+      finalScore: 0,
+    });
 
     await expect(
       service.createReview(baseAuthUser, request),
@@ -1223,3 +1233,121 @@ describe('ReviewService.deleteReview', () => {
     });
   });
 });
+
+describe('ReviewService.deleteReviewItem authorization checks', () => {
+  const prismaMock = {
+    reviewItem: {
+      findUnique: jest.fn(),
+      delete: jest.fn(),
+    },
+  } as unknown as any;
+
+  const prismaErrorServiceMock = {
+    handleError: jest.fn((error: any) => ({
+      message: error.message,
+      code: error.response?.code ?? 'UNKNOWN',
+      details: error.response?.details,
+    })),
+  } as unknown as any;
+
+  const resourceApiServiceMock = {
+    getMemberResourcesRoles: jest.fn(),
+  } as unknown as any;
+
+  const challengeApiServiceMock = {} as unknown as any;
+
+  const service = new ReviewService(
+    prismaMock,
+    prismaErrorServiceMock,
+    resourceApiServiceMock,
+    challengeApiServiceMock,
+  );
+
+  const reviewerUser: JwtUser = {
+    userId: 'member-100',
+    roles: [UserRole.Reviewer],
+    isMachine: false,
+  };
+
+  const baseReviewItem = {
+    id: 'item-1',
+    reviewId: 'review-1',
+    review: {
+      id: 'review-1',
+      resourceId: 'resource-1',
+      submission: {
+        challengeId: 'challenge-1',
+      },
+    },
+  } as any;
+
+  beforeEach(() => {
+    jest.resetAllMocks();
+
+    prismaMock.reviewItem.findUnique.mockResolvedValue(baseReviewItem);
+    prismaMock.reviewItem.delete.mockResolvedValue(undefined);
+
+    jest
+      .spyOn(service as any, 'recomputeAndUpdateReviewScores')
+      .mockResolvedValue(undefined);
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
+  it('blocks reviewers from deleting review items they do not own', async () => {
+    resourceApiServiceMock.getMemberResourcesRoles.mockResolvedValue([
+      {
+        id: 'resource-1',
+        memberId: 'someone-else',
+        challengeId: 'challenge-1',
+        memberHandle: 'otherHandle',
+        roleId: 'role-reviewer',
+        createdBy: 'system',
+        created: new Date().toISOString(),
+        roleName: 'Reviewer',
+      },
+    ]);
+
+    await expect(
+      service.deleteReviewItem(reviewerUser, 'item-1'),
+    ).rejects.toMatchObject({
+      status: 403,
+      response: expect.objectContaining({
+        code: 'REVIEW_ITEM_DELETE_FORBIDDEN_NOT_OWNER',
+      }),
+    });
+
+    expect(prismaMock.reviewItem.delete).not.toHaveBeenCalled();
+    expect(resourceApiServiceMock.getMemberResourcesRoles).toHaveBeenCalledWith(
+      'challenge-1',
+      'member-100',
+    );
+  });
+
+  it('allows reviewers to delete review items associated with their own review', async () => {
+    resourceApiServiceMock.getMemberResourcesRoles.mockResolvedValue([
+      {
+        id: 'resource-1',
+        memberId: 'member-100',
+        challengeId: 'challenge-1',
+        memberHandle: 'reviewerHandle',
+        roleId: 'role-reviewer',
+        createdBy: 'system',
+        created: new Date().toISOString(),
+        roleName: 'Reviewer',
+      },
+    ]);
+
+    await expect(
+      service.deleteReviewItem(reviewerUser, 'item-1'),
+    ).resolves.toMatchObject({
+      message: 'Review item item-1 deleted successfully.',
+    });
+
+    expect(prismaMock.reviewItem.delete).toHaveBeenCalledWith({
+      where: { id: 'item-1' },
+    });
+  });
+});

src/api/review/review.service.ts

@@ -277,7 +277,7 @@ export class ReviewService {
       });
     }
 
-    const requesterMemberId = String(requester.userId ?? '');
+    const requesterMemberId = String(requester.userId ?? '').trim();
 
     if (!requesterMemberId) {
       throw new ForbiddenException({
@@ -321,9 +321,15 @@ export class ReviewService {
     let ownsReview = false;
 
     if (hasReviewerRole) {
-      ownsReview = requesterResources?.some(
-        (resource) => resource.id === review.resourceId,
-      );
+      const normalizedReviewResourceId = String(review.resourceId ?? '').trim();
+      ownsReview = requesterResources?.some((resource) => {
+        const resourceId = String(resource.id ?? '').trim();
+        const resourceMemberId = String(resource.memberId ?? '').trim();
+        return (
+          resourceId === normalizedReviewResourceId &&
+          resourceMemberId === requesterMemberId
+        );
+      });
 
       if (!ownsReview) {
         throw new ForbiddenException({
@@ -806,6 +812,7 @@ export class ReviewService {
 
       const prismaBody = mapReviewRequestToDto(body) as any;
       prismaBody.phaseId = reviewPhaseId;
+      prismaBody.resourceId = reviewerResource.id;
       const createdReview = await this.prisma.review.create({
         data: prismaBody,
         include: {