diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index bd895ab3..2491c035 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -26,6 +26,8 @@ env: DB_PASSWORD: test-pw DB_NAME: wxyc_db AUTH_BYPASS: true + AUTH_USERNAME: test_dj1 + AUTH_PASSWORD: testpassword123 jobs: # Detect what changed to conditionally run jobs diff --git a/apps/backend/controllers/djs.controller.ts b/apps/backend/controllers/djs.controller.ts index 1e160cfd..bc6583be 100644 --- a/apps/backend/controllers/djs.controller.ts +++ b/apps/backend/controllers/djs.controller.ts @@ -10,12 +10,12 @@ export type binBody = { }; export const addToBin: RequestHandler = async (req, res, next) => { - if (req.body.album_id === undefined || req.body.dj_id === undefined) { + if (req.body.album_id === undefined) { console.error('Bad Request, Missing Album Identifier: album_id'); - res.status(400).send('Bad Request, Missing DJ or album identifier: album_id'); + res.status(400).send('Bad Request, Missing album identifier: album_id'); } else { const bin_entry: NewBinEntry = { - dj_id: req.body.dj_id, + dj_id: req.auth!.id!, album_id: req.body.album_id, track_title: req.body.track_title === undefined ? null : req.body.track_title, }; @@ -38,13 +38,12 @@ export type binQuery = { }; export const deleteFromBin: RequestHandler = async (req, res, next) => { - if (req.query.album_id === undefined || req.query.dj_id === undefined) { - console.error('Bad Request, Missing Bin Entry Identifier: album_id or dj_id'); - res.status(400).send('Bad Request, Missing Bin Entry Identifier: album_id or dj_id'); + if (req.query.album_id === undefined) { + console.error('Bad Request, Missing Bin Entry Identifier: album_id'); + res.status(400).send('Bad Request, Missing Bin Entry Identifier: album_id'); } else { try { - //check that the dj_id === dj_id of bin entry - const removed_bin_item = await DJService.removeFromBin(parseInt(req.query.album_id), req.query.dj_id); + const removed_bin_item = await DJService.removeFromBin(parseInt(req.query.album_id), req.auth!.id!); res.status(200).json(removed_bin_item); } catch (e) { console.error(e); @@ -53,19 +52,14 @@ export const deleteFromBin: RequestHandler = } }; -export const getBin: RequestHandler = async (req, res, next) => { - if (req.query.dj_id === undefined) { - console.error('Bad Request, Missing DJ Identifier: dj_id'); - res.status(400).send('Bad Request, Missing DJ Identifier: dj_id'); - } else { - try { - const dj_bin = await DJService.getBinFromDB(req.query.dj_id); - res.status(200).json(dj_bin); - } catch (e) { - console.error("Error: Failed to retrieve dj's bin"); - console.error(e); - next(e); - } +export const getBin: RequestHandler = async (req, res, next) => { + try { + const dj_bin = await DJService.getBinFromDB(req.auth!.id!); + res.status(200).json(dj_bin); + } catch (e) { + console.error("Error: Failed to retrieve dj's bin"); + console.error(e); + next(e); } }; diff --git a/shared/authentication/src/auth.middleware.ts b/shared/authentication/src/auth.middleware.ts index b7c7eb8f..a9270670 100644 --- a/shared/authentication/src/auth.middleware.ts +++ b/shared/authentication/src/auth.middleware.ts @@ -1,5 +1,5 @@ import { NextFunction, Request, Response } from 'express'; -import { createRemoteJWKSet, jwtVerify, type JWTPayload } from 'jose'; +import { createRemoteJWKSet, decodeJwt, jwtVerify, type JWTPayload } from 'jose'; import { AccessControlStatement, WXYCRole, WXYCRoles, normalizeRole } from './auth.roles'; // JWT payload structure expected from better-auth JWT plugin @@ -53,6 +53,26 @@ export type RequiredPermissions = { export function requirePermissions(required: RequiredPermissions) { return async (req: Request, res: Response, next: NextFunction) => { if (process.env.AUTH_BYPASS === 'true') { + // In bypass mode, try to decode the JWT (without verification) so that + // req.auth is populated for controllers that rely on req.auth.id. + // If the token is not a valid JWT (e.g. integration tests pass a raw + // user ID), fall back to using the token value as the user ID directly. + const authHeader = req.headers.authorization; + if (authHeader) { + const token = authHeader.startsWith('Bearer ') ? authHeader.slice(7).trim() : authHeader.trim(); + if (token) { + try { + const payload = decodeJwt(token) as WXYCAuthJwtPayload; + const userId = payload.id || payload.sub; + if (userId) { + req.auth = { ...payload, id: userId } as WXYCAuthJwtPayload; + } + } catch { + // Token is not a valid JWT -- treat it as a raw user ID. + req.auth = { id: token } as WXYCAuthJwtPayload; + } + } + } return next(); } diff --git a/tests/integration/djs.spec.js b/tests/integration/djs.spec.js index 46e0cc33..2f5eaa61 100644 --- a/tests/integration/djs.spec.js +++ b/tests/integration/djs.spec.js @@ -6,9 +6,9 @@ const { createAuthRequest, expectErrorContains, expectFields, expectArray } = re * DJ Endpoints Integration Tests * * Tests for: - * - GET /djs/bin - Retrieve DJ's bin - * - POST /djs/bin - Add entry to bin - * - DELETE /djs/bin - Remove entry from bin + * - GET /djs/bin - Retrieve authenticated DJ's bin + * - POST /djs/bin - Add entry to authenticated DJ's bin + * - DELETE /djs/bin - Remove entry from authenticated DJ's bin * - GET /djs/playlists - Get playlists for a DJ */ @@ -20,20 +20,8 @@ describe('DJ Bin', () => { }); describe('GET /djs/bin', () => { - test('returns array for DJ', async () => { - const res = await auth.get('/djs/bin').query({ dj_id: global.primary_dj_id }).expect(200); - - expectArray(res); - }); - - test('returns 400 when dj_id is missing', async () => { - const res = await auth.get('/djs/bin').expect(400); - - expectErrorContains(res, 'Missing DJ Identifier'); - }); - - test('returns empty array for DJ with no bin entries', async () => { - const res = await auth.get('/djs/bin').query({ dj_id: global.secondary_dj_id }).expect(200); + test('returns array for authenticated DJ', async () => { + const res = await auth.get('/djs/bin').expect(200); expectArray(res); }); @@ -42,28 +30,25 @@ describe('DJ Bin', () => { describe('POST /djs/bin', () => { afterEach(async () => { // Clean up any bin entries created during tests - await auth.delete('/djs/bin').query({ dj_id: global.primary_dj_id, album_id: 1 }); + await auth.delete('/djs/bin').query({ album_id: 1 }); }); test('adds entry to bin successfully', async () => { const res = await auth .post('/djs/bin') .send({ - dj_id: global.primary_dj_id, album_id: 1, }) .expect(200); expectFields(res.body, 'album_id', 'dj_id'); expect(res.body.album_id).toBe(1); - expect(res.body.dj_id).toBe(global.primary_dj_id); }); test('adds entry with track title to bin', async () => { const res = await auth .post('/djs/bin') .send({ - dj_id: global.primary_dj_id, album_id: 1, track_title: 'Carry the Zero', }) @@ -75,23 +60,7 @@ describe('DJ Bin', () => { }); test('returns 400 when album_id is missing', async () => { - const res = await auth - .post('/djs/bin') - .send({ - dj_id: global.primary_dj_id, - }) - .expect(400); - - expectErrorContains(res, 'Missing'); - }); - - test('returns 400 when dj_id is missing', async () => { - const res = await auth - .post('/djs/bin') - .send({ - album_id: 1, - }) - .expect(400); + const res = await auth.post('/djs/bin').send({}).expect(400); expectErrorContains(res, 'Missing'); }); @@ -101,31 +70,24 @@ describe('DJ Bin', () => { beforeEach(async () => { // Set up a bin entry to delete await auth.post('/djs/bin').send({ - dj_id: global.primary_dj_id, album_id: 2, }); }); test('removes entry from bin successfully', async () => { - const res = await auth.delete('/djs/bin').query({ dj_id: global.primary_dj_id, album_id: 2 }).expect(200); + const res = await auth.delete('/djs/bin').query({ album_id: 2 }).expect(200); expect(res.body).toBeDefined(); // Verify it was removed - const binRes = await auth.get('/djs/bin').query({ dj_id: global.primary_dj_id }).expect(200); + const binRes = await auth.get('/djs/bin').expect(200); const entry = binRes.body.find((e) => e.album_id === 2); expect(entry).toBeUndefined(); }); test('returns 400 when album_id is missing', async () => { - const res = await auth.delete('/djs/bin').query({ dj_id: global.primary_dj_id }).expect(400); - - expectErrorContains(res, 'Missing'); - }); - - test('returns 400 when dj_id is missing', async () => { - const res = await auth.delete('/djs/bin').query({ album_id: 2 }).expect(400); + const res = await auth.delete('/djs/bin').expect(400); expectErrorContains(res, 'Missing'); }); diff --git a/tests/mocks/database.mock.ts b/tests/mocks/database.mock.ts index 64a7e216..7e9bfb03 100644 --- a/tests/mocks/database.mock.ts +++ b/tests/mocks/database.mock.ts @@ -86,6 +86,7 @@ export const flowsheet = { message: 'message', add_time: 'add_time', }; +export const bins = {}; export const shows = {}; export const show_djs = {}; export const user = {}; @@ -131,3 +132,11 @@ export type NewFSEntry = Partial; export type Show = Record; export type ShowDJ = Record; export type User = Record; + +export type BinEntry = { + id: number; + dj_id: string; + album_id: number; + track_title: string | null; +}; +export type NewBinEntry = Omit; diff --git a/tests/setup/integration.setup.js b/tests/setup/integration.setup.js index d8b95c95..20029f03 100644 --- a/tests/setup/integration.setup.js +++ b/tests/setup/integration.setup.js @@ -49,10 +49,5 @@ async function getUserIdsFromDatabase() { // Note: Service readiness is checked in globalSetup.js (runs once for all tests) beforeAll(async () => { await getUserIdsFromDatabase(); - - if (process.env.AUTH_BYPASS === 'true') { - global.access_token = 'Auth Bypass Enabled'; - } else { - global.access_token = await get_access_token(); - } + global.access_token = await get_access_token(); }); diff --git a/tests/unit/authentication/auth.middleware.test.ts b/tests/unit/authentication/auth.middleware.test.ts index dd4b6857..ece38180 100644 --- a/tests/unit/authentication/auth.middleware.test.ts +++ b/tests/unit/authentication/auth.middleware.test.ts @@ -6,17 +6,19 @@ process.env.BETTER_AUTH_ISSUER = 'https://test.example.com'; process.env.BETTER_AUTH_AUDIENCE = 'https://test.example.com'; // Mock jose -- the middleware calls createRemoteJWKSet at module scope -// and jwtVerify on each request +// and jwtVerify on each request. decodeJwt is used in AUTH_BYPASS mode. jest.mock('jose', () => ({ createRemoteJWKSet: jest.fn(() => jest.fn()), jwtVerify: jest.fn(), + decodeJwt: jest.fn(), })); -import { jwtVerify } from 'jose'; +import { jwtVerify, decodeJwt } from 'jose'; import { requirePermissions } from '../../../shared/authentication/src/auth.middleware'; import type { Request, Response, NextFunction } from 'express'; const mockedJwtVerify = jwtVerify as jest.MockedFunction; +const mockedDecodeJwt = decodeJwt as jest.MockedFunction; function createMocks(authHeader?: string) { const req = { @@ -62,8 +64,11 @@ describe('requirePermissions middleware', () => { }); describe('AUTH_BYPASS', () => { - it('should skip all validation when AUTH_BYPASS is "true"', async () => { + beforeEach(() => { process.env.AUTH_BYPASS = 'true'; + }); + + it('should skip all validation when AUTH_BYPASS is "true"', async () => { const { req, res, next } = createMocks(); const middleware = requirePermissions({ catalog: ['read'] }); @@ -72,6 +77,35 @@ describe('requirePermissions middleware', () => { expect(next).toHaveBeenCalled(); expect(res.status).not.toHaveBeenCalled(); }); + + it('should populate req.auth from decoded JWT when token is a valid JWT', async () => { + mockedDecodeJwt.mockReturnValue({ + sub: 'jwt-user-id', + email: 'test@wxyc.org', + role: 'dj', + } as any); + const { req, res, next } = createMocks('Bearer some.jwt.token'); + const middleware = requirePermissions({ bin: ['read'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.auth?.id).toBe('jwt-user-id'); + }); + + it('should use raw token as user ID when token is not a valid JWT', async () => { + mockedDecodeJwt.mockImplementation(() => { + throw new Error('Invalid token'); + }); + const userId = 'abc-123-user-id'; + const { req, res, next } = createMocks(`Bearer ${userId}`); + const middleware = requirePermissions({ bin: ['read'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.auth?.id).toBe(userId); + }); }); describe('token validation', () => { diff --git a/tests/unit/controllers/djs.bin-idor.test.ts b/tests/unit/controllers/djs.bin-idor.test.ts new file mode 100644 index 00000000..106be817 --- /dev/null +++ b/tests/unit/controllers/djs.bin-idor.test.ts @@ -0,0 +1,62 @@ +import { jest } from '@jest/globals'; +import type { Request, Response, NextFunction } from 'express'; + +jest.mock('../../../apps/backend/services/djs.service', () => ({ + addToBin: jest.fn().mockResolvedValue({ id: 1, dj_id: 'user-A', album_id: 1, track_title: null } as never), + removeFromBin: jest.fn().mockResolvedValue({ id: 1, dj_id: 'user-A', album_id: 1, track_title: null } as never), + getBinFromDB: jest.fn().mockResolvedValue([] as never), +})); + +import * as DJService from '../../../apps/backend/services/djs.service'; +import { addToBin, deleteFromBin, getBin } from '../../../apps/backend/controllers/djs.controller'; + +function mockResponse(): Response { + const res = {} as Response; + res.status = jest.fn().mockReturnValue(res) as unknown as Response['status']; + res.json = jest.fn().mockReturnValue(res) as unknown as Response['json']; + res.send = jest.fn().mockReturnValue(res) as unknown as Response['send']; + return res; +} + +const next: NextFunction = jest.fn(); + +describe('djs.controller bin endpoints – IDOR protection', () => { + describe('addToBin', () => { + it('uses authenticated user ID, not client-supplied dj_id', async () => { + const req = { + auth: { id: 'user-A' }, + body: { dj_id: 'user-B', album_id: 1 }, + } as unknown as Request; + + await addToBin(req, mockResponse(), next); + + expect(DJService.addToBin).toHaveBeenCalledWith(expect.objectContaining({ dj_id: 'user-A' })); + }); + }); + + describe('deleteFromBin', () => { + it('uses authenticated user ID, not client-supplied dj_id', async () => { + const req = { + auth: { id: 'user-A' }, + query: { dj_id: 'user-B', album_id: '1' }, + } as unknown as Request; + + await deleteFromBin(req, mockResponse(), next); + + expect(DJService.removeFromBin).toHaveBeenCalledWith(1, 'user-A'); + }); + }); + + describe('getBin', () => { + it('uses authenticated user ID, not client-supplied dj_id', async () => { + const req = { + auth: { id: 'user-A' }, + query: { dj_id: 'user-B' }, + } as unknown as Request; + + await getBin(req, mockResponse(), next); + + expect(DJService.getBinFromDB).toHaveBeenCalledWith('user-A'); + }); + }); +});