diff --git a/apps/api/src/admin-organizations/admin-findings.controller.ts b/apps/api/src/admin-organizations/admin-findings.controller.ts index f7d694475..aedc2cbc7 100644 --- a/apps/api/src/admin-organizations/admin-findings.controller.ts +++ b/apps/api/src/admin-organizations/admin-findings.controller.ts @@ -87,7 +87,7 @@ export class AdminFindingsController { orgId, findingId, updateDto, - [], + true, true, req.userId, null, diff --git a/apps/api/src/findings/findings.controller.ts b/apps/api/src/findings/findings.controller.ts index 4917b5876..8f2f9139a 100644 --- a/apps/api/src/findings/findings.controller.ts +++ b/apps/api/src/findings/findings.controller.ts @@ -11,7 +11,6 @@ import { UsePipes, ValidationPipe, BadRequestException, - ForbiddenException, } from '@nestjs/common'; import { ApiBody, @@ -34,6 +33,7 @@ import { PermissionGuard } from '../auth/permission.guard'; import { RequirePermission } from '../auth/require-permission.decorator'; import { AuthContext } from '../auth/auth-context.decorator'; import type { AuthContext as AuthContextType } from '../auth/types'; +import { RolesService } from '../roles/roles.service'; import { FindingsService } from './findings.service'; import { CreateFindingDto } from './dto/create-finding.dto'; import { UpdateFindingDto } from './dto/update-finding.dto'; @@ -46,7 +46,10 @@ import { evidenceFormTypeSchema } from '@/evidence-forms/evidence-forms.definiti @UseGuards(HybridAuthGuard) @ApiSecurity('apikey') export class FindingsController { - constructor(private readonly findingsService: FindingsService) {} + constructor( + private readonly findingsService: FindingsService, + private readonly rolesService: RolesService, + ) {} /** * List findings for the organization. Supports optional target/status filters. @@ -203,15 +206,6 @@ export class FindingsController { throw new BadRequestException('User ID is required'); } - const isAuditor = authContext.userRoles?.includes('auditor'); - const isPlatformAdmin = await this.checkPlatformAdmin(authContext.userId); - - if (!isAuditor && !isPlatformAdmin) { - throw new ForbiddenException( - 'Only auditors or platform admins can create findings', - ); - } - const member = await db.member.findFirst({ where: { userId: authContext.userId, @@ -258,15 +252,20 @@ export class FindingsController { throw new BadRequestException('User ID is required'); } - const isPlatformAdmin = await this.checkPlatformAdmin(authContext.userId); - - const member = await db.member.findFirst({ - where: { - userId: authContext.userId, - organizationId: authContext.organizationId, - deactivated: false, - }, - }); + const [isPlatformAdmin, member, permissions] = await Promise.all([ + this.checkPlatformAdmin(authContext.userId), + db.member.findFirst({ + where: { + userId: authContext.userId, + organizationId: authContext.organizationId, + deactivated: false, + }, + }), + this.rolesService.resolvePermissions( + authContext.organizationId, + authContext.userRoles || [], + ), + ]); if (!member) { throw new BadRequestException( @@ -274,11 +273,13 @@ export class FindingsController { ); } + const canCreateFindings = permissions.finding?.includes('create') ?? false; + return await this.findingsService.update( authContext.organizationId, id, updateDto, - authContext.userRoles || [], + canCreateFindings, isPlatformAdmin, authContext.userId, member.id, @@ -298,15 +299,6 @@ export class FindingsController { throw new BadRequestException('User ID is required'); } - const isAuditor = authContext.userRoles?.includes('auditor'); - const isPlatformAdmin = await this.checkPlatformAdmin(authContext.userId); - - if (!isAuditor && !isPlatformAdmin) { - throw new ForbiddenException( - 'Only auditors or platform admins can delete findings', - ); - } - const member = await db.member.findFirst({ where: { userId: authContext.userId, diff --git a/apps/api/src/findings/findings.module.ts b/apps/api/src/findings/findings.module.ts index 642f8a49b..921fcc2b6 100644 --- a/apps/api/src/findings/findings.module.ts +++ b/apps/api/src/findings/findings.module.ts @@ -1,5 +1,6 @@ import { Module } from '@nestjs/common'; import { AuthModule } from '../auth/auth.module'; +import { RolesModule } from '../roles/roles.module'; import { TimelinesModule } from '../timelines/timelines.module'; import { NovuService } from '../notifications/novu.service'; import { FindingAuditService } from './finding-audit.service'; @@ -8,7 +9,7 @@ import { FindingsController } from './findings.controller'; import { FindingsService } from './findings.service'; @Module({ - imports: [AuthModule, TimelinesModule], + imports: [AuthModule, RolesModule, TimelinesModule], controllers: [FindingsController], providers: [ FindingsService, diff --git a/apps/api/src/findings/findings.service.spec.ts b/apps/api/src/findings/findings.service.spec.ts index a5c8ada81..8630dce93 100644 --- a/apps/api/src/findings/findings.service.spec.ts +++ b/apps/api/src/findings/findings.service.spec.ts @@ -1,10 +1,22 @@ -import { BadRequestException, NotFoundException } from '@nestjs/common'; +import { + BadRequestException, + ForbiddenException, + NotFoundException, +} from '@nestjs/common'; jest.mock('@trycompai/company', () => ({ toDbEvidenceFormType: (v: string) => v, toExternalEvidenceFormType: (v: string | null) => v, })); +jest.mock('../timelines/timelines.service', () => ({ + TimelinesService: jest.fn(), +})); + +jest.mock('../frameworks/frameworks-timeline.helper', () => ({ + checkAutoCompletePhases: jest.fn().mockResolvedValue(undefined), +})); + const mockDb = { task: { findFirst: jest.fn() }, evidenceSubmission: { findFirst: jest.fn(), findUnique: jest.fn() }, @@ -13,6 +25,7 @@ const mockDb = { risk: { findFirst: jest.fn() }, member: { findFirst: jest.fn(), findUnique: jest.fn() }, device: { findFirst: jest.fn(), findUnique: jest.fn() }, + user: { findUnique: jest.fn() }, findingTemplate: { findUnique: jest.fn() }, finding: { findFirst: jest.fn(), @@ -57,6 +70,7 @@ describe('FindingsService.create (target validator)', () => { const svc = new FindingsService( auditService as never, notifier as never, + {} as never, ); const baseDto = { content: 'Example finding' }; @@ -134,3 +148,84 @@ describe('FindingsService.create (target validator)', () => { expect(createArgs.data.taskId).toBeNull(); }); }); + +describe('FindingsService.update (status transition rules)', () => { + const auditService = { + logFindingStatusChanged: jest.fn(), + logFindingContentChanged: jest.fn(), + logFindingTypeChanged: jest.fn(), + }; + const notifier = { + notifyFindingCreated: jest.fn(), + notifyStatusChanged: jest.fn(), + }; + const svc = new FindingsService(auditService as never, notifier as never, {} as never); + const existingFinding = { + id: 'fnd_1', + organizationId: 'org_1', + content: 'Test finding', + status: 'open', + type: 'soc2', + severity: 'medium', + revisionNote: null, + area: null, + taskId: null, + policyId: null, + vendorId: null, + riskId: null, + memberId: null, + deviceId: null, + evidenceSubmissionId: null, + evidenceFormType: null, + createdBy: null, + createdByAdmin: null, + createdAt: new Date(), + updatedAt: new Date(), + }; + + beforeEach(() => { + jest.clearAllMocks(); + mockDb.finding.findFirst.mockResolvedValue(existingFinding); + mockDb.finding.update.mockResolvedValue({ + ...existingFinding, + createdBy: null, + createdByAdmin: null, + }); + mockDb.user.findUnique.mockResolvedValue({ + name: 'Test User', + email: 'test@example.com', + }); + }); + + it('allows ready_for_review regardless of canCreateFindings', async () => { + await svc.update('org_1', 'fnd_1', { status: 'ready_for_review' as never }, false, false, 'usr_1', 'mem_1'); + expect(mockDb.finding.update).toHaveBeenCalled(); + }); + + it('blocks needs_revision without canCreateFindings', async () => { + await expect( + svc.update('org_1', 'fnd_1', { status: 'needs_revision' as never }, false, false, 'usr_1', 'mem_1'), + ).rejects.toBeInstanceOf(ForbiddenException); + }); + + it('allows needs_revision with canCreateFindings', async () => { + await svc.update('org_1', 'fnd_1', { status: 'needs_revision' as never }, true, false, 'usr_1', 'mem_1'); + expect(mockDb.finding.update).toHaveBeenCalled(); + }); + + it('blocks closed without canCreateFindings', async () => { + await expect( + svc.update('org_1', 'fnd_1', { status: 'closed' as never }, false, false, 'usr_1', 'mem_1'), + ).rejects.toBeInstanceOf(ForbiddenException); + }); + + it('allows closed with canCreateFindings', async () => { + await svc.update('org_1', 'fnd_1', { status: 'closed' as never }, true, false, 'usr_1', 'mem_1'); + expect(mockDb.finding.update).toHaveBeenCalled(); + }); + + it('allows platform admin to set any status', async () => { + await svc.update('org_1', 'fnd_1', { status: 'closed' as never }, false, true, 'usr_1', 'mem_1'); + expect(mockDb.finding.update).toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/findings/findings.service.ts b/apps/api/src/findings/findings.service.ts index 371853bf8..838c9bab5 100644 --- a/apps/api/src/findings/findings.service.ts +++ b/apps/api/src/findings/findings.service.ts @@ -364,7 +364,7 @@ export class FindingsService { organizationId: string, findingId: string, updateDto: UpdateFindingDto, - userRoles: string[], + canCreateFindings: boolean, isPlatformAdmin: boolean, userId: string, memberId: string | null, @@ -375,26 +375,14 @@ export class FindingsService { const previousContent = finding.content; if (updateDto.status) { - const isAuditor = userRoles.includes('auditor'); - const canSetRestrictedStatus = isPlatformAdmin || isAuditor; - if ( (updateDto.status === FindingStatus.needs_revision || updateDto.status === FindingStatus.closed) && - !canSetRestrictedStatus - ) { - throw new ForbiddenException( - `Only auditors or platform admins can set status to '${updateDto.status}'`, - ); - } - - if ( - updateDto.status === FindingStatus.ready_for_review && - isAuditor && + !canCreateFindings && !isPlatformAdmin ) { throw new ForbiddenException( - `Auditors cannot set status to 'ready_for_review'. This status is for clients to signal readiness.`, + `Only auditors or platform admins can set status to '${updateDto.status}'`, ); } } diff --git a/apps/app/src/app/(app)/[orgId]/overview/components/FindingDetailSheet.tsx b/apps/app/src/app/(app)/[orgId]/overview/components/FindingDetailSheet.tsx index 59ef1f3f4..a16accc57 100644 --- a/apps/app/src/app/(app)/[orgId]/overview/components/FindingDetailSheet.tsx +++ b/apps/app/src/app/(app)/[orgId]/overview/components/FindingDetailSheet.tsx @@ -64,18 +64,18 @@ interface FindingDetailSheetProps { */ function allowedStatusOptions({ current, - isAuditor, + canCreateFindings, isPlatformAdmin, }: { current: FindingStatus; - isAuditor: boolean; + canCreateFindings: boolean; isPlatformAdmin: boolean; }): FindingStatus[] { - const canSetRestricted = isAuditor || isPlatformAdmin; - const canSetReadyForReview = !isAuditor || isPlatformAdmin; - const options: FindingStatus[] = [FindingStatus.open]; - if (canSetReadyForReview) options.push(FindingStatus.ready_for_review); - if (canSetRestricted) { + const options: FindingStatus[] = [ + FindingStatus.open, + FindingStatus.ready_for_review, + ]; + if (canCreateFindings || isPlatformAdmin) { options.push(FindingStatus.needs_revision, FindingStatus.closed); } if (!options.includes(current)) options.push(current); @@ -156,18 +156,13 @@ export function FindingDetailSheet({ onSaved, onDeleted, }: FindingDetailSheetProps) { - const { hasPermission, roles } = usePermissions(); + const { hasPermission } = usePermissions(); const { data: session } = useSession(); const canUpdate = hasPermission('finding', 'update'); const canDelete = hasPermission('finding', 'delete'); - // Match the API's literal role check (`userRoles.includes('auditor')` in - // findings.service.ts). A custom role granting `finding:create` does NOT - // count as auditor on the server, so we can't proxy via permissions here. - const isAuditor = roles.includes('auditor'); + const canCreateFindings = hasPermission('finding', 'create'); const isPlatformAdmin = session?.user?.role === 'admin'; - // Only auditors can rewrite a finding's content; owners/admins can still - // move the status forward but the audit narrative belongs to the auditor. - const canEditContent = canUpdate && isAuditor; + const canEditContent = canUpdate && canCreateFindings; const { updateFinding, deleteFinding } = useFindingActions(); const { data: historyData } = useFindingHistory(finding?.id ?? null); @@ -353,7 +348,7 @@ export function FindingDetailSheet({ {allowedStatusOptions({ current: status, - isAuditor, + canCreateFindings, isPlatformAdmin, }).map((s) => (