From 4986bade71935423addc069aecf0104dd5a0db2f Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 13 Feb 2026 16:45:16 -0800 Subject: [PATCH 1/6] fix: add admin role to WXYCRoles The requirePermissions middleware validates JWT roles against WXYCRoles, but WXYCRoles only included member, dj, musicDirector, and stationManager. Users whose organization member role is "admin" get 403 "Invalid role" on every authenticated request because WXYCRoles["admin"] is undefined. Add an admin role definition with the same permissions as stationManager (plus adminAc org-management statements) and register it in WXYCRoles so the middleware recognizes it. Also add unit tests for all WXYCRoles definitions and the mocks needed to test better-auth access control modules under Jest. --- jest.unit.config.ts | 3 + shared/authentication/src/auth.roles.ts | 8 +++ tests/mocks/better-auth-access.mock.ts | 31 ++++++++++ tests/mocks/better-auth-org-access.mock.ts | 23 ++++++++ tests/unit/authentication/auth.roles.test.ts | 62 ++++++++++++++++++++ 5 files changed, 127 insertions(+) create mode 100644 tests/mocks/better-auth-access.mock.ts create mode 100644 tests/mocks/better-auth-org-access.mock.ts create mode 100644 tests/unit/authentication/auth.roles.test.ts diff --git a/jest.unit.config.ts b/jest.unit.config.ts index acea7182..12e7e2cc 100644 --- a/jest.unit.config.ts +++ b/jest.unit.config.ts @@ -18,6 +18,9 @@ const config: Config = { '^@wxyc/database$': '/tests/mocks/database.mock.ts', // Mock database client for any path resolving to shared/database/src/client '^.*/shared/database/src/client(\\.js)?$': '/tests/mocks/database.mock.ts', + // Mock better-auth access control modules (ESM-only, can't be transformed by ts-jest) + '^better-auth/plugins/access$': '/tests/mocks/better-auth-access.mock.ts', + '^better-auth/plugins/organization/access$': '/tests/mocks/better-auth-org-access.mock.ts', // Remove .js extensions from relative imports (ESM compatibility) '^(\\.{1,2}/.*)\\.(js)$': '$1', }, diff --git a/shared/authentication/src/auth.roles.ts b/shared/authentication/src/auth.roles.ts index 386f3e1d..124a6272 100644 --- a/shared/authentication/src/auth.roles.ts +++ b/shared/authentication/src/auth.roles.ts @@ -40,11 +40,19 @@ export const stationManager = accessControl.newRole({ flowsheet: ["read", "write"], }); +export const admin = accessControl.newRole({ + ...adminAc.statements, + bin: ["read", "write"], + catalog: ["read", "write"], + flowsheet: ["read", "write"], +}); + export const WXYCRoles = { member, dj, musicDirector, stationManager, + admin, }; export type WXYCRole = keyof typeof WXYCRoles; diff --git a/tests/mocks/better-auth-access.mock.ts b/tests/mocks/better-auth-access.mock.ts new file mode 100644 index 00000000..9d835a27 --- /dev/null +++ b/tests/mocks/better-auth-access.mock.ts @@ -0,0 +1,31 @@ +/** + * Minimal mock for better-auth/plugins/access + * + * Provides createAccessControl and newRole that mimic the real behavior + * closely enough to validate role permission definitions. + */ + +type Statement = Record; + +export function createAccessControl(statements: S) { + return { + newRole(permissions: Partial<{ [K in keyof S]: readonly string[] }>) { + return { + authorize(request: Partial<{ [K in keyof S]: string[] }>) { + for (const [resource, actions] of Object.entries(request)) { + if (!actions || (actions as string[]).length === 0) continue; + const allowed = permissions[resource as keyof S]; + if (!allowed) return { success: false }; + for (const action of actions as string[]) { + if (!(allowed as readonly string[]).includes(action)) { + return { success: false }; + } + } + } + return { success: true }; + }, + statements: permissions, + }; + }, + }; +} diff --git a/tests/mocks/better-auth-org-access.mock.ts b/tests/mocks/better-auth-org-access.mock.ts new file mode 100644 index 00000000..ee715b5f --- /dev/null +++ b/tests/mocks/better-auth-org-access.mock.ts @@ -0,0 +1,23 @@ +/** + * Minimal mock for better-auth/plugins/organization/access + * + * Provides the same defaultStatements and adminAc values as the real module. + */ + +export const defaultStatements = { + organization: ["update", "delete"], + member: ["create", "update", "delete"], + invitation: ["create", "cancel"], + team: ["create", "update", "delete"], + ac: ["create", "read", "update", "delete"], +} as const; + +export const adminAc = { + statements: { + organization: ["update"], + invitation: ["create", "cancel"], + member: ["create", "update", "delete"], + team: ["create", "update", "delete"], + ac: ["create", "read", "update", "delete"], + } as const, +}; diff --git a/tests/unit/authentication/auth.roles.test.ts b/tests/unit/authentication/auth.roles.test.ts new file mode 100644 index 00000000..6d98fe5e --- /dev/null +++ b/tests/unit/authentication/auth.roles.test.ts @@ -0,0 +1,62 @@ +import { WXYCRoles, type WXYCRole } from '../../../shared/authentication/src/auth.roles'; + +describe('WXYCRoles', () => { + const allRoles = Object.keys(WXYCRoles) as WXYCRole[]; + + it('should include the admin role', () => { + expect(WXYCRoles).toHaveProperty('admin'); + }); + + /** + * These are the roles that better-auth's organization plugin and + * the auth hooks in auth.definition.ts may assign to members. + * If any of them are missing from WXYCRoles, the requirePermissions + * middleware will return 403 "Invalid role" for those users. + */ + it.each(['member', 'dj', 'musicDirector', 'stationManager', 'admin'])( + 'should recognize the "%s" role', + (role) => { + expect(WXYCRoles).toHaveProperty(role); + expect(WXYCRoles[role as WXYCRole]).toBeDefined(); + } + ); + + describe('role permissions', () => { + it.each(allRoles)('"%s" should have an authorize function', (role) => { + const roleDef = WXYCRoles[role]; + expect(typeof (roleDef as any).authorize).toBe('function'); + }); + + it.each(allRoles)('"%s" should authorize catalog:read', (role) => { + const roleDef = WXYCRoles[role]; + const result = (roleDef as any).authorize({ catalog: ['read'] }); + expect(result.success).toBe(true); + }); + + it.each(allRoles)('"%s" should authorize bin:read', (role) => { + const roleDef = WXYCRoles[role]; + const result = (roleDef as any).authorize({ bin: ['read'] }); + expect(result.success).toBe(true); + }); + + it('admin should authorize catalog:write', () => { + const result = (WXYCRoles.admin as any).authorize({ catalog: ['write'] }); + expect(result.success).toBe(true); + }); + + it('admin should authorize flowsheet:write', () => { + const result = (WXYCRoles.admin as any).authorize({ flowsheet: ['write'] }); + expect(result.success).toBe(true); + }); + + it('member should NOT authorize flowsheet:write', () => { + const result = (WXYCRoles.member as any).authorize({ flowsheet: ['write'] }); + expect(result.success).toBe(false); + }); + + it('member should NOT authorize catalog:write', () => { + const result = (WXYCRoles.member as any).authorize({ catalog: ['write'] }); + expect(result.success).toBe(false); + }); + }); +}); From ab73884e2acef25b87259dd721c2e47d1acffb34 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 13 Feb 2026 17:06:58 -0800 Subject: [PATCH 2/6] test: add requirePermissions middleware unit tests Tests the exact code path that caused Jackson's 403 bug: a JWT with role="admin" going through requirePermissions. Verifies the middleware calls next() instead of returning 403 "Invalid role" now that WXYCRoles includes admin. --- .../authentication/auth.middleware.test.ts | 189 ++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 tests/unit/authentication/auth.middleware.test.ts diff --git a/tests/unit/authentication/auth.middleware.test.ts b/tests/unit/authentication/auth.middleware.test.ts new file mode 100644 index 00000000..32c65f86 --- /dev/null +++ b/tests/unit/authentication/auth.middleware.test.ts @@ -0,0 +1,189 @@ +// Set required env vars before module load (ts-jest transforms imports to requires, +// so these execute before the middleware module's top-level code runs) +process.env.BETTER_AUTH_JWKS_URL = 'https://test.example.com/.well-known/jwks.json'; +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 +jest.mock('jose', () => ({ + createRemoteJWKSet: jest.fn(() => jest.fn()), + jwtVerify: jest.fn(), +})); + +import { jwtVerify } from 'jose'; +import { requirePermissions } from '../../../shared/authentication/src/auth.middleware'; +import type { Request, Response, NextFunction } from 'express'; + +const mockedJwtVerify = jwtVerify as jest.MockedFunction; + +/** Build minimal Express req/res/next mocks */ +function createMocks(authHeader?: string) { + const req = { + headers: authHeader ? { authorization: authHeader } : {}, + } as unknown as Request; + + const res = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + } as unknown as Response; + + const next = jest.fn() as NextFunction; + + return { req, res, next }; +} + +function mockJwtPayload(overrides: Record = {}) { + mockedJwtVerify.mockResolvedValue({ + payload: { + sub: 'test-user-id', + email: 'test@wxyc.org', + role: 'dj', + ...overrides, + }, + protectedHeader: { alg: 'RS256' }, + key: {} as any, + } as any); +} + +describe('requirePermissions middleware', () => { + const originalAuthBypass = process.env.AUTH_BYPASS; + + beforeEach(() => { + delete process.env.AUTH_BYPASS; + }); + + afterAll(() => { + if (originalAuthBypass !== undefined) { + process.env.AUTH_BYPASS = originalAuthBypass; + } else { + delete process.env.AUTH_BYPASS; + } + }); + + describe('AUTH_BYPASS', () => { + it('should skip all validation when AUTH_BYPASS is "true"', async () => { + process.env.AUTH_BYPASS = 'true'; + const { req, res, next } = createMocks(); // no auth header + const middleware = requirePermissions({ catalog: ['read'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }); + }); + + describe('token validation', () => { + it('should return 401 when Authorization header is missing', async () => { + const { req, res, next } = createMocks(); + const middleware = requirePermissions({ catalog: ['read'] }); + + await middleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(401); + expect(next).not.toHaveBeenCalled(); + }); + + it('should return 401 when JWT verification fails', async () => { + jest.spyOn(console, 'error').mockImplementation(() => {}); // suppress expected log + mockedJwtVerify.mockRejectedValue(new Error('invalid signature')); + const { req, res, next } = createMocks('Bearer bad-token'); + const middleware = requirePermissions({ catalog: ['read'] }); + + await middleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(401); + expect(next).not.toHaveBeenCalled(); + }); + }); + + describe('role validation', () => { + it('should return 403 "Invalid role" for an unrecognized role', async () => { + mockJwtPayload({ role: 'nonexistent_role' }); + const { req, res, next } = createMocks('Bearer valid-token'); + const middleware = requirePermissions({ catalog: ['read'] }); + + await middleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ error: expect.stringContaining('Invalid role') }), + ); + expect(next).not.toHaveBeenCalled(); + }); + + /** + * This is the exact regression test for Jackson's 403 bug. + * + * Before the fix, WXYCRoles did not include "admin". When better-auth + * assigned role="admin" to a user's JWT (via org hooks / syncAdminRoles), + * the middleware hit line 106-107: + * + * const roleImpl = WXYCRoles[payload.role]; // undefined + * if (!roleImpl) return res.status(403) // 403 "Invalid role" + * + * With the fix, WXYCRoles.admin is defined, so the middleware proceeds. + */ + it('should NOT return 403 for the "admin" role', async () => { + mockJwtPayload({ role: 'admin' }); + const { req, res, next } = createMocks('Bearer valid-token'); + const middleware = requirePermissions({ catalog: ['read'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }); + }); + + describe('permission checks', () => { + it.each(['member', 'dj', 'musicDirector', 'stationManager', 'admin'] as const)( + '"%s" should be authorized for catalog:read', + async (role) => { + mockJwtPayload({ role }); + const { req, res, next } = createMocks('Bearer valid-token'); + const middleware = requirePermissions({ catalog: ['read'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }, + ); + + it('should return 403 "insufficient permissions" when role lacks required permission', async () => { + mockJwtPayload({ role: 'member' }); + const { req, res, next } = createMocks('Bearer valid-token'); + const middleware = requirePermissions({ catalog: ['write'] }); + + await middleware(req, res, next); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ error: expect.stringContaining('insufficient permissions') }), + ); + expect(next).not.toHaveBeenCalled(); + }); + + it('admin should be authorized for flowsheet:write', async () => { + mockJwtPayload({ role: 'admin' }); + const { req, res, next } = createMocks('Bearer valid-token'); + const middleware = requirePermissions({ flowsheet: ['write'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + }); + + it('admin should be authorized for catalog:write', async () => { + mockJwtPayload({ role: 'admin' }); + const { req, res, next } = createMocks('Bearer valid-token'); + const middleware = requirePermissions({ catalog: ['write'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + }); + }); +}); From f5158151656c40e43a81fb9aaac4840676ca5cfd Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 13 Feb 2026 17:33:11 -0800 Subject: [PATCH 3/6] fix: prefix unused parameter to satisfy eslint no-unused-vars --- tests/mocks/better-auth-access.mock.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mocks/better-auth-access.mock.ts b/tests/mocks/better-auth-access.mock.ts index 9d835a27..3605c057 100644 --- a/tests/mocks/better-auth-access.mock.ts +++ b/tests/mocks/better-auth-access.mock.ts @@ -7,7 +7,7 @@ type Statement = Record; -export function createAccessControl(statements: S) { +export function createAccessControl(_statements: S) { return { newRole(permissions: Partial<{ [K in keyof S]: readonly string[] }>) { return { From bb16deb98896ee50a3b4383ec5615f7ec811bfe5 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 13 Feb 2026 17:46:21 -0800 Subject: [PATCH 4/6] fix: add argsIgnorePattern to test eslint config The _prefix convention for unused params was configured for apps/ and shared/ but not for tests/. This caused a lint error on the _statements param in the better-auth mock. --- eslint.config.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/eslint.config.mjs b/eslint.config.mjs index 8ebbdb64..351fb241 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -93,6 +93,7 @@ export default tseslint.config( '@typescript-eslint/no-unsafe-call': 'off', '@typescript-eslint/no-unsafe-member-access': 'off', '@typescript-eslint/no-unsafe-return': 'off', + '@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_' }], }, } ); From 0337f70e1259e6ce5d9933bb9e3ecf76a928fef6 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 13 Feb 2026 17:56:46 -0800 Subject: [PATCH 5/6] fix: disable unbound-method lint rule for test files Jest mock assertions like expect(res.status).toHaveBeenCalled() trigger false positives from @typescript-eslint/unbound-method. This is already downgraded to warn for app/shared code. --- eslint.config.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/eslint.config.mjs b/eslint.config.mjs index 351fb241..970de5c8 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -94,6 +94,7 @@ export default tseslint.config( '@typescript-eslint/no-unsafe-member-access': 'off', '@typescript-eslint/no-unsafe-return': 'off', '@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_' }], + '@typescript-eslint/unbound-method': 'off', }, } ); From 5316d29a49b188f0617bb5b9af881e28af7b3c62 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 13 Feb 2026 18:06:07 -0800 Subject: [PATCH 6/6] style: format files with Prettier --- shared/authentication/src/auth.roles.ts | 6 +++--- tests/mocks/better-auth-org-access.mock.ts | 20 +++++++++---------- .../authentication/auth.middleware.test.ts | 6 +++--- tests/unit/authentication/auth.roles.test.ts | 11 ++++------ 4 files changed, 20 insertions(+), 23 deletions(-) diff --git a/shared/authentication/src/auth.roles.ts b/shared/authentication/src/auth.roles.ts index e6c0e383..1a5bbe54 100644 --- a/shared/authentication/src/auth.roles.ts +++ b/shared/authentication/src/auth.roles.ts @@ -39,9 +39,9 @@ export const stationManager = accessControl.newRole({ export const admin = accessControl.newRole({ ...adminAc.statements, - bin: ["read", "write"], - catalog: ["read", "write"], - flowsheet: ["read", "write"], + bin: ['read', 'write'], + catalog: ['read', 'write'], + flowsheet: ['read', 'write'], }); export const WXYCRoles = { diff --git a/tests/mocks/better-auth-org-access.mock.ts b/tests/mocks/better-auth-org-access.mock.ts index ee715b5f..ac3e6d68 100644 --- a/tests/mocks/better-auth-org-access.mock.ts +++ b/tests/mocks/better-auth-org-access.mock.ts @@ -5,19 +5,19 @@ */ export const defaultStatements = { - organization: ["update", "delete"], - member: ["create", "update", "delete"], - invitation: ["create", "cancel"], - team: ["create", "update", "delete"], - ac: ["create", "read", "update", "delete"], + organization: ['update', 'delete'], + member: ['create', 'update', 'delete'], + invitation: ['create', 'cancel'], + team: ['create', 'update', 'delete'], + ac: ['create', 'read', 'update', 'delete'], } as const; export const adminAc = { statements: { - organization: ["update"], - invitation: ["create", "cancel"], - member: ["create", "update", "delete"], - team: ["create", "update", "delete"], - ac: ["create", "read", "update", "delete"], + organization: ['update'], + invitation: ['create', 'cancel'], + member: ['create', 'update', 'delete'], + team: ['create', 'update', 'delete'], + ac: ['create', 'read', 'update', 'delete'], } as const, }; diff --git a/tests/unit/authentication/auth.middleware.test.ts b/tests/unit/authentication/auth.middleware.test.ts index 32c65f86..a2d7fbb9 100644 --- a/tests/unit/authentication/auth.middleware.test.ts +++ b/tests/unit/authentication/auth.middleware.test.ts @@ -108,7 +108,7 @@ describe('requirePermissions middleware', () => { expect(res.status).toHaveBeenCalledWith(403); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: expect.stringContaining('Invalid role') }), + expect.objectContaining({ error: expect.stringContaining('Invalid role') }) ); expect(next).not.toHaveBeenCalled(); }); @@ -149,7 +149,7 @@ describe('requirePermissions middleware', () => { expect(next).toHaveBeenCalled(); expect(res.status).not.toHaveBeenCalled(); - }, + } ); it('should return 403 "insufficient permissions" when role lacks required permission', async () => { @@ -161,7 +161,7 @@ describe('requirePermissions middleware', () => { expect(res.status).toHaveBeenCalledWith(403); expect(res.json).toHaveBeenCalledWith( - expect.objectContaining({ error: expect.stringContaining('insufficient permissions') }), + expect.objectContaining({ error: expect.stringContaining('insufficient permissions') }) ); expect(next).not.toHaveBeenCalled(); }); diff --git a/tests/unit/authentication/auth.roles.test.ts b/tests/unit/authentication/auth.roles.test.ts index 6d98fe5e..b627bb46 100644 --- a/tests/unit/authentication/auth.roles.test.ts +++ b/tests/unit/authentication/auth.roles.test.ts @@ -13,13 +13,10 @@ describe('WXYCRoles', () => { * If any of them are missing from WXYCRoles, the requirePermissions * middleware will return 403 "Invalid role" for those users. */ - it.each(['member', 'dj', 'musicDirector', 'stationManager', 'admin'])( - 'should recognize the "%s" role', - (role) => { - expect(WXYCRoles).toHaveProperty(role); - expect(WXYCRoles[role as WXYCRole]).toBeDefined(); - } - ); + it.each(['member', 'dj', 'musicDirector', 'stationManager', 'admin'])('should recognize the "%s" role', (role) => { + expect(WXYCRoles).toHaveProperty(role); + expect(WXYCRoles[role as WXYCRole]).toBeDefined(); + }); describe('role permissions', () => { it.each(allRoles)('"%s" should have an authorize function', (role) => {