fix: apply requireAdmin middleware to admin user management routes#79
fix: apply requireAdmin middleware to admin user management routes#79FLASH2332 wants to merge 2 commits into
Conversation
aaronbrethorst
left a comment
There was a problem hiding this comment.
Review: fix: apply requireAdmin middleware to admin user management routes (#79)
Hi Jayadev — this is the kind of fix I love to review: small, surgical, and squarely on target. You found that the /api/v1/admin/users/* routes were guarded by authMiddleware alone, then closed the gap by chaining adminMiddleware exactly the way the /api/v1/admin/vehicles/* and /api/v1/admin/status routes already do. The PR description is excellent too — it names the threat (a driver promoting themselves or deleting admins), the fix, and the resulting behavior. That's a model writeup for a security change.
Critical Issues (0 found)
- Merge conflict: please fix.
Important Issues (0 found)
None.
Suggestions (1 found)
- Route wiring isn't covered by a regression test.
requireAdminitself is well-tested (TestRequireAdmin_DriverDenied,TestRequireAdmin_MissingClaims, expired/invalid tokens), but nothing asserts that the routes inmain.goare actually wrapped with it — which is precisely the bug this PR fixes. A table-driven integration test over the assembled mux that confirms each/api/v1/admin/*route returns403for a driver-role JWT would stop this class of mistake from recurring. This is a pre-existing gap shared with the vehicles/status routes, so it's out of scope here — I've filed it as #85 to track. [main.go:91-95]
Strengths
- Follows the established pattern exactly —
authMiddleware(adminMiddleware(...))matches the vehicles and status routes line for line, so the codebase stays consistent. [main.go:78-82] - Correct ordering:
requireAuthruns first to populate claims on the context, thenrequireAdminreadsrolefrom those claims. Getting that backwards would have broken the chain. [auth.go:118-143] - Tight scope — five lines, one file, no collateral changes. Easy to verify and easy to trust.
make testpasses,go vet ./...is clean, andgofmtreports nothing.
Recommended Action
Fix the merge conflict and then I can merge it! The follow-up test work is tracked in #85.
|
Linter diff in the way? Review this PR in Change Stack to focus on meaningful changes and expand context only when needed. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR tightens authorization on admin user management endpoints by adding ChangesAdmin User Management Authorization
Possibly related issues
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5-10 minutes 🚥 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 |
Thanks for the review @aaronbrethorst . Closes #82 |

Problem
The
/api/v1/admin/users/*routes only validated JWT signature and expiry viarequireAuth,but did not check the caller's role. Any authenticated driver could create, delete, and modify
all user accounts—including promoting themselves to admin or deleting other admins.
Solution
Applied the existing
requireAdminmiddleware to all five admin user management routes:GET /api/v1/admin/usersGET /api/v1/admin/users/{id}POST /api/v1/admin/usersPUT /api/v1/admin/users/{id}DELETE /api/v1/admin/users/{id}Routes now use
authMiddleware(adminMiddleware(...))to verify both JWT validity and admin role.Changes
adminMiddlewareSecurity Impact
403 Forbiddeninstead of succeeding/api/v1/admin/status,/api/v1/admin/vehicles/*)Closes #75
Summary by CodeRabbit