Conversation
- Update permissions.ts to extend defaultStatements from better-auth - Add GRC resources: control, evidence, policy, risk, vendor, task, framework, audit, finding, questionnaire, integration - Add program_manager role with full GRC access but no member management - Update owner/admin roles to extend ownerAc/adminAc from better-auth - Update auditor role with read + export permissions - Keep employee/contractor roles minimal with assignment-based access - Add ROLE_HIERARCHY, RESTRICTED_ROLES, PRIVILEGED_ROLES exports - Add placeholder for dynamicAccessControl in auth.ts (Sprint 2) Part of ENG-138: Complete Permission System Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create PermissionGuard that calls better-auth's hasPermission API - Add fallback role-based check when better-auth is unavailable - Create @RequirePermission decorator for route-level permission checks - Create @RequirePermissions decorator for multi-resource permissions - Export GRCResource and GRCAction types for type safety - Add program_manager to Role enum in database schema - Update AuthModule to export PermissionGuard The guard: - Validates permissions via better-auth's hasPermission endpoint - Falls back to role-based check if API unavailable - Logs warnings for API key bypass (TODO: add API key scopes) - Provides static isRestrictedRole() helper for assignment filtering Part of ENG-138: Complete Permission System Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update portal permissions.ts to match app version - Fix security issue where employee/contractor had excessive permissions - Add program_manager role to portal - Extend defaultStatements from better-auth - Add RESTRICTED_ROLES and PRIVILEGED_ROLES exports BREAKING CHANGE: Employee and contractor roles in portal now have restricted permissions matching the app. Previously they had member management and organization update permissions. Part of ENG-138: Complete Permission System Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add comprehensive tests for PermissionGuard covering: - Permission bypass when no permissions required - API key bypass behavior - Role-based access for privileged vs restricted roles - Fallback behavior when better-auth API unavailable - isRestrictedRole static method for all role types Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Migrate all API controllers to use the new better-auth permission system: - findings.controller.ts: finding create/update/delete permissions - task-management.controller.ts: task CRUD + assign permissions - people.controller.ts: member delete permission for removeHost - evidence-export.controller.ts: evidence export permission Also fix TypeScript errors in permission.guard.spec.ts for fetch mocking. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement assignment filtering to restrict employees/contractors to only see resources they are assigned to: - Add memberId to AuthContext for assignment checking - Create assignment-filter utility with filter builders and access checkers - Update tasks controller/service with assignment filtering on GET endpoints - Update risks controller/service with assignment filtering on GET endpoints - Add PermissionGuard and @RequirePermission to tasks and risks endpoints Employees/contractors now only see: - Tasks where they are the assignee - Risks where they are the assignee Privileged roles (owner, admin, program_manager, auditor) see all resources. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Allow admins to control which departments can see specific policies: Schema changes: - Add PolicyVisibility enum (ALL, DEPARTMENT) - Add visibility and visibleToDepartments fields to Policy model API changes: - Add memberDepartment to AuthContext for visibility filtering - Create department-visibility utility with filter builders - Update policies controller to filter by visibility for restricted roles - Update policies service to accept visibility filter Policies can now be: - Visible to ALL (default) - everyone in the organization sees them - Visible to specific DEPARTMENTS only - only members in those departments see them Privileged roles (owner, admin, program_manager, auditor) see all policies regardless of visibility settings. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Move auth server to API, app now uses proxy to forward auth requests - Remove localStorage token storage (XSS prevention) - Add rate limiting to auth proxy (60/min general, 10/min sensitive) - Add redirect URL validation to prevent open redirects - Add AUTH_SECRET validation at startup - Make all debug logging conditional on NODE_ENV - Simplify root page routing (no activeOrganizationId dependency) - Use URL-based RBAC with direct DB member lookup Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add @comp/auth package with centralized permissions and role definitions - Update API auth module to integrate with better-auth server - Add 403 responses to policy and risk endpoints for Swagger - Add assignment filter and department visibility utilities with tests - Sync permissions across app and portal - Update tsconfig and nest-cli for proper module resolution Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add dynamicAccessControl config to organization plugin - Add OrganizationRole table for storing custom roles - Configure maximum 20 roles per organization - Add schema mapping for better-auth role table Resolves: ENG-145 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add roles module with CRUD endpoints for custom roles - Implement privilege escalation prevention - Add permission validation against valid resources/actions - Protect built-in roles (owner, admin, auditor, employee, contractor) - Add OrganizationRole table migration - Limit to 20 custom roles per organization - Require ac:create/read/update/delete permissions for role management Implements: ENG-146 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update roles service to accept array of roles instead of single role - Add getCombinedPermissions to merge permissions from all user roles - Update controller to pass full userRoles array - Users with multiple roles now get combined permissions for validation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add explicit jwks configuration with rotationInterval to prevent better-auth from creating new JWKS keys on each request. Without this, all existing JWTs become invalid when the API restarts because new signing keys are generated. - Set rotationInterval to 30 days for monthly key rotation - Set gracePeriod to 7 days so old keys remain valid after rotation Fixes: Session persistence across API restarts References: - better-auth/better-auth#6215 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add 18 tests for RolesService covering CRUD operations - Add 9 tests for RolesController - Test permission validation and privilege escalation prevention - Test multiple roles support for privilege checking - Test edge cases (duplicate names, max roles limit, reserved names) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update .cursorrules with testing requirements and conventions - Add apps/api/CLAUDE.md with API-specific development guidelines - Document when to write tests, how to run them, and test patterns - Include RBAC system documentation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove API-specific testing rules from root .cursorrules - Create apps/api/.cursorrules with API testing requirements - Keep root .cursorrules focused on commit message conventions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Ensures that users cannot escalate privileges when updating role permissions, not just when creating roles. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add roles settings pages (list, create, edit) with permission matrix - Add "Select all" feature to quickly set all permissions - Integrate custom roles into member management UI: - Role filter dropdown shows all roles dynamically - Invite modal supports custom role selection - Edit member role supports custom roles - Allow normal spelling for role names (spaces, capitalization) - Add loading skeletons with proper PageLayout wrappers - Add comprehensive tests for RolesTable, RoleForm, PermissionMatrix Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR SummaryHigh Risk Overview Introduces automatic audit logging for API activity via a new Extends authentication and API-key handling: adds API key creation/revocation and optional Written by Cursor Bugbot for commit 582714d. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
apps/api/src/auth/auth.server.ts
Outdated
| apiKey: ['create', 'read', 'delete'], | ||
| app: ['read'], | ||
| trust: ['read', 'update'], | ||
| } as const; |
There was a problem hiding this comment.
Cloud-security resource missing from access control statement
High Severity
The cloud-security controller uses @RequirePermission('cloud-security', 'read') (and 'update', 'create', 'delete'), but the statement object in auth.server.ts does not define cloud-security as a resource. Since no built-in role (owner, admin, auditor, etc.) includes cloud-security permissions, auth.api.hasPermission will always return false for session-based users. This effectively blocks all regular users from accessing any cloud-security endpoint — only platform admins (who bypass permission checks) and legacy API keys (empty scopes) will have access.
Additional Locations (1)
| }, | ||
| }), | ||
| db.member.findMany({ | ||
| where: { userId, isActive: true }, |
There was a problem hiding this comment.
Inconsistent member filtering: isActive vs deactivated fields
Medium Severity
The getMe endpoint in auth.controller.ts filters memberships using isActive: true, while handleSessionAuth in hybrid-auth.guard.ts filters using deactivated: false. Since the Member model has both fields independently, they can get out of sync — a member with isActive: false, deactivated: false would pass the auth guard but not appear in the getMe organization list, creating a confusing state where an authenticated user sees no organizations.
Additional Locations (1)
| // PATCH /v1/policies/:id with isArchived field | ||
| if (method === 'PATCH' && requestBody && 'isArchived' in requestBody) { | ||
| return requestBody.isArchived ? 'Archived policy' : 'Restored policy'; | ||
| } |
There was a problem hiding this comment.
Archive detection not scoped to policy URLs
Medium Severity
extractPolicyActionDescription checks for isArchived in the request body without any URL validation, unlike its sibling check for /regenerate which at least matches the path. Because AuditLogInterceptor is a global APP_INTERCEPTOR, any PATCH request on any resource that includes isArchived in its body would get a hardcoded "Archived policy" / "Restored policy" description and have its change tracking suppressed (changes = null). The comment says "PATCH /v1/policies/:id" but the code doesn't enforce the path.
Additional Locations (1)
| // App access resources | ||
| app: ['read'], // Main app access | ||
| trust: ['read', 'update'], // Trust center access | ||
| } as const; |
There was a problem hiding this comment.
Missing resources in permission statement blocks user access
High Severity
The cloud-security and training resources are used in @RequirePermission decorators on their respective controllers, but neither is defined in the statement in packages/auth/src/permissions.ts. Since no role (owner, admin, auditor, etc.) includes these resources, auth.api.hasPermission will deny all session-authenticated users. Only platform admins (who bypass permission checks) can access these endpoints. Service tokens for these resources also reference cloud-security:update and training:read/training:update in service-token.config.ts, which work via simple string matching, but regular users are entirely locked out.
Additional Locations (2)
| // PATCH /v1/policies/:id with isArchived field | ||
| if (method === 'PATCH' && requestBody && 'isArchived' in requestBody) { | ||
| return requestBody.isArchived ? 'Archived policy' : 'Restored policy'; | ||
| } |
There was a problem hiding this comment.
Audit log archive description matches all resources incorrectly
Medium Severity
extractPolicyActionDescription checks for isArchived in the request body on ANY PATCH request, regardless of the URL path. If a non-policy resource (e.g., vendor, risk) is updated with isArchived, the audit log will incorrectly say "Archived policy" or "Restored policy." Additionally, the interceptor sets changes = null when policyActionDesc is truthy, so the actual field changes are suppressed from the audit log entirely.
Additional Locations (1)
| const mentionedUsers = await db.user.findMany({ | ||
| where: { | ||
| id: { in: mentionedUserIds }, | ||
| isPlatformAdmin: false, |
There was a problem hiding this comment.
Platform admin mention filter more restrictive than intended
Low Severity
Adding isPlatformAdmin: false to the mentioned users query silently excludes all platform admin users from receiving comment mention notifications, even those who are org owners. The isUserUnsubscribed function already correctly handles this case by allowing platform admin owners to receive notifications. The pattern in finding-notifier.service.ts uses an OR clause (isPlatformAdmin: false OR role contains 'owner') to preserve this behavior, but this filter doesn't follow that pattern.
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Audit log hardcodes "policy" for all regenerate endpoints
Medium Severity
extractPolicyActionDescription is called for every resource but hardcodes "policy" in its return values. The /regenerate regex matches the tasks controller endpoint at POST /v1/tasks/:taskId/regenerate, causing audit logs to say "Regenerated policy" instead of "Regenerated task". Similarly, the isArchived check matches any PATCH request body containing that field, not just policies. The resource variable is available in the interceptor but isn't passed to this function, so it can't produce resource-appropriate descriptions.
Additional Locations (1)
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Control mapping audit hardcodes "policy" for all resources
Low Severity
buildRelationMappingChanges uses a generic regex (/\/v1\/\w+\/[^/]+\/controls/) that matches any resource's control sub-route, but the descriptions are hardcoded to "Mapped controls to policy" and "Unmapped control from policy". Additionally, fetchControlIds always queries db.policy.findUnique, so it would fetch from the wrong table if a non-policy resource has a controls sub-route.
Additional Locations (1)
| })), | ||
| pendingInvitation, | ||
| }; | ||
| } |
There was a problem hiding this comment.
New getMe endpoint unreachable for users without organization
High Severity
The new GET /v1/auth/me endpoint returns user info, organization memberships, and pending invitations — but it's behind HybridAuthGuard which unconditionally requires activeOrganizationId in the session. The session creation hook in auth.server.ts explicitly handles users with no org by returning the session without activeOrganizationId. These users then get a 401 from the guard and can never reach getMe, blocking onboarding and invitation acceptance flows.
Additional Locations (1)
| TrainingModule, | ||
| OrgChartModule, | ||
| EvidenceFormsModule, | ||
| RolesModule, |
There was a problem hiding this comment.
AuditModule not imported, interceptor never activates
High Severity
The newly created AuditModule registers AuditLogInterceptor as an APP_INTERCEPTOR, but it is never imported into AppModule. Without the import, the interceptor is never instantiated by NestJS, so no audit logs will be written for any API mutations. All the audit logging infrastructure (interceptor, utils, resolvers, constants, tests) is dead code until the module is added to AppModule.imports.
Additional Locations (1)
| // ============================================================ | ||
|
|
||
| @Get('providers') | ||
| @RequirePermission('cloud-security', 'read') |
There was a problem hiding this comment.
Missing resources block session auth on endpoints
High Severity
The cloud-security and training resources are used in @RequirePermission decorators but are not defined in the statement in @comp/auth. Since no role (including owner and admin) includes these resources, auth.api.hasPermission will deny access for all session-authenticated users. Only service tokens and legacy API keys (which bypass the SDK check) can reach these endpoints. Interactive users will get 403 on all cloud security and training routes.
Additional Locations (1)
| const mentionedUsers = await db.user.findMany({ | ||
| where: { | ||
| id: { in: mentionedUserIds }, | ||
| isPlatformAdmin: false, |
There was a problem hiding this comment.
Platform admins silently excluded from mention notifications
Medium Severity
The newly added isPlatformAdmin: false filter in the mentionedUsers query silently excludes platform admins from receiving comment mention notifications. A platform admin can also be a regular member of an organization and can be explicitly @mentioned in a comment. With this filter, the mention is silently dropped — the mentioning user gets no indication the notification wasn't sent, and the admin never sees it.
| }, | ||
| }), | ||
| db.member.findMany({ | ||
| where: { userId, isActive: true }, |
There was a problem hiding this comment.
Inconsistent member activity filter in getMe endpoint
Low Severity
The new getMe endpoint filters members with isActive: true, while HybridAuthGuard uses deactivated: false. The Member model has both fields, and the rest of the codebase consistently uses deactivated: false for active-member checks. Although deactivation currently sets both fields in sync, using a different filter than the guard could produce inconsistent results if they diverge — a user could see orgs in getMe that the guard won't authenticate them for, or vice versa.
| TrainingModule, | ||
| OrgChartModule, | ||
| EvidenceFormsModule, | ||
| RolesModule, |
There was a problem hiding this comment.
AuditModule never imported, interceptor is inactive
High Severity
AuditModule is defined and registers AuditLogInterceptor as an APP_INTERCEPTOR, but it's never imported in app.module.ts. This means the audit logging interceptor won't be instantiated, and all audit logging for mutations (the entire audit system built in this PR) will be silently inactive.
Additional Locations (1)
| // PATCH /v1/policies/:id with isArchived field | ||
| if (method === 'PATCH' && requestBody && 'isArchived' in requestBody) { | ||
| return requestBody.isArchived ? 'Archived policy' : 'Restored policy'; | ||
| } |
There was a problem hiding this comment.
Archive check matches all resources, not just policies
Medium Severity
extractPolicyActionDescription checks for isArchived in the request body without verifying the URL path contains /policies/. Any PATCH request from any resource (vendor, risk, etc.) that includes isArchived would incorrectly produce "Archived policy" or "Restored policy" in the audit log and suppress actual field change logging (since policyActionDesc being truthy causes changes = null).
| app: AuditLogEntityType.organization, | ||
| questionnaire: AuditLogEntityType.organization, | ||
| audit: null, | ||
| }; |
There was a problem hiding this comment.
Missing trust key in entity type mapping
Low Severity
RESOURCE_TO_ENTITY_TYPE maps portal to AuditLogEntityType.trust, but the permission system uses trust as the resource name (e.g., @RequirePermission('trust', 'read')). Since there's no trust key in the map, audit logs for trust portal endpoints will have null entity type. The same mismatch exists in RESOURCE_TO_PRISMA_MODEL.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 6 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| TrainingModule, | ||
| OrgChartModule, | ||
| EvidenceFormsModule, | ||
| RolesModule, |
There was a problem hiding this comment.
AuditModule not imported in AppModule
High Severity
AuditModule registers AuditLogInterceptor as APP_INTERCEPTOR but is never imported in AppModule.imports, so the interceptor never runs and no audit logs are created for any API mutations. The entire audit logging feature for RBAC v1 is non-functional.
Additional Locations (1)
| // App access resources | ||
| app: ['read'], // Main app access | ||
| trust: ['read', 'update'], // Trust center access | ||
| } as const; |
There was a problem hiding this comment.
Missing cloud-security resource in permission statement
High Severity
The cloud-security resource is used in @RequirePermission decorators and service token permissions but not defined in the permission statement. This causes PermissionGuard to fail permission checks for all cloud security endpoints because better-auth's hasPermission API doesn't recognize cloud-security as a valid resource.
Additional Locations (1)
| // App access resources | ||
| app: ['read'], // Main app access | ||
| trust: ['read', 'update'], // Trust center access | ||
| } as const; |
There was a problem hiding this comment.
Missing training resource in permission statement
High Severity
The training resource is used in @RequirePermission decorators and the Portal service token permissions but not defined in the permission statement. Permission checks for training endpoints will fail when better-auth's hasPermission API rejects the undefined resource.
Additional Locations (1)
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Comment updates create incorrect audit logs
Medium Severity
The extractCommentContext function handles POST and DELETE for comments but not PUT. When comments are updated via PUT /comments/:commentId, the audit log incorrectly records "Updated task" with entityId set to the comment ID instead of the parent entity (policy/vendor/risk/task) that the comment belongs to, creating misleading audit trails.
Additional Locations (1)
| encryptedCredentials[key] = value | ||
| .filter(Boolean) | ||
| .map((item) => this.encrypt(item)); | ||
| } |
There was a problem hiding this comment.
Array credentials not validated before encryption
Medium Severity
The connectLegacy method encrypts array credential values without validating that items are strings. The encrypt method expects a string parameter, but array items are only filtered for truthiness. If non-string values like numbers or objects are passed in credential arrays (e.g., regions: [123, 456]), cipher.update() throws a TypeError when attempting to encrypt them.
Additional Locations (1)
| app: AuditLogEntityType.organization, | ||
| questionnaire: AuditLogEntityType.organization, | ||
| audit: null, | ||
| }; |
There was a problem hiding this comment.
Missing entity types for audited resources
Low Severity
The RESOURCE_TO_ENTITY_TYPE map is missing entries for cloud-security and training resources that are used in @RequirePermission decorators. Audit logs for these endpoints will have entityType: null, making it difficult to filter and query audit history for cloud security and training operations.
Additional Locations (1)
1 participant
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist
- I haven't read the contributing guide
- My code doesn't follow the style guidelines of this project
- I haven't commented my code, particularly in hard-to-understand areas
- I haven't checked if my changes generate no new warnings
Cursor Bugbot reviewed your changes and found no issues for commit c74407b