Merge pull request #1466 from Tria-plc/freight/fix/quick-fixes

fix

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

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<string>();
 
+    // 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;

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

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

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<typeof chatConfig>,
@@ -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<T>(
     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<AdminCheck> {
+    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<AdminCheck> {
+    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<void> {
+    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<string[]> {
     const res = await this.request<{ joined: Record<string, unknown> }>(

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

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" })

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