Enabling pat backend clean#336
Open
Ayush4958 wants to merge 4 commits into
Open
Conversation
Ayush4958
requested review from
Nitin-kumar-yadav1307 and
yash-pouranik
as code owners
Contributor
|
Warning Review limit reached
More reviews will be available in 25 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces a full Personal Access Token (PAT) system. It adds token generation utilities ( ChangesPAT Feature
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Client
participant AuthMW as authenticateCLI
participant Redis as Redis
participant DB as Developer (MongoDB)
participant Controller as PAT Controller
rect rgba(173, 216, 230, 0.5)
Note over CLI, Controller: PAT Creation
CLI->>Controller: POST /pats (Bearer dashboard JWT)
Controller->>DB: find Developer, push patSchema entry
DB-->>Controller: saved developer
Controller-->>CLI: { rawToken, suffix, ... } (once only)
end
rect rgba(144, 238, 144, 0.5)
Note over CLI, Controller: CLI Request with PAT
CLI->>AuthMW: GET /... Authorization: Bearer ubpat_live_*
AuthMW->>AuthMW: SHA-256 hash token
AuthMW->>Redis: GET cli:pat:cache:{hash}
Redis-->>AuthMW: developerId or null
AuthMW->>DB: findById, match PAT by tokenHash
DB-->>AuthMW: Developer + PAT entry
AuthMW->>DB: async update lastUsedAt/Ip
AuthMW->>Controller: next() with req.developer, req.cliScopes
end
rect rgba(255, 182, 193, 0.5)
Note over CLI, Controller: PAT Revocation
CLI->>Controller: DELETE /pats/:id
Controller->>DB: pull PAT, save developer
Controller->>Redis: DEL cli:pat:cache:{tokenHash}
Controller-->>CLI: { success: true }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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: 7
🧹 Nitpick comments (1)
apps/dashboard-api/src/__tests__/routes.user.test.js (1)
45-50: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd route-wiring assertions for the three new PAT endpoints.
This file mocks PAT handlers but doesn’t verify
POST/GET/DELETE /api/user/pats*wiring yet. Adding 3 lightweight route tests would close that regression gap.Example test additions
+const patController = require('../controllers/pat.controller'); + +test('POST /api/user/pats is wired to authenticated createPAT', async () => { + const res = await request(app).post('/api/user/pats').send({ label: 'cli', scopes: ['read'] }); + expect(res.status).toBe(200); + expect(authMiddleware).toHaveBeenCalledTimes(1); + expect(patController.createPAT).toHaveBeenCalledTimes(1); +}); + +test('GET /api/user/pats is wired to authenticated listPATs', async () => { + const res = await request(app).get('/api/user/pats'); + expect(res.status).toBe(200); + expect(authMiddleware).toHaveBeenCalledTimes(1); + expect(patController.listPATs).toHaveBeenCalledTimes(1); +}); + +test('DELETE /api/user/pats/:id is wired to authenticated revokePAT', async () => { + const res = await request(app).delete('/api/user/pats/mock_pat_id'); + expect(res.status).toBe(200); + expect(authMiddleware).toHaveBeenCalledTimes(1); + expect(patController.revokePAT).toHaveBeenCalledTimes(1); +});🤖 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/dashboard-api/src/__tests__/routes.user.test.js` around lines 45 - 50, Add three test cases after the mock definitions to verify route wiring for the PAT endpoints. Create tests that make HTTP requests to POST /api/user/pats (should trigger the mocked createPAT function), GET /api/user/pats (should trigger the mocked listPATs function), and DELETE /api/user/pats/:id (should trigger the mocked revokePAT function). Each test should verify that the corresponding mocked handler function was called with the request and response objects to ensure the routes are properly wired to their controller handlers.
🤖 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/dashboard-api/src/controllers/pat.controller.js`:
- Around line 96-98: The PAT revocation persists to the database first, then
attempts Redis cache eviction using redis.del with the key
`cli:pat:cache:${patToRevoke.tokenHash}`. Since the database revoke is the
critical operation and the Redis deletion is just a cache optimization, handle
any Redis errors separately by logging them without failing the entire response.
Modify the error handling so that if the Redis deletion fails, the client still
receives a success response because the actual revocation already succeeded in
the database, rather than returning a 500 error that misrepresents the
operation's status.
- Around line 15-16: The TTL validation logic in the pat.controller.js file has
a flaw where the default value assignment using `const days = Number(ttlDays) ||
30;` occurs before validation, causing invalid input like ttlDays=0 to be
silently replaced with 30 instead of being rejected. Separate the defaulting and
validation logic by first checking if ttlDays is provided and valid (between 1
and 365), then applying the default value of 30 only when ttlDays is undefined,
null, or empty, ensuring the bounds check on line 16 properly catches all
invalid inputs before they become the final days value.
- Around line 37-38: Replace all instances of req.user.id with req.user._id in
the pat.controller.js file. The production JWT tokens use the _id field (as
defined in auth.controller.js), not id, so the three occurrences of req.user.id
at the affected lines (used in database query operations with $push, $inc, and
other update operators) must be changed to req.user._id to ensure proper
database lookups in production. This fix is necessary because the test suite
masks this issue by using _id in mock objects, but real JWT tokens will only
contain the _id field.
In `@apps/dashboard-api/src/middlewares/authenticateCLI.js`:
- Around line 28-48: The Redis cache operations (redis.get, redis.del, and
redis.setex) are awaited synchronously and can block authentication if Redis is
slow or unavailable. Wrap each Redis operation (the redis.get call at the
beginning, the redis.del call when developer is not found in cache, and the
redis.setex call when caching valid tokens) in separate try-catch blocks so that
Redis failures are non-blocking. When a Redis operation fails, log a warning but
allow the authentication flow to continue by falling through to database
validation. This ensures that authentication proceeds with database lookups even
when Redis is unavailable, making the cache truly best-effort rather than a
blocker for valid PAT authentication.
- Around line 50-52: In the authenticateCLI middleware where the matchedPat
validation check occurs, when matchedPat is falsy (indicating a revoked or
rotated token), the code should clear the stale cache entry before returning the
401 error. Modify the if (!matchedPat) block to delete the cached developerId
entry from the cache before calling next(new AppError(401, ...)) to prevent
repeated cache hits on subsequently invalid tokens. This allows the cache to
self-heal when a developer's token has been revoked or rotated.
- Line 2: The CACHE_TTL constant in the authenticateCLI.js file is being
assigned directly from the process.env.CLI_PAT_CACHE_TTL environment variable,
which is a string type. Since this value is passed to redis.setex() which
expects an integer for the TTL parameter, parse the environment variable to an
integer using parseInt() or Number() before assignment, maintaining the fallback
default of 300 if the environment variable is not set or is not a valid number.
In `@packages/common/src/models/Developer.js`:
- Around line 121-123: The pats field in the Developer model lacks an index on
the nested tokenHash field, causing collection scans when the auth middleware
queries on pats.tokenHash. Add a compound index on the pats.tokenHash field in
the Developer schema definition to optimize lookups. You can do this by adding
an index option to the pats field definition that creates an index specifically
for the tokenHash nested property within the pats array.
---
Nitpick comments:
In `@apps/dashboard-api/src/__tests__/routes.user.test.js`:
- Around line 45-50: Add three test cases after the mock definitions to verify
route wiring for the PAT endpoints. Create tests that make HTTP requests to POST
/api/user/pats (should trigger the mocked createPAT function), GET
/api/user/pats (should trigger the mocked listPATs function), and DELETE
/api/user/pats/:id (should trigger the mocked revokePAT function). Each test
should verify that the corresponding mocked handler function was called with the
request and response objects to ensure the routes are properly wired to their
controller handlers.
🪄 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: 2e0e56c9-c40c-40bd-9ac4-cec416adca00
📒 Files selected for processing (7)
apps/dashboard-api/src/__tests__/routes.user.test.jsapps/dashboard-api/src/controllers/pat.controller.jsapps/dashboard-api/src/middlewares/authenticateCLI.jsapps/dashboard-api/src/routes/user.jspackages/common/src/index.jspackages/common/src/models/Developer.jspackages/common/src/utils/token.utils.js
Contributor
Author
|
@yash-pouranik |
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
1 participant
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.
🚀 Pull Request Description
I had enabled PAT for project, Now CLI/AI Agents can easily authenticate against Dashboard API.
Added the PAT generator which adds prefix to generated PAT.
Also included PAT schema in developer model.
Added strict PAT validation middleware with Redis caching
🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.✅ Checklist
Summary by CodeRabbit
New Features
Tests