Enabling PAT backend - #336
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 13 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 review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. 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 (8)
📝 WalkthroughWalkthroughThis PR adds PAT token generation and storage, new PAT create/list/revoke controllers, CLI PAT authentication middleware, user-route endpoints, and a test mock for the new controller module. ChangesPAT Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
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
|
@yash-pouranik |
| const patSchema = new mongoose.Schema({ | ||
| tokenHash: { type: String, required: true }, | ||
| suffix: { type: String, required: true }, | ||
| label: { type: String, required: true }, | ||
| type: { type: String, enum: ['human', 'agent'], required: true }, | ||
| scopes: { type: [String], required: true }, | ||
| lastUsedAt: { type: Date, default: null }, | ||
| lastUsedIp: { type: String, default: null }, | ||
| expiresAt: { type: Date, required: true }, | ||
| createdAt: { type: Date, default: Date.now } | ||
| }); |
There was a problem hiding this comment.
Add TTL to expire automatically, and default select to false.
| if (matchedPat.expiresAt && new Date() > new Date(matchedPat.expiresAt)) { | ||
| try { | ||
| await redis.del(cacheKey); | ||
| } catch (redisErr) { | ||
| console.warn("Redis DEL failed:", redisErr); | ||
| } | ||
| return next(new AppError(401, 'Unauthorized: Token has expired.')); | ||
| } |
There was a problem hiding this comment.
If in model we have added the ttl then we dont have to check the expiry.
| Developer.updateOne( | ||
| { _id: developer._id, 'pats._id': matchedPat._id }, | ||
| { | ||
| $set: { | ||
| 'pats.$.lastUsedAt': new Date(), | ||
| 'pats.$.lastUsedIp': ip | ||
| } | ||
| } | ||
| ).catch(err => console.error("Failed to update PAT metadata:", err)); |
There was a problem hiding this comment.
This query can b queued I think?
| if (developerId) { | ||
| developer = await Developer.findById(developerId); | ||
| if (!developer) { | ||
| try { | ||
| await redis.del(cacheKey); | ||
| } catch (redisErr) { | ||
| console.warn("Redis DEL failed:", redisErr); | ||
| } | ||
| return next(new AppError(401, 'Unauthorized: Developer not found.')); | ||
| } | ||
| matchedPat = developer.pats.find(p => p.tokenHash === tokenHash); | ||
| } else { | ||
| developer = await Developer.findOne({ 'pats.tokenHash': tokenHash }); | ||
| if (!developer) { | ||
| return next(new AppError(401, 'Unauthorized: Invalid or revoked token.')); | ||
| } | ||
| matchedPat = developer.pats.find(p => p.tokenHash === tokenHash); | ||
|
|
||
| // Cache valid token to prevent DB hammering | ||
| try { | ||
| await redis.setex(cacheKey, CACHE_TTL, developer._id.toString()); | ||
| } catch (redisErr) { | ||
| console.warn("Redis SETEX failed:", redisErr); | ||
| } | ||
| } | ||
|
|
||
| if (!matchedPat) { | ||
| try { | ||
| await redis.del(cacheKey); | ||
| } catch (redisErr) { | ||
| console.warn("Redis DEL failed:", redisErr); | ||
| } | ||
| return next(new AppError(401, 'Unauthorized: Invalid token state.')); | ||
| } |
There was a problem hiding this comment.
I feel this is not a true cache hit, since after retrieving the developerId from Redis we still perform a MongoDB lookup via Developer.findById().
Current flow:
Redis Hit > findById() > PAT lookup in document
So while it avoids the expensive findOne({ 'pats.tokenHash': ... }) query, it still hits the database on every authenticated request.
Would it make sense to cache the PAT auth context itself (developerId, patId, scopes, expiry, type, etc.) and invalidate the cache on PAT updates/revocation? That would allow Redis hits to bypass MongoDB entirely and provide a larger performance benefit for frequent CLI requests.
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Fetch all review comments and issue comments on the PR
gh pr view 336 --repo geturbackend/urBackend --json comments,reviews,reviewThreadsLength of output: 1163 🏁 Script executed: #!/bin/bash
# Fetch reviews and comments
gh pr view 336 --repo geturbackend/urBackend --json comments,reviews,latestReviews
echo "---REVIEW COMMENTS---"
# Fetch inline review comments via REST API
gh api repos/geturbackend/urBackend/pulls/336/comments --jq '.[] | {id: .id, user: .user.login, body: .body, path: .path, line: .line, diff_hunk: .diff_hunk}'Length of output: 76777
Comment 1 —
|
| Comment | Valid? | Action |
|---|---|---|
select: false on tokenHash |
✅ Yes | Apply the change |
MongoDB TTL on patSchema |
Use a cleanup job instead | |
| Skip app-level expiry check | ❌ No | Keep the check until cleanup job exists |
Queue lastUsedAt writes |
✅ Yes (future) | Good follow-up task |
| Cache full PAT context | ✅ Yes | Recommended improvement |
|
cool can we create pat as a seperate model that will never touch developer model?? |
|
Tip For best results, initiate chat on the files or code changes.
What GitHub & Stripe DoGitHub stores PATs in a separate table ( Stripe does the same — API keys live in a separate The pattern is universal: any system that issues more than a handful of tokens per user (GitHub, Stripe, GitLab, Vercel, Netlify) uses a separate collection/table. Why a Separate Model is Better Here
The biggest win for your use case: a separate collection fixes the MongoDB TTL problem from the previous comment — you can add Suggested Schema// packages/common/src/models/PAT.js
const patSchema = new mongoose.Schema({
developer: { type: mongoose.Schema.Types.ObjectId, ref: 'Developer', required: true, index: true },
tokenHash: { type: String, required: true, unique: true, select: false },
suffix: { type: String, required: true },
label: { type: String, required: true },
type: { type: String, enum: ['human', 'agent'], default: 'human' },
scopes: [{ type: String }],
expiresAt: { type: Date, required: true, index: true }, // TTL index works here ✅
lastUsedAt: { type: Date, default: null },
lastUsedIp: { type: String, default: null },
createdAt: { type: Date, default: Date.now },
});
// Native MongoDB TTL — auto-deletes expired PATs
patSchema.index({ expiresAt: 1 }, { expireAfterSeconds: 0 });
module.exports = mongoose.model('PAT', patSchema);And // Cache miss path — single indexed query, no Developer doc needed
const pat = await PAT.findOne({ tokenHash: hash }).select('+tokenHash').lean();
if (!pat) return next(new AppError(401, 'Invalid token'));
// Cache full context (fixes Comment 4 from earlier)
await redis.setex(cacheKey, CACHE_TTL, JSON.stringify({
developerId: pat.developer.toString(),
patId: pat._id.toString(),
scopes: pat.scopes,
type: pat.type,
expiresAt: pat.expiresAt,
}));RecommendationSwitch to a separate Would you like me to open a follow-up issue to track this refactor, or kick off the code changes directly? |
|
yes following this way will not have to run a cron just to expire those pats and in middleware we dont have to check for if that really exists or not |
|
Tip For best results, initiate chat on the files or code changes.
Fix 1 — Check
|
|
okay cool |
wow ! thats ... |
|
what happen sir? |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard-api/src/middlewares/authenticateCLI.js (1)
113-114: 🎯 Functional Correctness | 🟠 MajorSet
req.user._idfor CLI-authenticated requests.
Dashboard controllers readreq.user._id, soreq.user = { id: ... }leaves those requests with no usable user id.Suggested fix
- req.user = { id: developer._id.toString() }; // Maintain compatibility with dashboard controllers + req.user = { _id: developer._id.toString() }; // Match dashboard controller expectations🤖 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/middlewares/authenticateCLI.js` around lines 113 - 114, The CLI authentication flow in authenticateCLI currently sets req.user with only an id field, which leaves dashboard controllers unable to read req.user._id. Update the request shaping in authenticateCLI so req.user includes the developer id under _id (while preserving any existing compatibility fields if needed), and keep req.developer assigned to the developer object.
🧹 Nitpick comments (1)
apps/dashboard-api/src/middlewares/authenticateCLI.js (1)
80-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
!matchedPatbranch is unreachable.By this point
matchedPatis always truthy: the cache-hit branch (Lines 40–49) constructs it, and the cache-miss branch returns early at Line 54 when the lookup is empty. This block can be removed to reduce noise.🤖 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/middlewares/authenticateCLI.js` around lines 80 - 87, The !matchedPat branch in authenticateCLI is unreachable because matchedPat is already guaranteed to exist before this point; remove the entire conditional block, including the Redis DEL cleanup and the AppError(401) return, to simplify the control flow. Keep the surrounding authenticateCLI logic and its cache-hit/cache-miss handling intact, and ensure any remaining token-state validation is handled earlier where matchedPat is assigned or where the cache lookup can fail.
🤖 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 `@packages/common/src/index.js`:
- Line 18: The PAT model is required in index.js but not exposed through
module.exports, so consumers destructuring PAT from `@urbackend/common` will get
undefined. Update the exports in packages/common/src/index.js to include PAT
alongside the other shared models, using the existing PAT require symbol so
pat.controller.js and authenticateCLI.js can access
PAT.create/find/findOne/updateOne correctly.
In `@packages/common/src/models/PAT.js`:
- Around line 15-22: The PAT schema defines a duplicate index on expiresAt,
which causes the Mongoose 8 warning because the TTL index already covers it.
Update the PAT model in the patSchema definition to remove the inline index from
the expiresAt field and keep the existing patSchema.index({ expiresAt: 1 }, {
expireAfterSeconds: 0 }) TTL index as the single source of indexing.
---
Outside diff comments:
In `@apps/dashboard-api/src/middlewares/authenticateCLI.js`:
- Around line 113-114: The CLI authentication flow in authenticateCLI currently
sets req.user with only an id field, which leaves dashboard controllers unable
to read req.user._id. Update the request shaping in authenticateCLI so req.user
includes the developer id under _id (while preserving any existing compatibility
fields if needed), and keep req.developer assigned to the developer object.
---
Nitpick comments:
In `@apps/dashboard-api/src/middlewares/authenticateCLI.js`:
- Around line 80-87: The !matchedPat branch in authenticateCLI is unreachable
because matchedPat is already guaranteed to exist before this point; remove the
entire conditional block, including the Redis DEL cleanup and the AppError(401)
return, to simplify the control flow. Keep the surrounding authenticateCLI logic
and its cache-hit/cache-miss handling intact, and ensure any remaining
token-state validation is handled earlier where matchedPat is assigned or where
the cache lookup can fail.
🪄 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: a8021c4b-f700-428e-baf9-ee9a7e996946
📒 Files selected for processing (5)
apps/dashboard-api/src/controllers/pat.controller.jsapps/dashboard-api/src/middlewares/authenticateCLI.jspackages/common/src/index.jspackages/common/src/models/Developer.jspackages/common/src/models/PAT.js
💤 Files with no reviewable changes (1)
- packages/common/src/models/Developer.js
|
@yash-pouranik , |
|
looks goot t ome have a look |
|
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes.
What happened:
If you want, ask me to review the whole PR again, and I’ll trigger a full re-review of (◔_◔) |
|
Please review the new commit on the basis of all the past comments and what we designed earlier> |
|
✅ Action performedFull review finished. |
|
Leave, looks good to me |
|
now UI for this |
|
@yash-pouranik |
yash-pouranik
added
level3
|
isnot GSSOC ended?? |
|
I don't think so |
|
@coderabbitai tell me PAT apis and location |
|
Tip For best results, initiate chat on the files or code changes.
Routes registered there:
Controller location:
Related PAT files:
One note: the exact full API URL depends on where |
3 participants
🚀 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