Conversation
…l-tooltip-info PM-3717 - map challenge role to clear labels
fix(PM-2662): prevent download for submitters if challenge is configured that way
…r profile sensitive data
…e-privacy PM-3825 make sure we properly restrict visibility over user profile sensitive data
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| if (authProfile | ||
| .roles?.some( | ||
| role => EMAIL_VIEW_ROLES.some(allowed => role.toLowerCase() === allowed.toLowerCase()), |
There was a problem hiding this comment.
[💡 performance]
Consider using includes instead of some for checking role membership, as it can improve readability and performance slightly when checking for a single role match.
| const Phones: FC<PhonesProps> = (props: PhonesProps) => { | ||
| const canEdit: boolean = props.authProfile?.handle === props.profile.handle | ||
| const canSeePhonesValue: boolean = canSeePhones(props.authProfile, props.profile) | ||
| const canSeeEmailValue: boolean = canSeeEmail(props.authProfile, props.profile) |
There was a problem hiding this comment.
[❗❗ security]
The canSeeEmail function is newly introduced here. Ensure that this function is properly tested to verify that it correctly determines email visibility based on the provided profiles. This is important to prevent unauthorized access to sensitive information.
| const cannotDownloadSubmission = ( | ||
| !canViewSubmissions && String(submission.memberId) === String(loginUserInfo?.userId) | ||
| ) | ||
| const isRestricted = isRestrictedBase || failedScan || !cannotDownloadSubmission |
There was a problem hiding this comment.
[❗❗ correctness]
The logic for isRestricted seems incorrect. The condition !cannotDownloadSubmission should be cannotDownloadSubmission to ensure that the restriction is applied when the user cannot download the submission.
|
|
||
| const { canViewAllSubmissions }: UseRolePermissionsResult = useRolePermissions() | ||
|
|
||
| const isCompletedDesignChallenge = useMemo(() => { |
There was a problem hiding this comment.
[💡 readability]
The isCompletedDesignChallenge function could be simplified by directly returning the boolean expression without the if statement. This would improve readability.
| ) | ||
| }, [challengeInfo]) | ||
|
|
||
| const isSubmissionsViewable = useMemo(() => { |
There was a problem hiding this comment.
[correctness]
The isSubmissionsViewable function uses some to check metadata, which is efficient. However, ensure that metadata is always an array to avoid potential runtime errors.
| return true | ||
| }, [isCompletedDesignChallenge, isSubmissionsViewable, canViewAllSubmissions]) | ||
|
|
||
| const isSubmissionNotViewable = (submission: SubmissionRow): boolean => ( |
There was a problem hiding this comment.
[correctness]
The isSubmissionNotViewable function compares memberId and userId as strings. Ensure these values are always strings to prevent unexpected behavior.
| getRestrictionMessageForMember, | ||
| }: UseSubmissionDownloadAccessResult = useSubmissionDownloadAccess() | ||
|
|
||
| const isCompletedDesignChallenge = useMemo(() => { |
There was a problem hiding this comment.
[performance]
The use of useMemo here is appropriate for optimizing performance by memoizing the result of the computation. However, ensure that challengeInfo is a stable reference or consider using a deep comparison to avoid unnecessary recalculations.
| ) | ||
| }, [challengeInfo]) | ||
|
|
||
| const isSubmissionsViewable = useMemo(() => { |
There was a problem hiding this comment.
[performance]
The useMemo hook is used to memoize the result of checking if submissions are viewable. Ensure that challengeInfo.metadata is a stable reference or consider using a deep comparison to avoid unnecessary recalculations.
| .toLowerCase() === 'true') | ||
| }, [challengeInfo]) | ||
|
|
||
| const canViewSubmissions = useMemo(() => { |
There was a problem hiding this comment.
[performance]
The useMemo hook is used to determine if submissions can be viewed. Ensure that all dependencies (isCompletedDesignChallenge, isSubmissionsViewable, canViewAllSubmissions) are stable references or consider using a deep comparison to avoid unnecessary recalculations.
| const isRestrictedForRow = isRestrictedBase || failedScan | ||
| const tooltipMessage = failedScan | ||
| const isDownloadDisabled = ( | ||
| !canViewSubmissions && String(data.memberId) !== String(loginUserInfo?.userId) |
There was a problem hiding this comment.
[💡 readability]
The condition String(data.memberId) !== String(loginUserInfo?.userId) could be simplified by ensuring data.memberId and loginUserInfo?.userId are of the same type before comparison, which would improve readability and potentially avoid unnecessary type conversion.
| ) | ||
| }, [challengeInfo]) | ||
|
|
||
| const isSubmissionsViewable = useMemo(() => { |
There was a problem hiding this comment.
[correctness]
The isSubmissionsViewable function uses challengeInfo.metadata.some() to check for a specific metadata entry. Ensure that metadata is always an array to avoid potential runtime errors. Consider adding a type guard or defaulting to an empty array if metadata might be undefined.
| .toLowerCase() === 'true') | ||
| }, [challengeInfo]) | ||
|
|
||
| const canViewSubmissions = useMemo(() => { |
There was a problem hiding this comment.
[design]
The canViewSubmissions logic assumes that if a challenge is not a completed design challenge, submissions are always viewable. Verify that this assumption aligns with the intended business logic, as it might lead to unintended access if the conditions change.
| : data.virusScan | ||
| const failedScan = normalizedVirusScan === false | ||
| const isDownloadDisabled = ( | ||
| !canViewSubmissions && String(data.memberId) !== String(loginUserInfo?.userId) |
There was a problem hiding this comment.
[correctness]
The isDownloadDisabled condition checks if data.memberId is not equal to loginUserInfo?.userId. Ensure that both memberId and userId are consistently formatted (e.g., trimmed, lowercased) to avoid false negatives due to formatting differences.
| }, [challengeInfo]) | ||
|
|
||
| const canViewSubmissions = useMemo(() => { | ||
| if (isCompletedDesignChallenge) { |
There was a problem hiding this comment.
[💡 maintainability]
The canViewSubmissions logic could be simplified by returning canViewAllSubmissions || isSubmissionsViewable directly, as the isCompletedDesignChallenge check is redundant when canViewAllSubmissions or isSubmissionsViewable is true.
| }, [isCompletedDesignChallenge, isSubmissionsViewable, canViewAllSubmissions]) | ||
|
|
||
| const isSubmissionNotViewable = (submission: SubmissionRow): boolean => ( | ||
| !canViewSubmissions && String(submission.memberId) !== String(loginUserInfo?.userId) |
There was a problem hiding this comment.
[correctness]
The function isSubmissionNotViewable uses String(submission.memberId) !== String(loginUserInfo?.userId), which could potentially lead to incorrect comparisons if memberId or userId are not strings. Consider ensuring these values are always strings before comparison.
| !canViewSubmissions && String(submission.memberId) !== String(loginUserInfo?.userId) | ||
| ) | ||
|
|
||
| const aggregatedSubmissionRows = useMemo<SubmissionRow[]>( |
There was a problem hiding this comment.
[❗❗ correctness]
The isSubmissionNotViewable function is defined but not used in the aggregatedSubmissionRows computation. Ensure that the logic for filtering submissions based on viewability is correctly integrated, as this might affect the correctness of the displayed data.
| }))), | ||
| [aggregatedRows, filterFunc, canViewSubmissions, loginUserInfo?.userId], | ||
| })), | ||
| [aggregatedRows], |
There was a problem hiding this comment.
[❗❗ correctness]
The dependency array for aggregatedSubmissionRows is missing dependencies related to submission viewability logic, such as canViewSubmissions and loginUserInfo?.userId. This could lead to stale data being used if these values change.
| ) | ||
|
|
||
| const isSubmissionNotViewable = (submission: Screening): boolean => ( | ||
| !canViewSubmissions && String(submission.memberId) !== String(loginUserInfo?.userId) |
There was a problem hiding this comment.
[correctness]
The function isSubmissionNotViewable checks if submission.memberId is not equal to loginUserInfo?.userId. Ensure that both submission.memberId and loginUserInfo?.userId are always defined and of the same type to avoid unexpected behavior. Consider adding type checks or default values if necessary.
| }, [challengeInfo]) | ||
|
|
||
| const canViewSubmissions = useMemo(() => { | ||
| if (isCompletedDesignChallenge) { |
There was a problem hiding this comment.
[correctness]
In the canViewSubmissions useMemo hook, the logic checks isCompletedDesignChallenge and isSubmissionsViewable to determine the return value. Ensure that these conditions cover all possible states of challengeInfo to avoid returning incorrect permissions.
| * Table Winners. | ||
| */ | ||
| import { FC, useCallback, useContext, useMemo } from 'react' | ||
| import { FC, useContext, useMemo } from 'react' |
There was a problem hiding this comment.
[maintainability]
The useCallback import has been removed, but the filterFunc function that used it has also been removed. Ensure that this change is intentional and that filterFunc is no longer needed.
| @@ -310,11 +308,11 @@ export const TableWinners: FC<Props> = (props: Props) => { | |||
| )} | |||
| > | |||
| {isTablet ? ( | |||
There was a problem hiding this comment.
[❗❗ correctness]
The winnerData variable has been removed and replaced with datas directly in the TableMobile and Table components. Ensure that the filtering logic previously applied by filterFunc is no longer required, as this could affect the data displayed in the table.
| ? ownedMemberIds.has(submission.memberId) | ||
| : false | ||
| const isOwnershipRestricted = shouldRestrictSubmitterToOwnSubmission && !isOwnedSubmission | ||
| const isSubmissionNotAllowedToView = (isSubmissionNotViewable && isSubmissionNotViewable(submission)) |
There was a problem hiding this comment.
[❗❗ correctness]
The function isSubmissionNotViewable is used as both a boolean and a function. Ensure that isSubmissionNotViewable is indeed a function and not a boolean value to avoid runtime errors.
| ? downloadOwnSubmissionTooltip | ||
| : (isSubmissionDownloadRestricted && restrictionMessage) || undefined | ||
|
|
||
| if (isSubmissionNotAllowedToView) { |
There was a problem hiding this comment.
[maintainability]
The reassignment of tooltipContent here may lead to unexpected behavior if isSubmissionNotAllowedToView is true. Consider restructuring the logic to ensure tooltipContent is set correctly based on all conditions.
| Challenges - | ||
| {' '} | ||
| {role} | ||
| {skillEventTypeMap[role as keyof typeof skillEventTypeMap] ?? ''} |
There was a problem hiding this comment.
[correctness]
Using as keyof typeof skillEventTypeMap for type assertion can be risky if role is not guaranteed to be a key in skillEventTypeMap. Consider adding a runtime check or handling the case where role is not a valid key to avoid potential runtime errors.
…e-privacy PM-3825 #time 30min profile privac
| ) | ||
| // Check if user has admin roles or talent manager | ||
| if ( | ||
| authProfile.roles?.some(role => [ |
There was a problem hiding this comment.
[correctness]
The use of role.toLowerCase() assumes that all roles are case-insensitive. If UserRole.talentManager or any role in ADMIN_ROLES is case-sensitive, this could lead to incorrect behavior. Consider ensuring that all roles are consistently cased or explicitly handle case sensitivity.
| authProfile.roles?.some(role => [ | ||
| UserRole.talentManager, | ||
| ...ADMIN_ROLES, | ||
| ].map(r => r.toLowerCase()) |
There was a problem hiding this comment.
[performance]
The use of map followed by includes could be optimized by using some with a case-insensitive comparison directly. This avoids creating an intermediate array and improves performance slightly.
|
|
||
| // Don't render anything if user cannot edit AND cannot see any contact info | ||
| const hasContactInfo = props.profile?.email || phones.length > 0 | ||
| if (!canEdit && (!canSeeEmailValue || !hasContactInfo)) { |
There was a problem hiding this comment.
[❗❗ correctness]
The condition !canSeeEmailValue || !hasContactInfo might not work as intended if the user can see the email but there is no contact info. Consider changing the condition to !canSeeEmailValue && !hasContactInfo to ensure that the component is only hidden when both conditions are false.
4 participants
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?
https://topcoder.atlassian.net/browse/PM-3825
https://topcoder.atlassian.net/browse/PM-3717