Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 21 additions & 0 deletions sql/reports/topcoder/registrant-countries.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
SELECT DISTINCT
res."memberHandle" AS handle,
mem.email AS email,
COALESCE(home_code.name, home_id.name, mem."homeCountryCode") AS home_country,
COALESCE(comp_code.name, comp_id.name, mem."competitionCountryCode") AS competition_country
FROM resources."Resource" AS res
JOIN resources."ResourceRole" AS rr
ON rr.id = res."roleId"
LEFT JOIN members."member" AS mem
ON mem."userId"::text = res."memberId"

Choose a reason for hiding this comment

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

[❗❗ 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 home_code
ON UPPER(home_code."countryCode") = UPPER(mem."homeCountryCode")
LEFT JOIN lookups."Country" AS home_id
ON UPPER(home_id.id) = UPPER(mem."homeCountryCode")
LEFT JOIN lookups."Country" AS comp_code
ON UPPER(comp_code."countryCode") = UPPER(mem."competitionCountryCode")
LEFT JOIN lookups."Country" AS comp_id
ON UPPER(comp_id.id) = UPPER(mem."competitionCountryCode")
WHERE rr.name = 'Submitter'
AND res."challengeId" = $1::text

Choose a reason for hiding this comment

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

[⚠️ 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.

ORDER BY res."memberHandle";
5 changes: 5 additions & 0 deletions src/app-constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export const Scopes = {
TopgearChallenge: "reports:topgear-challenge",
TopgearCancelledChallenge: "reports:topgear-cancelled-challenge",
AllReports: "reports:all",
TopcoderReports: "reports:topcoder",
TopgearChallengeTechnology: "reports:topgear-challenge-technology",
TopgearChallengeStatsByUser: "reports:topgear-challenge-stats-by-user",
TopgearChallengeRegistrantDetails:
Expand All @@ -18,3 +19,7 @@ export const Scopes = {
SubmissionLinks: "reports:challenge-submission-links",
},
};

export const UserRoles = {
Admin: "Administrator",
};
59 changes: 59 additions & 0 deletions src/auth/guards/topcoder-reports.guard.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import {
CanActivate,
ExecutionContext,
ForbiddenException,
Injectable,
UnauthorizedException,
} from "@nestjs/common";

import { Scopes, UserRoles } from "../../app-constants";

@Injectable()
export class TopcoderReportsGuard implements CanActivate {
private static readonly adminRoles = new Set(
Object.values(UserRoles).map((role) => role.toLowerCase()),

Choose a reason for hiding this comment

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

[⚠️ 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.

);

canActivate(context: ExecutionContext): boolean {
const request = context.switchToHttp().getRequest();
const authUser = request.authUser;

if (!authUser) {
throw new UnauthorizedException("You are not authenticated.");
}

if (authUser.isMachine) {
const scopes: string[] = authUser.scopes ?? [];
if (this.hasRequiredScope(scopes)) {
return true;
}

throw new ForbiddenException(
"You do not have the required permissions to access this resource.",
);
}

const roles: string[] = authUser.roles ?? [];
if (this.isAdmin(roles)) {
return true;
}

throw new ForbiddenException(
"You do not have the required permissions to access this resource.",
);
}

private hasRequiredScope(scopes: string[]): boolean {
const normalizedScopes = scopes.map((scope) => scope?.toLowerCase());

Choose a reason for hiding this comment

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

[⚠️ 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.

return (
normalizedScopes.includes(Scopes.TopcoderReports.toLowerCase()) ||
normalizedScopes.includes(Scopes.AllReports.toLowerCase())
);
}

private isAdmin(roles: string[]): boolean {
return roles.some((role) =>
TopcoderReportsGuard.adminRoles.has(role?.toLowerCase()),

Choose a reason for hiding this comment

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

[⚠️ 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.

);
}
}
11 changes: 11 additions & 0 deletions src/reports/topcoder/dto/registrant-countries.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Transform } from "class-transformer";
import { IsNotEmpty, IsString } from "class-validator";

export class RegistrantCountriesQueryDto {
@Transform(({ value }) =>

Choose a reason for hiding this comment

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

[⚠️ 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.

typeof value === "string" ? value.trim() : value,
)
@IsString()
@IsNotEmpty()
challengeId!: string;
}
28 changes: 26 additions & 2 deletions src/reports/topcoder/topcoder-reports.controller.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import { Controller, Get } from "@nestjs/common";
import { ApiOperation, ApiTags } from "@nestjs/swagger";
import { Controller, Get, Query, Res, UseGuards } from "@nestjs/common";
import { ApiBearerAuth, ApiOperation, ApiTags } from "@nestjs/swagger";
import { Response } from "express";
import { TopcoderReportsService } from "./topcoder-reports.service";
import { RegistrantCountriesQueryDto } from "./dto/registrant-countries.dto";
import { TopcoderReportsGuard } from "../../auth/guards/topcoder-reports.guard";

@ApiTags("Topcoder Reports")
@ApiBearerAuth()

Choose a reason for hiding this comment

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

[❗❗ 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.

@UseGuards(TopcoderReportsGuard)

Choose a reason for hiding this comment

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

[❗❗ 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.

@Controller("/topcoder")
export class TopcoderReportsController {
constructor(private readonly reports: TopcoderReportsService) {}
Expand All @@ -13,6 +18,25 @@ export class TopcoderReportsController {
return this.reports.getMemberCount();
}

@Get("/registrant-countries")
@ApiOperation({
summary: "Countries of all registrants for the specified challenge",
})
async getRegistrantCountries(
@Query() query: RegistrantCountriesQueryDto,
@Res() res: Response,

Choose a reason for hiding this comment

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

[⚠️ 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.

) {
const { challengeId } = query;

Choose a reason for hiding this comment

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

[❗❗ security]
Ensure that challengeId is validated properly in RegistrantCountriesQueryDto to prevent potential security issues such as injection attacks.

const csv = await this.reports.getRegistrantCountriesCsv(challengeId);
const filename =
challengeId.length > 0
? `registrant-countries-${challengeId}.csv`
: "registrant-countries.csv";
res.setHeader("Content-Type", "text/csv");
res.setHeader("Content-Disposition", `attachment; filename="${filename}"`);
res.send(csv);
}

@Get("/total-copilots")
@ApiOperation({ summary: "Total number of Copilots" })
getTotalCopilots() {
Expand Down
5 changes: 3 additions & 2 deletions src/reports/topcoder/topcoder-reports.module.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { Module } from "@nestjs/common";
import { TopcoderReportsController } from "./topcoder-reports.controller";
import { TopcoderReportsService } from "./topcoder-reports.service";
import { SqlLoaderService } from "../../common/sql-loader.service";
import { TopcoderReportsController } from "./topcoder-reports.controller";
import { TopcoderReportsGuard } from "../../auth/guards/topcoder-reports.guard";

@Module({
controllers: [TopcoderReportsController],
providers: [TopcoderReportsService, SqlLoaderService],
providers: [TopcoderReportsService, SqlLoaderService, TopcoderReportsGuard],
})
export class TopcoderReportsModule {}
52 changes: 52 additions & 0 deletions src/reports/topcoder/topcoder-reports.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@ import { Injectable } from "@nestjs/common";
import { DbService } from "../../db/db.service";
import { SqlLoaderService } from "../../common/sql-loader.service";

type RegistrantCountriesRow = {
handle: string | null;
email: string | null;
home_country: string | null;
competition_country: string | null;
};

@Injectable()
export class TopcoderReportsService {
constructor(
Expand Down Expand Up @@ -387,4 +394,49 @@ export class TopcoderReportsService {
),
}));
}

async getRegistrantCountriesCsv(challengeId: string) {
const query = this.sql.load("reports/topcoder/registrant-countries.sql");

Choose a reason for hiding this comment

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

[⚠️ 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);

Choose a reason for hiding this comment

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

[⚠️ 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.

}

private rowsToCsv(rows: RegistrantCountriesRow[]) {
const header = [
"Handle",
"Email",
"Home country",
"Competition country",
];

const lines = [
header.map((value) => this.toCsvCell(value)).join(","),
...rows.map((row) =>
[
row.handle,
row.email,
row.home_country,
row.competition_country,
]
.map((value) => this.toCsvCell(value))
.join(","),
),
];

return lines.join("\n");
}

private toCsvCell(value: string | null | undefined) {

Choose a reason for hiding this comment

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

[💡 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.

if (value === null || value === undefined) {
return "";
}
const text = String(value);
if (!/[",\r\n]/.test(text)) {
return text;
}
const escaped = text.replace(/"/g, '""');
return `"${escaped}"`;
}
}