Skip to content

fix(PM-2662): prevent download for submitters if challenge is configured that way #1385

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.

Sign up for GitHub

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

Open
hentrymartin wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

fix(PM-2662): prevent download for submitters if challenge is configured that way #1385

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
caf0a6a
fix: prevent download for submitters if challenge is configured that way
hentrymartin Dec 15, 2025
ebce38b
Merge branch 'dev' into pm-2662_4
hentrymartin Dec 18, 2025
caeaa4f
fix: review comments
hentrymartin Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions src/apps/review/src/lib/components/ChallengeDetailsContent/TabContentSubmissions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import type { SubmissionHistoryPartition } from '../../utils'
import { TABLE_DATE_FORMAT } from '../../../config/index.config'
import { CollapsibleAiReviewsRow } from '../CollapsibleAiReviewsRow'
import { useRolePermissions, UseRolePermissionsResult } from '../../hooks'
import { SUBMISSION_DOWNLOAD_RESTRICTION_MESSAGE } from '../../constants'

import styles from './TabContentSubmissions.module.scss'

Expand Down Expand Up @@ -234,24 +235,13 @@ export const TabContentSubmissions: FC<Props> = props => {

const filteredSubmissions = useMemo<BackendSubmission[]>(
() => {

const filterFunc = (submissions: BackendSubmission[]): BackendSubmission[] => submissions
.filter(submission => {
if (!canViewSubmissions) {
return String(submission.memberId) === String(loginUserInfo?.userId)
}

return true
})
const filteredByUserId = filterFunc(latestBackendSubmissions)
const filteredByUserIdSubmissions = filterFunc(props.submissions)
if (restrictToLatest && hasLatestFlag) {
return latestBackendSubmissions.length
? filteredByUserId
: filteredByUserIdSubmissions
? latestBackendSubmissions
: props.submissions
}

return filteredByUserIdSubmissions
return props.submissions
},
[
latestBackendSubmissions,
Expand Down Expand Up @@ -280,13 +270,21 @@ export const TabContentSubmissions: FC<Props> = props => {
? undefined
: submission.virusScan
const failedScan = normalizedVirusScan === false
const isRestricted = isRestrictedBase || failedScan
const tooltipMessage = failedScan
const canDownloadSubmission = (
!canViewSubmissions && String(submission.memberId) === String(loginUserInfo?.userId)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for canDownloadSubmission is inverted. The condition checks if the user can download their own submission, but then it's used with a negation in the isRestricted calculation. This should be cannotDownloadSubmission or the logic should be canViewSubmissions || String(submission.memberId) === String(loginUserInfo?.userId) to properly determine if download is allowed.

Suggested change
!canViewSubmissions && String(submission.memberId) === String(loginUserInfo?.userId)
canViewSubmissions || String(submission.memberId) === String(loginUserInfo?.userId)

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@kkartunov kkartunov Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hentrymartin can update? Both AIs are in agreement about the inverted logic.

)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name canDownloadSubmission is misleading. It actually represents whether the user CAN download (they own it and can't view all), but it's used with a negation in the restriction check. Consider renaming to cannotDownloadSubmission or isDownloadDisabled to match the naming pattern used in other files like TableIterativeReview.tsx and TableCheckpointSubmissions.tsx.

Copilot uses AI. Check for mistakes.
const isRestricted = isRestrictedBase || failedScan || !canDownloadSubmission
let tooltipMessage = failedScan
? VIRUS_SCAN_FAILED_MESSAGE
: (
getRestrictionMessageForMember(submission.memberId)
?? restrictionMessage
)

if (!canDownloadSubmission) {
tooltipMessage = SUBMISSION_DOWNLOAD_RESTRICTION_MESSAGE
}

const isButtonDisabled = Boolean(
props.isDownloading[submission.id]
|| isRestricted,
Expand Down
37 changes: 35 additions & 2 deletions src/apps/review/src/lib/components/TableAppeals/TableAppeals.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { useWindowSize, WindowSize } from '~/libs/shared'
import { Table, TableColumn } from '~/libs/ui'

import { WITHOUT_APPEAL } from '../../../config/index.config'
import { ChallengeDetailContext } from '../../contexts'
import { ChallengeDetailContext, ReviewAppContext } from '../../contexts'
import { useSubmissionDownloadAccess } from '../../hooks/useSubmissionDownloadAccess'
import type { UseSubmissionDownloadAccessResult } from '../../hooks/useSubmissionDownloadAccess'
import { useRolePermissions } from '../../hooks/useRolePermissions'
Expand All @@ -22,6 +22,7 @@ import {
ChallengeDetailContextModel,
ChallengeInfo,
MappingReviewAppeal,
ReviewAppContextModel,
SubmissionInfo,
} from '../../models'
import { TableWrapper } from '../TableWrapper'
Expand Down Expand Up @@ -70,7 +71,7 @@ export const TableAppeals: FC<TableAppealsProps> = (props: TableAppealsProps) =>
reviewers,
}: ChallengeDetailContextModel = useContext(ChallengeDetailContext)
const { width: screenWidth }: WindowSize = useWindowSize()

const { loginUserInfo }: ReviewAppContextModel = useContext(ReviewAppContext)
const downloadAccess: UseSubmissionDownloadAccessResult = useSubmissionDownloadAccess()
const {
getRestrictionMessageForMember,
Expand Down Expand Up @@ -219,13 +220,45 @@ export const TableAppeals: FC<TableAppealsProps> = (props: TableAppealsProps) =>
[aggregatedResults],
)

const { canViewAllSubmissions }: UseRolePermissionsResult = useRolePermissions()

const isCompletedDesignChallenge = useMemo(() => {
if (!challengeInfo) return false
const type = challengeInfo.track.name ? String(challengeInfo.track.name)
.toLowerCase() : ''
const status = challengeInfo.status ? String(challengeInfo.status)
.toLowerCase() : ''
return type === 'design' && (
status === 'completed'
)
}, [challengeInfo])

const isSubmissionsViewable = useMemo(() => {
if (!challengeInfo?.metadata?.length) return false
return challengeInfo.metadata.some(m => m.name === 'submissionsViewable' && String(m.value)
.toLowerCase() === 'true')
}, [challengeInfo])

const canViewSubmissions = useMemo(() => {
if (isCompletedDesignChallenge) {
return canViewAllSubmissions || isSubmissionsViewable
}

return true
}, [isCompletedDesignChallenge, isSubmissionsViewable, canViewAllSubmissions])

const isSubmissionNotViewable = (submission: SubmissionRow): boolean => (
!canViewSubmissions && String(submission.memberId) !== String(loginUserInfo?.userId)
)

const downloadButtonConfig = useMemo<DownloadButtonConfig>(
() => ({
downloadSubmission,
getRestrictionMessageForMember,
isDownloading,
isSubmissionDownloadRestricted,
isSubmissionDownloadRestrictedForMember,
isSubmissionNotViewable,
ownedMemberIds,
restrictionMessage,
shouldRestrictSubmitterToOwnSubmission: false,
Comment on lines 254 to 264
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependency array is missing isSubmissionNotViewable which is included in the config object on line 261. Since isSubmissionNotViewable references canViewSubmissions and loginUserInfo, these dependencies should be added to the dependency array.

Copilot uses AI. Check for mistakes.
Expand Down
41 changes: 38 additions & 3 deletions src/apps/review/src/lib/components/TableCheckpointSubmissions/TableCheckpointSubmissions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ import {
import { ChallengeDetailContext, ReviewAppContext } from '../../contexts'
import { updateReview } from '../../services'
import { ConfirmModal } from '../ConfirmModal'
import { useSubmissionDownloadAccess } from '../../hooks'
import { useRolePermissions, UseRolePermissionsResult, useSubmissionDownloadAccess } from '../../hooks'
import type { UseSubmissionDownloadAccessResult } from '../../hooks/useSubmissionDownloadAccess'
import { CollapsibleAiReviewsRow } from '../CollapsibleAiReviewsRow'
import { SUBMISSION_DOWNLOAD_RESTRICTION_MESSAGE } from '../../constants'

import styles from './TableCheckpointSubmissions.module.scss'

Expand Down Expand Up @@ -65,6 +66,7 @@ export const TableCheckpointSubmissions: FC<Props> = (props: Props) => {
const datas: Screening[] | undefined = props.datas
const downloadSubmission = props.downloadSubmission
const isDownloading = props.isDownloading
const { canViewAllSubmissions }: UseRolePermissionsResult = useRolePermissions()

const {
challengeInfo,
Expand Down Expand Up @@ -115,6 +117,31 @@ export const TableCheckpointSubmissions: FC<Props> = (props: Props) => {
getRestrictionMessageForMember,
}: UseSubmissionDownloadAccessResult = useSubmissionDownloadAccess()

const isCompletedDesignChallenge = useMemo(() => {
if (!challengeInfo) return false
const type = challengeInfo.track.name ? String(challengeInfo.track.name)
.toLowerCase() : ''
const status = challengeInfo.status ? String(challengeInfo.status)
.toLowerCase() : ''
return type === 'design' && (
status === 'completed'
)
}, [challengeInfo])

const isSubmissionsViewable = useMemo(() => {
if (!challengeInfo?.metadata?.length) return false
return challengeInfo.metadata.some(m => m.name === 'submissionsViewable' && String(m.value)
.toLowerCase() === 'true')
}, [challengeInfo])

const canViewSubmissions = useMemo(() => {
if (isCompletedDesignChallenge) {
return canViewAllSubmissions || isSubmissionsViewable
}

return true
}, [isCompletedDesignChallenge, isSubmissionsViewable, canViewAllSubmissions])

const openReopenDialog = useCallback(
(entry: Screening, isOwnReview: boolean): void => {
if (!entry.reviewId) {
Expand Down Expand Up @@ -211,10 +238,18 @@ export const TableCheckpointSubmissions: FC<Props> = (props: Props) => {
? undefined
: data.virusScan
const failedScan = normalizedVirusScan === false
const isRestrictedForRow = isRestrictedBase || failedScan
const tooltipMessage = failedScan
const isDownloadDisabled = (
!canViewSubmissions && String(data.memberId) !== String(loginUserInfo?.userId)
)
const isRestrictedForRow = isRestrictedBase || failedScan || isDownloadDisabled
let tooltipMessage = failedScan
? 'Submission failed virus scan'
: (getRestrictionMessageForMember(data.memberId) ?? restrictionMessage)

if (isDownloadDisabled) {
tooltipMessage = SUBMISSION_DOWNLOAD_RESTRICTION_MESSAGE
}

const isButtonDisabled = Boolean(
isDownloading[data.submissionId]
|| isRestrictedForRow,
Expand Down
40 changes: 37 additions & 3 deletions src/apps/review/src/lib/components/TableIterativeReview/TableIterativeReview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import { resolveSubmissionReviewResult } from '../common/reviewResult'
import { ProgressBar } from '../ProgressBar'
import { TableWrapper } from '../TableWrapper'
import { CollapsibleAiReviewsRow } from '../CollapsibleAiReviewsRow'
import { SUBMISSION_DOWNLOAD_RESTRICTION_MESSAGE } from '../../constants'

import styles from './TableIterativeReview.module.scss'

Expand Down Expand Up @@ -505,7 +506,7 @@ export const TableIterativeReview: FC<Props> = (props: Props) => {
const isTablet = useMemo(() => screenWidth <= 744, [screenWidth])
const { loginUserInfo }: ReviewAppContextModel = useContext(ReviewAppContext)
const { actionChallengeRole, myChallengeResources }: useRoleProps = useRole()
const { isCopilotWithReviewerAssignments }: UseRolePermissionsResult = useRolePermissions()
const { isCopilotWithReviewerAssignments, canViewAllSubmissions }: UseRolePermissionsResult = useRolePermissions()
const isSubmitterView = actionChallengeRole === SUBMITTER
const ownedMemberIds: Set<string> = useMemo(
(): Set<string> => new Set(
Expand Down Expand Up @@ -658,6 +659,31 @@ export const TableIterativeReview: FC<Props> = (props: Props) => {
const isPostMortemColumn = columnLabelKey === 'postmortem'
const isApprovalColumn = columnLabelKey === 'approval'

const isCompletedDesignChallenge = useMemo(() => {
if (!challengeInfo) return false
const type = challengeInfo.track.name ? String(challengeInfo.track.name)
.toLowerCase() : ''
const status = challengeInfo.status ? String(challengeInfo.status)
.toLowerCase() : ''
return type === 'design' && (
status === 'completed'
)
}, [challengeInfo])

const isSubmissionsViewable = useMemo(() => {
if (!challengeInfo?.metadata?.length) return false
return challengeInfo.metadata.some(m => m.name === 'submissionsViewable' && String(m.value)
.toLowerCase() === 'true')
}, [challengeInfo])

const canViewSubmissions = useMemo(() => {
if (isCompletedDesignChallenge) {
return canViewAllSubmissions || isSubmissionsViewable
}

return true
}, [isCompletedDesignChallenge, isSubmissionsViewable, canViewAllSubmissions])
Comment on lines +679 to +685
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependency array is missing canViewSubmissions and loginUserInfo which are used in the isDownloadDisabled calculation within the renderer function. These should be added to the dependency array.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@kkartunov kkartunov Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hentrymartin please add all missing dependencies.


const submissionColumn: TableColumn<SubmissionInfo> = useMemo(
() => ({
className: styles.submissionColumn,
Expand All @@ -682,10 +708,14 @@ export const TableIterativeReview: FC<Props> = (props: Props) => {
? undefined
: data.virusScan
const failedScan = normalizedVirusScan === false
const isDownloadDisabled = (
!canViewSubmissions && String(data.memberId) !== String(loginUserInfo?.userId)
)
const isButtonDisabled = Boolean(
isDownloading[data.id]
|| isRestrictedForMember
|| failedScan,
|| failedScan
|| isDownloadDisabled,
)

const downloadButton = (
Expand All @@ -712,14 +742,18 @@ export const TableIterativeReview: FC<Props> = (props: Props) => {
})
}

const tooltipContent = failedScan
let tooltipContent = failedScan
? 'Submission failed virus scan'
: isRestrictedForMember
? memberRestrictionMessage ?? restrictionMessage
: isOwnershipRestricted
? DOWNLOAD_OWN_SUBMISSION_TOOLTIP
: (isSubmissionDownloadRestricted && restrictionMessage) || undefined

if (isDownloadDisabled) {
tooltipContent = SUBMISSION_DOWNLOAD_RESTRICTION_MESSAGE
}

const downloadControl = isOwnershipRestricted ? (
<span className={styles.textBlue}>
{data.id}
Expand Down
36 changes: 35 additions & 1 deletion src/apps/review/src/lib/components/TableReview/TableReview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { MobileTableColumn } from '~/apps/admin/src/lib/models/MobileTableColumn
import { handleError, useWindowSize, WindowSize } from '~/libs/shared'
import { IconOutline, Table, TableColumn } from '~/libs/ui'

import { ChallengeDetailContext } from '../../contexts'
import { ChallengeDetailContext, ReviewAppContext } from '../../contexts'
import { useRole, useScorecardPassingScores, useSubmissionDownloadAccess } from '../../hooks'
import type { useRoleProps } from '../../hooks/useRole'
import { useSubmissionHistory } from '../../hooks/useSubmissionHistory'
Expand All @@ -28,6 +28,7 @@ import type { UseSubmissionDownloadAccessResult } from '../../hooks/useSubmissio
import {
ChallengeDetailContextModel,
MappingReviewAppeal,
ReviewAppContextModel,
SubmissionInfo,
} from '../../models'
import {
Expand Down Expand Up @@ -120,6 +121,7 @@ export const TableReview: FC<TableReviewProps> = (props: TableReviewProps) => {
isSubmissionDownloadRestrictedForMember,
restrictionMessage,
}: UseSubmissionDownloadAccessResult = useSubmissionDownloadAccess()
const { loginUserInfo }: ReviewAppContextModel = useContext(ReviewAppContext)

const isTablet = useMemo<boolean>(() => screenWidth <= 744, [screenWidth])
const reviewPhaseDatas = useMemo<SubmissionInfo[]>(
Expand Down Expand Up @@ -379,13 +381,45 @@ export const TableReview: FC<TableReviewProps> = (props: TableReviewProps) => {
[],
)

const { canViewAllSubmissions }: UseRolePermissionsResult = useRolePermissions()

const isCompletedDesignChallenge = useMemo(() => {
if (!challengeInfo) return false
const type = challengeInfo.track.name ? String(challengeInfo.track.name)
.toLowerCase() : ''
const status = challengeInfo.status ? String(challengeInfo.status)
.toLowerCase() : ''
return type === 'design' && (
status === 'completed'
)
}, [challengeInfo])

const isSubmissionsViewable = useMemo(() => {
if (!challengeInfo?.metadata?.length) return false
return challengeInfo.metadata.some(m => m.name === 'submissionsViewable' && String(m.value)
.toLowerCase() === 'true')
}, [challengeInfo])

const canViewSubmissions = useMemo(() => {
if (isCompletedDesignChallenge) {
return canViewAllSubmissions || isSubmissionsViewable
}

return true
}, [isCompletedDesignChallenge, isSubmissionsViewable, canViewAllSubmissions])

const isSubmissionNotViewable = (submission: SubmissionRow): boolean => (
!canViewSubmissions && String(submission.memberId) !== String(loginUserInfo?.userId)
)

const downloadButtonConfig = useMemo<DownloadButtonConfig>(
() => ({
downloadSubmission,
getRestrictionMessageForMember,
isDownloading,
isSubmissionDownloadRestricted,
isSubmissionDownloadRestrictedForMember,
isSubmissionNotViewable,
ownedMemberIds,
restrictionMessage,
shouldRestrictSubmitterToOwnSubmission: false,
Comment on lines 415 to 425
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependency array is missing isSubmissionNotViewable which is included in the config object. Since isSubmissionNotViewable references canViewSubmissions and loginUserInfo, the useMemo should include these dependencies or include isSubmissionNotViewable itself in the dependency array.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@kkartunov kkartunov Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hentrymartin I think this will be fixed by above but please do check.

Expand Down
Loading
Loading