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
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export class AdminFindingsController {
orgId,
findingId,
updateDto,
[],
true,
true,
req.userId,
null,
Expand Down
52 changes: 22 additions & 30 deletions apps/api/src/findings/findings.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
UsePipes,
ValidationPipe,
BadRequestException,
ForbiddenException,
} from '@nestjs/common';
import {
ApiBody,
Expand All @@ -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';
Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -258,27 +252,34 @@ 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(
'User is not a member of this organization',
);
}

const canCreateFindings = permissions.finding?.includes('create') ?? false;

return await this.findingsService.update(
authContext.organizationId,
id,
updateDto,
authContext.userRoles || [],
canCreateFindings,
isPlatformAdmin,
authContext.userId,
member.id,
Expand All @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion apps/api/src/findings/findings.module.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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,
Expand Down
97 changes: 96 additions & 1 deletion apps/api/src/findings/findings.service.spec.ts
Original file line number Diff line number Diff line change
@@ -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() },
Expand All @@ -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(),
Expand Down Expand Up @@ -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' };

Expand Down Expand Up @@ -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();
});
});
18 changes: 3 additions & 15 deletions apps/api/src/findings/findings.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ export class FindingsService {
organizationId: string,
findingId: string,
updateDto: UpdateFindingDto,
userRoles: string[],
canCreateFindings: boolean,
isPlatformAdmin: boolean,
userId: string,
memberId: string | null,
Expand All @@ -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}'`,
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -353,7 +348,7 @@ export function FindingDetailSheet({
<SelectContent>
{allowedStatusOptions({
current: status,
isAuditor,
canCreateFindings,
isPlatformAdmin,
}).map((s) => (
<SelectItem key={s} value={s}>
Expand Down
Loading