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
64 changes: 35 additions & 29 deletions src/api/admin/admin.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,14 @@ import { TopcoderMembersService } from 'src/shared/topcoder/members.service';
import { Role } from 'src/core/auth/auth.constants';
import { Roles, User } from 'src/core/auth/decorators';

import { UserInfo } from 'src/dto/user.type';

import { AdminService } from './admin.service';
import { ResponseDto, ResponseStatusType } from 'src/dto/api-response.dto';
import { WinningAuditDto, AuditPayoutDto } from './dto/audit.dto';

import { WinningRequestDto, SearchWinningResult } from 'src/dto/winning.dto';
import { WinningsRepository } from '../repository/winnings.repo';
import { WinningUpdateRequestDto } from './dto/winnings.dto';
import { AccessControlService } from 'src/shared/access-control';

@ApiTags('AdminWinnings')
@Controller('/admin')
Expand All @@ -42,20 +41,14 @@ export class AdminController {
private readonly adminService: AdminService,
private readonly winningsRepo: WinningsRepository,
private readonly tcMembersService: TopcoderMembersService,
private readonly accessControlService: AccessControlService,

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]
Consider verifying that accessControlService is used appropriately in the controller to ensure that the new role EngagementPaymentApprover is correctly integrated and that access control checks are performed where necessary.

) {}

private isBaAdmin(user?: { roles?: string[] }) {
return (user?.roles || []).some(
(r) =>
r &&
r.trim().toLowerCase() === Role.PaymentBaAdmin.trim().toLowerCase(),
);
}

@Post('/winnings/search')
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.EngagementPaymentApprover,

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]
Ensure that the addition of Role.EngagementPaymentApprover to the @Roles decorator is backed by appropriate logic in the controller methods to handle this role's permissions correctly.

Role.PaymentEditor,
Role.PaymentViewer,
)
Expand All @@ -77,13 +70,14 @@ export class AdminController {
@Body() body: WinningRequestDto,
@User() user: any,
): Promise<ResponseDto<SearchWinningResult>> {
const result = await this.winningsRepo.searchWinnings(
await this.adminService.applyBaAdminUserFilters(
const filters =
await this.accessControlService.applyFilters<WinningRequestDto>(

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 method applyFilters is being called with a generic type WinningRequestDto. Ensure that this method is designed to handle this type correctly, as incorrect handling could lead to runtime errors. Verify that the type constraints and transformations within applyFilters are appropriate for WinningRequestDto.

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 change from this.isBaAdmin(user) to user.roles suggests a shift in how roles are being handled. Ensure that user.roles contains the necessary information to apply the correct filters, and that this change does not inadvertently alter the logic or permissions.

this.isBaAdmin(user),
user.roles,
body,
),
);
);

const result = await this.winningsRepo.searchWinnings(filters);

if (result.error) {
result.status = ResponseStatusType.ERROR;
Expand All @@ -98,6 +92,7 @@ export class AdminController {
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.EngagementPaymentApprover,
Role.PaymentEditor,
Role.PaymentViewer,
)
Expand All @@ -118,16 +113,16 @@ export class AdminController {
@Header('Content-Type', 'text/csv')
@Header('Content-Disposition', 'attachment; filename="winnings.csv"')
async exportWinnings(@Body() body: WinningRequestDto, @User() user: any) {
const result = await this.winningsRepo.searchWinnings(
await this.adminService.applyBaAdminUserFilters(
const filters =
await this.accessControlService.applyFilters<WinningRequestDto>(

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 method applyFilters is being used with a generic type WinningRequestDto. Ensure that applyFilters is designed to handle this specific type correctly, as incorrect handling could lead to unexpected behavior or errors.

user.id,
this.isBaAdmin(user),
user.roles,
{
...body,
limit: 999,

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 use of limit: 999 is hardcoded. Consider making this a configurable parameter or constant to improve maintainability and flexibility.

},
),
);
);
const result = await this.winningsRepo.searchWinnings(filters);

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

@Patch('/winnings')
@Roles(Role.PaymentAdmin, Role.PaymentBaAdmin, Role.PaymentEditor)
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.EngagementPaymentApprover,
Role.PaymentEditor,
)
@ApiOperation({
summary: 'Update winnings with given parameter',
description:
Expand All @@ -194,7 +194,7 @@ export class AdminController {
})
async updateWinning(
@Body() body: WinningUpdateRequestDto,
@User() user: UserInfo,
@User() user: any,

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]
Changing the type of user from UserInfo to any can lead to potential runtime errors and makes the code less type-safe. Consider maintaining the UserInfo type or using a more specific type to ensure type safety and clarity.

): Promise<ResponseDto<string>> {
if (
!body.paymentAmount &&
Expand All @@ -210,7 +210,7 @@ export class AdminController {
const result = await this.adminService.updateWinnings(
body,
user.id,
this.isBaAdmin(user),
user.roles,

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 change from this.isBaAdmin(user) to user.roles alters the method signature of updateWinnings. Ensure that updateWinnings can handle the user.roles array correctly and that this change does not introduce any unintended behavior. Consider whether the logic that previously relied on isBaAdmin needs to be replicated or adjusted elsewhere.

);

result.status = ResponseStatusType.SUCCESS;
Expand All @@ -225,6 +225,7 @@ export class AdminController {
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.EngagementPaymentApprover,
Role.PaymentEditor,
Role.PaymentViewer,
)
Expand All @@ -246,9 +247,11 @@ export class AdminController {
@Param('winningID') winningId: string,
@User() user: any,
): Promise<ResponseDto<WinningAuditDto[]>> {
if (this.isBaAdmin(user)) {
await this.adminService.verifyBaAdminAccessToWinning(winningId, user.id);
}
await this.adminService.verifyUserAccessToWinning(

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 verifyUserAccessToWinning now takes user.roles as an additional parameter. Ensure that this method properly handles role-based access control and that the roles are validated correctly to prevent unauthorized access.

winningId,
user.id,
user.roles,
);

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

Expand All @@ -264,6 +267,7 @@ export class AdminController {
@Roles(
Role.PaymentAdmin,
Role.PaymentBaAdmin,
Role.EngagementPaymentApprover,
Role.PaymentEditor,
Role.PaymentViewer,
)
Expand All @@ -286,9 +290,11 @@ export class AdminController {
@Param('winningID') winningId: string,
@User() user: any,
): Promise<ResponseDto<AuditPayoutDto[]>> {
if (this.isBaAdmin(user)) {
await this.adminService.verifyBaAdminAccessToWinning(winningId, user.id);
}
await this.adminService.verifyUserAccessToWinning(

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 verifyUserAccessToWinning now takes user.roles as an additional parameter compared to the previous verifyBaAdminAccessToWinning. Ensure that this change in method signature is correctly handled in the adminService implementation and that it does not introduce any unintended access control issues.

winningId,
user.id,
user.roles,
);

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

Expand Down
3 changes: 2 additions & 1 deletion src/api/admin/admin.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import { AdminService } from './admin.service';
import { WinningsRepository } from '../repository/winnings.repo';
import { TopcoderModule } from 'src/shared/topcoder/topcoder.module';
import { PaymentsModule } from 'src/shared/payments';
import { AccessControlModule } from 'src/shared/access-control';

@Module({
imports: [TopcoderModule, PaymentsModule],
imports: [TopcoderModule, PaymentsModule, AccessControlModule],
controllers: [AdminController],
providers: [AdminService, WinningsRepository],
})
Expand Down
29 changes: 13 additions & 16 deletions src/api/admin/admin.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,19 @@ import {
HttpStatus,
NotFoundException,
BadRequestException,
UnauthorizedException,
} from '@nestjs/common';

import { Prisma } from '@prisma/client';
import { PrismaService } from 'src/shared/global/prisma.service';
import { PaymentsService } from 'src/shared/payments';
import { AccessControlService } from 'src/shared/access-control/access-control.service';

import { ResponseDto } from 'src/dto/api-response.dto';
import { PaymentStatus } from 'src/dto/payment.dto';
import { WinningAuditDto, AuditPayoutDto } from './dto/audit.dto';
import { WinningUpdateRequestDto } from './dto/winnings.dto';
import { Logger } from 'src/shared/global';
import { WinningRequestDto } from 'src/dto/winning.dto';
import { BillingAccountsService } from 'src/shared/topcoder/billing-accounts.service';

/**
Expand All @@ -32,21 +33,19 @@ export class AdminService {
private readonly prisma: PrismaService,
private readonly paymentsService: PaymentsService,
private readonly baService: BillingAccountsService,
private readonly accessControlService: AccessControlService,
) {}

async applyBaAdminUserFilters(
async verifyUserAccessToWinning(
winningsId: string,
userId: string,
isBaAdmin?: boolean,
filters: WinningRequestDto = {},
) {
if (!isBaAdmin) {
return filters;
roles: string[] = [],
): Promise<void> {
try {
await this.accessControlService.verifyAccess(winningsId, userId, roles);
} catch (err) {
throw new UnauthorizedException(err?.message ?? 'access denied');

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]
Catching a generic err and throwing an UnauthorizedException with err?.message could potentially expose sensitive error details. Consider logging the error internally and providing a generic message to the user.

}

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

private getWinningById(winningId: string) {
Expand Down Expand Up @@ -121,7 +120,7 @@ export class AdminService {
async updateWinnings(
body: WinningUpdateRequestDto,
userId: string,
isBaAdmin?: boolean,
roles: string[] = [],

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 parameter roles is initialized with an empty array by default. Ensure that the logic within the method properly handles cases where roles is empty, as this could affect authorization checks or role-based logic.

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

Expand All @@ -132,9 +131,7 @@ export class AdminService {
);
this.logger.log(`updateWinnings payload: ${JSON.stringify(body)}`);

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

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 function verifyUserAccessToWinning is called with roles as an argument, but the diff does not show the definition or implementation of this function. Ensure that roles is correctly handled within verifyUserAccessToWinning to avoid potential security issues related to access control.


try {
const payments = await this.getPaymentsByWinningsId(
Expand Down
1 change: 1 addition & 0 deletions src/core/auth/auth.constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export enum Role {
Administrator = 'Administrator',
PaymentAdmin = 'Payment Admin',
PaymentBaAdmin = 'Payment BA Admin',
EngagementPaymentApprover = 'Engagement Payment Approver',
PaymentEditor = 'Payment Editor',
PaymentViewer = 'Payment Viewer',
TaskManager = 'Task Manager',
Expand Down
31 changes: 31 additions & 0 deletions src/shared/access-control/access-control.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { Module } from '@nestjs/common';
import { AccessControlService } from 'src/shared/access-control/access-control.service';
import { PaymentBaProvider } from 'src/shared/access-control/payment-ba.provider';
import { EngagementPaymentApproverProvider } from 'src/shared/access-control/engagement-pa.provider';
import { Injectable } from '@nestjs/common';
import { TopcoderModule } from '../topcoder/topcoder.module';

@Injectable()

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 @Injectable() decorator is not necessary for the AccessControlRegistrar class since it is not being injected anywhere. Consider removing it to avoid confusion.

class AccessControlRegistrar {
constructor(
accessControlService: AccessControlService,
paymentBaProvider: PaymentBaProvider,
engagementPaymentApproverProvider: EngagementPaymentApproverProvider,
) {
accessControlService.register(paymentBaProvider);
accessControlService.register(engagementPaymentApproverProvider);
}
}

@Module({
imports: [TopcoderModule],
controllers: [],
providers: [
AccessControlService,
PaymentBaProvider,
EngagementPaymentApproverProvider,
AccessControlRegistrar,
],
exports: [AccessControlService],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ design]
Consider exporting PaymentBaProvider and EngagementPaymentApproverProvider if they are intended to be used outside this module. This will improve the module's reusability.

})
export class AccessControlModule {}
35 changes: 35 additions & 0 deletions src/shared/access-control/access-control.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { Injectable } from '@nestjs/common';
import { RoleAccessProvider } from './role-access.interface';

@Injectable()
export class AccessControlService {
private providers = new Map<string, RoleAccessProvider<any>>();

register(provider: RoleAccessProvider) {
this.providers.set(provider.roleName.trim().toLowerCase(), provider);
}

async applyFilters<T>(
userId: string,
roles: string[] = [],
req: any,

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]
Using any for the req parameter can lead to runtime errors and makes the code harder to understand. Consider defining a specific type or interface for req to improve type safety and maintainability.

): Promise<T> {
let out = { ...req };

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 spread operator is used to create a shallow copy of req. If req contains nested objects, changes made by applyFilter could affect the original req. Consider using a deep copy if this is not the intended behavior.

for (const r of roles || []) {
const p = this.providers.get(r?.trim().toLowerCase());
if (p?.applyFilter) {
out = await p.applyFilter(userId, out);
}
}
return out as T;
}

async verifyAccess(resourceId: string, userId: string, roles: string[] = []) {

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 verifyAccess method does not return any value or throw an error if access is denied. Consider implementing a mechanism to handle access denial, such as throwing an exception or returning a boolean.

for (const r of roles || []) {
const p = this.providers.get(r?.trim().toLowerCase());
if (p?.verifyAccessToResource) {
await p.verifyAccessToResource(resourceId, userId);
}
}
}
}
37 changes: 37 additions & 0 deletions src/shared/access-control/engagement-pa.provider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { Injectable } from '@nestjs/common';
import { RoleAccessProvider } from './role-access.interface';
import { PrismaService } from 'src/shared/global/prisma.service';
import { Role } from 'src/core/auth/auth.constants';
import { winnings_category } from '@prisma/client';

@Injectable()
export class EngagementPaymentApproverProvider implements RoleAccessProvider {
roleName = Role.EngagementPaymentApprover;

constructor(private readonly prisma: PrismaService) {}

// disable rule: prefer this format instead of returning resolved promise (required by interface)
// eslint-disable-next-line @typescript-eslint/require-await
async applyFilter<T>(userId: string, req: any): Promise<T> {
return { ...req, category: winnings_category.ENGAGEMENT_PAYMENT } as T;
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
async verifyAccessToResource(winningsId: string | string[], _userId: string) {

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 parameter _userId is unused in the verifyAccessToResource method. Consider removing it if it's not needed, or use it if it serves a purpose in the logic.

const winningsIds = ([] as string[]).concat(winningsId);

const winnings = await this.prisma.winnings.findMany({
where: { winning_id: { in: winningsIds } },
select: { category: true },
});

const unauthorized = winnings.filter(
(w) => w.category !== winnings_category.ENGAGEMENT_PAYMENT,
);
if (unauthorized.length > 0) {
throw new Error(
`${Role.EngagementPaymentApprover} user is trying to access winning with category='${unauthorized.map(w => w.category).join(', ')}'`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ design]
Throwing a generic Error may not provide enough context for error handling. Consider using a custom error class or a more specific error type to improve error handling and debugging.

);
}
}
}
4 changes: 4 additions & 0 deletions src/shared/access-control/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export * from './access-control.module';
export * from './access-control.service';
export * from './payment-ba.provider';
export * from './role-access.interface';
Loading
Loading