Registrant country report for a challenge, to help with challenges that only allow submissions from a subset of countries #20
Conversation
…at only allow submissions from a subset of countries
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| JOIN resources."ResourceRole" AS rr | ||
| ON rr.id = res."roleId" | ||
| LEFT JOIN members."member" AS mem | ||
| ON mem."userId"::text = res."memberId" |
There was a problem hiding this comment.
[❗❗ correctness]
Casting mem."userId" to text for comparison with res."memberId" could lead to unexpected results if there are leading zeros or other formatting differences. Consider ensuring both fields are consistently formatted or use a more reliable method for comparison.
| LEFT JOIN lookups."Country" AS comp_id | ||
| ON UPPER(comp_id.id) = UPPER(mem."competitionCountryCode") | ||
| WHERE rr.name = 'Submitter' | ||
| AND res."challengeId" = 'e12ee862-474a-4e40-9d2d-2699ae1dfc2a' |
There was a problem hiding this comment.
[maintainability]
Hardcoding the challengeId value may limit the reusability of this query. Consider parameterizing the challengeId to make the query more flexible and maintainable.
| import { IsNotEmpty, IsString } from "class-validator"; | ||
|
|
||
| export class RegistrantCountriesQueryDto { | ||
| @Transform(({ value }) => |
There was a problem hiding this comment.
[correctness]
The @Transform decorator is used to trim the challengeId, but it does not handle cases where value might be null or undefined. Consider adding a check to ensure value is a non-null string before trimming.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
| }) | ||
| async getRegistrantCountries( | ||
| @Query() query: RegistrantCountriesQueryDto, | ||
| @Res() res: Response, |
There was a problem hiding this comment.
[maintainability]
Using @Res() in combination with @Query() can lead to issues with NestJS's automatic response handling. Consider returning the CSV data directly and allowing NestJS to handle the response, which can improve maintainability and consistency with other endpoints.
| @Query() query: RegistrantCountriesQueryDto, | ||
| @Res() res: Response, | ||
| ) { | ||
| const { challengeId } = query; |
There was a problem hiding this comment.
[❗❗ security]
Ensure that challengeId is validated properly in RegistrantCountriesQueryDto to prevent potential security issues such as injection attacks.
| } | ||
|
|
||
| async getRegistrantCountriesCsv(challengeId: string) { | ||
| const query = this.sql.load("reports/topcoder/registrant-countries.sql"); |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the database query to ensure that any issues during the query execution are properly managed and do not cause the application to crash.
| const rows = await this.db.query<RegistrantCountriesRow>(query, [ | ||
| challengeId, | ||
| ]); | ||
| return this.rowsToCsv(rows); |
There was a problem hiding this comment.
[correctness]
The rowsToCsv method assumes that the rows parameter will always be an array of RegistrantCountriesRow. Consider adding validation to ensure that the input is as expected to prevent potential runtime errors.
| return lines.join("\n"); | ||
| } | ||
|
|
||
| private toCsvCell(value: string | null | undefined) { |
There was a problem hiding this comment.
[💡 correctness]
The toCsvCell function currently handles null and undefined values by returning an empty string. Ensure that this behavior is consistent with the requirements for CSV generation, as it may lead to data misinterpretation if not handled correctly.
| LEFT JOIN lookups."Country" AS comp_id | ||
| ON UPPER(comp_id.id) = UPPER(mem."competitionCountryCode") | ||
| WHERE rr.name = 'Submitter' | ||
| AND res."challengeId" = $1::text |
There was a problem hiding this comment.
[security]
Using $1::text for parameterized queries is a good practice for preventing SQL injection. Ensure that the parameter passed is properly validated and sanitized before execution.
| @Injectable() | ||
| export class TopcoderReportsGuard implements CanActivate { | ||
| private static readonly adminRoles = new Set( | ||
| Object.values(UserRoles).map((role) => role.toLowerCase()), |
There was a problem hiding this comment.
[correctness]
Converting roles to lowercase when creating the adminRoles set may lead to unexpected behavior if UserRoles contains roles with case-sensitive distinctions. Ensure that all role comparisons are intended to be case-insensitive.
| } | ||
|
|
||
| private hasRequiredScope(scopes: string[]): boolean { | ||
| const normalizedScopes = scopes.map((scope) => scope?.toLowerCase()); |
There was a problem hiding this comment.
[correctness]
The use of optional chaining (scope?.toLowerCase()) is unnecessary here since scope is expected to be a string. If scope can be undefined, it should be handled explicitly before this point.
|
|
||
| private isAdmin(roles: string[]): boolean { | ||
| return roles.some((role) => | ||
| TopcoderReportsGuard.adminRoles.has(role?.toLowerCase()), |
There was a problem hiding this comment.
[correctness]
The use of optional chaining (role?.toLowerCase()) is unnecessary here since role is expected to be a string. If role can be undefined, it should be handled explicitly before this point.
| import { TopcoderReportsGuard } from "../../auth/guards/topcoder-reports.guard"; | ||
|
|
||
| @ApiTags("Topcoder Reports") | ||
| @ApiBearerAuth() |
There was a problem hiding this comment.
[❗❗ security]
The @ApiBearerAuth() decorator is added, which implies that authentication is required for this controller. Ensure that all endpoints within this controller are intended to be protected and that the authentication mechanism is correctly implemented and tested.
|
|
||
| @ApiTags("Topcoder Reports") | ||
| @ApiBearerAuth() | ||
| @UseGuards(TopcoderReportsGuard) |
There was a problem hiding this comment.
[❗❗ security]
The @UseGuards(TopcoderReportsGuard) decorator is added, indicating that a guard is used for authorization or authentication. Verify that the TopcoderReportsGuard is correctly implemented to handle the necessary authorization logic and that it aligns with the intended security requirements.
For NASA MM