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
22 changes: 0 additions & 22 deletions .github/workflows/code_reviewer.yml

This file was deleted.

26 changes: 26 additions & 0 deletions src/api/project/dto/update-project.dto.spec.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
45 changes: 41 additions & 4 deletions src/api/project/dto/update-project.dto.ts
Original file line number Diff line number Diff line change
@@ -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`.
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/api/project/project.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
31 changes: 28 additions & 3 deletions src/api/project/project.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -617,6 +617,7 @@ describe('ProjectService', () => {

const result = await service.getProjectBillingAccount('1001', {
userId: '123',
roles: [UserRole.TC_COPILOT],
isMachine: false,
});

Expand All @@ -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),
Expand Down Expand Up @@ -1621,8 +1646,8 @@ describe('ProjectService', () => {
await service.updateProject(
'1001',
{
clearBillingAccountId: true,
} as any,
billingAccountId: null,
},
{
userId: '100',
isMachine: false,
Expand Down
62 changes: 57 additions & 5 deletions src/api/project/project.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1083,7 +1104,7 @@ export class ProjectService {
tcBillingAccountId: projectBillingAccountId,
};

if (this.isMachinePrincipal(user)) {
if (!this.shouldHideBillingAccountMarkupForCopilot(user)) {
return billingAccount;
}

Expand Down Expand Up @@ -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.
*
Expand Down
6 changes: 1 addition & 5 deletions src/shared/constants/permissions.constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},

Expand Down
26 changes: 26 additions & 0 deletions src/shared/services/permission.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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],
Expand Down
1 change: 0 additions & 1 deletion src/shared/services/permission.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,6 @@ export class PermissionService {
return (
isAdmin ||
isManagementMember ||
this.isCopilot(member?.role) ||
this.hasProjectUpdateTopcoderRole(user) ||
hasMachineProjectWriteScope
);
Expand Down
2 changes: 1 addition & 1 deletion src/shared/utils/permission-docs.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});

Expand Down
Loading