[1c] MCP entrypoint + identity resolution#26
Merged
Conversation
- Protect POST /mcp with JwtAuthGuard; missing/invalid/expired tokens
return 401 with structured error
- Add IdentityService that resolves the authenticated user's primary
Department from the UserDepartment join table
- MCPController now extracts userId and organizationId from the validated
JWT (req.user), delegates scope resolution to IdentityService, and
includes { scope: { organizationId, departmentId } } in every response
- Update MCPModule to import AuthModule (JwtStrategy) and PrismaModule
- Add supertest + @types/supertest for integration testing
- Rewrite mcp.controller.spec.ts for the auth-aware controller API
- Update mcp.module.spec.ts with new module dependencies and providers
- Add mcp.integration.spec.ts covering authenticated happy path and all
rejection paths (missing token, invalid token, expired token, wrong secret)
- Update app.module.spec.ts and db-client.mock.ts for the new shape
Co-authored-by: Andrea Mazzucchelli <andrmaz@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds ChangesJWT Auth and Identity Scope for MCP Endpoint
Sequence Diagram(s)sequenceDiagram
participant Client
participant JwtAuthGuard
participant MCPController
participant IdentityService
participant PrismaService
Client->>JwtAuthGuard: POST /mcp { query } + Bearer token
JwtAuthGuard-->>MCPController: req.user (AuthenticatedUser)
MCPController->>IdentityService: resolveScope(userId, organizationId)
IdentityService->>PrismaService: userDepartment.findFirst({ userId, isPrimary: true, department.organizationId })
PrismaService-->>IdentityService: { departmentId } or null
IdentityService-->>MCPController: { organizationId, departmentId | null }
MCPController-->>Client: 201 { scope, context, policy, answer }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4 tasks
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
✅ Unit tests committed locally. Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/api/src/mcp/mcp.controller.ts (1)
8-10: 💤 Low valueConsider adding request body validation.
MCPQueryBodyis a plain interface without validation decorators. If a request arrives with a missing or non-stringquery, it will pass through unvalidated. Consider converting to a class withclass-validatordecorators and applyingValidationPipe:♻️ Optional enhancement
import { IsString, IsNotEmpty } from "class-validator"; export class MCPQueryBody { `@IsString`() `@IsNotEmpty`() query: string; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/mcp/mcp.controller.ts` around lines 8 - 10, The MCPQueryBody interface lacks request body validation, allowing invalid requests to pass through without validation errors. Convert MCPQueryBody from a plain interface to a class and add class-validator decorators to the query property by importing IsString and IsNotEmpty from class-validator, then applying these decorators above the query field definition to ensure the field is a non-empty string at runtime.apps/api/src/mcp/mcp.integration.spec.ts (1)
141-199: ⚡ Quick winAssert the structured 401 response body, not only the status code.
These tests validate rejection status, but they don’t lock the structured error payload contract described in this PR. Add body-shape assertions for at least one or two rejection paths.
Example assertion upgrade
it("returns 401 when Authorization header is missing", async () => { - await request(app.getHttpServer()) + const response = await request(app.getHttpServer()) .post("/mcp") .send({ query: "test" }) .expect(401); + + expect(response.body).toMatchObject({ + statusCode: 401, + }); + expect(typeof response.body.message).toBe("string"); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/mcp/mcp.integration.spec.ts` around lines 141 - 199, The test cases for the POST /mcp endpoint authorization failures (such as "returns 401 when Authorization header is missing", "returns 401 when Bearer token value is invalid", "returns 401 when token is signed with wrong secret", "returns 401 when token is expired", and "returns 401 for a malformed Authorization header (no Bearer prefix)") currently only validate the HTTP status code. Add assertions to verify the structure and content of the 401 response body for at least one or two of these test cases. This should include validating that expected error fields (such as error message, error code, or status) are present in the JSON response to lock in the structured error payload contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/mcp/identity.service.ts`:
- Around line 25-27: The findFirst query on this.prisma.userDepartment does not
include an organization-level constraint in the where clause. First, retrieve
the user's organization ID by querying the user record, then add an additional
filter in the where clause to ensure the returned UserDepartment belongs to the
user's organization (you may need to adjust the field name based on the actual
UserDepartment schema structure, but the goal is to filter by the organization
ID alongside the existing userId and isPrimary filters).
In `@apps/api/src/mcp/mcp.integration.spec.ts`:
- Around line 43-62: In the beforeAll hook, save the original value of
process.env["JWT_SECRET"] before overwriting it with TEST_JWT_SECRET. Then in
the afterAll hook, restore the original value instead of unconditionally
deleting the environment variable. This ensures that if JWT_SECRET was already
set before the test suite ran, it will be restored to its previous state,
preventing cross-suite side effects.
In `@apps/api/src/mcp/mcp.module.spec.ts`:
- Around line 23-30: In the MCP module spec test file, the beforeAll and
afterAll hooks are mutating global process state without preserving the original
state. Save the original value of process.env["JWT_SECRET"] before the beforeAll
hook executes, then in the afterAll hook, restore it to its original value
instead of unconditionally deleting it. This prevents other tests from becoming
flaky if they relied on a pre-existing JWT_SECRET value.
- Around line 45-56: The test "should not throw when creating the module" uses
an incorrect assertion pattern with `.resolves.not.toThrow()`. The `toThrow()`
matcher is designed for functions, not for resolved Promise values, which will
cause a matcher error. Replace `.resolves.not.toThrow()` with
`.resolves.toBeDefined()` to properly assert that the Promise resolves
successfully to a defined module object. Apply the same fix to similar test
patterns in apps/api/src/app.module.spec.ts.
---
Nitpick comments:
In `@apps/api/src/mcp/mcp.controller.ts`:
- Around line 8-10: The MCPQueryBody interface lacks request body validation,
allowing invalid requests to pass through without validation errors. Convert
MCPQueryBody from a plain interface to a class and add class-validator
decorators to the query property by importing IsString and IsNotEmpty from
class-validator, then applying these decorators above the query field definition
to ensure the field is a non-empty string at runtime.
In `@apps/api/src/mcp/mcp.integration.spec.ts`:
- Around line 141-199: The test cases for the POST /mcp endpoint authorization
failures (such as "returns 401 when Authorization header is missing", "returns
401 when Bearer token value is invalid", "returns 401 when token is signed with
wrong secret", "returns 401 when token is expired", and "returns 401 for a
malformed Authorization header (no Bearer prefix)") currently only validate the
HTTP status code. Add assertions to verify the structure and content of the 401
response body for at least one or two of these test cases. This should include
validating that expected error fields (such as error message, error code, or
status) are present in the JSON response to lock in the structured error payload
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b1849fe-8eea-485f-b87a-5631b05d6648
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/api/package.jsonapps/api/src/__mocks__/db-client.mock.tsapps/api/src/app.module.spec.tsapps/api/src/mcp/identity.service.tsapps/api/src/mcp/mcp.controller.spec.tsapps/api/src/mcp/mcp.controller.tsapps/api/src/mcp/mcp.integration.spec.tsapps/api/src/mcp/mcp.module.spec.tsapps/api/src/mcp/mcp.module.ts
Owner
Author
|
@cursoragent Fix and resolve all the conversations |
Contributor
|
All five CodeRabbit conversations have been resolved in a single commit:
All 73 tests pass across 8 suites.   |
- identity.service.ts: add department.organizationId filter to the findFirst query to prevent cross-organization department access - identity.service.spec.ts: update exact toHaveBeenCalledWith assertions to match the new where clause shape; fix makeMockPrisma cast so jest.fn() methods are accessible on the mock without Prisma type interference - mcp.integration.spec.ts: save and restore JWT_SECRET around the suite instead of unconditionally deleting it - mcp.module.spec.ts: save and restore JWT_SECRET; replace .resolves.not.toThrow() with .resolves.toBeDefined() - app.module.spec.ts: replace .resolves.not.toThrow() with .resolves.toBeDefined() Co-authored-by: Andrea Mazzucchelli <andrmaz@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What was built
Stand up the authenticated
/mcpentrypoint. Validated JWT tokens are accepted, identity is resolved from the token + DB, and the response includes org + department scope. Unauthorized and invalid tokens are rejected with structured 401 errors.Changes
New:
IdentityService(apps/api/src/mcp/identity.service.ts)Resolves the authenticated user's identity scope. Reads
organizationIddirectly from the validated JWT claim and looks up the user's Primary Department from theUserDepartmentjoin table. ReturnsnullfordepartmentIdwhen no primary department is configured.Updated:
MCPControllerJwtAuthGuard— unauthenticated requests never reach the handleruserIdandorganizationIdfromreq.user(set by Passport JWT strategy)IdentityServicescope: { organizationId, departmentId }Updated:
MCPModuleImports
AuthModule(providesJwtStrategy) andPrismaModule; providesIdentityService.Updated:
db-client.mock.tsAdded
userDepartment.findFirstmock for test isolation.Tests
mcp.controller.spec.ts— unit tests for the auth-aware controller (17 tests)mcp.module.spec.ts— module compilation and provider wiring (5 tests)mcp.integration.spec.ts— HTTP-level integration tests using supertest (9 tests):departmentIdwhen user has no primary departmentorganizationIdsourced from JWT, not request bodyIdentityServicenot called when auth failsapp.module.spec.ts— updated for newhandleMCPsignatureAcceptance criteria
/mcpand receive a valid stub responseTest results
Summary by CodeRabbit
New Features
Tests
Chores