Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates authorization behavior and API documentation for the Billing Accounts service to support additional JWT roles (Talent Manager / Project Manager variants), including restricting Project Managers’ billing-account reads to only accounts granted to their own user id.
Changes:
- Added new JWT role constants and expanded controller role allowlists for billing-account endpoints.
- Enforced Project Manager read restrictions in
BillingAccountsService.list()andBillingAccountsService.get()based onreq.authUser.userId. - Improved role matching behavior (case-insensitive) in
RolesGuardand refreshed Swagger/README docs; plus minor formatting and startup error-handling tweaks.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.ts | Swagger bearer auth description update; bootstrap now logs startup failures and exits non-zero. |
| src/health/health.service.ts | Removes redundant try/catch and normalizes return object formatting. |
| src/health/health.controller.ts | Formatting-only changes to Swagger operation doc and method signature spacing. |
| src/common/swagger/swagger-auth.util.ts | Formatting-only change to OperationDocOptions type declaration. |
| src/common/prisma.service.ts | Adjusts shutdown hook callback to not await app.close() (API unchanged; helper still unused). |
| src/common/members-lookup.service.ts | Formatting-only changes and safer BigInt parsing readability. |
| src/clients/dto/create-client.dto.ts | Import formatting only. |
| src/clients/clients.controller.ts | Import + Swagger decorator formatting only. |
| src/billing-accounts/dto/query-billing-accounts.dto.ts | Import formatting only. |
| src/billing-accounts/dto/create-billing-account.dto.ts | Import formatting and Swagger decorator formatting only. |
| src/billing-accounts/billing-accounts.service.ts | Adds role/user normalization helpers and enforces restricted Project Manager reads in list/get. |
| src/billing-accounts/billing-accounts.controller.ts | Expands role access for read/manage endpoints; passes req.authUser into service for PM restriction enforcement; updates Swagger docs. |
| src/auth/guards/scopes.guard.ts | Formatting-only; role fallback behavior remains case-sensitive. |
| src/auth/guards/roles.guard.ts | Makes role (and fallback scope) checks case-insensitive via normalization. |
| src/auth/constants.ts | Adds role constants for Project Manager and Talent Manager variants. |
| src/auth/auth.middleware.ts | Switches CJS require to import * as for tc-core-library-js. |
| README.md | Documents new role permissions and Project Manager read restrictions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Billing-account management endpoints accept `administrator`, `Talent Manager`, | ||
| and `Topcoder Talent Manager` JWT roles; read-only billing-account lookups | ||
| also continue to allow `copilot`, `Project Manager`, and `Topcoder Project Manager`. |
There was a problem hiding this comment.
README claims billing-account management endpoints accept administrator, Talent Manager, and Topcoder Talent Manager JWT roles, but PATCH /billing-accounts/:billingAccountId/lock-amount and PATCH /billing-accounts/:billingAccountId/consume-amount are still restricted to @Roles(ADMIN_ROLE) (and their Swagger docs list only administrator). Either adjust the README wording to exclude these endpoints, or broaden those endpoints’ role checks/docs to match the documented policy.
| - Billing-account management endpoints accept `administrator`, `Talent Manager`, | |
| and `Topcoder Talent Manager` JWT roles; read-only billing-account lookups | |
| also continue to allow `copilot`, `Project Manager`, and `Topcoder Project Manager`. | |
| - Most billing-account management endpoints accept `administrator`, `Talent Manager`, | |
| and `Topcoder Talent Manager` JWT roles. The budget mutation endpoints | |
| `PATCH /billing-accounts/:billingAccountId/lock-amount` and | |
| `PATCH /billing-accounts/:billingAccountId/consume-amount` remain | |
| `administrator`-only. Read-only billing-account lookups also continue to | |
| allow `copilot`, `Project Manager`, and `Topcoder Project Manager`. |
| description: | ||
| "Reserve an amount on a billing account for a specific challenge.", | ||
| jwtRoles: [ADMIN_ROLE], | ||
| m2mScopes: [SCOPES.UPDATE_BA, SCOPES.ALL_BA], |
There was a problem hiding this comment.
These endpoints still document/require only administrator (jwtRoles: [ADMIN_ROLE] and @Roles(ADMIN_ROLE)), which seems inconsistent with the PR goal/README update that Talent Managers can use billing-account management endpoints. If Talent Manager / Topcoder Talent Manager should also be able to lock/consume funds, update both the @Roles(...) decorator and the Swagger jwtRoles here (and on consume-amount) to match the new management role set.
| summary: "Consume reserved funds", | ||
| description: "Consume a previously locked amount for a challenge and record the transaction.", | ||
| description: | ||
| "Consume a previously locked amount for a challenge and record the transaction.", | ||
| jwtRoles: [ADMIN_ROLE], | ||
| m2mScopes: [SCOPES.UPDATE_BA, SCOPES.ALL_BA], | ||
| }), |
There was a problem hiding this comment.
consume-amount is still restricted/documented as administrator only (jwtRoles: [ADMIN_ROLE] and @Roles(ADMIN_ROLE)), which conflicts with the PR’s stated permission expansion for Talent Manager roles. If Talent Managers are intended to manage billing accounts (including budget operations), align this endpoint’s role guard + Swagger jwtRoles with BILLING_ACCOUNT_MANAGE_ROLES.
| const fallbackRoles = this.reflector.getAllAndOverride<string[]>( | ||
| ROLES_KEY, | ||
| [context.getHandler(), context.getClass()], | ||
| ); | ||
|
|
There was a problem hiding this comment.
The scopes fallback-to-roles path in ScopesGuard still performs a case-sensitive role comparison (later in this method) even though RolesGuard now normalizes roles/scopes to be case-insensitive and the README documents case-insensitive role checks. This can cause JWT requests without scopes (common) to pass RolesGuard but fail ScopesGuard if token roles are cased differently (e.g. Administrator). Consider normalizing both fallbackRoles and the extracted roles (trim + lowercase) before comparing, mirroring RolesGuard.
Permission updates for Talent Manager / Project Manager