feat: enhance account update logic with admin checks and authentication#388
Open
pradipthaadhi wants to merge 3 commits intotestfrom
Open
Conversation
- Added authentication validation to the PATCH /api/accounts endpoint using validateAuthContext. - Implemented logic to restrict accountId overrides to admin users only. - Updated validateUpdateAccountBody to make accountId optional. - Introduced unit tests for various scenarios including authentication errors, account updates, and admin checks.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughPATCH /api/accounts now delegates request handling to a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Route as "app/api/accounts/route.ts"
participant Handler as "patchAccountHandler"
participant Auth as "validateAuthContext"
participant Admin as "checkIsAdmin"
participant Update as "updateAccountHandler"
Client->>Route: PATCH /api/accounts
Route->>Handler: patchAccountHandler(req)
Handler->>Auth: validateAuthContext(req)
Auth-->>Handler: authResult { accountId, ... }
Handler->>Handler: safeParseJson(req) & validateUpdateAccountBody
Handler->>Handler: resolve targetAccountId (body.accountId || authResult.accountId)
alt override provided && target != authResult.accountId
Handler->>Admin: checkIsAdmin(authResult.accountId)
Admin-->>Handler: isAdmin?
alt not admin
Handler-->>Client: 403 Forbidden + CORS headers
else is admin
Handler->>Update: updateAccountHandler(validatedData + targetAccountId)
Update-->>Client: 200/Success
end
else same account
Handler->>Update: updateAccountHandler(validatedData + targetAccountId)
Update-->>Client: 200/Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/api/accounts/route.ts (1)
61-69: Consider wrappingcheckIsAdminin error handling for resilience.The authorization logic is sound—the admin check correctly guards
accountIdoverrides. However,checkIsAdmincallsvalidateOrganizationAccesswhich may throw on database errors (seelib/admins/checkIsAdmin.ts:12-15). An unhandled exception here would result in a 500 rather than a graceful error response.For consistency with the structured error responses elsewhere in this handler, consider:
♻️ Proposed resilient error handling
if (validated.accountId && validated.accountId !== authResult.accountId) { - const isAdmin = await checkIsAdmin(authResult.accountId); - if (!isAdmin) { + let isAdmin = false; + try { + isAdmin = await checkIsAdmin(authResult.accountId); + } catch { + return NextResponse.json( + { status: "error", error: "Unable to verify admin status" }, + { status: 500, headers: getCorsHeaders() }, + ); + } + if (!isAdmin) { return NextResponse.json( { status: "error", error: "accountId override is only allowed for admin accounts" }, { status: 403, headers: getCorsHeaders() }, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/accounts/route.ts` around lines 61 - 69, Wrap the call to checkIsAdmin used in the accountId override guard in a try/catch to handle possible exceptions from validateOrganizationAccess; specifically around the existing call in the if (validated.accountId && validated.accountId !== authResult.accountId) block, catch errors from checkIsAdmin(authResult.accountId) and return a structured NextResponse.json error (use the same shape as other handler errors and include getCorsHeaders()) with an appropriate 500 status and a generic message like "failed to verify admin status" while avoiding leaking internal details; ensure the catch path logs the error if logging exists and preserves the existing non-admin 403 response when checkIsAdmin resolves to false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/api/accounts/route.ts`:
- Around line 61-69: Wrap the call to checkIsAdmin used in the accountId
override guard in a try/catch to handle possible exceptions from
validateOrganizationAccess; specifically around the existing call in the if
(validated.accountId && validated.accountId !== authResult.accountId) block,
catch errors from checkIsAdmin(authResult.accountId) and return a structured
NextResponse.json error (use the same shape as other handler errors and include
getCorsHeaders()) with an appropriate 500 status and a generic message like
"failed to verify admin status" while avoiding leaking internal details; ensure
the catch path logs the error if logging exists and preserves the existing
non-admin 403 response when checkIsAdmin resolves to false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0e3603d-fae9-4dfa-b4b0-7f74addc3334
⛔ Files ignored due to path filters (1)
app/api/accounts/__tests__/route.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**
📒 Files selected for processing (2)
app/api/accounts/route.tslib/accounts/validateUpdateAccountBody.ts
- Updated the updateAccountBodySchema to include a refinement that enforces at least one update field to be provided in the request body. - Added unit tests for validateUpdateAccountBody to ensure proper validation behavior, including cases with no fields and valid updates.
sweetmantech
reviewed
Apr 2, 2026
| { | ||
| message: "At least one update field is required", | ||
| path: ["body"], | ||
| }, |
Contributor
There was a problem hiding this comment.
Suggestion: This chained || check works but is fragile — if a new field gets added to the schema, it's easy to forget to add it here too.
A more maintainable approach:
.refine(
body => {
const { accountId, ...updateFields } = body;
return Object.values(updateFields).some(v => v !== undefined);
},
{
message: "At least one update field is required",
path: ["body"],
},
)This destructures out accountId (not an update field) and dynamically checks the rest. Any new fields added to the schema are automatically covered.
sweetmantech
reviewed
Apr 2, 2026
app/api/accounts/route.ts
Outdated
Comment on lines
48
to
75
| const authResult = await validateAuthContext(req); | ||
| if (authResult instanceof NextResponse) { | ||
| return authResult; | ||
| } | ||
|
|
||
| const body = await safeParseJson(req); | ||
|
|
||
| const validated = validateUpdateAccountBody(body); | ||
| if (validated instanceof NextResponse) { | ||
| return validated; | ||
| } | ||
|
|
||
| return updateAccountHandler(validated as UpdateAccountBody); | ||
| const targetAccountId = validated.accountId ?? authResult.accountId; | ||
| if (validated.accountId && validated.accountId !== authResult.accountId) { | ||
| const isAdmin = await checkIsAdmin(authResult.accountId); | ||
| if (!isAdmin) { | ||
| return NextResponse.json( | ||
| { status: "error", error: "accountId override is only allowed for admin accounts" }, | ||
| { status: 403, headers: getCorsHeaders() }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return updateAccountHandler({ | ||
| ...(validated as UpdateAccountBody), | ||
| accountId: targetAccountId, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
SRP - handler function.
…dler - Replaced the previous update logic with a dedicated patchAccountHandler for handling account updates. - Updated validation to ensure at least one profile field is provided, with optional accountId for admin overrides. - Refactored tests to cover new patchAccountHandler functionality and validation scenarios. - Added unit tests for patchAccountHandler to verify authentication, admin checks, and validation errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/accounts/patchAccountHandler.ts`:
- Around line 31-45: The code currently lets validated.accountId become the
write target (targetAccountId) and passes it to updateAccountHandler,
reintroducing a client-controlled account selector; instead, always derive the
account to update from authResult.accountId when calling updateAccountHandler
(do not use validated.accountId as the accountId argument), keep the
checkIsAdmin block only for deciding whether to reject non-admins but do not use
it to allow body-based overrides, and if cross-account admin updates are
required implement them via a separate privileged route or server-only param
rather than using validated.accountId; update the use of
targetAccountId/validated.accountId/authResult.accountId and ensure
updateAccountHandler receives authResult.accountId.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71951922-f18e-47dc-951d-8426af9ae12d
⛔ Files ignored due to path filters (3)
app/api/accounts/__tests__/route.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included byapp/**lib/accounts/__tests__/patchAccountHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/accounts/__tests__/validateUpdateAccountBody.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (3)
app/api/accounts/route.tslib/accounts/patchAccountHandler.tslib/accounts/validateUpdateAccountBody.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/api/accounts/route.ts
- lib/accounts/validateUpdateAccountBody.ts
Comment on lines
+31
to
+45
| const targetAccountId = validated.accountId ?? authResult.accountId; | ||
| if (validated.accountId && validated.accountId !== authResult.accountId) { | ||
| const isAdmin = await checkIsAdmin(authResult.accountId); | ||
| if (!isAdmin) { | ||
| return NextResponse.json( | ||
| { status: "error", error: "accountId override is only allowed for admin accounts" }, | ||
| { status: 403, headers: getCorsHeaders() }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return updateAccountHandler({ | ||
| ...(validated as UpdateAccountBody), | ||
| accountId: targetAccountId, | ||
| }); |
There was a problem hiding this comment.
Keep the target account derived from auth, not the PATCH body.
validated.accountId still becomes the authoritative write target here, and updateAccountHandler uses that accountId for all downstream reads and writes. Even with the admin gate, this reintroduces a public body parameter for account selection on /api/accounts; if admins need cross-account updates, move that to a separate privileged route or a server-only parameter instead.
Based on learnings: "Never use account_id in request bodies or tool schemas; always derive the account ID from authentication".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/accounts/patchAccountHandler.ts` around lines 31 - 45, The code currently
lets validated.accountId become the write target (targetAccountId) and passes it
to updateAccountHandler, reintroducing a client-controlled account selector;
instead, always derive the account to update from authResult.accountId when
calling updateAccountHandler (do not use validated.accountId as the accountId
argument), keep the checkIsAdmin block only for deciding whether to reject
non-admins but do not use it to allow body-based overrides, and if cross-account
admin updates are required implement them via a separate privileged route or
server-only param rather than using validated.accountId; update the use of
targetAccountId/validated.accountId/authResult.accountId and ensure
updateAccountHandler receives authResult.accountId.
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.
Summary by CodeRabbit
Bug Fixes
New Features
Validation