Fix role impersonation via update_role

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]);
   }
 

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: {

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<AvatarRepository>;
   let memberRepository: jest.Mocked<MemberRepository>;
+  let roleAssignmentRepository: jest.Mocked<RoleAssignmentRepository>;
   let transactionRepository: jest.Mocked<TransactionRepository>;
   let walletRepository: jest.Mocked<WalletRepository>;
   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();
+      });
+    });
+  });
 });

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<void> {
+    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 });
   }