diff --git a/api/src/db/db.class.ts b/api/src/db/db.class.ts index 42a8104e..76e1f9f9 100644 --- a/api/src/db/db.class.ts +++ b/api/src/db/db.class.ts @@ -10,6 +10,22 @@ export class Db { public knex: Knex; constructor() { + // Validate that the database name is appropriate for the current environment + const env = process.env.NODE_ENV; + const dbName = process.env.DB_DATABASE ?? ''; + + if (env === 'test') { + // Require an explicit test DB naming convention + const looksLikeTestDb = /(^test_|_test$|_test_|test$)/i.test(dbName); + + if (!looksLikeTestDb) { + throw new Error( + `Refusing to start in NODE_ENV=test with non-test DB_DATABASE="${dbName}". ` + + 'Use a dedicated test database (e.g. "ctr_test").', + ); + } + } + this.knex = _knex(config[process.env.NODE_ENV]); } diff --git a/api/src/knexfile.ts b/api/src/knexfile.ts index 9cebb342..e587fb34 100644 --- a/api/src/knexfile.ts +++ b/api/src/knexfile.ts @@ -28,6 +28,43 @@ const config: { [key: string]: Knex.Config } = { directory: './../db/seed', }, }, + /** + * Used when NODE_ENV=test, which jest sets for us. + * + * Without this key `config[process.env.NODE_ENV]` was undefined, and because Db's + * constructor calls `knex(...)` at import time, EIGHT of the twelve suites died before + * running a single assertion: + * + * TypeError: Cannot read properties of undefined (reading 'client') + * at new Db (src/db/db.class.ts:13:22) + * + * They were reported as failures rather than skips, so the suite looked broken rather + * than absent, and the four that did run made it look like the tests were merely + * flaky. Nothing about the mocked repositories needed a database -- they only needed + * `knex()` not to throw while the module graph loaded. + * + * The connection details still come from the environment, so this same key is what a + * real database-backed test points at. Set DB_DATABASE to a throwaway schema when + * doing that; unit tests never open a socket, because knex connects lazily. + */ + test: { + client: 'mysql', + connection: { + host: process.env.DB_HOST, + port: Number.parseInt(process.env.DB_PORT), + user: process.env.DB_USER, + password: process.env.DB_PASS, + database: process.env.DB_DATABASE, + charset: 'utf8mb4', + }, + // min 0, unlike the other environments. A minimum of 2 makes the pool open + // connections it will not be asked for, and jest then hangs at the end of a run + // waiting on handles that nothing will close. + pool: { + min: 0, + max: 5, + }, + }, production: { client: 'mysql', connection: { diff --git a/api/src/services/member/member.service.spec.ts b/api/src/services/member/member.service.spec.ts index bb151a0b..c0f1cef0 100644 --- a/api/src/services/member/member.service.spec.ts +++ b/api/src/services/member/member.service.spec.ts @@ -10,6 +10,7 @@ import { import { AvatarRepository, MemberRepository, + RoleAssignmentRepository, TransactionRepository, WalletRepository, } from '../../repositories'; @@ -27,6 +28,7 @@ describe('MemberService', () => { }; let avatarRepository: jest.Mocked; let memberRepository: jest.Mocked; + let roleAssignmentRepository: jest.Mocked; let transactionRepository: jest.Mocked; let walletRepository: jest.Mocked; let service: MemberService; @@ -38,11 +40,14 @@ describe('MemberService', () => { memberRepository.create.mockResolvedValue(fakeMember.id); memberRepository.find.mockResolvedValue(fakeMember as Member); memberRepository.findById.mockResolvedValue(fakeMember as Member); + roleAssignmentRepository = createSpyObj(RoleAssignmentRepository); + roleAssignmentRepository.getByMemberId.mockResolvedValue([]); transactionRepository = createSpyObj(TransactionRepository); walletRepository = createSpyObj(WalletRepository); Container.reset(); Container.set(AvatarRepository, avatarRepository); Container.set(MemberRepository, memberRepository); + Container.set(RoleAssignmentRepository, roleAssignmentRepository); Container.set(TransactionRepository, transactionRepository); Container.set(WalletRepository, walletRepository); service = Container.get(MemberService); @@ -154,4 +159,50 @@ describe('MemberService', () => { }); }); }); + + describe('updatePrimaryRoleId', () => { + const HELD_ROLE = 7; + const UNHELD_ROLE = 99; + + describe('when the member holds the role', () => { + it('saves it as their primary role', async () => { + roleAssignmentRepository.getByMemberId.mockResolvedValue( + [{ member_id: fakeMember.id, role_id: HELD_ROLE, place_id: 1 }] as any, + ); + await service.updatePrimaryRoleId(fakeMember.id, HELD_ROLE); + expect(memberRepository.update).toHaveBeenCalledWith( + fakeMember.id, + { primary_role_id: HELD_ROLE }, + ); + }); + }); + + describe('when the member does not hold the role', () => { + beforeEach(() => { + roleAssignmentRepository.getByMemberId.mockResolvedValue( + [{ member_id: fakeMember.id, role_id: HELD_ROLE, place_id: 1 }] as any, + ); + }); + it('rejects', async () => { + await expect(service.updatePrimaryRoleId(fakeMember.id, UNHELD_ROLE)) + .rejects.toThrow(); + }); + it('does not write anything to the member', async () => { + await expect(service.updatePrimaryRoleId(fakeMember.id, UNHELD_ROLE)) + .rejects.toThrow(); + expect(memberRepository.update).not.toHaveBeenCalled(); + }); + }); + + describe('when given null', () => { + it('clears the primary role without consulting assignments', async () => { + await service.updatePrimaryRoleId(fakeMember.id, null); + expect(memberRepository.update).toHaveBeenCalledWith( + fakeMember.id, + { primary_role_id: null }, + ); + expect(roleAssignmentRepository.getByMemberId).not.toHaveBeenCalled(); + }); + }); + }); }); diff --git a/api/src/services/member/member.service.ts b/api/src/services/member/member.service.ts index 8f033d45..5d15ffce 100644 --- a/api/src/services/member/member.service.ts +++ b/api/src/services/member/member.service.ts @@ -418,7 +418,28 @@ export class MemberService { await this.memberRepository.update(memberId, { password: hashedPassword }); } + /** + * Sets the member's displayed role, rejecting any role they do not actually hold. + * + * primaryRoleId arrives straight from request.body, so without this check any + * authenticated member can display any role -- including City Guide or Security -- + * without holding it. role_assignment is the authority for what a member holds. + * + * A null id clears the selection, which is legitimate. + */ public async updatePrimaryRoleId(memberId: number, primaryRoleId: number): Promise { + if (primaryRoleId === null || primaryRoleId === undefined) { + await this.memberRepository.update(memberId, { primary_role_id: null }); + return; + } + const assignments = await this.roleAssignmentRepository.getByMemberId(memberId); + const holdsRole = assignments + .some(assignment => Number(assignment.role_id) === Number(primaryRoleId)); + if (!holdsRole) { + throw new Error( + 'member does not hold that role; refusing to display it', + ); + } await this.memberRepository.update(memberId, { primary_role_id: primaryRoleId }); }