CS-472 Failed to mark video as completed - #3455
Conversation
|
@cubic-dev-ai please review it. |
@chasprowebdev I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
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/api/src/roles/roles.service.ts">
<violation number="1" location="apps/api/src/roles/roles.service.ts:448">
P2: PATCH requests with a nullable optional field now throw a server error in normalization. Reject `null` before deriving effective values (or require non-null DTO fields) so invalid payloads return `BadRequestException`.</violation>
<violation number="2" location="apps/api/src/roles/roles.service.ts:459">
P1: An obligations-only role update can grant `portal:read/update` without checking the caller holds those permissions. Validate `permissionsToPersist` after deriving it, including when only obligations changed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai please review it |
@chasprowebdev I have started the AI code review. It will take a few minutes to complete. |
|
🎉 This PR is included in version 3.107.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
* fix(training): remove rbac gate from mark-complete endpoint
## 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 ✅.
* fix(todo): address cubic review
Address review findings.
* fix(todo): address cubic review
Address review findings.
* fix: address cubic review
Address review findings.
* fix(portal): log training completion-email failures in production (CS-774)
The completion-email failure path logged via the `logger` util, which no-ops
outside development — so in production a failed completion email was swallowed
silently (negating the response.ok check). Use console.error for this
operational error so it is visible in prod. Test now asserts console.error.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
# [3.111.0](v3.110.1...v3.111.0) (2026-07-29) ### Bug Fixes * **api:** classify the take-over method on an unclear outcome too ([539fdba](539fdba)) * **api:** only classify a switchable passkey when a code method exists ([#3528](#3528)) ([0d79093](0d79093)) * **app:** make a half-finished connect resumable, not a forced full-screen step ([#3525](#3525)) ([b1afcae](b1afcae)) * **cloud-security:** add missing logGroupName to CreateLogGroup remediation ([#3515](#3515)) ([76ae56c](76ae56c)) * harden sign-in classification + align take-over messaging ([#3527](#3527)) ([914cb1e](914cb1e)) * **integrations:** wrap aws add account form in dialog and scroll into view ([#3526](#3526)) ([07e91ae](07e91ae)), closes [#418](#418) * make the 2FA take-over universal — tailor guidance to what the page asks for ([#3520](#3520)) ([b12f3fe](b12f3fe)) * **policies:** remove archived policies from framework controls after unlinking ([#3513](#3513)) ([d639df2](d639df2)) * **training:** defer completion email and share canonical training IDs ([#3529](#3529)) ([fecb556](fecb556)) * **training:** remove rbac gate from mark-complete endpoint ([#3501](#3501)) ([8c5e98f](8c5e98f)), closes [#3455](#3455) ### Features * connection longevity + one-click 'Make permanent' 2FA ([#3524](#3524)) ([7345e3c](7345e3c)) * **policies:** add bulk upload for policy migration ([#3514](#3514)) ([1940f06](1940f06)) * **security-questionnaire:** add browser extension ([#3064](#3064)) ([e678421](e678421))
This is an automated pull request to merge chas/mark-video-as-custom-role into dev.
It was created by the [Auto Pull Request] action.
Summary by cubic
Enabling the Employee Compliance obligation now auto-adds
portal:read/updatein both the app and API, fixing CS-472 where users couldn’t mark training videos as completed. Applies to new roles and updates, including roles created via API or outside the UI.portalon create/update (covers obligations-only and permissions-only), validate privilege escalation against the final merged permissions, avoid dup actions, and never auto-strip explicitportalwhen compliance is off.portalinPermissionMatrix; disabling it removesportal, and unrelated permission edits don’t touch obligations.portalwithout compliance, dedupedportal, obligations/permissions merges (incl. no-op updates), and privilege-escalation rejections.Written for commit bca28f7. Summary will update on new commits.