diff --git a/apps/portal/src/app/api/portal/accept-policies/route.test.ts b/apps/portal/src/app/api/portal/accept-policies/route.test.ts new file mode 100644 index 0000000000..e67bfa7fbc --- /dev/null +++ b/apps/portal/src/app/api/portal/accept-policies/route.test.ts @@ -0,0 +1,193 @@ +import { NextRequest } from 'next/server'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// CS-790 regression guard: accepting a policy only pushed the member id onto +// `Policy.signedBy`, which carries no timestamp — so nothing recorded WHEN an +// employee signed and auditors could not be shown an acknowledgment date. The +// acceptance must also write an audit log (the timestamped record the policy +// Activity tab and /v1/audit-logs read), atomically with the signature, and +// exactly once per member even when accepts for the same policy overlap. + +const mocks = vi.hoisted(() => ({ + getSession: vi.fn(), + memberFindFirst: vi.fn(), + policyFindFirst: vi.fn(), + policyUpdateMany: vi.fn(), + auditLogCreate: vi.fn(), + transaction: vi.fn(), +})); + +vi.mock('@/app/lib/auth', () => ({ + auth: { api: { getSession: mocks.getSession } }, +})); + +vi.mock('@db/server', () => ({ + db: { + member: { findFirst: mocks.memberFindFirst }, + policy: { findFirst: mocks.policyFindFirst, updateMany: mocks.policyUpdateMany }, + auditLog: { create: mocks.auditLogCreate }, + $transaction: mocks.transaction, + }, +})); + +import { POST } from './route'; + +const MEMBER = { + id: 'mem_1', + userId: 'user_1', + organizationId: 'org_1', + deactivated: false, +}; + +const POLICY = { + id: 'pol_1', + name: 'Code of Conduct', + currentVersionId: 'pv_1', +}; + +/** The transaction client exposes the same models as `db`, like Prisma's does. */ +const TX = { + policy: { updateMany: mocks.policyUpdateMany }, + auditLog: { create: mocks.auditLogCreate }, +}; + +function makeRequest(body: unknown): NextRequest { + return new NextRequest('http://localhost/api/portal/accept-policies', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }); +} + +describe('POST /api/portal/accept-policies', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.transaction.mockImplementation((arg) => arg(TX)); + // Models the row lock behind the conditional claim: whichever accept gets + // to the row first matches it, every later one matches zero rows. + let claimed = false; + mocks.policyUpdateMany.mockImplementation(async () => { + if (claimed) { + return { count: 0 }; + } + claimed = true; + return { count: 1 }; + }); + }); + + it('returns 401 when there is no session', async () => { + mocks.getSession.mockResolvedValue(null); + + const res = await POST(makeRequest({ policyIds: ['pol_1'], memberId: 'mem_1' })); + + expect(res.status).toBe(401); + expect(mocks.auditLogCreate).not.toHaveBeenCalled(); + }); + + it('returns 403 when the member does not belong to the authenticated user', async () => { + mocks.getSession.mockResolvedValue({ user: { id: 'user_1' } }); + mocks.memberFindFirst.mockResolvedValue(null); + + const res = await POST(makeRequest({ policyIds: ['pol_1'], memberId: 'mem_other' })); + + expect(res.status).toBe(403); + expect(mocks.policyUpdateMany).not.toHaveBeenCalled(); + expect(mocks.auditLogCreate).not.toHaveBeenCalled(); + }); + + it('records the signature and its timestamp in one transaction', async () => { + mocks.getSession.mockResolvedValue({ user: { id: 'user_1' } }); + mocks.memberFindFirst.mockResolvedValue(MEMBER); + mocks.policyFindFirst.mockResolvedValue(POLICY); + + const res = await POST(makeRequest({ policyIds: ['pol_1'], memberId: 'mem_1' })); + + expect(res.status).toBe(200); + // Conditional write, so an already-signed member cannot be appended twice. + expect(mocks.policyUpdateMany).toHaveBeenCalledWith({ + where: { + id: 'pol_1', + organizationId: 'org_1', + NOT: { signedBy: { has: 'mem_1' } }, + }, + data: { signedBy: { push: 'mem_1' } }, + }); + // The timestamped acceptance record: AuditLog.timestamp defaults to now(), + // so this row is what answers "when did this member sign this policy". + expect(mocks.auditLogCreate).toHaveBeenCalledWith({ + data: expect.objectContaining({ + organizationId: 'org_1', + userId: 'user_1', + memberId: 'mem_1', + entityType: 'policy', + entityId: 'pol_1', + description: 'accepted this policy', + data: expect.objectContaining({ action: 'accept', policyVersionId: 'pv_1' }), + }), + }); + // Claim and log are one atomic unit — a signature without its timestamp is + // the bug, so the check must happen inside the transaction, not before it. + expect(mocks.transaction).toHaveBeenCalledTimes(1); + expect(mocks.transaction.mock.calls[0][0]).toBeTypeOf('function'); + }); + + it('does not re-log an acceptance the member already made', async () => { + mocks.getSession.mockResolvedValue({ user: { id: 'user_1' } }); + mocks.memberFindFirst.mockResolvedValue(MEMBER); + mocks.policyFindFirst.mockResolvedValue(POLICY); + // The member is already in signedBy, so the conditional claim matches nothing. + mocks.policyUpdateMany.mockResolvedValue({ count: 0 }); + + const res = await POST(makeRequest({ policyIds: ['pol_1'], memberId: 'mem_1' })); + + expect(res.status).toBe(200); + // Re-accepting must keep the original signing time, not stamp a new one. + expect(mocks.auditLogCreate).not.toHaveBeenCalled(); + }); + + it('logs one acceptance when two accepts of the same policy overlap', async () => { + mocks.getSession.mockResolvedValue({ user: { id: 'user_1' } }); + mocks.memberFindFirst.mockResolvedValue(MEMBER); + mocks.policyFindFirst.mockResolvedValue(POLICY); + + const [first, second] = await Promise.all([ + POST(makeRequest({ policyIds: ['pol_1'], memberId: 'mem_1' })), + POST(makeRequest({ policyIds: ['pol_1'], memberId: 'mem_1' })), + ]); + + expect(first.status).toBe(200); + expect(second.status).toBe(200); + // Both requests read the policy before either wrote, so only the atomic + // claim keeps this from becoming two signatures with two timestamps. + expect(mocks.policyUpdateMany).toHaveBeenCalledTimes(2); + expect(mocks.auditLogCreate).toHaveBeenCalledTimes(1); + }); + + it('accepts a duplicated policy id once', async () => { + mocks.getSession.mockResolvedValue({ user: { id: 'user_1' } }); + mocks.memberFindFirst.mockResolvedValue(MEMBER); + mocks.policyFindFirst.mockResolvedValue(POLICY); + + const res = await POST(makeRequest({ policyIds: ['pol_1', 'pol_1'], memberId: 'mem_1' })); + + expect(res.status).toBe(200); + expect(mocks.policyFindFirst).toHaveBeenCalledTimes(1); + expect(mocks.auditLogCreate).toHaveBeenCalledTimes(1); + }); + + it('does not sign a policy outside the member organization', async () => { + mocks.getSession.mockResolvedValue({ user: { id: 'user_1' } }); + mocks.memberFindFirst.mockResolvedValue(MEMBER); + // Scoped lookup finds nothing for another org's policy id. + mocks.policyFindFirst.mockResolvedValue(null); + + const res = await POST(makeRequest({ policyIds: ['pol_other_org'], memberId: 'mem_1' })); + + expect(res.status).toBe(200); + expect(mocks.policyFindFirst).toHaveBeenCalledWith( + expect.objectContaining({ where: { id: 'pol_other_org', organizationId: 'org_1' } }), + ); + expect(mocks.policyUpdateMany).not.toHaveBeenCalled(); + expect(mocks.auditLogCreate).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/portal/src/app/api/portal/accept-policies/route.ts b/apps/portal/src/app/api/portal/accept-policies/route.ts index 6fb0e43d64..9a4a8afe09 100644 --- a/apps/portal/src/app/api/portal/accept-policies/route.ts +++ b/apps/portal/src/app/api/portal/accept-policies/route.ts @@ -1,4 +1,5 @@ import { auth } from '@/app/lib/auth'; +import { acceptPolicyForMember } from '@/lib/policy-acceptance'; import { db } from '@db/server'; import { type NextRequest, NextResponse } from 'next/server'; import { z } from 'zod'; @@ -44,25 +45,12 @@ export async function POST(req: NextRequest) { } try { - const updatePromises = policyIds.map(async (policyId) => { - const policy = await db.policy.findUnique({ - where: { id: policyId }, - }); - - if (policy && !policy.signedBy.includes(memberId)) { - return db.policy.update({ - where: { id: policyId }, - data: { - signedBy: { - push: memberId, - }, - }, - }); - } - return null; - }); - - await Promise.all(updatePromises); + await Promise.all( + // Deduplicated so a repeated id does not race itself into two acceptances. + [...new Set(policyIds)].map((policyId) => + acceptPolicyForMember({ policyId, member, userId: session.user.id }), + ), + ); return NextResponse.json({ success: true }); } catch (error) { diff --git a/apps/portal/src/app/api/portal/mark-policy-completed/route.ts b/apps/portal/src/app/api/portal/mark-policy-completed/route.ts index f360a25a3e..2d6cc79187 100644 --- a/apps/portal/src/app/api/portal/mark-policy-completed/route.ts +++ b/apps/portal/src/app/api/portal/mark-policy-completed/route.ts @@ -1,4 +1,5 @@ import { auth } from '@/app/lib/auth'; +import { acceptPolicyForMember } from '@/lib/policy-acceptance'; import { db } from '@db/server'; import { type NextRequest, NextResponse } from 'next/server'; import { z } from 'zod'; @@ -40,25 +41,19 @@ export async function POST(req: NextRequest) { return NextResponse.json({ error: 'Member not found' }, { status: 404 }); } - const policy = await db.policy.findUnique({ - where: { id: policyId }, + const result = await acceptPolicyForMember({ + policyId, + member, + userId: session.user.id, }); - if (!policy) { + if (result === 'not-found') { return NextResponse.json({ error: 'Policy not found' }, { status: 404 }); } - // Check if user has already signed this policy - if (policy.signedBy.includes(member.id)) { + if (result === 'already-signed') { return NextResponse.json({ success: true, alreadySigned: true }); } - await db.policy.update({ - where: { id: policyId }, - data: { - signedBy: [...policy.signedBy, member.id], - }, - }); - return NextResponse.json({ success: true }); } diff --git a/apps/portal/src/lib/policy-acceptance.ts b/apps/portal/src/lib/policy-acceptance.ts new file mode 100644 index 0000000000..b663c0c442 --- /dev/null +++ b/apps/portal/src/lib/policy-acceptance.ts @@ -0,0 +1,77 @@ +import type { Member } from '@db'; +import { db } from '@db/server'; + +/** Outcome of a single acceptance attempt, so routes can map it to a response. */ +export type PolicyAcceptanceResult = 'accepted' | 'already-signed' | 'not-found'; + +/** + * Accepts a policy for `member` and records WHEN it happened. + * + * `Policy.signedBy` is a flat array of member ids with no timestamp, so the + * acceptance audit log is the only record of the signing time. It is what the + * policy Activity tab and `GET /v1/audit-logs?entityType=policy&entityId=...` + * read to answer "when did X sign policy Y" for auditors. Both writes go in one + * transaction so a signature can never exist without its timestamp. + * + * Re-accepting is a no-op, keeping the logged time the first acceptance of the + * current version (publishing a new version clears `signedBy`). + */ +export async function acceptPolicyForMember({ + policyId, + member, + userId, +}: { + policyId: string; + member: Pick; + userId: string; +}): Promise { + const policy = await db.policy.findFirst({ + where: { id: policyId, organizationId: member.organizationId }, + select: { id: true, name: true, currentVersionId: true }, + }); + + if (!policy) { + return 'not-found'; + } + + const accepted = await db.$transaction(async (tx) => { + // The signature is claimed with a conditional update instead of a + // read-then-write check: Postgres re-evaluates the `NOT has` filter after + // taking the row lock, so a concurrent accept of the same policy matches + // zero rows rather than appending a second signature and a second log. + const { count } = await tx.policy.updateMany({ + where: { + id: policy.id, + organizationId: member.organizationId, + NOT: { signedBy: { has: member.id } }, + }, + data: { signedBy: { push: member.id } }, + }); + + if (count === 0) { + return false; + } + + await tx.auditLog.create({ + data: { + organizationId: member.organizationId, + userId, + memberId: member.id, + entityType: 'policy', + entityId: policy.id, + description: 'accepted this policy', + data: { + action: 'accept', + policyName: policy.name, + // Which version was accepted — a republish clears signedBy, so an + // acceptance always belongs to the version current at that moment. + policyVersionId: policy.currentVersionId, + }, + }, + }); + + return true; + }); + + return accepted ? 'accepted' : 'already-signed'; +}