Skip to content

Commit 4f56528

Browse files
committed
Allow submitters to edit their own appeals (PM-2030)
1 parent ba8770b commit 4f56528

File tree

2 files changed

+112
-3
lines changed

2 files changed

+112
-3
lines changed

src/api/appeal/appeal.service.spec.ts

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ describe('AppealService.createAppeal', () => {
190190
describe('AppealService.updateAppeal', () => {
191191
const baseAppeal = {
192192
id: 'appeal-1',
193-
resourceId: 'member-123',
193+
resourceId: 'resource-123',
194194
reviewItemCommentId: 'review-item-comment-1',
195195
content: 'Original appeal content',
196196
reviewItemComment: {
@@ -209,7 +209,6 @@ describe('AppealService.updateAppeal', () => {
209209
const updateRequest = {
210210
reviewItemCommentId: baseAppeal.reviewItemCommentId,
211211
content: 'Updated appeal content',
212-
resourceId: baseAppeal.resourceId,
213212
} as any;
214213

215214
const submitterAuthUser = {
@@ -230,6 +229,10 @@ describe('AppealService.updateAppeal', () => {
230229
...baseAppeal,
231230
content: updateRequest.content,
232231
});
232+
resourcePrismaMock.resource.findUnique.mockResolvedValue({
233+
id: baseAppeal.resourceId,
234+
memberId: submitterAuthUser.userId,
235+
});
233236
prismaErrorServiceMock.handleError.mockImplementation((error: any) => {
234237
throw error;
235238
});
@@ -250,6 +253,9 @@ describe('AppealService.updateAppeal', () => {
250253
expect(challengeApiServiceMock.getChallengeDetail).toHaveBeenCalledWith(
251254
'challenge-1',
252255
);
256+
expect(resourcePrismaMock.resource.findUnique).toHaveBeenCalledWith({
257+
where: { id: baseAppeal.resourceId },
258+
});
253259
expect(prismaMock.appeal.update).toHaveBeenCalledWith({
254260
where: { id: baseAppeal.id },
255261
data: expect.objectContaining({
@@ -275,12 +281,30 @@ describe('AppealService.updateAppeal', () => {
275281

276282
expect(prismaMock.appeal.update).not.toHaveBeenCalled();
277283
});
284+
285+
it('prevents updates when resource ownership does not match the requester', async () => {
286+
resourcePrismaMock.resource.findUnique.mockResolvedValueOnce({
287+
id: baseAppeal.resourceId,
288+
memberId: 'member-999',
289+
});
290+
291+
await expect(
292+
service.updateAppeal(submitterAuthUser, baseAppeal.id, updateRequest),
293+
).rejects.toMatchObject({
294+
status: 403,
295+
response: expect.objectContaining({
296+
code: 'APPEAL_RESOURCE_OWNER_FORBIDDEN',
297+
}),
298+
});
299+
300+
expect(prismaMock.appeal.update).not.toHaveBeenCalled();
301+
});
278302
});
279303

280304
describe('AppealService.deleteAppeal', () => {
281305
const baseAppeal = {
282306
id: 'appeal-1',
283-
resourceId: 'member-123',
307+
resourceId: 'resource-123',
284308
reviewItemComment: {
285309
reviewItem: {
286310
review: {
@@ -309,6 +333,10 @@ describe('AppealService.deleteAppeal', () => {
309333
});
310334
prismaMock.appeal.findUnique.mockResolvedValue(baseAppeal);
311335
prismaMock.appeal.delete.mockResolvedValue(undefined);
336+
resourcePrismaMock.resource.findUnique.mockResolvedValue({
337+
id: baseAppeal.resourceId,
338+
memberId: submitterAuthUser.userId,
339+
});
312340
prismaErrorServiceMock.handleError.mockImplementation((error: any) => {
313341
throw error;
314342
});
@@ -328,6 +356,9 @@ describe('AppealService.deleteAppeal', () => {
328356
expect(challengeApiServiceMock.getChallengeDetail).toHaveBeenCalledWith(
329357
'challenge-1',
330358
);
359+
expect(resourcePrismaMock.resource.findUnique).toHaveBeenCalledWith({
360+
where: { id: baseAppeal.resourceId },
361+
});
331362
expect(prismaMock.appeal.delete).toHaveBeenCalledWith({
332363
where: { id: baseAppeal.id },
333364
});

src/api/appeal/appeal.service.ts

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,14 @@ export class AppealService {
256256
?.challengeId;
257257
const isPrivileged = Boolean(authUser?.isMachine || isAdmin(authUser));
258258

259+
if (!isPrivileged) {
260+
await this.ensureAppealResourceOwnership(
261+
authUser,
262+
existingAppeal.resourceId,
263+
appealId,
264+
);
265+
}
266+
259267
if (!isPrivileged) {
260268
await this.ensureChallengeAllowsAppealChange(challengeId, {
261269
logContext: 'updateAppeal',
@@ -269,6 +277,7 @@ export class AppealService {
269277
if (
270278
!authUser?.isMachine &&
271279
!isAdmin(authUser) &&
280+
body.resourceId !== undefined &&
272281
body.resourceId !== existingAppeal.resourceId
273282
) {
274283
throw new ForbiddenException({
@@ -370,6 +379,14 @@ export class AppealService {
370379
?.challengeId;
371380
const isPrivileged = Boolean(authUser?.isMachine || isAdmin(authUser));
372381

382+
if (!isPrivileged) {
383+
await this.ensureAppealResourceOwnership(
384+
authUser,
385+
existingAppeal.resourceId,
386+
appealId,
387+
);
388+
}
389+
373390
if (!isPrivileged) {
374391
await this.ensureChallengeAllowsAppealChange(challengeId, {
375392
logContext: 'deleteAppeal',
@@ -829,6 +846,67 @@ export class AppealService {
829846
);
830847
}
831848

849+
private async ensureAppealResourceOwnership(
850+
authUser: JwtUser,
851+
resourceId: string | null | undefined,
852+
appealId: string,
853+
): Promise<void> {
854+
const requesterMemberId = authUser?.userId ? String(authUser.userId) : '';
855+
856+
if (!requesterMemberId) {
857+
throw new ForbiddenException({
858+
message:
859+
'Only the owner of the resource associated with this appeal may modify it.',
860+
code: 'APPEAL_RESOURCE_OWNER_FORBIDDEN',
861+
details: { appealId, resourceId, requesterMemberId },
862+
});
863+
}
864+
865+
if (!resourceId) {
866+
throw new BadRequestException({
867+
message: `Appeal ${appealId} does not have an associated resourceId.`,
868+
code: 'APPEAL_MISSING_RESOURCE_ID',
869+
details: { appealId },
870+
});
871+
}
872+
873+
const resource = await this.resourcePrisma.resource.findUnique({
874+
where: { id: resourceId },
875+
});
876+
877+
if (!resource) {
878+
throw new NotFoundException({
879+
message: `Resource ${resourceId} associated with appeal ${appealId} was not found.`,
880+
code: 'APPEAL_RESOURCE_NOT_FOUND',
881+
details: { appealId, resourceId },
882+
});
883+
}
884+
885+
const resourceMemberId = resource.memberId ? String(resource.memberId) : '';
886+
887+
if (!resourceMemberId) {
888+
throw new BadRequestException({
889+
message: `Resource ${resourceId} associated with appeal ${appealId} does not have a memberId.`,
890+
code: 'APPEAL_RESOURCE_MISSING_MEMBER_ID',
891+
details: { appealId, resourceId },
892+
});
893+
}
894+
895+
if (resourceMemberId !== requesterMemberId) {
896+
throw new ForbiddenException({
897+
message:
898+
'Only the owner of the resource associated with this appeal may modify it.',
899+
code: 'APPEAL_RESOURCE_OWNER_FORBIDDEN',
900+
details: {
901+
appealId,
902+
resourceId,
903+
resourceMemberId,
904+
requesterMemberId,
905+
},
906+
});
907+
}
908+
}
909+
832910
private async ensureChallengeAllowsAppealChange(
833911
challengeId: string | null | undefined,
834912
context: {

0 commit comments

Comments
 (0)