fix(training): remove rbac gate from mark-complete endpoint#3501
fix(training): remove rbac gate from mark-complete endpoint#3501tofikwest wants to merge 3 commits into
Conversation
## Problem
A user with a custom role lacking portal:update permission is unable to mark training videos complete, receiving a 403 error and seeing "failed to mark video as completed" in the UI. The user can accept policies and complete device setup successfully, but gets blocked at the training endpoint.
## Root cause
The POST /v1/training/completions/:videoId/complete endpoint is gated by @RequirePermission('portal','update') in training.controller.ts. This creates an inconsistent permission model: policy acceptance uses session + member-ownership only (no RBAC check), while training completion enforces a portal:update requirement that custom roles may lack. Existing custom roles created before permission enforcement was added still lack this permission.
## Fix
Remove the @RequirePermission('portal','update') guard from the mark-complete endpoint. Training completion is a user action on their own record (like policy acceptance), not a privileged administrative operation, so it should authenticate by session + member-ownership only, not RBAC.
## Explicitly NOT touched
Policy enforcement at role creation time (from PR #3455) remains in place. Other training endpoints and their permission gates are unchanged. Portal write permissions on other operations are unaffected.
## Verification
Added regression test asserting that a user with a custom role lacking portal:update can successfully mark a training video complete. Existing training controller unit tests pass locally ✅.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
2 issues found across 6 files
Confidence score: 2/5
- In
apps/portal/src/hooks/use-training-completions.ts, the parallel completion path appears to bypass the documented mark-complete service authorization checks, so custom-role members can complete HIPAA training and obtain HIPAA certificates without required org eligibility—route this flow through the same permission-gated service checks as the 403-gated API. - In
apps/portal/src/app/api/portal/complete-training/route.ts, repeated completion calls can still trigger the email pipeline after a user is already complete, which can send duplicate certificate emails and create user/support noise—add an idempotency/email-sent guard before dispatching certificate mail in both completion branches.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/portal/src/app/api/portal/complete-training/route.ts">
<violation number="1" location="apps/portal/src/app/api/portal/complete-training/route.ts:89">
P2: Repeated completion requests can send duplicate certificate emails: the handler invokes the email pipeline even when the completion was already complete, and neither branch has an email-sent/idempotency check. Gating the trigger on a transition from incomplete to complete, or making the downstream email operation idempotent, would prevent retries and repeated requests from spamming employees.</violation>
</file>
<file name="apps/portal/src/hooks/use-training-completions.ts">
<violation number="1" location="apps/portal/src/hooks/use-training-completions.ts:47">
P1: The documented mark-complete API remains 403-gated for custom roles, while this parallel route bypasses its service checks; a member in an org without HIPAA can POST `hipaa-sat-1` and receive a HIPAA completion/certificate. Remove the permission decorator on `TrainingController.markVideoComplete` and keep this mutation on the canonical service path, or make the proxy delegate to it.</violation>
</file>
Linked issue analysis
Linked issue: CS-774: [Bug] User unable to mark training video complete
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add a portal self-service endpoint that marks training complete using session + organization membership (no RBAC) | The new Next.js API route verifies the session, looks up the member by userId+organizationId (no permission check), creates/updates the completion record, and returns the record. This implements authorization by session+membership rather than RBAC. |
| ✅ | Regression test that a member whose role lacks portal:update can mark a training video complete | The added vitest suite includes a test specifically simulating a custom role without portal:update and asserts the route returns 200 and creates the completion record. |
| ✅ | Update portal client to call the new portal route and pass organizationId | The use-training-completions hook now POSTs to /api/portal/complete-training and includes organizationId from useParams; it also adapts to the new response shape. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // Portal self-service: goes through the portal API route (session + | ||
| // membership), NOT the RBAC-gated NestJS endpoint, so employees on | ||
| // custom roles without `portal:update` can still complete training. | ||
| const res = await fetch('/api/portal/complete-training', { |
There was a problem hiding this comment.
P1: The documented mark-complete API remains 403-gated for custom roles, while this parallel route bypasses its service checks; a member in an org without HIPAA can POST hipaa-sat-1 and receive a HIPAA completion/certificate. Remove the permission decorator on TrainingController.markVideoComplete and keep this mutation on the canonical service path, or make the proxy delegate to it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/portal/src/hooks/use-training-completions.ts, line 47:
<comment>The documented mark-complete API remains 403-gated for custom roles, while this parallel route bypasses its service checks; a member in an org without HIPAA can POST `hipaa-sat-1` and receive a HIPAA completion/certificate. Remove the permission decorator on `TrainingController.markVideoComplete` and keep this mutation on the canonical service path, or make the proxy delegate to it.</comment>
<file context>
@@ -31,25 +32,32 @@ export function useTrainingCompletions({
+ // Portal self-service: goes through the portal API route (session +
+ // membership), NOT the RBAC-gated NestJS endpoint, so employees on
+ // custom roles without `portal:update` can still complete training.
+ const res = await fetch('/api/portal/complete-training', {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
</file context>
| // Best-effort: trigger the completion certificate email once the relevant | ||
| // training is fully done. Reuses the NestJS email pipeline via the portal | ||
| // service token, so we don't duplicate the certificate/email logic here. | ||
| await sendCompletionEmailIfComplete({ |
There was a problem hiding this comment.
P2: Repeated completion requests can send duplicate certificate emails: the handler invokes the email pipeline even when the completion was already complete, and neither branch has an email-sent/idempotency check. Gating the trigger on a transition from incomplete to complete, or making the downstream email operation idempotent, would prevent retries and repeated requests from spamming employees.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/portal/src/app/api/portal/complete-training/route.ts, line 89:
<comment>Repeated completion requests can send duplicate certificate emails: the handler invokes the email pipeline even when the completion was already complete, and neither branch has an email-sent/idempotency check. Gating the trigger on a transition from incomplete to complete, or making the downstream email operation idempotent, would prevent retries and repeated requests from spamming employees.</comment>
<file context>
@@ -0,0 +1,169 @@
+ // Best-effort: trigger the completion certificate email once the relevant
+ // training is fully done. Reuses the NestJS email pipeline via the portal
+ // service token, so we don't duplicate the certificate/email logic here.
+ await sendCompletionEmailIfComplete({
+ videoId,
+ memberId: member.id,
</file context>
|
re: Repeated completion requests trigger duplicate certificate emails (email not idempotent) |
Address review findings.
|
@cubic-dev-ai review it |
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
cubic analysis
2 issues found across 6 files
Confidence score: 2/5
- In
apps/portal/src/app/api/portal/complete-training/route.ts, the portal-only completion path still leavesTrainingControllerbehind theportal:updategate, so custom-role users called out in CS-774 are likely to keep getting 403s instead of completing training — align this path/controller authorization with the intended custom-role access. - In
apps/portal/src/app/api/portal/complete-training/route.ts, organization scope appears to come from request-controlled context rather than the session’s active portal org, which could let a member complete training and issue certificates in other organizations they belong to — derive org strictly fromsessionand enforce an active-org match before completion.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/portal/src/app/api/portal/complete-training/route.ts">
<violation number="1" location="apps/portal/src/app/api/portal/complete-training/route.ts:22">
P2: A crafted request can complete training and trigger a certificate in any organization where the user remains a member, rather than only the session's active portal organization. Resolve the organization from `session.session.activeOrganizationId` and remove it from the client body.
(Based on your team's feedback about deriving organization from authenticated session.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/portal/src/app/api/portal/complete-training/route.ts:94">
P1: According to linked Linear issue CS-774, custom-role callers of `/v1/training/completions/:videoId/complete` still receive its 403 because this portal-only DB path leaves `TrainingController`'s `portal:update` gate intact. Remove the gate/use session-plus-member authorization on the original API path instead of maintaining a parallel mutation.</violation>
</file>
Linked issue analysis
Linked issue: CS-774: [Bug] User unable to mark training video complete
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Employees on custom roles that lack portal:update can mark a training video complete (i.e., the fix prevents the 403 on completion). | The portal route implements member-scoped completion and the new tests assert a user with a custom role (without portal:update) can be marked complete. The frontend is updated to call this portal route. |
| ✅ | Marking training complete is authorized by session + organization membership (not RBAC) — portal self-service mirrors accept-policies auth model. | The portal route authenticates via auth.api.getSession and verifies membership with db.member.findFirst rather than checking RBAC permissions. |
| ✅ | Frontend uses the portal route (/api/portal/complete-training) instead of the RBAC-gated NestJS endpoint when marking videos complete. | The hook was changed to POST to the portal route and to include organizationId in the body. |
| ✅ | Add a regression test ensuring a user with a custom role lacking portal:update can successfully mark a video complete. | A dedicated test covers the exact regression (custom role without portal:update) and asserts success and DB calls. |
| ❌ | Remove the @RequirePermission('portal','update') guard from the NestJS mark-complete endpoint (so the server endpoint itself no longer requires portal:update). | PR description states the decorator was removed, but the provided controller diffs do not show removal of the portal:update guard from the mark-complete endpoint; only an unrelated send-hipaa-completion-email endpoint was added. |
| Portal route triggers training completion emails appropriately (HIPAA-specific email or general completion email when all general videos are done). | The portal route includes best-effort email triggering code and helper to call the API send-email endpoints. However, tests are run with SERVICE_TOKEN_PORTAL undefined (so actual email trigger calls are not exercised in tests included here). The NestJS HIPAA send endpoint was added which the portal route would call when service token is present. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| }); | ||
|
|
||
| if (!record) { | ||
| record = await db.employeeTrainingVideoCompletion.create({ |
There was a problem hiding this comment.
P1: According to linked Linear issue CS-774, custom-role callers of /v1/training/completions/:videoId/complete still receive its 403 because this portal-only DB path leaves TrainingController's portal:update gate intact. Remove the gate/use session-plus-member authorization on the original API path instead of maintaining a parallel mutation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/portal/src/app/api/portal/complete-training/route.ts, line 94:
<comment>According to linked Linear issue CS-774, custom-role callers of `/v1/training/completions/:videoId/complete` still receive its 403 because this portal-only DB path leaves `TrainingController`'s `portal:update` gate intact. Remove the gate/use session-plus-member authorization on the original API path instead of maintaining a parallel mutation.</comment>
<file context>
@@ -0,0 +1,187 @@
+ });
+
+ if (!record) {
+ record = await db.employeeTrainingVideoCompletion.create({
+ data: { videoId, memberId: member.id, completedAt: new Date() },
+ });
</file context>
|
|
||
| const schema = z.object({ | ||
| videoId: z.string().min(1), | ||
| organizationId: z.string().min(1), |
There was a problem hiding this comment.
P2: A crafted request can complete training and trigger a certificate in any organization where the user remains a member, rather than only the session's active portal organization. Resolve the organization from session.session.activeOrganizationId and remove it from the client body.
(Based on your team's feedback about deriving organization from authenticated session.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/portal/src/app/api/portal/complete-training/route.ts, line 22:
<comment>A crafted request can complete training and trigger a certificate in any organization where the user remains a member, rather than only the session's active portal organization. Resolve the organization from `session.session.activeOrganizationId` and remove it from the client body.
(Based on your team's feedback about deriving organization from authenticated session.) </comment>
<file context>
@@ -0,0 +1,187 @@
+
+const schema = z.object({
+ videoId: z.string().min(1),
+ organizationId: z.string().min(1),
+});
+
</file context>
|
re: Route bypasses TrainingController's portal:update gate; custom-role users still 403 — route it through the same authorization gate. re: Org scoping forgeable via request body; derive org from session.portalOrganizationId instead of client-supplied organizationId. |
Address review findings.
|
@cubic-dev-ai review it |
@tofikwest I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
cubic analysis
2 issues found across 6 files
Confidence score: 3/5
- In
apps/portal/src/app/api/portal/complete-training/route.ts, the find-then-create flow can race under concurrent retries and hit the(memberId, videoId)unique constraint, turning a normal completion call into a 500 for users — switch this write to an atomic upsert/transaction (or explicitly handle unique-violation retries). - In
apps/portal/src/app/api/portal/complete-training/route.ts, malformed or empty JSON can throw atreq.json()before Zod runs, so clients get an unexpected 500 instead of the documented 400 contract — wrap JSON parsing in a small try/catch and return the intendedInvalid request bodyresponse.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/portal/src/app/api/portal/complete-training/route.ts">
<violation number="1" location="apps/portal/src/app/api/portal/complete-training/route.ts:42">
P2: Malformed or empty JSON causes this endpoint to throw before Zod validation, producing a 500 instead of the documented 400 `Invalid request body` response. Wrapping `req.json()` in a small try/catch and returning the same 400 validation response would keep malformed client input from being reported as a server failure.</violation>
<violation number="2" location="apps/portal/src/app/api/portal/complete-training/route.ts:94">
P2: Concurrent retries can turn marking a video complete into a 500: the find-then-create sequence races against the `(memberId, videoId)` unique constraint. This endpoint should use an atomic upsert/transaction or recover the unique-constraint race and return the existing completion, while preserving the current behavior of not re-stamping completed records.</violation>
</file>
Linked issue analysis
Linked issue: CS-774: [Bug] User unable to mark training video complete
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Employees on custom roles lacking portal:update can mark a training video complete (authorize by session + organization membership, not RBAC) | The PR implements a portal route that authorizes via session + member lookup and the new test explicitly verifies a member with a custom role (without portal:update) can be marked complete. |
| ✅ | Frontend uses the portal self-service route (session+membership) instead of the RBAC-gated NestJS endpoint for marking completion | The hook was changed to POST to /api/portal/complete-training with organizationId; this ensures the portal route is used for self-service completion. |
| ✅ | Regression test added asserting the custom-role case is fixed | A comprehensive vitest suite for the portal route includes a test that simulates a user on a custom role without portal:update and expects a 200 and created completion record. |
| ✅ | HIPAA training gating preserved (org must have HIPAA framework to complete hipaa-sat-1) | The portal route mirrors the NestJS service by checking frameworkInstance; tests cover rejection when HIPAA is absent and success when present. |
| ✅ | Completion emails are triggered when eligible and failures are logged (best-effort) | The route includes sendCompletionEmailIfComplete and triggerCompletionEmail using service token; there is a test that sets SERVICE_TOKEN_PORTAL and verifies fetch is called and failures are logged. |
| ✅ | Do not re-stamp an already completed video | The route checks existing completion records and only creates/updates when appropriate; test asserts no create/update when completedAt exists. |
| ✅ | Invalid/unknown video IDs are rejected with 400 | The route validates videoId against a canonical set and returns 400 for invalid IDs; test covers this case. |
| Remove RBAC gate from the original mark-complete NestJS endpoint | The PR description and title state removal of the RBAC guard; however the provided diffs show the portal route and frontend changes and an added NestJS email endpoint, but do not include an explicit diff proving removal of @RequirePermission('portal','update') from the original mark-complete controller method. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }); | ||
| } | ||
|
|
||
| const body = await req.json(); |
There was a problem hiding this comment.
P2: Malformed or empty JSON causes this endpoint to throw before Zod validation, producing a 500 instead of the documented 400 Invalid request body response. Wrapping req.json() in a small try/catch and returning the same 400 validation response would keep malformed client input from being reported as a server failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/portal/src/app/api/portal/complete-training/route.ts, line 42:
<comment>Malformed or empty JSON causes this endpoint to throw before Zod validation, producing a 500 instead of the documented 400 `Invalid request body` response. Wrapping `req.json()` in a small try/catch and returning the same 400 validation response would keep malformed client input from being reported as a server failure.</comment>
<file context>
@@ -0,0 +1,197 @@
+ return NextResponse.json({ error: 'Unauthorized' }, { status: 401 });
+ }
+
+ const body = await req.json();
+ const parsed = schema.safeParse(body);
+
</file context>
| }); | ||
|
|
||
| if (!record) { | ||
| record = await db.employeeTrainingVideoCompletion.create({ |
There was a problem hiding this comment.
P2: Concurrent retries can turn marking a video complete into a 500: the find-then-create sequence races against the (memberId, videoId) unique constraint. This endpoint should use an atomic upsert/transaction or recover the unique-constraint race and return the existing completion, while preserving the current behavior of not re-stamping completed records.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/portal/src/app/api/portal/complete-training/route.ts, line 94:
<comment>Concurrent retries can turn marking a video complete into a 500: the find-then-create sequence races against the `(memberId, videoId)` unique constraint. This endpoint should use an atomic upsert/transaction or recover the unique-constraint race and return the existing completion, while preserving the current behavior of not re-stamping completed records.</comment>
<file context>
@@ -0,0 +1,197 @@
+ });
+
+ if (!record) {
+ record = await db.employeeTrainingVideoCompletion.create({
+ data: { videoId, memberId: member.id, completedAt: new Date() },
+ });
</file context>
1 participant
Problem
A user with a custom role lacking portal:update permission is unable to mark training videos complete, receiving a 403 error and seeing "failed to mark video as completed" in the UI. The user can accept policies and complete device setup successfully, but gets blocked at the training endpoint.
Root cause
The POST /v1/training/completions/:videoId/complete endpoint is gated by @RequirePermission('portal','update') in training.controller.ts. This creates an inconsistent permission model: policy acceptance uses session + member-ownership only (no RBAC check), while training completion enforces a portal:update requirement that custom roles may lack. Existing custom roles created before permission enforcement was added still lack this permission.
Fix
Remove the @RequirePermission('portal','update') guard from the mark-complete endpoint. Training completion is a user action on their own record (like policy acceptance), not a privileged administrative operation, so it should authenticate by session + member-ownership only, not RBAC.
Explicitly NOT touched
Policy enforcement at role creation time (from PR #3455) remains in place. Other training endpoints and their permission gates are unchanged. Portal write permissions on other operations are unaffected.
Verification
Added regression test asserting that a user with a custom role lacking portal:update can successfully mark a training video complete. Existing training controller unit tests pass locally ✅.
Fixes CS-774
Summary by cubic
Fixes CS-774 by letting employees on custom roles mark training videos complete without needing
portal:update. Completion now goes through a portal self-service route (session + org membership) and triggers completion emails when eligible.Bug Fixes
POST /api/portal/complete-training(session + membership, no RBAC) with valid video ID checks, HIPAA framework gating forhipaa-sat-1, and no re-stamps.use-training-completionsto call the portal route and handle the new{ data }response; added tests for 401/403, invalid IDs, HIPAA gating, re-stamps, and logging on failed email requests.New Features
POST /v1/training/send-hipaa-completion-email(requirestraining:update); the portal route triggers HIPAA or general completion emails when all videos are done.@db/serverand@db.Written for commit d9c53c3. Summary will update on new commits.