feat(webapp): admin can delete users from /admin#3463
feat(webapp): admin can delete users from /admin#3463
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an admin user-deletion flow: documentation for a server-side delete endpoint; a new DeleteUserDialog React component requiring the exact Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Additional context
🚥 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 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 |
| const targetId = deleteAttempt.data.id; | ||
|
|
||
| if (targetId === admin.id) { | ||
| return typedjson( | ||
| { error: "You can't delete your own account from the admin UI." }, | ||
| { status: 400 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Missing server-side guard prevents deletion of other admin users
The UI hides the Delete button for admin users via !user.admin at apps/webapp/app/routes/admin._index.tsx:229, but the server-side action handler only prevents self-deletion (targetId === admin.id at line 71). There is no server-side check verifying the target user is not an admin. A crafted POST request with intent=delete&id=<other-admin-id> bypasses the UI restriction and deletes another admin account. Authorization checks must be enforced server-side to be effective.
Prompt for agents
In the delete action handler in apps/webapp/app/routes/admin._index.tsx, after the self-deletion check (targetId === admin.id), add a server-side check that loads the target user from the database and verifies they are not an admin before proceeding with deletion. This mirrors the client-side guard at line 229 that hides the Delete button when user.admin is true. Without this, a crafted POST request can delete any admin user. You could use prisma to find the target user and check their admin flag, returning a 403/400 error if the target is an admin.
Was this helpful? React with 👍 or 👎 to provide feedback.
CI typecheck is expected to fail until the cloud PR ships. This PR consumes BillingClient.deleteUser and DeleteUserResponse, both of which are added in the pair PR's release of @trigger.dev/platform@1.0.28. The webapp is currently pinned to 1.0.27 (the latest published version), which doesn't yet have those exports.
Summary
/admin, with atype-to-confirm modal and a server-side guard against self-delete.
The action proxies to the platform service, which performs the
deletion server-side.
PersonalAccessToken.userIdandMfaBackupCode.userIdswitch from default
RestricttoonDelete: Cascadeso user deletionsucceeds at the FK level. All other User relations were already correct.
UserDeletionAuditLogtable records each admin deletion(denormalized, no FKs to User so the row survives).
Test plan
pnpm run db:migrate)/adminshows Delete next to Impersonate; the button does notappear on the signed-in admin's own row
delete <email>exactly to enable/adminwithintent=delete+ your own user id returns 400/admin?deleted=1with a successbanner; an audit row is written to
UserDeletionAuditLog(idempotent)