Skip to content

Commit 4bd1f23

Browse files
committed
Limit reviews to only those owned by the reviewer in the JWT
1 parent d5e37fe commit 4bd1f23

File tree

2 files changed

+134
-6
lines changed

2 files changed

+134
-6
lines changed

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

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -425,6 +425,97 @@ describe('ReviewService.getReview authorization checks', () => {
425425
});
426426
});
427427

428+
describe('ReviewService.getReviews reviewer visibility', () => {
429+
let prismaMock: any;
430+
let prismaErrorServiceMock: any;
431+
let resourceApiServiceMock: any;
432+
let challengeApiServiceMock: any;
433+
let service: ReviewService;
434+
435+
const baseAuthUser: JwtUser = {
436+
userId: 'reviewer-1',
437+
roles: [],
438+
isMachine: false,
439+
};
440+
441+
const baseSubmission = { id: 'submission-1' };
442+
443+
const buildResource = (roleName: string) => ({
444+
id: 'resource-1',
445+
challengeId: 'challenge-1',
446+
memberId: baseAuthUser.userId,
447+
memberHandle: 'reviewerHandle',
448+
roleId: 'role-reviewer',
449+
phaseId: 'phase-review',
450+
createdBy: 'tc',
451+
created: new Date().toISOString(),
452+
roleName,
453+
});
454+
455+
beforeEach(() => {
456+
jest.resetAllMocks();
457+
458+
prismaMock = {
459+
submission: {
460+
findMany: jest.fn().mockResolvedValue([baseSubmission]),
461+
},
462+
review: {
463+
findMany: jest.fn().mockResolvedValue([]),
464+
count: jest.fn().mockResolvedValue(0),
465+
},
466+
};
467+
468+
prismaErrorServiceMock = {
469+
handleError: jest.fn(),
470+
};
471+
472+
resourceApiServiceMock = {
473+
getMemberResourcesRoles: jest.fn(),
474+
};
475+
476+
challengeApiServiceMock = {
477+
getChallengeDetail: jest.fn(),
478+
getChallenges: jest.fn(),
479+
};
480+
481+
service = new ReviewService(
482+
prismaMock,
483+
prismaErrorServiceMock,
484+
resourceApiServiceMock,
485+
challengeApiServiceMock,
486+
);
487+
});
488+
489+
it('filters reviews by the reviewer resource id when the requester is a reviewer', async () => {
490+
resourceApiServiceMock.getMemberResourcesRoles.mockResolvedValue([
491+
buildResource('Reviewer'),
492+
]);
493+
494+
await service.getReviews(baseAuthUser, undefined, 'challenge-1');
495+
496+
expect(resourceApiServiceMock.getMemberResourcesRoles).toHaveBeenCalledWith(
497+
'challenge-1',
498+
baseAuthUser.userId,
499+
);
500+
501+
expect(prismaMock.review.findMany).toHaveBeenCalledTimes(1);
502+
const callArgs = prismaMock.review.findMany.mock.calls[0][0];
503+
expect(callArgs.where.resourceId).toEqual({ in: ['resource-1'] });
504+
expect(callArgs.where.submissionId).toEqual({ in: [baseSubmission.id] });
505+
});
506+
507+
it('does not restrict resource visibility for copilots', async () => {
508+
resourceApiServiceMock.getMemberResourcesRoles.mockResolvedValue([
509+
buildResource('Copilot'),
510+
]);
511+
512+
await service.getReviews(baseAuthUser, undefined, 'challenge-1');
513+
514+
const callArgs = prismaMock.review.findMany.mock.calls[0][0];
515+
expect(callArgs.where.resourceId).toBeUndefined();
516+
});
517+
});
518+
428519
describe('ReviewService.updateReviewItem validations', () => {
429520
const prismaMock = {
430521
reviewItem: {

src/api/review/review.service.ts

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1414,6 +1414,32 @@ export class ReviewService {
14141414
}
14151415
};
14161416

1417+
const restrictToResourceIds = (allowedIds: string[]) => {
1418+
if (!allowedIds || allowedIds.length === 0) {
1419+
reviewWhereClause.resourceId = { in: ['__none__'] };
1420+
return;
1421+
}
1422+
const existing = reviewWhereClause.resourceId;
1423+
if (!existing) {
1424+
reviewWhereClause.resourceId = { in: allowedIds };
1425+
} else if (typeof existing === 'string') {
1426+
if (!allowedIds.includes(existing)) {
1427+
reviewWhereClause.resourceId = { in: ['__none__'] };
1428+
} else {
1429+
reviewWhereClause.resourceId = existing;
1430+
}
1431+
} else if (existing.in && Array.isArray(existing.in)) {
1432+
const intersect = existing.in.filter((id: string) =>
1433+
allowedIds.includes(id),
1434+
);
1435+
reviewWhereClause.resourceId = {
1436+
in: intersect.length ? intersect : ['__none__'],
1437+
};
1438+
} else {
1439+
reviewWhereClause.resourceId = { in: allowedIds };
1440+
}
1441+
};
1442+
14171443
if (submissionId) {
14181444
reviewWhereClause.submissionId = submissionId;
14191445
}
@@ -1452,25 +1478,36 @@ export class ReviewService {
14521478

14531479
// If a challengeId is specified, check role context for that challenge
14541480
if (challengeId) {
1455-
let isReviewerOrCopilot = false;
1481+
let reviewerResourceIds: string[] = [];
1482+
let hasCopilotRole = false;
14561483
try {
14571484
const resources =
14581485
await this.resourceApiService.getMemberResourcesRoles(
14591486
challengeId,
14601487
uid,
14611488
);
1462-
isReviewerOrCopilot = resources.some((r) => {
1463-
const rn = (r.roleName || '').toLowerCase();
1464-
return rn.includes('reviewer') || rn.includes('copilot');
1465-
});
1489+
1490+
const normalized = resources || [];
1491+
reviewerResourceIds = normalized
1492+
.filter((r) =>
1493+
(r.roleName || '').toLowerCase().includes('reviewer'),
1494+
)
1495+
.map((r) => r.id);
1496+
hasCopilotRole = normalized.some((r) =>
1497+
(r.roleName || '').toLowerCase().includes('copilot'),
1498+
);
14661499
} catch (e) {
14671500
const msg = e instanceof Error ? e.message : String(e);
14681501
this.logger.debug(
14691502
`Failed to verify reviewer/copilot roles via Resource API: ${msg}`,
14701503
);
14711504
}
14721505

1473-
if (!isReviewerOrCopilot) {
1506+
if (hasCopilotRole) {
1507+
// Copilots retain full visibility for the challenge
1508+
} else if (reviewerResourceIds.length) {
1509+
restrictToResourceIds(reviewerResourceIds);
1510+
} else {
14741511
// Confirm the user has actually submitted to this challenge
14751512
const mySubs = await this.prisma.submission.findMany({
14761513
where: { challengeId, memberId: uid },

0 commit comments

Comments
 (0)