From 0ac85ebc1f82a4944ee193086663d6b129c65b0c Mon Sep 17 00:00:00 2001 From: Nathnael Date: Mon, 31 Aug 2026 11:45:55 +0000 Subject: [PATCH] fix --- .../chat/chat-provisioning.service.spec.ts | 92 ++++++++++++++ .../modules/chat/chat-provisioning.service.ts | 13 ++ .../src/modules/chat/chat.module.ts | 4 +- .../src/modules/chat/matrix.client.spec.ts | 118 +++++++++++++++++- .../src/modules/chat/matrix.client.ts | 88 ++++++++++++- .../modules/health/health.controller.spec.ts | 100 +++++++++++++++ .../src/modules/health/health.controller.ts | 40 +++++- .../src/modules/health/health.module.ts | 4 +- 8 files changed, 451 insertions(+), 8 deletions(-) create mode 100644 apps/edr-freight-api/src/modules/chat/chat-provisioning.service.spec.ts create mode 100644 apps/edr-freight-api/src/modules/health/health.controller.spec.ts diff --git a/apps/edr-freight-api/src/modules/chat/chat-provisioning.service.spec.ts b/apps/edr-freight-api/src/modules/chat/chat-provisioning.service.spec.ts new file mode 100644 index 000000000..935f03ab4 --- /dev/null +++ b/apps/edr-freight-api/src/modules/chat/chat-provisioning.service.spec.ts @@ -0,0 +1,92 @@ +import 'reflect-metadata'; + +import type { DataSource } from 'typeorm'; + +import { ChatProvisioningService } from './chat-provisioning.service'; +import type { MatrixClient } from './matrix.client'; + +/** + * `joinUserRooms` is the only thing standing between a first sign-in and an + * empty Element — the reconcile that would otherwise fill the room list runs + * nightly. Both branches below are outages that actually happened on dev. + */ +describe('ChatProvisioningService.joinUserRooms', () => { + const NAA = '03f5eb9e-23a0-4413-8d98-8de4b98b1be2'; + const SUPER_ADMIN = 'f1534714-fa4a-4780-a081-05d4c1f6c25f'; + + function harness(holders: unknown[]) { + const matrix = { + mxidFor: jest.fn( + (userId: string, name: string) => `@${name}.${userId.slice(0, 6)}:m.test`, + ), + ensureUser: jest.fn(async (_mxid: string, _name?: string) => undefined), + ensureRoom: jest.fn( + async (alias: string, _name?: string, _opts?: unknown) => `!${alias}:m.test`, + ), + ensureJoined: jest.fn(async (_roomId: string, _mxid: string) => undefined), + }; + const dataSource = { query: jest.fn(async () => holders) }; + const service = new ChatProvisioningService( + dataSource as unknown as DataSource, + matrix as unknown as MatrixClient, + ); + return { service, matrix }; + } + + it('creates nothing for a user holding no current position', async () => { + // Super Admin on dev: three iam.employees rows, zero employee_positions. + // Synapse still auto-registers the account on JWT login, so the only + // symptom is a working sign-in into a client with no rooms in it. + const { service, matrix } = harness([]); + + await expect(service.joinUserRooms(SUPER_ADMIN, 'Super Admin')).resolves.toBe(0); + + expect(matrix.ensureUser).not.toHaveBeenCalled(); + expect(matrix.ensureRoom).not.toHaveBeenCalled(); + expect(matrix.ensureJoined).not.toHaveBeenCalled(); + }); + + it('joins a position holder to the space, #general and their dept room', async () => { + const { service, matrix } = harness([ + { + positionKey: 'edr_freight_app/marketer', + positionName: 'Marketer', + userId: NAA, + userName: 'naa', + }, + ]); + + await expect(service.joinUserRooms(NAA, 'naa')).resolves.toBe(2); + + // The account has to exist before the admin join API will touch it — JWT + // auto-registration happens after this runs. + expect(matrix.ensureUser).toHaveBeenCalledWith('@naa.03f5eb:m.test', 'naa'); + + expect(matrix.ensureRoom.mock.calls.map(([alias]) => alias)).toEqual([ + 'edr-freight', + 'general', + 'dept-edr_freight_app/marketer', + ]); + + // The space itself is joined, not only the rooms under it: Element shows a + // space in the left rail only to its members, so dropping this scatters + // every dept room loose into Home. + expect(matrix.ensureJoined.mock.calls.map(([roomId]) => roomId)).toEqual([ + '!edr-freight:m.test', + '!general:m.test', + '!dept-edr_freight_app/marketer:m.test', + ]); + }); + + it('scopes the position lookup to the one user', async () => { + const { service } = harness([]); + await service.joinUserRooms(NAA, 'naa'); + // Without the third parameter this would reconcile the whole unit on every + // click of "Open EDR Chat". + const [sql, params] = (service as unknown as { + dataSource: { query: jest.Mock }; + }).dataSource.query.mock.calls[0]; + expect(sql).toContain('AND e.user_id = $3'); + expect(params).toEqual(['edr_freight', 'edr_freight_app', NAA]); + }); +}); diff --git a/apps/edr-freight-api/src/modules/chat/chat-provisioning.service.ts b/apps/edr-freight-api/src/modules/chat/chat-provisioning.service.ts index b122d8073..23d05d8e6 100644 --- a/apps/edr-freight-api/src/modules/chat/chat-provisioning.service.ts +++ b/apps/edr-freight-api/src/modules/chat/chat-provisioning.service.ts @@ -113,6 +113,11 @@ export class ChatProvisioningService { const spaceId = await this.matrix.ensureRoom(SPACE_ALIAS, 'EDR Freight', { isSpace: true, }); + // The space itself, not only the rooms under it: Element lists a space in + // the left rail only for members of that space, so skipping this scatters + // every dept room loose into Home and the "EDR Freight" grouping never + // appears at all. + await this.matrix.ensureJoined(spaceId, mxid); const generalRoomId = await this.matrix.ensureRoom(GENERAL_ALIAS, 'General', { parentSpaceId: spaceId, }); @@ -192,6 +197,14 @@ export class ChatProvisioningService { // leaver, not just moved between positions — deactivate their account. const kickedUserIds = new Set(); + // Space membership follows the org tree exactly like room membership — + // see the ensureJoined in joinUserRooms for why the space needs joining + // at all. + const spaceDiff = await this.syncMembership(spaceId, allUserIds, botMxid); + joined += spaceDiff.joined; + kicked += spaceDiff.kicked.length; + spaceDiff.kicked.forEach((uid) => kickedUserIds.add(uid)); + const generalDiff = await this.syncMembership(generalRoomId, allUserIds, botMxid); joined += generalDiff.joined; kicked += generalDiff.kicked.length; diff --git a/apps/edr-freight-api/src/modules/chat/chat.module.ts b/apps/edr-freight-api/src/modules/chat/chat.module.ts index 8df827339..db2e9c44c 100644 --- a/apps/edr-freight-api/src/modules/chat/chat.module.ts +++ b/apps/edr-freight-api/src/modules/chat/chat.module.ts @@ -11,6 +11,8 @@ import { MatrixClient } from './matrix.client'; providers: [MatrixClient, ChatSsoService, ChatProvisioningService, ChatBridgeService], // ChatBridgeService: consumed by NotificationInboxModule to mirror // BACKOFFICE notifications into chat — see notification-inbox.module.ts. - exports: [ChatBridgeService], + // MatrixClient: HealthModule's readiness probe reports whether + // MATRIX_ADMIN_TOKEN really carries server-admin rights. + exports: [ChatBridgeService, MatrixClient], }) export class ChatModule {} diff --git a/apps/edr-freight-api/src/modules/chat/matrix.client.spec.ts b/apps/edr-freight-api/src/modules/chat/matrix.client.spec.ts index ea5faa978..73798bce9 100644 --- a/apps/edr-freight-api/src/modules/chat/matrix.client.spec.ts +++ b/apps/edr-freight-api/src/modules/chat/matrix.client.spec.ts @@ -1,4 +1,5 @@ -import { chatLocalpart } from './matrix.client'; +import type { ChatConfig } from '../../config/chat.config'; +import { MatrixClient, chatLocalpart } from './matrix.client'; describe('chatLocalpart', () => { it('reads from the name, not the id', () => { @@ -33,3 +34,118 @@ describe('chatLocalpart', () => { } }); }); + +const config: ChatConfig = { + enabled: true, + baseUrl: 'https://matrix.test', + publicBaseUrl: 'https://matrix.test', + webUrl: 'https://chat.test', + serverName: 'matrix.test', + jwtSecret: 'secret', + adminToken: 'syt_whatever', +}; + +type FetchFn = typeof globalThis.fetch; + +/** Just enough of a Response for {@link MatrixClient}'s fetch wrappers. */ +function response(status: number, body: unknown) { + return { + ok: status >= 200 && status < 300, + status, + json: async () => body, + text: async () => JSON.stringify(body), + }; +} + +const realFetch: FetchFn = globalThis.fetch; +const fetchMock = jest.fn(); + +beforeEach(() => { + fetchMock.mockReset(); + globalThis.fetch = fetchMock as unknown as FetchFn; +}); + +afterAll(() => { + globalThis.fetch = realFetch; +}); + +describe('MatrixClient.verifyServerAdmin', () => { + it('accepts a token that can actually call the Synapse admin API', async () => { + fetchMock + .mockResolvedValueOnce(response(200, { user_id: '@edrbot:matrix.test' })) + .mockResolvedValueOnce(response(200, { users: [], total: 1 })); + + const check = await new MatrixClient(config).verifyServerAdmin(); + + expect(check).toEqual({ ok: true, actingAs: '@edrbot:matrix.test' }); + // The admin ping is the check. If this ever regresses to whoami alone, + // the assertion below is what catches it. + expect(String(fetchMock.mock.calls[1][0])).toContain('/_synapse/admin/'); + }); + + it('rejects a valid token that is not a server admin', async () => { + // The dev outage, exactly: MATRIX_ADMIN_TOKEN held @super-admin's own + // token. whoami answered 200, every /_synapse/admin call answered 403, + // ensureUser threw, ChatSsoService swallowed it, and every employee got a + // working sign-in into an Element with no rooms in it. + fetchMock + .mockResolvedValueOnce( + response(200, { user_id: '@super-admin.f15347:matrix.test' }), + ) + .mockResolvedValueOnce( + response(403, { + errcode: 'M_FORBIDDEN', + error: 'You are not a server admin', + }), + ); + + const check = await new MatrixClient(config).verifyServerAdmin(); + + expect(check.ok).toBe(false); + // Naming the account the token belongs to is the whole point — it is what + // turns "chat is broken" into "wrong token in the env". + expect(check.actingAs).toBe('@super-admin.f15347:matrix.test'); + expect(check.error).toContain('403'); + }); + + it('rejects a token that is not valid at all', async () => { + fetchMock.mockResolvedValueOnce( + response(401, { errcode: 'M_UNKNOWN_TOKEN', error: 'Invalid access token' }), + ); + + const check = await new MatrixClient(config).verifyServerAdmin(); + + expect(check.ok).toBe(false); + expect(check.actingAs).toBeUndefined(); + expect(fetchMock).toHaveBeenCalledTimes(1); // no point pinging admin after this + }); +}); + +describe('MatrixClient.adminCheck', () => { + it('does not re-hit Synapse on every readiness probe', async () => { + fetchMock + .mockResolvedValueOnce(response(200, { user_id: '@edrbot:matrix.test' })) + .mockResolvedValueOnce(response(200, { users: [] })); + + const client = new MatrixClient(config); + const first = await client.adminCheck(); + const second = await client.adminCheck(); + + expect(second).toBe(first); + expect(fetchMock).toHaveBeenCalledTimes(2); // whoami + admin ping, once + }); + + it('re-checks when forced, so boot never reads a stale verdict', async () => { + fetchMock + .mockResolvedValueOnce(response(200, { user_id: '@edrbot:matrix.test' })) + .mockResolvedValueOnce(response(200, { users: [] })) + .mockResolvedValueOnce(response(200, { user_id: '@edrbot:matrix.test' })) + .mockResolvedValueOnce(response(200, { users: [] })); + + const client = new MatrixClient(config); + await client.adminCheck(); + await client.adminCheck(true); + + expect(fetchMock).toHaveBeenCalledTimes(4); + }); +}); diff --git a/apps/edr-freight-api/src/modules/chat/matrix.client.ts b/apps/edr-freight-api/src/modules/chat/matrix.client.ts index 1cd09ac33..fdfab052d 100644 --- a/apps/edr-freight-api/src/modules/chat/matrix.client.ts +++ b/apps/edr-freight-api/src/modules/chat/matrix.client.ts @@ -1,4 +1,5 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Inject, Injectable, Logger } from '@nestjs/common'; +import type { OnApplicationBootstrap } from '@nestjs/common'; import type { ConfigType } from '@nestjs/config'; import chatConfig from '../../config/chat.config'; @@ -40,8 +41,24 @@ export function chatLocalpart(userId: string, displayName: string): string { return `${slug || 'user'}.${userId.replace(/-/g, '').slice(0, 6)}`; } +/** Result of {@link MatrixClient.verifyServerAdmin}. */ +export interface AdminCheck { + ok: boolean; + /** Who MATRIX_ADMIN_TOKEN belongs to — present whenever the token is valid + * at all, including when it is valid but carries no admin rights. */ + actingAs?: string; + error?: string; +} + @Injectable() -export class MatrixClient { +export class MatrixClient implements OnApplicationBootstrap { + private readonly logger = new Logger(MatrixClient.name); + + /** The token is a deploy-time fact and the readiness probe runs every few + * seconds, so {@link adminCheck} memoises for this long. */ + private static readonly ADMIN_CHECK_TTL_MS = 5 * 60_000; + private adminCheckCache?: { at: number; result: AdminCheck }; + constructor( @Inject(chatConfig.KEY) private readonly config: ConfigType, @@ -73,6 +90,11 @@ export class MatrixClient { return this.config.serverName; } + /** MATRIX_ENABLED — read by the readiness probe to tell "off" from "broken". */ + get enabled(): boolean { + return this.config.enabled; + } + private async request( method: string, path: string, @@ -157,6 +179,68 @@ export class MatrixClient { return res.user_id; } + /** + * Is MATRIX_ADMIN_TOKEN actually a *server admin* token? + * + * `whoami` cannot answer this: it returns 200 for any valid user token at + * all. Dev shipped with MATRIX_ADMIN_TOKEN holding an ordinary staff + * account's token — whoami said 200, every `/_synapse/admin/*` call said + * 403 "You are not a server admin", `ensureUser` threw, ChatSsoService + * swallowed it (by design — a failed room join must not deny anyone a + * sign-in link), and every employee got a working sign-in into a client + * with no rooms in it. Nothing else in the system noticed. + * + * So this pings an endpoint only a server admin may call, and reports who + * the token belongs to — the one fact that makes the mix-up obvious. + */ + async verifyServerAdmin(): Promise { + let actingAs: string | undefined; + try { + actingAs = await this.whoami(); + await this.request('GET', '/_synapse/admin/v2/users?limit=1'); + return { ok: true, actingAs }; + } catch (err) { + return { ok: false, actingAs, error: (err as Error).message }; + } + } + + /** {@link verifyServerAdmin}, memoised for {@link ADMIN_CHECK_TTL_MS}. */ + async adminCheck(force = false): Promise { + const cached = this.adminCheckCache; + if ( + !force && + cached && + Date.now() - cached.at < MatrixClient.ADMIN_CHECK_TTL_MS + ) { + return cached.result; + } + const result = await this.verifyServerAdmin(); + this.adminCheckCache = { at: Date.now(), result }; + return result; + } + + /** + * Fail loud at boot instead of silently on every sign-in. Logged, never + * thrown: chat provisioning must not be able to stop the API from starting, + * the same contract the reconcile cron and the notification bridge hold to. + */ + async onApplicationBootstrap(): Promise { + if (!this.config.enabled) return; + const check = await this.adminCheck(true); + if (check.ok) { + this.logger.log( + `MATRIX_ADMIN_TOKEN verified — server admin as ${check.actingAs}`, + ); + return; + } + this.logger.error( + 'MATRIX_ADMIN_TOKEN is not a server-admin token' + + (check.actingAs ? ` (it belongs to ${check.actingAs})` : '') + + `: ${check.error}. Chat provisioning will create no rooms, and every ` + + 'employee who opens chat will land in an empty Element.', + ); + } + /** Currently-joined user ids for a room (not full member-event state). */ async joinedMembers(roomId: string): Promise { const res = await this.request<{ joined: Record }>( diff --git a/apps/edr-freight-api/src/modules/health/health.controller.spec.ts b/apps/edr-freight-api/src/modules/health/health.controller.spec.ts new file mode 100644 index 000000000..26c873567 --- /dev/null +++ b/apps/edr-freight-api/src/modules/health/health.controller.spec.ts @@ -0,0 +1,100 @@ +import 'reflect-metadata'; + +import type { Response } from 'express'; +import type { DataSource } from 'typeorm'; + +import type { MatrixClient } from '../chat/matrix.client'; +import type { EmailClientService } from '../notifications/email-client.service'; +import type { SmsClientService } from '../notifications/sms-client.service'; +import { HealthController } from './health.controller'; + +type ReadinessBody = { + status: string; + checks: { + chat: { status: string; enabled: boolean; actingAs?: string; error?: string }; + }; +}; + +/** Captures what the controller wrote, in place of an express Response. */ +function recorder() { + const sent: { code?: number; body?: ReadinessBody } = {}; + const res = { + status(code: number) { + sent.code = code; + return this; + }, + json(body: ReadinessBody) { + sent.body = body; + return this; + }, + }; + return { sent, res: res as unknown as Response }; +} + +function controllerWith(matrix: Partial) { + const dataSource = { query: jest.fn(async () => [{ '?column?': 1 }]) }; + return new HealthController( + dataSource as unknown as DataSource, + { brokerConnected: true } as unknown as SmsClientService, + { brokerConnected: true } as unknown as EmailClientService, + matrix as MatrixClient, + ); +} + +describe('HealthController readiness — chat check', () => { + it('reports degraded, not 503, when MATRIX_ADMIN_TOKEN is not a server admin', async () => { + // The dev outage. Chat is broken, but chat is not worth pulling the pod + // out of the load balancer for — bookings and billing still work. + const controller = controllerWith({ + enabled: true, + adminCheck: jest.fn(async () => ({ + ok: false, + actingAs: '@super-admin.f15347:matrixdev.edrsc.com', + error: 'Matrix GET /_synapse/admin/v2/users?limit=1 -> 403: not a server admin', + })), + }); + + const { sent, res } = recorder(); + await controller.readiness(res); + + expect(sent.code).toBe(200); + expect(sent.body?.status).toBe('degraded'); + expect(sent.body?.checks.chat.status).toBe('error'); + // The account name is the actionable half — it says *which* token is wired up. + expect(sent.body?.checks.chat.actingAs).toBe( + '@super-admin.f15347:matrixdev.edrsc.com', + ); + }); + + it('reports ok when the token really is a server admin', async () => { + const controller = controllerWith({ + enabled: true, + adminCheck: jest.fn(async () => ({ + ok: true, + actingAs: '@edrbot:matrixdev.edrsc.com', + })), + }); + + const { sent, res } = recorder(); + await controller.readiness(res); + + expect(sent.body?.status).toBe('ok'); + expect(sent.body?.checks.chat).toMatchObject({ + status: 'ok', + enabled: true, + actingAs: '@edrbot:matrixdev.edrsc.com', + }); + }); + + it('does not call Synapse, or degrade, when chat is switched off', async () => { + const adminCheck = jest.fn(); + const controller = controllerWith({ enabled: false, adminCheck }); + + const { sent, res } = recorder(); + await controller.readiness(res); + + expect(adminCheck).not.toHaveBeenCalled(); + expect(sent.body?.status).toBe('ok'); + expect(sent.body?.checks.chat).toEqual({ status: 'unknown', enabled: false }); + }); +}); diff --git a/apps/edr-freight-api/src/modules/health/health.controller.ts b/apps/edr-freight-api/src/modules/health/health.controller.ts index 6b559f2e3..94b8eb698 100644 --- a/apps/edr-freight-api/src/modules/health/health.controller.ts +++ b/apps/edr-freight-api/src/modules/health/health.controller.ts @@ -7,6 +7,7 @@ import { Public } from "@edr/api-common"; import { Response } from "express"; import { DataSource } from "typeorm"; +import { MatrixClient } from "../chat/matrix.client"; import { EmailClientService } from "../notifications/email-client.service"; import { SmsClientService } from "../notifications/sms-client.service"; @@ -32,6 +33,7 @@ export class HealthController { private readonly dataSource: DataSource, private readonly smsClient: SmsClientService, private readonly emailClient: EmailClientService, + private readonly matrix: MatrixClient, ) {} @Get() @@ -45,7 +47,7 @@ export class HealthController { @Public() @ApiOperation({ summary: - "Readiness probe — database plus SMS/email broker connectivity. Broker failures report as degraded unless READINESS_REQUIRES_BROKER=true.", + "Readiness probe — database, SMS/email broker connectivity, and the Matrix admin token. Broker failures report as degraded unless READINESS_REQUIRES_BROKER=true; chat failures always report as degraded.", }) async readiness(@Res() res: Response) { const startedAt = Date.now(); @@ -76,23 +78,55 @@ export class HealthController { enabled: process.env.RABBITMQ_ENABLED !== "false", }; + const chat = await this.chatCheck(); + const brokerDown = broker.sms.status === "error" || broker.email.status === "error"; const failed = database.status === "error" || (READINESS_REQUIRES_BROKER && brokerDown); - const status = failed ? "error" : brokerDown ? "degraded" : "ok"; + const status = failed + ? "error" + : brokerDown || chat.status === "error" + ? "degraded" + : "ok"; return res .status(failed ? HttpStatus.SERVICE_UNAVAILABLE : HttpStatus.OK) .json({ status, timestamp: new Date().toISOString(), - checks: { database, broker }, + checks: { database, broker, chat }, }); } + /** + * Chat provisioning runs entirely on MATRIX_ADMIN_TOKEN, and a token that is + * valid but not *server admin* fails only the `/_synapse/admin` half: rooms + * are never created, joins never happen, and the sole symptom is an empty + * Element for every employee. Nothing else in the probe would catch that. + * + * Degraded, never a 503 — chat is not worth pulling the pod out of the load + * balancer for, by the same reasoning as the broker check above. `unknown` + * when MATRIX_ENABLED is off: a feature that is switched off is not a fault. + */ + private async chatCheck(): Promise<{ + status: CheckStatus; + enabled: boolean; + actingAs?: string; + error?: string; + }> { + if (!this.matrix.enabled) return { status: "unknown", enabled: false }; + const check = await this.matrix.adminCheck(); + return { + status: check.ok ? "ok" : "error", + enabled: true, + actingAs: check.actingAs, + error: check.error, + }; + } + @Get("info") @Public() @ApiOperation({ summary: "App info — version, environment, uptime" }) diff --git a/apps/edr-freight-api/src/modules/health/health.module.ts b/apps/edr-freight-api/src/modules/health/health.module.ts index 572e5eb86..1aa74f56f 100644 --- a/apps/edr-freight-api/src/modules/health/health.module.ts +++ b/apps/edr-freight-api/src/modules/health/health.module.ts @@ -2,13 +2,15 @@ import { Module } from "@nestjs/common"; +import { ChatModule } from "../chat/chat.module"; import { HealthController } from "./health.controller"; import { NotificationsModule } from "../notifications/notifications.module"; @Module({ // NotificationsModule exports the SMS/email clients; the readiness probe reads // their broker connection state rather than opening a second connection. - imports: [NotificationsModule], + // ChatModule exports MatrixClient for the MATRIX_ADMIN_TOKEN check. + imports: [NotificationsModule, ChatModule], controllers: [HealthController], }) export class HealthModule {}