From e749f865ad32887768bfdbe272710badae706bd4 Mon Sep 17 00:00:00 2001 From: Justin Gasper Date: Sat, 4 Apr 2026 03:00:08 +1100 Subject: [PATCH] PM-3764: allow legacy topcoder_manager through route guards What was broken The previous PM-3764 fix restored legacy read parity inside PermissionService, but some QA users could still be denied before those permission checks ran. Tokens carrying the legacy `topcoder_manager` role were excluded from controller-level `@Roles(...Object.values(UserRole))` allowlists, so the read-parity logic never executed for them. Root cause (if identifiable) PermissionService and the Swagger permission documentation still knew about the legacy `topcoder_manager` role, but the shared UserRole enum did not. Route-level role gates derive their allowlists from `Object.values(UserRole)`, which left the coarse auth layer out of sync with the PM-3764 compatibility logic. What was changed Added `UserRole.TOPCODER_MANAGER` for the legacy JWT role and updated the permission/documentation helpers to use the enum-backed value. Kept the existing PM-3764 read-parity behavior intact while extending regression coverage for legacy `topcoder_manager` access through the route guard and read-permission paths. Documented that `topcoder_manager` is accepted by both route guards and PermissionService. Any added/updated tests Added a TokenRolesGuard regression test covering `topcoder_manager` against `Object.values(UserRole)` route allowlists. Expanded PermissionService regression coverage for `topcoder_manager` project, member, invite, and attachment read access. Validated with `pnpm lint`, targeted auth regression tests, and `pnpm build`. The full `pnpm test` suite still has the existing unrelated metadata event-bus failures on the current `dev` baseline. --- docs/PERMISSIONS.md | 1 + src/shared/enums/userRole.enum.ts | 4 ++ src/shared/guards/tokenRoles.guard.spec.ts | 44 ++++++++++++ .../services/permission.service.spec.ts | 70 +++++++++++-------- src/shared/services/permission.service.ts | 11 ++- src/shared/utils/permission-docs.utils.ts | 4 +- 6 files changed, 95 insertions(+), 39 deletions(-) diff --git a/docs/PERMISSIONS.md b/docs/PERMISSIONS.md index 8150d65..2580a8c 100644 --- a/docs/PERMISSIONS.md +++ b/docs/PERMISSIONS.md @@ -39,6 +39,7 @@ Swagger auth notes: - `Project Manager`, `Task Manager`, `Topcoder Task Manager`, `Talent Manager`, and `Topcoder Talent Manager` retain the legacy v5 ability to view projects without being explicit project members. - Manager-tier platform roles also retain legacy read access to project members, invites, and attachments on those projects. +- The legacy JWT role `topcoder_manager` is accepted end-to-end by both route-level role guards and `PermissionService`, so those users are not blocked before the PM-3764 read-parity checks run. ## Billing Account Editing diff --git a/src/shared/enums/userRole.enum.ts b/src/shared/enums/userRole.enum.ts index 1d9b011..792ee82 100644 --- a/src/shared/enums/userRole.enum.ts +++ b/src/shared/enums/userRole.enum.ts @@ -10,6 +10,10 @@ export enum UserRole { * Connect manager role. */ MANAGER = 'Connect Manager', + /** + * Legacy manager role still emitted by some JWTs. + */ + TOPCODER_MANAGER = 'topcoder_manager', /** * Connect account manager role. */ diff --git a/src/shared/guards/tokenRoles.guard.spec.ts b/src/shared/guards/tokenRoles.guard.spec.ts index 015a4e6..5a53d0a 100644 --- a/src/shared/guards/tokenRoles.guard.spec.ts +++ b/src/shared/guards/tokenRoles.guard.spec.ts @@ -6,6 +6,7 @@ import { import { Reflector } from '@nestjs/core'; import { IS_PUBLIC_KEY } from '../decorators/public.decorator'; import { SCOPES_KEY } from '../decorators/scopes.decorator'; +import { UserRole } from '../enums/userRole.enum'; import { JwtService } from '../modules/global/jwt.service'; import { M2MService } from '../modules/global/m2m.service'; import { ADMIN_ONLY_KEY } from './auth-metadata.constants'; @@ -219,6 +220,49 @@ describe('TokenRolesGuard', () => { expect(request.user).toEqual(user); }); + it('allows legacy topcoder_manager human tokens on routes that accept all known user roles', async () => { + const request: Record = { + headers: { + authorization: 'Bearer human-token', + }, + }; + + reflectorMock.getAllAndOverride.mockImplementation((key: string) => { + if (key === IS_PUBLIC_KEY) { + return false; + } + if (key === ROLES_KEY) { + return Object.values(UserRole); + } + if (key === SCOPES_KEY) { + return []; + } + return undefined; + }); + + jwtServiceMock.validateToken.mockResolvedValue({ + roles: [UserRole.TOPCODER_MANAGER], + scopes: [], + isMachine: false, + tokenPayload: { + sub: '123', + }, + }); + m2mServiceMock.validateMachineToken.mockReturnValue({ + isMachine: false, + scopes: [], + }); + + const result = await guard.canActivate(createExecutionContext(request)); + + expect(result).toBe(true); + expect(request.user).toEqual( + expect.objectContaining({ + roles: [UserRole.TOPCODER_MANAGER], + }), + ); + }); + it('allows human token when required scope is present', async () => { const request: Record = { headers: { diff --git a/src/shared/services/permission.service.spec.ts b/src/shared/services/permission.service.spec.ts index a203341..2f82b4b 100644 --- a/src/shared/services/permission.service.spec.ts +++ b/src/shared/services/permission.service.spec.ts @@ -347,6 +347,7 @@ describe('PermissionService', () => { }); it.each([ + UserRole.TOPCODER_MANAGER, UserRole.PROJECT_MANAGER, UserRole.TASK_MANAGER, UserRole.TOPCODER_TASK_MANAGER, @@ -392,41 +393,50 @@ describe('PermissionService', () => { expect(allowed).toBe(true); }); - it('allows manager-tier roles to read project members without membership', () => { - const allowed = service.hasNamedPermission(Permission.READ_PROJECT_MEMBER, { - userId: '555', - roles: [UserRole.PROGRAM_MANAGER], - isMachine: false, - }); - - expect(allowed).toBe(true); - }); - - it('allows manager-tier roles to read project invites without membership', () => { - const allowed = service.hasNamedPermission( - Permission.READ_PROJECT_INVITE_NOT_OWN, - { + it.each([UserRole.PROGRAM_MANAGER, UserRole.TOPCODER_MANAGER])( + 'allows manager-tier role %s to read project members without membership', + (role) => { + const allowed = service.hasNamedPermission(Permission.READ_PROJECT_MEMBER, { userId: '555', - roles: [UserRole.PROGRAM_MANAGER], + roles: [role], isMachine: false, - }, - ); + }); - expect(allowed).toBe(true); - }); + expect(allowed).toBe(true); + }, + ); - it('allows manager-tier roles to view project attachments without membership', () => { - const allowed = service.hasNamedPermission( - Permission.VIEW_PROJECT_ATTACHMENT, - { - userId: '555', - roles: [UserRole.PROGRAM_MANAGER], - isMachine: false, - }, - ); + it.each([UserRole.PROGRAM_MANAGER, UserRole.TOPCODER_MANAGER])( + 'allows manager-tier role %s to read project invites without membership', + (role) => { + const allowed = service.hasNamedPermission( + Permission.READ_PROJECT_INVITE_NOT_OWN, + { + userId: '555', + roles: [role], + isMachine: false, + }, + ); - expect(allowed).toBe(true); - }); + expect(allowed).toBe(true); + }, + ); + + it.each([UserRole.PROGRAM_MANAGER, UserRole.TOPCODER_MANAGER])( + 'allows manager-tier role %s to view project attachments without membership', + (role) => { + const allowed = service.hasNamedPermission( + Permission.VIEW_PROJECT_ATTACHMENT, + { + userId: '555', + roles: [role], + isMachine: false, + }, + ); + + expect(allowed).toBe(true); + }, + ); it('allows creating other project members for machine token with project-member write scope', () => { const allowed = service.hasNamedPermission( diff --git a/src/shared/services/permission.service.ts b/src/shared/services/permission.service.ts index 0537428..6143fb6 100644 --- a/src/shared/services/permission.service.ts +++ b/src/shared/services/permission.service.ts @@ -138,8 +138,8 @@ export class PermissionService { * @returns `true` when the user satisfies the named permission rule * @security `CREATE_PROJECT` currently trusts a permissive `isAuthenticated` * check: any non-empty `userId`, any role, any scope, or `isMachine`. - * @security Admin detection currently includes the raw role string - * `'topcoder_manager'`, which can drift from enum-backed role names. + * @security Legacy manager JWTs use `UserRole.TOPCODER_MANAGER`, which is + * broader than strict admin access and retained for v5 compatibility. */ hasNamedPermission( permission: NamedPermission, @@ -160,11 +160,10 @@ export class PermissionService { machineContext.isMachine; // TODO: intentionally permissive authentication gate for CREATE_PROJECT; reassess whether any role/scope/machine token should qualify. - // TODO: replace 'topcoder_manager' string literal with UserRole enum value. const isAdmin = this.hasIntersection(user.roles || [], [ ...ADMIN_ROLES, UserRole.MANAGER, - 'topcoder_manager', + UserRole.TOPCODER_MANAGER, ]); const hasProjectReadTopcoderRole = this.hasProjectReadTopcoderRole(user); const hasManagerTopcoderRole = this.hasManagerTopcoderRole(user); @@ -833,7 +832,7 @@ export class PermissionService { UserRole.TOPCODER_TASK_MANAGER, UserRole.TALENT_MANAGER, UserRole.TOPCODER_TALENT_MANAGER, - 'topcoder_manager', + UserRole.TOPCODER_MANAGER, ]); } @@ -847,7 +846,7 @@ export class PermissionService { private hasManagerTopcoderRole(user: JwtUser): boolean { return this.hasIntersection(user.roles || [], [ ...MANAGER_ROLES, - 'topcoder_manager', + UserRole.TOPCODER_MANAGER, ]); } diff --git a/src/shared/utils/permission-docs.utils.ts b/src/shared/utils/permission-docs.utils.ts index 950afaf..0ff7d88 100644 --- a/src/shared/utils/permission-docs.utils.ts +++ b/src/shared/utils/permission-docs.utils.ts @@ -48,12 +48,10 @@ export interface PermissionDocumentationSummary { scopes: string[]; } -const LEGACY_TOPCODER_MANAGER_ROLE = 'topcoder_manager'; - const ADMIN_AND_MANAGER_ROLES = [ ...ADMIN_ROLES, UserRole.MANAGER, - LEGACY_TOPCODER_MANAGER_ROLE, + UserRole.TOPCODER_MANAGER, ]; const STRICT_ADMIN_ACCESS_ROLES = [...ADMIN_ROLES];