-
Notifications
You must be signed in to change notification settings - Fork 8
[PROD] - PS489 #187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PROD] - PS489 #187
Changes from all commits
371cf2c
e1b55ad
fd3c185
ef2d5db
ba865b2
0bf1311
6dab29b
f94c2ce
2d38b73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -510,6 +510,7 @@ describe('SubmissionService', () => { | |
| reviewType: { | ||
| findMany: jest.Mock; | ||
| }; | ||
| $queryRaw: jest.Mock; | ||
| }; | ||
| let prismaErrorServiceMock: { handleError: jest.Mock }; | ||
| let challengePrismaMock: { | ||
|
|
@@ -537,9 +538,14 @@ describe('SubmissionService', () => { | |
| reviewType: { | ||
| findMany: jest.fn().mockResolvedValue([]), | ||
| }, | ||
| $queryRaw: jest.fn().mockResolvedValue([]), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [❗❗ |
||
| }; | ||
| prismaErrorServiceMock = { | ||
| handleError: jest.fn(), | ||
| handleError: jest.fn().mockReturnValue({ | ||
| message: 'Unexpected error', | ||
| code: 'INTERNAL_ERROR', | ||
| details: {}, | ||
| }), | ||
| }; | ||
| challengePrismaMock = { | ||
| $queryRaw: jest.fn().mockResolvedValue([]), | ||
|
|
@@ -624,6 +630,7 @@ describe('SubmissionService', () => { | |
| prismaMock.submission.findFirst.mockResolvedValue({ | ||
| id: 'submission-new', | ||
| }); | ||
| prismaMock.$queryRaw.mockResolvedValue([{ id: 'submission-new' }]); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [❗❗ |
||
|
|
||
| const result = await listService.listSubmission( | ||
| { isMachine: false } as any, | ||
|
|
@@ -1241,5 +1248,56 @@ describe('SubmissionService', () => { | |
| expect(screeningReview?.reviewerHandle).toBe('screeningHandle'); | ||
| expect(screeningReview?.reviewerMaxRating).toBe(2000); | ||
| }); | ||
|
|
||
| it('exposes submitter identity but strips reviews for anonymous challenge queries', async () => { | ||
| const now = new Date('2025-02-01T12:00:00Z'); | ||
| const submissions = [ | ||
| { | ||
| id: 'submission-anon', | ||
| challengeId: 'challenge-1', | ||
| memberId: '101', | ||
| submittedDate: now, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| type: SubmissionType.CONTEST_SUBMISSION, | ||
| status: SubmissionStatus.ACTIVE, | ||
| review: [{ id: 'review-public', score: 100 }], | ||
| reviewSummation: [{ id: 'summation-public' }], | ||
| url: 'https://example.com/submission.zip', | ||
| legacyChallengeId: null, | ||
| prizeId: null, | ||
| }, | ||
| ]; | ||
|
|
||
| prismaMock.submission.findMany.mockResolvedValue( | ||
| submissions.map((entry) => ({ ...entry })), | ||
| ); | ||
| prismaMock.submission.count.mockResolvedValue(submissions.length); | ||
| prismaMock.submission.findFirst.mockResolvedValue({ | ||
| id: 'submission-anon', | ||
| }); | ||
|
|
||
| memberPrismaMock.member.findMany.mockResolvedValue([ | ||
| { | ||
| userId: BigInt(101), | ||
| handle: 'anonUser', | ||
| maxRating: { rating: 1500 }, | ||
| }, | ||
| ]); | ||
|
|
||
| const result = await listService.listSubmission( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [❗❗ |
||
| { isMachine: false, roles: [] } as any, | ||
| { challengeId: 'challenge-1' } as any, | ||
| { page: 1, perPage: 10 } as any, | ||
| ); | ||
|
|
||
| const submissionResult = result.data[0]; | ||
| expect(submissionResult.memberId).toBe('101'); | ||
| expect(submissionResult.submitterHandle).toBe('anonUser'); | ||
| expect(submissionResult.submitterMaxRating).toBe(1500); | ||
| expect(submissionResult).not.toHaveProperty('review'); | ||
| expect(submissionResult).not.toHaveProperty('reviewSummation'); | ||
| expect(submissionResult.url).toBeNull(); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1812,9 +1812,9 @@ export class SubmissionService { | |||||||||||||||||||||||||||||||||||||||||
| : undefined; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| if (requestedMemberId) { | ||||||||||||||||||||||||||||||||||||||||||
| const userId = authUser.userId ? String(authUser.userId) : undefined; | ||||||||||||||||||||||||||||||||||||||||||
| const userId = authUser?.userId ? String(authUser.userId) : undefined; | ||||||||||||||||||||||||||||||||||||||||||
| const isRequestingMember = userId === requestedMemberId; | ||||||||||||||||||||||||||||||||||||||||||
| const hasCopilotRole = (authUser.roles ?? []).includes( | ||||||||||||||||||||||||||||||||||||||||||
| const hasCopilotRole = (authUser?.roles ?? []).includes( | ||||||||||||||||||||||||||||||||||||||||||
| UserRole.Copilot, | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
| const hasElevatedAccess = isAdmin(authUser) || hasCopilotRole; | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1862,7 +1862,7 @@ export class SubmissionService { | |||||||||||||||||||||||||||||||||||||||||
| : ''; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| let restrictedChallengeIds = new Set<string>(); | ||||||||||||||||||||||||||||||||||||||||||
| if (!isPrivilegedRequester && requesterUserId) { | ||||||||||||||||||||||||||||||||||||||||||
| if (!isPrivilegedRequester && requesterUserId && !queryDto.challengeId) { | ||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||
| restrictedChallengeIds = | ||||||||||||||||||||||||||||||||||||||||||
| await this.getActiveSubmitterRestrictedChallengeIds( | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1885,7 +1885,8 @@ export class SubmissionService { | |||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||
| !isPrivilegedRequester && | ||||||||||||||||||||||||||||||||||||||||||
| requesterUserId && | ||||||||||||||||||||||||||||||||||||||||||
| restrictedChallengeIds.size | ||||||||||||||||||||||||||||||||||||||||||
| restrictedChallengeIds.size && | ||||||||||||||||||||||||||||||||||||||||||
| !queryDto.challengeId | ||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||
| const restrictedList = Array.from(restrictedChallengeIds); | ||||||||||||||||||||||||||||||||||||||||||
| const restrictionCriteria: Prisma.submissionWhereInput = { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1928,12 +1929,16 @@ export class SubmissionService { | |||||||||||||||||||||||||||||||||||||||||
| orderBy, | ||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Enrich with submitter handle and max rating for authorized callers | ||||||||||||||||||||||||||||||||||||||||||
| const canViewSubmitter = await this.canViewSubmitterIdentity( | ||||||||||||||||||||||||||||||||||||||||||
| authUser, | ||||||||||||||||||||||||||||||||||||||||||
| queryDto.challengeId, | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
| if (canViewSubmitter && submissions.length) { | ||||||||||||||||||||||||||||||||||||||||||
| // Enrich with submitter handle and max rating (always for challenge listings) | ||||||||||||||||||||||||||||||||||||||||||
| const shouldEnrichSubmitter = | ||||||||||||||||||||||||||||||||||||||||||
| submissions.length > 0 && | ||||||||||||||||||||||||||||||||||||||||||
| (queryDto.challengeId | ||||||||||||||||||||||||||||||||||||||||||
| ? true | ||||||||||||||||||||||||||||||||||||||||||
| : await this.canViewSubmitterIdentity( | ||||||||||||||||||||||||||||||||||||||||||
| authUser, | ||||||||||||||||||||||||||||||||||||||||||
| queryDto.challengeId, | ||||||||||||||||||||||||||||||||||||||||||
| )); | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1933
to
+1940
|
||||||||||||||||||||||||||||||||||||||||||
| const shouldEnrichSubmitter = | |
| submissions.length > 0 && | |
| (queryDto.challengeId | |
| ? true | |
| : await this.canViewSubmitterIdentity( | |
| authUser, | |
| queryDto.challengeId, | |
| )); | |
| let shouldEnrichSubmitter = false; | |
| if (submissions.length > 0) { | |
| if (queryDto.challengeId) { | |
| // For challenge listings, always enrich submitter data | |
| shouldEnrichSubmitter = true; | |
| } else { | |
| shouldEnrichSubmitter = await this.canViewSubmitterIdentity( | |
| authUser, | |
| undefined, | |
| ); | |
| } | |
| } |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreachable code detected. The condition if (!uid) at line 2456 will never be true because it's checked after a previous condition at line 2430 that handles the case where !isPrivilegedRequester && !requesterUserId. Since uid is assigned from requesterUserId at line 2454, and privileged requesters return early at line 2447-2452, the only way to reach line 2456 is if requesterUserId has a value (making uid truthy).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; | ||
| import type { ReviewOpportunityType as PrismaReviewOpportunityType } from '@prisma/client'; | ||
| import { IsIn, IsNotEmpty, IsOptional, IsString } from 'class-validator'; | ||
| import { ReviewOpportunityType } from './reviewOpportunity.dto'; | ||
|
|
||
|
|
@@ -37,7 +38,7 @@ export const ReviewApplicationRoleIds: Record<ReviewApplicationRole, number> = { | |
| // read from review_application_role_lu.review_auction_type_id | ||
| export const ReviewApplicationRoleOpportunityTypeMap: Record< | ||
| ReviewApplicationRole, | ||
| ReviewOpportunityType | ||
| PrismaReviewOpportunityType | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [❗❗ |
||
| > = { | ||
| PRIMARY_REVIEWER: ReviewOpportunityType.COMPONENT_DEV_REVIEW, | ||
| SECONDARY_REVIEWER: ReviewOpportunityType.COMPONENT_DEV_REVIEW, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ describe('TokenRolesGuard', () => { | |
| type TestRequest = Record<string, unknown> & { | ||
| method: string; | ||
| query: Record<string, unknown>; | ||
| user: { | ||
| user?: { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [❗❗ |
||
| userId: string; | ||
| isMachine: boolean; | ||
| roles?: unknown[]; | ||
|
|
@@ -132,5 +132,16 @@ describe('TokenRolesGuard', () => { | |
|
|
||
| expect(() => guard.canActivate(context)).toThrow(ForbiddenException); | ||
| }); | ||
|
|
||
| it('allows anonymous access when challengeId is provided', () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [❗❗ |
||
| const request = { | ||
| method: 'GET', | ||
| query: { challengeId: '12345' }, | ||
| }; | ||
|
|
||
| const context = createExecutionContext(request as TestRequest); | ||
|
|
||
| expect(guard.canActivate(context)).toBe(true); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[⚠️
correctness]Using the nullish coalescing operator (
??) to provide a default value forauthUseris a good approach. However, ensure that this default value is appropriate for all cases wherereq['user']might benullorundefined. If there are specific roles or machine states that should be handled differently, consider refining this default value.