-
Notifications
You must be signed in to change notification settings - Fork 248
Feat/rbac v1 #2092
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Marfuen
wants to merge
30
commits into
main
Choose a base branch
from
feat/rbac-v1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+16,437
−11,405
Open
Feat/rbac v1 #2092
Changes from 19 commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
a9c314b
feat(rbac): extend better-auth permissions with GRC resources
Marfuen 565917e
feat(rbac): add PermissionGuard and @RequirePermission decorator
Marfuen 1521c94
feat(rbac): sync portal permissions with app permissions
Marfuen 68d4cbf
test(rbac): add PermissionGuard unit tests
Marfuen 25ef8cf
feat(rbac): migrate controllers from RequireRoles to RequirePermission
Marfuen 3f8c37c
feat(rbac): add assignment-based filtering for employee/contractor roles
Marfuen 4c2d143
feat(rbac): add department-based policy visibility
Marfuen 3304ee2
feat(auth): centralize auth on API with security hardening
Marfuen b1bd6d4
feat(rbac): add shared auth package and API integration
Marfuen 2234ada
feat(rbac): enable dynamic access control for custom roles
Marfuen 5060265
feat(rbac): add Custom Roles API for dynamic role management
Marfuen 741b7ea
feat(rbac): support multiple roles for privilege escalation checks
Marfuen e490de8
fix(auth): prevent JWKS key regeneration causing session loss
Marfuen 8708bfe
test(rbac): add unit tests for Custom Roles API
Marfuen 882368b
docs: add testing guidelines for API development
Marfuen 27acc06
refactor(docs): move API testing rules to apps/api
Marfuen 1209d32
test(rbac): add privilege escalation test for role updates
Marfuen 81bf6cd
feat(rbac): implement Custom Roles UI (ENG-148)
Marfuen eaf197e
chore(docs): add API endpoints for managing custom roles
Marfuen c59d04f
chore(auth): implement cleanup of stale JWKS records on secret change
Marfuen 3a9e9f1
chore(permissions): implement user permissions resolution and route p…
Marfuen bb323bb
chore(api): migrate to session-based authentication and add controls …
Marfuen 71d8751
refactor(auth): update permission checks to include cookie header
Marfuen 2c2989f
refactor(api): update permission guard logging to include request det…
Marfuen fe67db9
refactor(api): remove unnecessary logging from getPolicy function
Marfuen 09ded08
Merge origin/main into feat/rbac-v1
Marfuen 2d3fa3c
chore(api): handle optional chaining for user ID in various actions
Marfuen 7ef29c3
refactor(app): update policy editor to check version read-only state
Marfuen de2273a
refactor(api): enhance version content handling and validation in pol…
Marfuen c74407b
refactor(app): update PolicyArchiveSheet to use new design system com…
Marfuen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| # API Development Rules | ||
|
|
||
| ## Testing Requirements | ||
|
|
||
| **Every new feature MUST include tests.** This is mandatory, not optional. | ||
|
|
||
| ### When to Write Tests | ||
|
|
||
| 1. **New features**: Every new service, controller, or significant function MUST have accompanying unit tests | ||
| 2. **Bug fixes**: Add a test that reproduces the bug before fixing it | ||
| 3. **Refactoring**: Ensure existing tests pass; add tests if coverage is lacking | ||
|
|
||
| ### When to Run Tests | ||
|
|
||
| Run tests after significant changes: | ||
|
|
||
| ```bash | ||
| # Run tests for a specific module | ||
| npx jest src/<module-name> --passWithNoTests | ||
|
|
||
| # Run all API tests | ||
| npx turbo run test --filter=@comp/api | ||
|
|
||
| # Run tests in watch mode during development | ||
| npx jest --watch | ||
| ``` | ||
|
|
||
| ### Test File Conventions | ||
|
|
||
| - Place test files next to the source file: `foo.service.ts` → `foo.service.spec.ts` | ||
| - Use Jest and NestJS testing utilities | ||
| - Mock external dependencies (database, external APIs) | ||
| - Test both success and error cases | ||
|
|
||
| ### Minimum Test Coverage | ||
|
|
||
| - **Services**: Test all public methods, including error handling | ||
| - **Controllers**: Test endpoint routing and parameter passing | ||
| - **Guards**: Test authorization logic | ||
| - **Utils**: Test edge cases and error conditions | ||
|
|
||
| ### Example Test Structure | ||
|
|
||
| ```typescript | ||
| describe('MyService', () => { | ||
| describe('myMethod', () => { | ||
| it('should handle success case', async () => { ... }); | ||
| it('should throw on invalid input', async () => { ... }); | ||
| it('should handle edge case X', async () => { ... }); | ||
| }); | ||
| }); | ||
| ``` | ||
|
|
||
| ## Development Workflow | ||
|
|
||
| 1. **Before coding**: Read existing code patterns in the module | ||
| 2. **During coding**: Follow established patterns, add types | ||
| 3. **After significant changes**: | ||
| - Run type-check: `npx turbo run typecheck --filter=@comp/api` | ||
| - Run tests: `npx jest src/<module>` | ||
| 4. **Before committing**: Ensure all tests pass | ||
|
|
||
| ## Code Style | ||
|
|
||
| ### Authentication & Authorization | ||
|
|
||
| - Use `@UseGuards(HybridAuthGuard, PermissionGuard)` for protected endpoints | ||
| - Use `@RequirePermission('resource', 'action')` decorator for RBAC | ||
| - Access auth context via `@AuthContext()` decorator | ||
| - Access organization ID via `@OrganizationId()` decorator | ||
|
|
||
| ### Error Handling | ||
|
|
||
| - Use NestJS exceptions: `BadRequestException`, `NotFoundException`, `ForbiddenException` | ||
| - Provide clear, actionable error messages | ||
|
|
||
| ### Database Access | ||
|
|
||
| - Always scope queries by `organizationId` for multi-tenancy | ||
| - Use transactions for operations that modify multiple records |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| # API Development Guidelines | ||
|
|
||
| This document provides guidelines for AI assistants (Claude, Cursor, etc.) when working on the API codebase. | ||
|
|
||
| ## Project Structure | ||
|
|
||
| ``` | ||
| apps/api/src/ | ||
| ├── auth/ # Authentication (better-auth, guards, decorators) | ||
| ├── roles/ # Custom roles CRUD API | ||
| ├── <module>/ # Feature modules (controller, service, DTOs) | ||
| └── utils/ # Shared utilities | ||
| ``` | ||
|
|
||
| ## Testing Requirements | ||
|
|
||
| ### Mandatory Testing | ||
|
|
||
| **Every new feature MUST include tests.** Before marking a task as complete: | ||
|
|
||
| 1. Write unit tests for new services and controllers | ||
| 2. Run the tests to verify they pass | ||
| 3. Commit tests alongside the feature code | ||
|
|
||
| ### Running Tests | ||
|
|
||
| ```bash | ||
| # Run tests for a specific module (from apps/api directory) | ||
| npx jest src/<module-name> --passWithNoTests | ||
|
|
||
| # Run tests for changed files only | ||
| npx jest --onlyChanged | ||
|
|
||
| # Run all API tests (from repo root) | ||
| npx turbo run test --filter=@comp/api | ||
|
|
||
| # Type-check before committing | ||
| npx turbo run typecheck --filter=@comp/api | ||
| ``` | ||
|
|
||
| ### Test Patterns | ||
|
|
||
| Follow existing test patterns in the codebase: | ||
|
|
||
| ```typescript | ||
| // Mock external dependencies | ||
| jest.mock('@trycompai/db', () => ({ | ||
| db: { | ||
| someTable: { | ||
| findFirst: jest.fn(), | ||
| create: jest.fn(), | ||
| // ... | ||
| }, | ||
| }, | ||
| })); | ||
|
|
||
| // Mock ESM modules if needed (e.g., jose) | ||
| jest.mock('jose', () => ({ | ||
| createRemoteJWKSet: jest.fn(), | ||
| jwtVerify: jest.fn(), | ||
| })); | ||
|
|
||
| // Override guards in controller tests | ||
| const module = await Test.createTestingModule({ | ||
| controllers: [MyController], | ||
| providers: [{ provide: MyService, useValue: mockService }], | ||
| }) | ||
| .overrideGuard(HybridAuthGuard) | ||
| .useValue({ canActivate: () => true }) | ||
| .compile(); | ||
| ``` | ||
|
|
||
| ### What to Test | ||
|
|
||
| | Component | Test Coverage | | ||
| |-----------|---------------| | ||
| | Services | All public methods, validation logic, error handling | | ||
| | Controllers | Parameter passing to services, response mapping | | ||
| | Guards | Authorization decisions, edge cases | | ||
| | DTOs | Validation decorators (via e2e or integration tests) | | ||
| | Utils | All functions, edge cases, error conditions | | ||
|
|
||
| ## Code Style | ||
|
|
||
| ### Authentication & Authorization | ||
|
|
||
| - Use `@UseGuards(HybridAuthGuard, PermissionGuard)` for protected endpoints | ||
| - Use `@RequirePermission('resource', 'action')` decorator for RBAC | ||
| - Access auth context via `@AuthContext()` decorator | ||
| - Access organization ID via `@OrganizationId()` decorator | ||
|
|
||
| ### Error Handling | ||
|
|
||
| - Use NestJS exceptions: `BadRequestException`, `NotFoundException`, `ForbiddenException` | ||
| - Provide clear, actionable error messages | ||
| - Don't expose internal details in error responses | ||
|
|
||
| ### Database Access | ||
|
|
||
| - Use Prisma via `@trycompai/db` | ||
| - Always scope queries by `organizationId` for multi-tenancy | ||
| - Use transactions for operations that modify multiple records | ||
|
|
||
| ## Development Workflow | ||
|
|
||
| 1. **Before coding**: Read existing code patterns in the module | ||
| 2. **During coding**: Follow established patterns, add types | ||
| 3. **After coding**: | ||
| - Run `npx turbo run typecheck --filter=@comp/api` | ||
| - Write and run tests: `npx jest src/<module>` | ||
| - Commit with conventional commit message | ||
|
|
||
| ## Common Commands | ||
|
|
||
| ```bash | ||
| # Start API in development | ||
| npx turbo run dev --filter=@comp/api | ||
|
|
||
| # Type-check | ||
| npx turbo run typecheck --filter=@comp/api | ||
|
|
||
| # Run specific test file | ||
| npx jest src/roles/roles.service.spec.ts | ||
|
|
||
| # Generate Prisma client after schema changes | ||
| cd packages/db && npx prisma generate | ||
| ``` | ||
|
|
||
| ## RBAC System | ||
|
|
||
| The API uses a hybrid RBAC system: | ||
|
|
||
| - **Built-in roles**: owner, admin, auditor, employee, contractor | ||
| - **Custom roles**: Stored in `organization_role` table | ||
| - **Permissions**: `resource:action` format (e.g., `control:read`) | ||
| - **Multiple roles**: Users can have multiple roles (comma-separated in `member.role`) | ||
|
|
||
| When checking permissions: | ||
| - Use `@RequirePermission('resource', 'action')` for endpoint protection | ||
| - For privilege escalation checks, combine permissions from all user roles |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,35 @@ | ||
| import { Module } from '@nestjs/common'; | ||
| import { AuthModule as BetterAuthModule } from '@thallesp/nestjs-better-auth'; | ||
| import { auth } from './auth.server'; | ||
| import { ApiKeyGuard } from './api-key.guard'; | ||
| import { ApiKeyService } from './api-key.service'; | ||
| import { HybridAuthGuard } from './hybrid-auth.guard'; | ||
| import { InternalTokenGuard } from './internal-token.guard'; | ||
| import { PermissionGuard } from './permission.guard'; | ||
|
|
||
| @Module({ | ||
| providers: [ApiKeyService, ApiKeyGuard, HybridAuthGuard, InternalTokenGuard], | ||
| exports: [ApiKeyService, ApiKeyGuard, HybridAuthGuard, InternalTokenGuard], | ||
| imports: [ | ||
| // Better Auth NestJS integration - handles /api/auth/* routes | ||
| BetterAuthModule.forRoot({ | ||
| auth, | ||
| // Don't register global auth guard - we use HybridAuthGuard | ||
| disableGlobalAuthGuard: true, | ||
| }), | ||
| ], | ||
| providers: [ | ||
| ApiKeyService, | ||
| ApiKeyGuard, | ||
| HybridAuthGuard, | ||
| InternalTokenGuard, | ||
| PermissionGuard, | ||
| ], | ||
| exports: [ | ||
| ApiKeyService, | ||
| ApiKeyGuard, | ||
| HybridAuthGuard, | ||
| InternalTokenGuard, | ||
| PermissionGuard, | ||
| BetterAuthModule, | ||
| ], | ||
| }) | ||
| export class AuthModule {} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused
MemberIddecorator exportLow Severity
The
MemberIddecorator is exported but never imported or used anywhere in the codebase. Controllers accessmemberIdvia theAuthContextdecorator instead. This is dead code.