[PROD RELEASE] - EngagementPaymentApprover role implementation by kkartunov · Pull Request #143 · topcoder-platform/tc-finance-api

kkartunov · 2026-02-10T09:59:09Z

https://topcoder.atlassian.net/browse/PM-3697

…ent-payment-approver-role PM-3697 handle engagement payment approver role

github-actions · 2026-02-10T09:59:27Z

    private readonly adminService: AdminService,
    private readonly winningsRepo: WinningsRepository,
    private readonly tcMembersService: TopcoderMembersService,
+    private readonly accessControlService: AccessControlService,


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

github-actions · 2026-02-10T09:59:27Z

  @Roles(
    Role.PaymentAdmin,
    Role.PaymentBaAdmin,
+    Role.EngagementPaymentApprover,


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

github-actions · 2026-02-10T09:59:31Z

-    const result = await this.winningsRepo.searchWinnings(
-      await this.adminService.applyBaAdminUserFilters(
+    const filters =
+      await this.accessControlService.applyFilters<WinningRequestDto>(


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

github-actions · 2026-02-10T09:59:31Z

-      await this.adminService.applyBaAdminUserFilters(
+    const filters =
+      await this.accessControlService.applyFilters<WinningRequestDto>(
        user.id,


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

github-actions · 2026-02-10T09:59:35Z

-    const result = await this.winningsRepo.searchWinnings(
-      await this.adminService.applyBaAdminUserFilters(
+    const filters =
+      await this.accessControlService.applyFilters<WinningRequestDto>(


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

github-actions · 2026-02-10T09:59:35Z

+        user.roles,
        {
          ...body,
          limit: 999,


[⚠️ maintainability]
The use of limit: 999 is hardcoded. Consider making this a configurable parameter or constant to improve maintainability and flexibility.

github-actions · 2026-02-10T09:59:38Z

  async updateWinning(
    @Body() body: WinningUpdateRequestDto,
-    @User() user: UserInfo,
+    @User() user: any,


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

github-actions · 2026-02-10T09:59:41Z

      body,
      user.id,
-      this.isBaAdmin(user),
+      user.roles,


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

github-actions · 2026-02-10T09:59:45Z

-    if (this.isBaAdmin(user)) {
-      await this.adminService.verifyBaAdminAccessToWinning(winningId, user.id);
-    }
+    await this.adminService.verifyUserAccessToWinning(


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

github-actions · 2026-02-10T09:59:49Z

-    if (this.isBaAdmin(user)) {
-      await this.adminService.verifyBaAdminAccessToWinning(winningId, user.id);
-    }
+    await this.adminService.verifyUserAccessToWinning(


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

github-actions · 2026-02-10T09:59:54Z

+    try {
+      await this.accessControlService.verifyAccess(winningsId, userId, roles);
+    } catch (err) {
+      throw new UnauthorizedException(err?.message ?? 'access denied');


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

github-actions · 2026-02-10T09:59:57Z

    body: WinningUpdateRequestDto,
    userId: string,
-    isBaAdmin?: boolean,
+    roles: string[] = [],


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

github-actions · 2026-02-10T09:59:59Z

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


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

github-actions · 2026-02-10T10:00:03Z

+import { Injectable } from '@nestjs/common';
+import { TopcoderModule } from '../topcoder/topcoder.module';
+
+@Injectable()


[💡 maintainability]
The @Injectable() decorator is not necessary for the AccessControlRegistrar class since it is not being injected anywhere. Consider removing it to avoid confusion.

github-actions · 2026-02-10T10:00:03Z

+    EngagementPaymentApproverProvider,
+    AccessControlRegistrar,
+  ],
+  exports: [AccessControlService],


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

github-actions · 2026-02-10T10:00:08Z

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


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

github-actions · 2026-02-10T10:00:08Z

+    roles: string[] = [],
+    req: any,
+  ): Promise<T> {
+    let out = { ...req };


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

github-actions · 2026-02-10T10:00:08Z

+    return out as T;
+  }
+
+  async verifyAccess(resourceId: string, userId: string, roles: string[] = []) {


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

github-actions · 2026-02-10T10:00:11Z

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


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

github-actions · 2026-02-10T10:00:11Z

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


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

github-actions · 2026-02-10T10:00:18Z

+    private readonly prisma: PrismaService,
+  ) {}
+
+  async applyFilter<T>(userId: string, req: any): Promise<T> {


[⚠️ maintainability]
The req parameter is typed as any, which can lead to runtime errors and makes the function harder to understand. Consider defining a specific type or interface for req to improve type safety and maintainability.

github-actions · 2026-02-10T10:00:18Z

+    return { ...req, billingAccounts: bas } as T;
+  }
+
+  async verifyAccessToResource(winningsId: string, userId: string) {


[💡 readability]
The verifyAccessToResource method does not return a value when access is verified, which might be confusing. Consider explicitly returning a boolean or a specific result to indicate the outcome of the access verification.

github-actions · 2026-02-10T10:00:18Z

+
+    const unauthorized = paymentBAs.some((ba) => !allowedBAs.includes(`${ba}`));
+    if (unauthorized) {
+      throw new Error(


[⚠️ design]
Throwing a generic Error with a string message can make error handling less precise. Consider creating a custom error class to provide more context and improve error handling.

github-actions · 2026-02-10T10:00:22Z

@@ -0,0 +1,5 @@
+export interface RoleAccessProvider<Req = any> {


[⚠️ correctness]
Using any as a default type for Req can lead to potential type safety issues. Consider using a more specific type or a generic constraint to ensure better type safety.

github-actions · 2026-02-10T10:00:22Z

@@ -0,0 +1,5 @@
+export interface RoleAccessProvider<Req = any> {
+  roleName: string;
+  applyFilter?(userId: string, req: Req): Promise<Req>;


[⚠️ correctness]
The method applyFilter is optional, but its return type is a Promise<Req>. Ensure that the implementation handles cases where this method might not be defined, to avoid runtime errors.

github-actions · 2026-02-10T10:00:22Z

+export interface RoleAccessProvider<Req = any> {
+  roleName: string;
+  applyFilter?(userId: string, req: Req): Promise<Req>;
+  verifyAccessToResource?(resourceId: string, userId: string): Promise<void>;


[⚠️ correctness]
The method verifyAccessToResource is optional, but its return type is a Promise<void>. Ensure that the implementation handles cases where this method might not be defined, to avoid runtime errors.

vas3a and others added 4 commits February 6, 2026 15:58

PM-3697 - handle EngagementPaymentApprover role

1be8d60

Cleanup

39d0cd4

fix import

da56556

Merge pull request #142 from topcoder-platform/PM-3697_handle-engagem…

409b628

…ent-payment-approver-role PM-3697 handle engagement payment approver role

github-actions Bot reviewed Feb 10, 2026

View reviewed changes

kkartunov merged commit cfc4e01 into master Feb 10, 2026
10 checks passed

		@@ -0,0 +1,5 @@
		export interface RoleAccessProvider<Req = any> {

Uh oh!

Conversation

kkartunov commented Feb 10, 2026

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Feb 10, 2026