diff --git a/.github/workflows/code_reviewer.yml b/.github/workflows/code_reviewer.yml deleted file mode 100644 index 9b6a6ce..0000000 --- a/.github/workflows/code_reviewer.yml +++ /dev/null @@ -1,22 +0,0 @@ -name: AI PR Reviewer - -on: - pull_request: - types: - - opened - - synchronize -permissions: - pull-requests: write -jobs: - tc-ai-pr-review: - runs-on: ubuntu-latest - steps: - - name: Checkout Repo - uses: actions/checkout@v3 - - - name: TC AI PR Reviewer - uses: topcoder-platform/tc-ai-pr-reviewer@master - with: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # The GITHUB_TOKEN is there by default so you just need to keep it like it is and not necessarily need to add it as secret as it will throw an error. [More Details](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret) - LAB45_API_KEY: ${{ secrets.LAB45_API_KEY }} - exclude: '**/*.json, **/*.md, **/*.jpg, **/*.png, **/*.jpeg, **/*.bmp, **/*.webp' # Optional: exclude patterns separated by commas \ No newline at end of file diff --git a/src/api/project/dto/update-project.dto.spec.ts b/src/api/project/dto/update-project.dto.spec.ts new file mode 100644 index 0000000..9132e10 --- /dev/null +++ b/src/api/project/dto/update-project.dto.spec.ts @@ -0,0 +1,26 @@ +import { plainToInstance } from 'class-transformer'; +import { validate } from 'class-validator'; +import { UpdateProjectDto } from './update-project.dto'; + +describe('UpdateProjectDto', () => { + it.each([null, ''])( + 'preserves %p billingAccountId as an explicit clear request', + async (billingAccountId) => { + const dto = plainToInstance(UpdateProjectDto, { + billingAccountId, + }); + + expect(dto.billingAccountId).toBeNull(); + await expect(validate(dto)).resolves.toHaveLength(0); + }, + ); + + it('parses numeric billingAccountId updates', async () => { + const dto = plainToInstance(UpdateProjectDto, { + billingAccountId: '80001063', + }); + + expect(dto.billingAccountId).toBe(80001063); + await expect(validate(dto)).resolves.toHaveLength(0); + }); +}); diff --git a/src/api/project/dto/update-project.dto.ts b/src/api/project/dto/update-project.dto.ts index 80f6b13..b4aea56 100644 --- a/src/api/project/dto/update-project.dto.ts +++ b/src/api/project/dto/update-project.dto.ts @@ -1,8 +1,33 @@ -import { ApiHideProperty } from '@nestjs/swagger'; +import { ApiHideProperty, ApiPropertyOptional } from '@nestjs/swagger'; import { Transform } from 'class-transformer'; -import { PartialType } from '@nestjs/mapped-types'; +import { IsNumber, IsOptional } from 'class-validator'; +import { OmitType, PartialType } from '@nestjs/mapped-types'; import { CreateProjectDto } from './create-project.dto'; +/** + * Parses optional project billing-account update input. + * + * @param value Raw `billingAccountId` value from request payload. + * @returns Parsed integer, `null` when clearing, or `undefined` when omitted. + */ +function parseOptionalNullableInteger(value: unknown): number | null | undefined { + if (typeof value === 'undefined') { + return undefined; + } + + if (value === null || value === '') { + return null; + } + + const parsed = Number(value); + + if (Number.isNaN(parsed)) { + return undefined; + } + + return Math.trunc(parsed); +} + /** * Resolves whether the patch payload explicitly requests clearing * `billingAccountId`. @@ -17,9 +42,21 @@ function parseClearBillingAccountFlag(value: unknown): boolean { /** * Request DTO for `PATCH /projects/:projectId`. * - * Reuses `CreateProjectDto` and makes all fields optional via `PartialType`. + * Reuses `CreateProjectDto`, makes all fields optional via `PartialType`, and + * allows `billingAccountId` to be explicitly cleared with `null` or `''`. */ -export class UpdateProjectDto extends PartialType(CreateProjectDto) { +export class UpdateProjectDto extends PartialType( + OmitType(CreateProjectDto, ['billingAccountId'] as const), +) { + @ApiPropertyOptional({ + description: 'Project billing account id. Send null or empty string to clear.', + nullable: true, + }) + @IsOptional() + @Transform(({ value }) => parseOptionalNullableInteger(value)) + @IsNumber() + billingAccountId?: number | null; + @ApiHideProperty() @Transform(({ obj }) => parseClearBillingAccountFlag(obj?.billingAccountId)) clearBillingAccountId?: boolean; diff --git a/src/api/project/project.controller.ts b/src/api/project/project.controller.ts index 8ea95fc..67bc56b 100644 --- a/src/api/project/project.controller.ts +++ b/src/api/project/project.controller.ts @@ -273,7 +273,7 @@ export class ProjectController { * @throws UnauthorizedException When the caller is unauthenticated. * @throws ForbiddenException When the caller lacks required permissions. * @throws NotFoundException When project or billing account is missing. - * @security The service strips `markup` for non-machine callers before + * @security The service strips `markup` for copilot-only callers before * returning the response payload. */ @Get(':projectId/billingAccount') diff --git a/src/api/project/project.service.spec.ts b/src/api/project/project.service.spec.ts index c3be179..9df8331 100644 --- a/src/api/project/project.service.spec.ts +++ b/src/api/project/project.service.spec.ts @@ -604,7 +604,7 @@ describe('ProjectService', () => { ).toHaveBeenCalledWith('1001', '123'); }); - it('returns project billing account and strips markup for user tokens', async () => { + it('returns project billing account and strips markup for copilot-only user tokens', async () => { prismaMock.project.findFirst.mockResolvedValue({ id: BigInt(1001), billingAccountId: BigInt(12), @@ -617,6 +617,7 @@ describe('ProjectService', () => { const result = await service.getProjectBillingAccount('1001', { userId: '123', + roles: [UserRole.TC_COPILOT], isMachine: false, }); @@ -629,6 +630,30 @@ describe('ProjectService', () => { ).toHaveBeenCalledWith('12'); }); + it('returns project billing account markup for Project Manager tokens', async () => { + prismaMock.project.findFirst.mockResolvedValue({ + id: BigInt(1001), + billingAccountId: BigInt(12), + }); + billingAccountServiceMock.getDefaultBillingAccount.mockResolvedValue({ + tcBillingAccountId: '12', + markup: 50, + active: true, + }); + + const result = await service.getProjectBillingAccount('1001', { + userId: '123', + roles: [UserRole.PROJECT_MANAGER], + isMachine: false, + }); + + expect(result).toEqual({ + tcBillingAccountId: '12', + markup: 50, + active: true, + }); + }); + it('returns project billing account markup for m2m tokens', async () => { prismaMock.project.findFirst.mockResolvedValue({ id: BigInt(1001), @@ -1621,8 +1646,8 @@ describe('ProjectService', () => { await service.updateProject( '1001', { - clearBillingAccountId: true, - } as any, + billingAccountId: null, + }, { userId: '100', isMachine: false, diff --git a/src/api/project/project.service.ts b/src/api/project/project.service.ts index e51db00..9da39d1 100644 --- a/src/api/project/project.service.ts +++ b/src/api/project/project.service.ts @@ -49,6 +49,24 @@ import { ProjectWithRelationsDto } from './dto/project-response.dto'; import { UpgradeProjectDto } from './dto/upgrade-project.dto'; import { UpdateProjectDto } from './dto/update-project.dto'; +const BILLING_MARKUP_COPILOT_ROLES = [ + UserRole.COPILOT, + UserRole.TC_COPILOT, +]; + +const BILLING_MARKUP_VISIBLE_HUMAN_ROLES = [ + ...ADMIN_ROLES, + UserRole.MANAGER, + UserRole.TOPCODER_MANAGER, + UserRole.TOPCODER_ACCOUNT_MANAGER, + UserRole.COPILOT_MANAGER, + UserRole.PROJECT_MANAGER, + UserRole.TASK_MANAGER, + UserRole.TOPCODER_TASK_MANAGER, + UserRole.TALENT_MANAGER, + UserRole.TOPCODER_TALENT_MANAGER, +]; + interface PaginatedProjectResponse { data: ProjectWithRelationsDto[]; page: number; @@ -648,8 +666,10 @@ export class ProjectService { throw new ForbiddenException('Insufficient permissions'); } + const shouldClearBillingAccountId = + dto.clearBillingAccountId === true || dto.billingAccountId === null; const requestedBillingAccountId = - dto.clearBillingAccountId === true + shouldClearBillingAccountId ? null : typeof dto.billingAccountId === 'number' ? String(dto.billingAccountId) @@ -708,7 +728,7 @@ export class ProjectService { cancelReason: typeof dto.cancelReason === 'string' ? dto.cancelReason : undefined, billingAccountId: - dto.clearBillingAccountId === true + shouldClearBillingAccountId ? null : typeof dto.billingAccountId === 'number' ? BigInt(dto.billingAccountId) @@ -1038,8 +1058,9 @@ export class ProjectService { * @param user Authenticated caller context. * @returns Default billing account details. * @throws NotFoundException When project or billing account is missing. - * @security Removes `markup` for non-machine callers to avoid exposing - * markup details to interactive users. + * @security Removes `markup` for copilot-only callers while preserving the + * existing response shape for M2M, Project Manager, Talent Manager, and + * administrator callers. */ async getProjectBillingAccount( projectId: string, @@ -1083,7 +1104,7 @@ export class ProjectService { tcBillingAccountId: projectBillingAccountId, }; - if (this.isMachinePrincipal(user)) { + if (!this.shouldHideBillingAccountMarkupForCopilot(user)) { return billingAccount; } @@ -2175,6 +2196,37 @@ export class ProjectService { return false; } + /** + * Determines whether project billing-account markup must be hidden. + * + * Copilot-only human callers should not receive raw billing-account markup. + * Manager, Talent Manager, and administrator roles retain the existing + * response shape even when the same token also carries a copilot role. + * + * @param user Authenticated caller context. + * @returns `true` when billing-account markup should be removed. + */ + private shouldHideBillingAccountMarkupForCopilot(user: JwtUser): boolean { + if (this.isMachinePrincipal(user)) { + return false; + } + + const roles = user.roles || []; + const hasCopilotRole = this.permissionService.hasIntersection( + roles, + BILLING_MARKUP_COPILOT_ROLES, + ); + + if (!hasCopilotRole) { + return false; + } + + return !this.permissionService.hasIntersection( + roles, + BILLING_MARKUP_VISIBLE_HUMAN_ROLES, + ); + } + /** * Safely extracts template phase objects from JSON payload. * diff --git a/src/shared/constants/permissions.constants.ts b/src/shared/constants/permissions.constants.ts index 0dfff45..3beb9cd 100644 --- a/src/shared/constants/permissions.constants.ts +++ b/src/shared/constants/permissions.constants.ts @@ -265,11 +265,7 @@ export const PERMISSION = { 'There are additional limitations on editing some parts of the project.', }, topcoderRoles: TOPCODER_ROLES_MANAGERS_AND_ADMINS, - projectRoles: [ - ...PROJECT_ROLES_MANAGEMENT, - PROJECT_MEMBER_ROLE.COPILOT, - PROJECT_MEMBER_ROLE.CUSTOMER, - ], + projectRoles: PROJECT_ROLES_MANAGEMENT, scopes: SCOPES_PROJECTS_WRITE, }, diff --git a/src/shared/services/permission.service.spec.ts b/src/shared/services/permission.service.spec.ts index 747ffcb..ccd19ee 100644 --- a/src/shared/services/permission.service.spec.ts +++ b/src/shared/services/permission.service.spec.ts @@ -2,6 +2,7 @@ import { ProjectMemberRole } from '../enums/projectMemberRole.enum'; import { Scope } from '../enums/scopes.enum'; import { UserRole } from '../enums/userRole.enum'; import { Permission } from '../constants/permissions'; +import { PERMISSION } from '../constants/permissions.constants'; import { PermissionService } from './permission.service'; describe('PermissionService', () => { @@ -722,6 +723,31 @@ describe('PermissionService', () => { expect(allowed).toBe(true); }); + it('blocks project copilots from editing project details', () => { + const user = { + userId: '3001', + roles: [UserRole.TC_COPILOT], + isMachine: false, + }; + const projectMembers = [ + { + userId: '3001', + role: ProjectMemberRole.COPILOT, + }, + ]; + + expect( + service.hasNamedPermission( + Permission.EDIT_PROJECT, + user, + projectMembers, + ), + ).toBe(false); + expect( + service.matchPermissionRule(PERMISSION.UPDATE_PROJECT, user, projectMembers), + ).toBe(false); + }); + it('allows deleting project for machine token with project write scope', () => { const allowed = service.hasNamedPermission(Permission.DELETE_PROJECT, { scopes: [Scope.PROJECTS_ALL], diff --git a/src/shared/services/permission.service.ts b/src/shared/services/permission.service.ts index b2df6c1..a10308e 100644 --- a/src/shared/services/permission.service.ts +++ b/src/shared/services/permission.service.ts @@ -271,7 +271,6 @@ export class PermissionService { return ( isAdmin || isManagementMember || - this.isCopilot(member?.role) || this.hasProjectUpdateTopcoderRole(user) || hasMachineProjectWriteScope ); diff --git a/src/shared/utils/permission-docs.utils.ts b/src/shared/utils/permission-docs.utils.ts index 76202d0..b9ba79e 100644 --- a/src/shared/utils/permission-docs.utils.ts +++ b/src/shared/utils/permission-docs.utils.ts @@ -255,7 +255,7 @@ function getNamedPermissionDocumentation( case NamedPermission.EDIT_PROJECT: return createSummary({ userRoles: PROJECT_UPDATE_TOPCODER_ROLES, - projectRoles: PROJECT_MEMBER_MANAGEMENT_AND_COPILOT_ROLES, + projectRoles: PROJECT_MEMBER_MANAGEMENT_ROLES, scopes: PROJECT_WRITE_SCOPES, });