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
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"@nestjs/platform-express": "^11.1.7",
"@nestjs/swagger": "^11.2.1",
"@prisma/client": "^6.18.0",
"@topcoder/billing-accounts-api-v6": "github:topcoder-platform/billing-accounts-api-v6#develop",
"class-transformer": "^0.5.1",
"class-validator": "^0.14.2",
"cors": "^2.8.5",
Expand Down
2,933 changes: 1,759 additions & 1,174 deletions pnpm-lock.yaml

Large diffs are not rendered by default.

80 changes: 68 additions & 12 deletions src/api/admin/admin.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,21 @@ export class AdminController {
private readonly tcMembersService: TopcoderMembersService,
) {}

private isBaAdmin(user?: { roles?: string[] }) {
Comment thread
kkartunov marked this conversation as resolved.
return (user?.roles || []).some(
(r) =>
r &&
r.trim().toLowerCase() === Role.PaymentBaAdmin.trim().toLowerCase(),
);
}

@Post('/winnings/search')
@Roles(Role.PaymentAdmin, Role.PaymentEditor, Role.PaymentViewer)
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.PaymentEditor,
Role.PaymentViewer,
)
@ApiOperation({
summary: 'Search winnings with parameters',
description: 'Roles: Payment Admin, Payment Editor, Payment Viewer',
Expand All @@ -62,8 +75,16 @@ export class AdminController {
@HttpCode(HttpStatus.OK)
async searchWinnings(
@Body() body: WinningRequestDto,
@User() user: any,
Comment thread
kkartunov marked this conversation as resolved.
): Promise<ResponseDto<SearchWinningResult>> {
const result = await this.winningsRepo.searchWinnings(body);
const result = await this.winningsRepo.searchWinnings(
await this.adminService.applyBaAdminUserFilters(
Comment thread
kkartunov marked this conversation as resolved.
user.id,
this.isBaAdmin(user),
body,
),
);

if (result.error) {
result.status = ResponseStatusType.ERROR;
}
Expand All @@ -74,7 +95,12 @@ export class AdminController {
}

@Post('/winnings/export')
@Roles(Role.PaymentAdmin, Role.PaymentEditor, Role.PaymentViewer)
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.PaymentEditor,
Role.PaymentViewer,
)
@ApiOperation({
summary: 'Export search winnings result in csv file format',
description: 'Roles: Payment Admin, Payment Editor, Payment Viewer',
Expand All @@ -91,11 +117,17 @@ export class AdminController {
@HttpCode(HttpStatus.OK)
@Header('Content-Type', 'text/csv')
@Header('Content-Disposition', 'attachment; filename="winnings.csv"')
async exportWinnings(@Body() body: WinningRequestDto) {
const result = await this.winningsRepo.searchWinnings({
...body,
limit: 999,
});
async exportWinnings(@Body() body: WinningRequestDto, @User() user: any) {
Comment thread
kkartunov marked this conversation as resolved.
const result = await this.winningsRepo.searchWinnings(
await this.adminService.applyBaAdminUserFilters(
user.id,
this.isBaAdmin(user),
Comment thread
kkartunov marked this conversation as resolved.
{
...body,
limit: 999,
},
),
);

const handles = await this.tcMembersService.getHandlesByUserIds(
result.data.winnings.map((d) => d.winnerId),
Expand Down Expand Up @@ -149,7 +181,7 @@ export class AdminController {
}

@Patch('/winnings')
@Roles(Role.PaymentAdmin, Role.PaymentEditor)
@Roles(Role.PaymentAdmin, Role.PaymentBaAdmin, Role.PaymentEditor)
Comment thread
kkartunov marked this conversation as resolved.
@ApiOperation({
summary: 'Update winnings with given parameter',
description:
Expand All @@ -175,7 +207,11 @@ export class AdminController {
);
}

const result = await this.adminService.updateWinnings(body, user.id);
const result = await this.adminService.updateWinnings(
body,
user.id,
this.isBaAdmin(user),
Comment thread
vas3a marked this conversation as resolved.
);

result.status = ResponseStatusType.SUCCESS;
if (result.error) {
Expand All @@ -186,7 +222,12 @@ export class AdminController {
}

@Get('/winnings/:winningID/audit')
@Roles(Role.PaymentAdmin, Role.PaymentEditor, Role.PaymentViewer)
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.PaymentEditor,
Role.PaymentViewer,
)
@ApiOperation({
summary: 'List winning audit logs with given winning id',
description: 'Roles: Payment Admin, Payment Editor, Payment Viewer',
Expand All @@ -203,7 +244,12 @@ export class AdminController {
})
async getWinningAudit(
@Param('winningID') winningId: string,
@User() user: any,
): Promise<ResponseDto<WinningAuditDto[]>> {
if (this.isBaAdmin(user)) {
Comment thread
vas3a marked this conversation as resolved.
await this.adminService.verifyBaAdminAccessToWinning(winningId, user.id);
Comment thread
vas3a marked this conversation as resolved.
}

const result = await this.adminService.getWinningAudit(winningId);

result.status = ResponseStatusType.SUCCESS;
Expand All @@ -215,7 +261,12 @@ export class AdminController {
}

@Get('/winnings/:winningID/audit-payout')
@Roles(Role.PaymentAdmin, Role.PaymentEditor, Role.PaymentViewer)
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.PaymentEditor,
Role.PaymentViewer,
)
@ApiOperation({
summary: 'Fetch winnings payout audit logs with given winning id.',
description:
Expand All @@ -233,7 +284,12 @@ export class AdminController {
})
async getWinningAuditPayout(
@Param('winningID') winningId: string,
@User() user: any,
Comment thread
vas3a marked this conversation as resolved.
): Promise<ResponseDto<AuditPayoutDto[]>> {
if (this.isBaAdmin(user)) {
Comment thread
vas3a marked this conversation as resolved.
await this.adminService.verifyBaAdminAccessToWinning(winningId, user.id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[❗❗ security]
The method verifyBaAdminAccessToWinning is called to verify access. Ensure that this method handles all edge cases and potential errors, such as invalid winningId or user ID, to prevent security vulnerabilities.

}

const result = await this.adminService.getWinningAuditPayout(winningId);

result.status = ResponseStatusType.SUCCESS;
Expand Down
81 changes: 79 additions & 2 deletions src/api/admin/admin.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import { PaymentStatus } from 'src/dto/payment.dto';
import { WinningAuditDto, AuditPayoutDto } from './dto/audit.dto';
import { WinningUpdateRequestDto } from './dto/winnings.dto';
import { TopcoderChallengesService } from 'src/shared/topcoder/challenges.service';
import { Logger } from 'src/shared/global';
import { Logger, getBaClient } from 'src/shared/global';
import { WinningRequestDto } from 'src/dto/winning.dto';

function formatDate(date = new Date()) {
const pad = (n, z = 2) => String(n).padStart(z, '0');
Expand Down Expand Up @@ -43,6 +44,32 @@ export class AdminService {
private readonly tcChallengesService: TopcoderChallengesService,
) {}

async getBillingAccountsForUser(userId: string): Promise<string[]> {
const baPrisma = getBaClient();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Consider handling potential errors when calling getBaClient() to ensure the application can gracefully handle any issues with the database connection.

const baRows = await baPrisma.billingAccountAccess.findMany({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ performance]
The findMany operation could potentially return a large dataset. Consider adding pagination or limiting the number of results to improve performance and prevent excessive memory usage.

where: {
userId,
}
});

return baRows.map(r => `${r.billingAccountId}`);
}

async applyBaAdminUserFilters(
userId: string,
isBaAdmin?: boolean,
filters: WinningRequestDto = {},
) {
if (!isBaAdmin) {
return filters;
}

return {
...filters,
billingAccounts: await this.getBillingAccountsForUser(userId),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
The billingAccounts field in the returned object could be undefined if getBillingAccountsForUser returns an empty array. Ensure that the consuming code handles this scenario appropriately.

};
}

private getWinningById(winningId: string) {
return this.prisma.winnings.findFirst({ where: { winning_id: winningId } });
}
Expand All @@ -65,6 +92,47 @@ export class AdminService {
});
}

/**
* Verify that a BA admin user has access to the billing account(s)
* associated with the given winningsId. Throws BadRequestException when
* access is not allowed.
*/
async verifyBaAdminAccessToWinning(
winningsId: string,
userId: string,
): Promise<void> {
const payments = await this.prisma.payment.findMany({
where: {
winnings_id: {
equals: winningsId,
},
},
select: {
billing_account: true,
}
});;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[💡 style]
Remove the extra semicolon at the end of the statement. It is unnecessary and could lead to confusion.


if (!payments || payments.length === 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider handling the case where payments is null or undefined explicitly before checking payments.length. This will improve clarity and ensure robustness.

// nothing to check
return;
}

const allowedBAs = await this.getBillingAccountsForUser(userId);
const paymentBAs = payments
.map((p) => p.billing_account)
.filter((b) => b !== null && b !== undefined);

const unauthorized = paymentBAs.some((ba) => !allowedBAs.includes(`${ba}`));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
The use of includes with string interpolation (${ba}) could lead to unexpected results if allowedBAs contains non-string elements. Ensure that allowedBAs is an array of strings to avoid potential type issues.

if (unauthorized) {
this.logger.warn(
`BA admin ${userId} attempted to access winnings ${winningsId} for unauthorized billing account(s)`,
);
throw new BadRequestException(
'BA admin user does not have access to the billing account for this winnings',
);
}
}

/**
* Update winnings with parameters
* @param body the request body
Expand All @@ -74,6 +142,7 @@ export class AdminService {
async updateWinnings(
body: WinningUpdateRequestDto,
userId: string,
isBaAdmin?: boolean,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[❗❗ security]
The addition of the isBaAdmin parameter should be carefully considered for its impact on the method's logic. Ensure that the parameter is used securely and correctly within the method to prevent unauthorized access or incorrect behavior. If this parameter affects authorization logic, it should be validated and documented appropriately.

): Promise<ResponseDto<string>> {
const result = new ResponseDto<string>();

Expand All @@ -84,6 +153,10 @@ export class AdminService {
);
this.logger.log(`updateWinnings payload: ${JSON.stringify(body)}`);

if (isBaAdmin) {
await this.verifyBaAdminAccessToWinning(body.winningsId, userId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
Consider handling potential errors from verifyBaAdminAccessToWinning. If this function throws an error, it might cause the entire operation to fail without a clear error message or recovery path.

}

try {
const payments = await this.getPaymentsByWinningsId(
winningsId,
Expand All @@ -100,6 +173,8 @@ export class AdminService {
throw new NotFoundException('failed to get current payments');
}


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[💡 style]
Remove the unnecessary blank lines to maintain code consistency and avoid unnecessary diff noise.


let releaseDate;
if (body.paymentStatus) {
releaseDate = await this.getPaymentReleaseDateByWinningsId(winningsId);
Expand Down Expand Up @@ -385,7 +460,9 @@ export class AdminService {
// Run all transaction tasks in a single prisma transaction
await this.prisma.$transaction(async (tx) => {
for (let i = 0; i < transactions.length; i++) {
this.logger.log(`Executing transaction ${i + 1}/${transactions.length}`);
this.logger.log(
`Executing transaction ${i + 1}/${transactions.length}`,
);
await transactions[i](tx);
}
});
Expand Down
13 changes: 12 additions & 1 deletion src/api/repository/winnings.repo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export class WinningsRepository {
winnerIds?: string[],
externalIds?: string[],
date?: DateFilterType,
): Prisma.winningsFindManyArgs['where'] {
): Prisma.winningsWhereInput {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[❗❗ correctness]
The return type has been changed from Prisma.winningsFindManyArgs['where'] to Prisma.winningsWhereInput. Ensure that this change is intentional and that all usages of this method are compatible with the new return type. This change could potentially impact the correctness of the code if not handled properly.

const typeFilter = type
? {
equals: type as winnings_type,
Expand Down Expand Up @@ -185,6 +185,17 @@ export class WinningsRepository {
searchProps.date,
);

if (searchProps.billingAccounts) {
// override payment filter to include billing account constraint
// while preserving status/installment constraints
(queryWhere as any).payment.some = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Casting queryWhere to any can lead to runtime errors if the structure of queryWhere changes unexpectedly. Consider defining a proper type for queryWhere to ensure type safety and maintainability.

...queryWhere.payment!.some,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[❗❗ correctness]
The use of non-null assertion ! on queryWhere.payment assumes that payment is always defined. If payment can be undefined, this will cause a runtime error. Consider adding a check to ensure payment is defined before accessing its properties.

billing_account: {
in: searchProps.billingAccounts,
},
};
}

const orderBy = this.getOrderByWithWinnerId(
searchProps.sortBy,
searchProps.sortOrder,
Expand Down
3 changes: 3 additions & 0 deletions src/config/config.env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,7 @@ export class ConfigEnv {

@IsInt({ each: true })
TGBillingAccounts = [80000062, 80002800];

@IsString()
BILLING_ACCOUNTS_DB_URL!: string;
Comment thread
kkartunov marked this conversation as resolved.
}
1 change: 1 addition & 0 deletions src/core/auth/auth.constants.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export enum Role {
Administrator = 'Administrator',
PaymentAdmin = 'Payment Admin',
PaymentBaAdmin = 'Payment BA Admin',
PaymentEditor = 'Payment Editor',
PaymentViewer = 'Payment Viewer',
TaskManager = 'Task Manager',
Expand Down
1 change: 1 addition & 0 deletions src/core/auth/guards/roles.guard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export class RolesGuard implements CanActivate {
id: userId,
handle: userHandle,
email: request.email,
roles: userRoles,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[❗❗ security]
Ensure that userRoles is properly validated and sanitized before being assigned to the roles property. This is important to prevent potential security vulnerabilities such as injection attacks if userRoles can be influenced by external input.

};

return true;
Expand Down
11 changes: 11 additions & 0 deletions src/dto/winning.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,17 @@ export class WinningRequestDto extends SortPagination {
@IsOptional()
@IsEnum(DateFilterType)
date?: DateFilterType;

@ApiProperty({
description: 'The array of billing account ids',
example: ['1234'],
})
@IsOptional()
@IsArray()
@ArrayNotEmpty()
@IsString({ each: true })

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[💡 maintainability]
The @IsString({ each: true }) decorator is redundant here because @IsNotEmpty({ each: true }) already ensures that each element is a non-empty string. Consider removing @IsString({ each: true }) to simplify the validation logic.

@IsNotEmpty({ each: true })
billingAccounts?: string[];
}

export class WinningCreateRequestDto {
Expand Down
31 changes: 31 additions & 0 deletions src/shared/global/ba-prisma.client.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { PrismaClientOptions } from '@prisma/client/runtime/library';
import { PrismaClient as BaPrismaClient } from '@topcoder/billing-accounts-api-v6/packages/ba-prisma-client';
import { ENV_CONFIG } from 'src/config';

const clientOptions = {
transactionOptions: {
timeout: 20000,
},
log: [
{ level: 'query', emit: 'event' },
{ level: 'info', emit: 'event' },
{ level: 'warn', emit: 'event' },
{ level: 'error', emit: 'event' },
] as PrismaClientOptions['log'],
};

let baPrismaClient: BaPrismaClient;
export const getBaClient = () => {
if (!baPrismaClient) {
if (!ENV_CONFIG.BILLING_ACCOUNTS_DB_URL) {
throw new Error(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[❗❗ correctness]
The error message references CHALLENGES_DB_URL instead of BILLING_ACCOUNTS_DB_URL. This could lead to confusion when debugging. Ensure the error message accurately reflects the environment variable being checked.

'BILLING_ACCOUNTS_DB_URL must be set for challenges Prisma client',
);
}
baPrismaClient = new BaPrismaClient({
...clientOptions,
datasources: { db: { url: ENV_CONFIG.BILLING_ACCOUNTS_DB_URL } },
});
}
return baPrismaClient;
};
1 change: 1 addition & 0 deletions src/shared/global/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export * from './logger';
export * from './ba-prisma.client';
Loading