From bd865719a26ab031b37095194795ea5ea39ff5cd Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Tue, 2 Jun 2026 11:02:46 -0500 Subject: [PATCH] refactor(auth): add withAdmin() wrapper for admin API routes. Extract shared 401/403 gate into permission-middleware and sweep the four inline admin checks (import MTG/Pokemon, sync-catalog, card-submissions). Co-authored-by: Cursor --- .cursor/rules/api-routes.mdc | 2 +- .cursor/rules/auth-and-permissions.mdc | 2 +- lib/permission-middleware.js | 17 +++++++++ pages/api/admin/card-submissions.js | 15 ++------ pages/api/admin/sync-catalog.js | 14 ++------ pages/api/cards/import-mtg.js | 14 ++------ pages/api/cards/import-pokemon.js | 14 ++------ test/lib/permission-middleware.test.js | 49 +++++++++++++++++++++++++- 8 files changed, 79 insertions(+), 48 deletions(-) diff --git a/.cursor/rules/api-routes.mdc b/.cursor/rules/api-routes.mdc index 3b99133..0e7edc0 100644 --- a/.cursor/rules/api-routes.mdc +++ b/.cursor/rules/api-routes.mdc @@ -145,7 +145,7 @@ export default async function handler(req, res) { 1. **Method check first.** Reject the wrong verb with 405 before doing any limiter work. 2. **Auth check before any user-keyed limiter.** `extractUserIdentifier(userId)` THROWS when `userId` is null/undefined/empty (defensive). For `upload`, `generate`, `import`, and `scan`, the handler MUST call `getUserFromRequest(req)` (or equivalent JWT verification) and confirm a non-null user BEFORE calling the limiter. Wrong order = anonymous user bypasses (the THROW surfaces immediately during dev; do not catch and silently fall back to IP). 3. **For IP-keyed limiters (`auth`, `search`), gate placement is flexible** — either at the top of the handler (after the method check) or after a separate auth check that the route happens to also have (e.g. `users/search` JWT-verifies before rate-limiting, both are correct). The limiter only needs `req` for IP extraction. -4. **Admin-role check, if applicable, goes between auth and rate-limit.** Used by both `/api/cards/import-*` routes: `if (user.role !== 'admin') return res.status(403).json({ error: 'Admin access required' })` sits between the `if (!user)` 401 and the import rate-limit call. +4. **Admin-role check, if applicable, goes between auth and rate-limit.** Import and admin routes use `withAdmin(handler)` from `lib/permission-middleware.js` (401/403 before the inner handler runs); per-user rate-limit calls sit inside the wrapped handler after auth. **Identifier extraction:** diff --git a/.cursor/rules/auth-and-permissions.mdc b/.cursor/rules/auth-and-permissions.mdc index 98f7bbf..b63d18d 100644 --- a/.cursor/rules/auth-and-permissions.mdc +++ b/.cursor/rules/auth-and-permissions.mdc @@ -69,4 +69,4 @@ From there: - **Owner-only** (delete, settings): inside the handler, `if (user.userId !== resource.user_id) return res.status(403)`. - **Editor-or-owner**: use `withCollectionPermission('editor')`. - **Public read**: use `withCollectionPermission('viewer')` — handles `is_public` and explicit-permission case. -- **Admin-only**: check `user.role === 'admin'` directly; consider extracting `withAdmin()` if a third call site appears. +- **Admin-only**: use `withAdmin(handler)` from `lib/permission-middleware.js`; inner handler receives `(req, res, user)` after 401/403 gates. diff --git a/lib/permission-middleware.js b/lib/permission-middleware.js index e505d43..e8502b9 100644 --- a/lib/permission-middleware.js +++ b/lib/permission-middleware.js @@ -105,6 +105,23 @@ function checkRolePermission(userRole, requiredPermission) { return userLevel >= requiredLevel; } +/** + * Wrap an API handler with authenticated admin gate. + * Inner handler receives (req, res, user) after 401/403 checks pass. + */ +export function withAdmin(handler) { + return async function adminHandler(req, res) { + const user = await getUserFromRequest(req); + if (!user) { + return res.status(401).json({ error: 'Authentication required' }); + } + if (user.role !== 'admin') { + return res.status(403).json({ error: 'Admin access required' }); + } + return handler(req, res, user); + }; +} + /** * Middleware to protect collection routes */ diff --git a/pages/api/admin/card-submissions.js b/pages/api/admin/card-submissions.js index 1cd2b86..cf977ad 100644 --- a/pages/api/admin/card-submissions.js +++ b/pages/api/admin/card-submissions.js @@ -1,17 +1,8 @@ import { sql } from '@vercel/postgres'; -import { getUserFromRequest } from '../../../lib/permission-middleware'; +import { withAdmin } from '../../../lib/permission-middleware'; -export default async function handler(req, res) { +export default withAdmin(async function handler(req, res, user) { try { - const user = await getUserFromRequest(req); - if (!user) { - return res.status(401).json({ error: 'Authentication required' }); - } - - if (user.role !== 'admin') { - return res.status(403).json({ error: 'Admin access required' }); - } - if (req.method === 'GET') { const { status = 'pending' } = req.query; const result = await sql` @@ -101,4 +92,4 @@ export default async function handler(req, res) { console.error('[admin/card-submissions]', error); return res.status(500).json({ error: 'Internal server error' }); } -} +}); diff --git a/pages/api/admin/sync-catalog.js b/pages/api/admin/sync-catalog.js index e246178..d74b855 100644 --- a/pages/api/admin/sync-catalog.js +++ b/pages/api/admin/sync-catalog.js @@ -1,20 +1,12 @@ -import { getUserFromRequest } from '../../../lib/permission-middleware'; +import { withAdmin } from '../../../lib/permission-middleware'; import { checkImportRateLimit } from '../../../lib/rate-limit.js'; import { runCatalogSync } from '../../../lib/card-import/sync-catalog.js'; -export default async function handler(req, res) { +export default withAdmin(async function handler(req, res, user) { if (req.method !== 'POST') { return res.status(405).json({ error: 'Method not allowed' }); } - const user = await getUserFromRequest(req); - if (!user) { - return res.status(401).json({ error: 'Authentication required' }); - } - if (user.role !== 'admin') { - return res.status(403).json({ error: 'Admin access required' }); - } - const { allowed, reset } = await checkImportRateLimit(req, user.userId); if (!allowed) { res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000)); @@ -28,4 +20,4 @@ export default async function handler(req, res) { console.error('[POST /api/admin/sync-catalog]', error); return res.status(500).json({ error: 'Catalog sync failed', details: error.message }); } -} +}); diff --git a/pages/api/cards/import-mtg.js b/pages/api/cards/import-mtg.js index 7ae165e..6e6554c 100644 --- a/pages/api/cards/import-mtg.js +++ b/pages/api/cards/import-mtg.js @@ -1,20 +1,12 @@ -import { getUserFromRequest } from '../../../lib/permission-middleware'; +import { withAdmin } from '../../../lib/permission-middleware'; import { checkImportRateLimit } from '../../../lib/rate-limit.js'; import { importMtgSet } from '../../../lib/card-import/mtg.js'; -export default async function handler(req, res) { +export default withAdmin(async function handler(req, res, user) { if (req.method !== 'POST') { return res.status(405).json({ error: 'Method not allowed' }); } - const user = await getUserFromRequest(req); - if (!user) { - return res.status(401).json({ error: 'Authentication required' }); - } - if (user.role !== 'admin') { - return res.status(403).json({ error: 'Admin access required' }); - } - const { allowed, reset } = await checkImportRateLimit(req, user.userId); if (!allowed) { res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000)); @@ -42,4 +34,4 @@ export default async function handler(req, res) { details: error.message, }); } -} +}); diff --git a/pages/api/cards/import-pokemon.js b/pages/api/cards/import-pokemon.js index dab4199..83a3775 100644 --- a/pages/api/cards/import-pokemon.js +++ b/pages/api/cards/import-pokemon.js @@ -1,20 +1,12 @@ -import { getUserFromRequest } from '../../../lib/permission-middleware'; +import { withAdmin } from '../../../lib/permission-middleware'; import { checkImportRateLimit } from '../../../lib/rate-limit.js'; import { importPokemonSet } from '../../../lib/card-import/pokemon.js'; -export default async function handler(req, res) { +export default withAdmin(async function handler(req, res, user) { if (req.method !== 'POST') { return res.status(405).json({ error: 'Method not allowed' }); } - const user = await getUserFromRequest(req); - if (!user) { - return res.status(401).json({ error: 'Authentication required' }); - } - if (user.role !== 'admin') { - return res.status(403).json({ error: 'Admin access required' }); - } - const { allowed, reset } = await checkImportRateLimit(req, user.userId); if (!allowed) { res.setHeader('Retry-After', Math.ceil((reset - Date.now()) / 1000)); @@ -46,4 +38,4 @@ export default async function handler(req, res) { details: error.message, }); } -} +}); diff --git a/test/lib/permission-middleware.test.js b/test/lib/permission-middleware.test.js index 86584fa..ab0d859 100644 --- a/test/lib/permission-middleware.test.js +++ b/test/lib/permission-middleware.test.js @@ -5,7 +5,7 @@ vi.mock('@vercel/postgres', () => ({ sql: vi.fn() })); import { sql } from '@vercel/postgres'; import { JWT_SECRET } from '../../lib/auth-secret.js'; -import { getUserFromRequest } from '../../lib/permission-middleware.js'; +import { getUserFromRequest, withAdmin } from '../../lib/permission-middleware.js'; function makeToken(payload, opts = {}) { return jwt.sign(payload, JWT_SECRET, { expiresIn: opts.expiresIn ?? '1h' }); @@ -97,3 +97,50 @@ describe('getUserFromRequest', () => { expect(user).toBeNull(); }); }); + +describe('withAdmin', () => { + beforeEach(() => { + sql.mockReset(); + sql.mockResolvedValue({ rows: [] }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + + it('returns 401 when unauthenticated', async () => { + const inner = vi.fn(); + const res = { status: vi.fn().mockReturnThis(), json: vi.fn() }; + await withAdmin(inner)({ headers: {} }, res); + expect(res.status).toHaveBeenCalledWith(401); + expect(inner).not.toHaveBeenCalled(); + }); + + it('returns 403 when user is not admin', async () => { + sql.mockResolvedValueOnce({ + rows: [{ id: 2, email: 'alice@deckhearth.com', role: 'user' }], + }); + const token = makeToken({ userId: 2, email: 'alice@deckhearth.com' }); + const inner = vi.fn(); + const res = { status: vi.fn().mockReturnThis(), json: vi.fn() }; + await withAdmin(inner)( + { headers: { authorization: `Bearer ${token}` } }, + res + ); + expect(res.status).toHaveBeenCalledWith(403); + expect(inner).not.toHaveBeenCalled(); + }); + + it('calls inner handler with admin user', async () => { + sql.mockResolvedValueOnce({ + rows: [{ id: 1, email: 'admin@deckhearth.com', role: 'admin' }], + }); + const token = makeToken({ userId: 1, email: 'admin@deckhearth.com' }); + const inner = vi.fn().mockResolvedValue(undefined); + const req = { headers: { authorization: `Bearer ${token}` } }; + const res = {}; + await withAdmin(inner)(req, res); + expect(inner).toHaveBeenCalledWith(req, res, { + userId: 1, + email: 'admin@deckhearth.com', + role: 'admin', + }); + }); +});