diff --git a/apps/edr-freight-api/src/modules/chat/chat-bridge.service.spec.ts b/apps/edr-freight-api/src/modules/chat/chat-bridge.service.spec.ts new file mode 100644 index 000000000..05dc84b29 --- /dev/null +++ b/apps/edr-freight-api/src/modules/chat/chat-bridge.service.spec.ts @@ -0,0 +1,78 @@ +import 'reflect-metadata'; + +import { NotificationType, type NotifyInput } from '@edr/types'; + +import type { ChatConfig } from '../../config/chat.config'; +import { ChatBridgeService } from './chat-bridge.service'; +import type { MatrixClient } from './matrix.client'; + +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', +}; + +function harness(overrides: Partial = {}) { + const matrix = { + ensureRoom: jest.fn(async (alias: string) => `!${alias}:matrix.test`), + sendMessage: jest.fn( + async (_roomId: string, _body: string, _html?: string) => undefined, + ), + }; + const service = new ChatBridgeService( + { ...config, ...overrides }, + matrix as unknown as MatrixClient, + ); + return { service, matrix }; +} + +const notification = (type: NotificationType): NotifyInput => + ({ type, title: 'Booking BK-1', body: 'needs review' }) as unknown as NotifyInput; + +describe('ChatBridgeService', () => { + it('posts every notification type into #freight-alerts', async () => { + // This used to route REQUEST_SUBMITTED and CLEARANCE_REVIEW to a hardcoded + // `dept-operation` alias, but the reconcile derives dept aliases from the + // IAM position key (`edr_freight_app/opn` shaped), so nothing it created + // ever matched. The bridge made its own empty room and posted there, where + // no employee was a member. + const { service, matrix } = harness(); + + for (const type of [ + NotificationType.REQUEST_SUBMITTED, + NotificationType.CLEARANCE_REVIEW, + NotificationType.GENERIC, + ]) { + await service.bridge(notification(type)); + } + + expect(new Set(matrix.ensureRoom.mock.calls.map(([alias]) => alias))).toEqual( + new Set(['freight-alerts']), + ); + expect(matrix.sendMessage).toHaveBeenCalledTimes(3); + }); + + it('does nothing at all when chat is switched off', async () => { + const { service, matrix } = harness({ enabled: false }); + + await service.bridge(notification(NotificationType.GENERIC)); + + expect(matrix.ensureRoom).not.toHaveBeenCalled(); + expect(matrix.sendMessage).not.toHaveBeenCalled(); + }); + + it('never lets a chat failure escape into the notification that triggered it', async () => { + // Same contract as NotificationInboxService.notify(): bridging is + // best-effort and must not roll back the caller's transaction. + const { service, matrix } = harness(); + matrix.ensureRoom.mockRejectedValueOnce(new Error('Matrix POST ... -> 429')); + + await expect( + service.bridge(notification(NotificationType.GENERIC)), + ).resolves.toBeUndefined(); + }); +}); diff --git a/apps/edr-freight-api/src/modules/chat/chat-bridge.service.ts b/apps/edr-freight-api/src/modules/chat/chat-bridge.service.ts index 75bbe9a29..55c98b897 100644 --- a/apps/edr-freight-api/src/modules/chat/chat-bridge.service.ts +++ b/apps/edr-freight-api/src/modules/chat/chat-bridge.service.ts @@ -1,28 +1,11 @@ import { Inject, Injectable, Logger } from '@nestjs/common'; import type { ConfigType } from '@nestjs/config'; -import { NotificationType, type NotifyInput } from '@edr/types'; +import type { NotifyInput } from '@edr/types'; import chatConfig from '../../config/chat.config'; +import { ALERTS_ROOM } from './chat-provisioning.service'; import { MatrixClient } from './matrix.client'; -const FALLBACK_ROOM = { alias: 'freight-alerts', name: 'Freight Alerts' }; - -/** - * Best-effort per-type routing to an existing dept room. Anything not listed - * (including GENERIC) falls through to #freight-alerts — safer than a wrong - * guess at which department a type belongs to. Extend as real usage shows - * which types actually want a dept room instead of the shared feed. - * - * `name` matters only if this bridge is the very first thing to touch that - * alias (normally the nightly/on-demand reconcile creates dept rooms first, - * with the position's real name) — ensureRoom never renames an existing - * room, so this must match what ChatProvisioningService would have used. - */ -const ROOM_FOR_TYPE: Partial> = { - [NotificationType.REQUEST_SUBMITTED]: { alias: 'dept-operation', name: 'Operation' }, - [NotificationType.CLEARANCE_REVIEW]: { alias: 'dept-operation', name: 'Operation' }, -}; - /** * Mirrors BACKOFFICE-audience notifications into chat so staff see them * without having the inbox open. Hooked once into @@ -31,6 +14,15 @@ const ROOM_FOR_TYPE: Partial${escapeHtml(input.title)}
${escapeHtml(input.body)}${ input.link ? `
${escapeHtml(input.link)}` : '' 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 index 935f03ab4..3b06cc518 100644 --- 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 @@ -5,34 +5,57 @@ import type { DataSource } from 'typeorm'; import { ChatProvisioningService } from './chat-provisioning.service'; import type { MatrixClient } from './matrix.client'; +const NAA = '03f5eb9e-23a0-4413-8d98-8de4b98b1be2'; +const SUPER_ADMIN = 'f1534714-fa4a-4780-a081-05d4c1f6c25f'; +const BOT = '@edrbot:m.test'; + +interface Holder { + positionKey: string; + positionName: string; + userId: string; + userName: string; +} + +const holder = ( + userId: string, + userName: string, + positionKey: string, + positionName = positionKey, +): Holder => ({ positionKey, positionName, userId, userName }); + +/** + * `members` maps a room id to who Matrix currently reports as joined, so a + * test can put a leaver in a room and watch what the reconcile does about it. + */ +function harness(holders: Holder[], members: Record = {}) { + const matrix = { + mxidFor: jest.fn( + (userId: string, name: string) => `@${name}.${userId.slice(0, 6)}:m.test`, + ), + whoami: jest.fn(async () => BOT), + 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), + joinedMembers: jest.fn(async (roomId: string) => members[roomId] ?? [BOT]), + kick: jest.fn(async (_roomId: string, _mxid: string, _reason: string) => undefined), + lockUser: jest.fn(async (_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, dataSource }; +} + /** * `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. + * nightly. */ 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 @@ -46,17 +69,12 @@ describe('ChatProvisioningService.joinUserRooms', () => { expect(matrix.ensureJoined).not.toHaveBeenCalled(); }); - it('joins a position holder to the space, #general and their dept room', async () => { + it('joins a holder to the space, #general, #freight-alerts and their dept room', async () => { const { service, matrix } = harness([ - { - positionKey: 'edr_freight_app/marketer', - positionName: 'Marketer', - userId: NAA, - userName: 'naa', - }, + holder(NAA, 'naa', 'edr_freight_app/marketer', 'Marketer'), ]); - await expect(service.joinUserRooms(NAA, 'naa')).resolves.toBe(2); + await expect(service.joinUserRooms(NAA, 'naa')).resolves.toBe(4); // The account has to exist before the admin join API will touch it — JWT // auto-registration happens after this runs. @@ -65,28 +83,112 @@ describe('ChatProvisioningService.joinUserRooms', () => { expect(matrix.ensureRoom.mock.calls.map(([alias]) => alias)).toEqual([ 'edr-freight', 'general', + 'freight-alerts', '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. + // every dept room loose into Home. #freight-alerts is joined here too, or + // a new hire sees no bridged notification until the nightly reconcile. expect(matrix.ensureJoined.mock.calls.map(([roomId]) => roomId)).toEqual([ '!edr-freight:m.test', '!general:m.test', + '!freight-alerts:m.test', '!dept-edr_freight_app/marketer:m.test', ]); }); it('scopes the position lookup to the one user', async () => { - const { service } = harness([]); + const { service, dataSource } = 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]; + const [sql, params] = dataSource.query.mock.calls[0] as unknown as [ + string, + unknown[], + ]; expect(sql).toContain('AND e.user_id = $3'); expect(params).toEqual(['edr_freight', 'edr_freight_app', NAA]); }); }); + +describe('ChatProvisioningService.reconcile', () => { + it('aborts instead of emptying every room when the holder query returns nothing', async () => { + // Zero holders never means "every employee left at once" — it means the + // query failed, the org/unit keys drifted, or a migration is mid-flight. + // Acting on it would kick every member of every room and lock every + // account, which is exactly the outage this guard exists to prevent. + const { service, matrix } = harness([]); + + await expect(service.reconcile()).rejects.toThrow(/no current position holders/i); + + expect(matrix.kick).not.toHaveBeenCalled(); + expect(matrix.lockUser).not.toHaveBeenCalled(); + }); + + it('locks a departed member rather than deactivating them', async () => { + const leaver = '@gone.999999:m.test'; + const { service, matrix } = harness( + [holder(NAA, 'naa', 'marketer', 'Marketer')], + { + '!edr-freight:m.test': [BOT, '@naa.03f5eb:m.test', leaver], + '!general:m.test': [BOT, '@naa.03f5eb:m.test', leaver], + '!freight-alerts:m.test': [BOT, '@naa.03f5eb:m.test'], + '!dept-marketer:m.test': [BOT, '@naa.03f5eb:m.test'], + }, + ); + + const result = await service.reconcile(); + + expect(matrix.kick.mock.calls.map(([, mxid]) => mxid)).toEqual([leaver, leaver]); + // Locking is reversible; deactivation is not, and on a homeserver with no + // password login it cannot be undone at all. + expect(matrix.lockUser).toHaveBeenCalledTimes(1); + expect(matrix.lockUser).toHaveBeenCalledWith(leaver); + expect(result.locked).toBe(1); + }); + + it('does not lock someone who only moved between positions', async () => { + const naaMxid = '@naa.03f5eb:m.test'; + // naa holds `marketer` now; the room for their old position still lists them. + const { service, matrix } = harness( + [ + holder(NAA, 'naa', 'marketer', 'Marketer'), + holder('aaa04914-b7ee-47b3-9c63-4324046a26bd', 'nati', 'opn', 'Operation'), + ], + { '!dept-opn:m.test': [BOT, naaMxid, '@nati.aaa049:m.test'] }, + ); + + const result = await service.reconcile(); + + expect(matrix.kick).toHaveBeenCalledWith( + '!dept-opn:m.test', + naaMxid, + expect.any(String), + ); + // Kicked from one room, still current elsewhere — their account stays open. + expect(matrix.lockUser).not.toHaveBeenCalled(); + expect(result.locked).toBe(0); + }); + + it('refuses to empty a populated room when its desired set is empty', async () => { + // Per-room backstop for the paths the unit-level guard above cannot see. + const { service, matrix } = harness([holder(NAA, 'naa', 'marketer')], { + '!room:m.test': [BOT, '@naa.03f5eb:m.test', '@nati.aaa049:m.test'], + }); + + const diff = await ( + service as unknown as { + syncMembership: ( + roomId: string, + desired: Set, + bot: string, + ) => Promise<{ joined: number; kicked: string[] }>; + } + ).syncMembership('!room:m.test', new Set(), BOT); + + expect(diff).toEqual({ joined: 0, kicked: [] }); + expect(matrix.kick).not.toHaveBeenCalled(); + }); +}); 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 23d05d8e6..102cb071a 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 @@ -13,6 +13,11 @@ const UNIT_KEY = 'edr_freight_app'; const SPACE_ALIAS = 'edr-freight'; const GENERAL_ALIAS = 'general'; +/** Where ChatBridgeService mirrors backoffice notifications. Provisioned here, + * with every position holder in it, so bridged messages land somewhere staff + * actually are — the bridge only ever get-or-creates it as a safety net. */ +export const ALERTS_ROOM = { alias: 'freight-alerts', name: 'Freight Alerts' }; + interface PositionHolder { positionKey: string; positionName: string; @@ -24,7 +29,8 @@ export interface ReconcileResult { rooms: number; joined: number; kicked: number; - deactivated: number; + /** Departed accounts locked — reversible. See {@link MatrixClient.lockUser}. */ + locked: number; } /** @@ -55,7 +61,7 @@ export class ChatProvisioningService { const result = await this.reconcile(); this.logger.log( `Chat reconcile: ${result.rooms} room(s), ${result.joined} joined, ` + - `${result.kicked} kicked, ${result.deactivated} deactivated`, + `${result.kicked} kicked, ${result.locked} locked`, ); } catch (err) { // Never throws into the scheduler — chat provisioning must not be able @@ -122,6 +128,14 @@ export class ChatProvisioningService { parentSpaceId: spaceId, }); await this.matrix.ensureJoined(generalRoomId, mxid); + // Without this a new hire sees no bridged notification until the nightly + // reconcile puts them in the alerts room. + const alertsRoomId = await this.matrix.ensureRoom( + ALERTS_ROOM.alias, + ALERTS_ROOM.name, + { parentSpaceId: spaceId }, + ); + await this.matrix.ensureJoined(alertsRoomId, mxid); for (const position of positions) { const roomId = await this.matrix.ensureRoom( @@ -132,10 +146,10 @@ export class ChatProvisioningService { await this.matrix.ensureJoined(roomId, mxid); } - return positions.length + 1; + return positions.length + 3; // space + general + alerts } - /** Force-joins additions, kicks+deactivates users no longer entitled anywhere. */ + /** Force-joins additions, kicks users no longer entitled to this room. */ private async syncMembership( roomId: string, desiredUserIds: Set, @@ -144,6 +158,19 @@ export class ChatProvisioningService { const current = await this.matrix.joinedMembers(roomId); const currentSet = new Set(current.filter((id) => id !== botMxid)); + // An empty desired set against a populated room is not "everyone left" — + // it is a query that failed, a key that drifted, or a migration caught + // mid-flight. Acting on it would clear the room and then lock every + // account that was in it. {@link reconcile} guards the same shape at the + // unit level; this is the per-room backstop for the paths it cannot see. + if (desiredUserIds.size === 0 && currentSet.size > 0) { + this.logger.warn( + `Refusing to empty room ${roomId}: desired membership is empty while ` + + `${currentSet.size} member(s) are joined. Left untouched.`, + ); + return { joined: 0, kicked: [] }; + } + let joined = 0; for (const userId of desiredUserIds) { if (!currentSet.has(userId)) { @@ -165,6 +192,17 @@ export class ChatProvisioningService { async reconcile(): Promise { const holders = await this.currentHolders(); + // The desired state for the whole unit. Empty means the IAM query failed, + // the org/unit keys drifted, or a migration is mid-flight — it never means + // every employee left at once. Continuing would kick every member of every + // room and lock every account, so refuse the run and keep yesterday's + // state, which is wrong at worst by a day. + if (holders.length === 0) { + throw new Error( + `Chat reconcile aborted: no current position holders for ${ORG_KEY}/${UNIT_KEY}. ` + + 'Refusing to read that as "remove everyone".', + ); + } const botMxid = await this.matrix.whoami(); const spaceId = await this.matrix.ensureRoom(SPACE_ALIAS, 'EDR Freight', { @@ -173,6 +211,11 @@ export class ChatProvisioningService { const generalRoomId = await this.matrix.ensureRoom(GENERAL_ALIAS, 'General', { parentSpaceId: spaceId, }); + const alertsRoomId = await this.matrix.ensureRoom( + ALERTS_ROOM.alias, + ALERTS_ROOM.name, + { parentSpaceId: spaceId }, + ); const allUserIds = new Set( holders.map((h) => this.matrix.mxidFor(h.userId, h.userName)), @@ -189,12 +232,12 @@ export class ChatProvisioningService { await this.matrix.ensureUser(mxid, h.userName); } - let rooms = 2; // space + general + let rooms = 3; // space + general + alerts let joined = 0; let kicked = 0; // A user kicked from anything while holding zero current positions // anywhere in the unit (allUserIds spans every position) is a full - // leaver, not just moved between positions — deactivate their account. + // leaver, not just moved between positions — lock their account. const kickedUserIds = new Set(); // Space membership follows the org tree exactly like room membership — @@ -210,6 +253,11 @@ export class ChatProvisioningService { kicked += generalDiff.kicked.length; generalDiff.kicked.forEach((uid) => kickedUserIds.add(uid)); + const alertsDiff = await this.syncMembership(alertsRoomId, allUserIds, botMxid); + joined += alertsDiff.joined; + kicked += alertsDiff.kicked.length; + alertsDiff.kicked.forEach((uid) => kickedUserIds.add(uid)); + const byPosition = new Map }>(); for (const h of holders) { const entry = byPosition.get(h.positionKey) ?? { @@ -232,19 +280,19 @@ export class ChatProvisioningService { diff.kicked.forEach((uid) => kickedUserIds.add(uid)); } - let deactivated = 0; + let locked = 0; for (const userId of kickedUserIds) { if (allUserIds.has(userId)) continue; // moved position, still current elsewhere try { - await this.matrix.deactivateUser(userId); - deactivated += 1; + await this.matrix.lockUser(userId); + locked += 1; } catch (err) { this.logger.warn( - `Failed to deactivate departed user ${userId}: ${(err as Error).message}`, + `Failed to lock departed user ${userId}: ${(err as Error).message}`, ); } } - return { rooms, joined, kicked, deactivated }; + return { rooms, joined, kicked, locked }; } } 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 3fd203543..22cf8afd8 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 @@ -121,6 +121,35 @@ describe('MatrixClient.verifyServerAdmin', () => { }); }); +describe('MatrixClient.ensureUser', () => { + it('lifts the lock on a returning employee', async () => { + // A previous reconcile locked them as a leaver. Force-joining them back + // into rooms while they still cannot log in is a silent half-restore. + fetchMock + .mockResolvedValueOnce( + response(200, { name: '@naa.03f5eb:matrix.test', locked: true }), + ) + .mockResolvedValueOnce(response(200, {})); + + await new MatrixClient(config).ensureUser('@naa.03f5eb:matrix.test', 'naa'); + + expect(fetchMock).toHaveBeenCalledTimes(2); + const [url, init] = fetchMock.mock.calls[1] as [string, { body: string }]; + expect(String(url)).toContain('/_synapse/admin/v2/users/'); + expect(JSON.parse(init.body)).toEqual({ locked: false }); + }); + + it('leaves an account that is not locked alone', async () => { + fetchMock.mockResolvedValueOnce( + response(200, { name: '@naa.03f5eb:matrix.test', locked: false }), + ); + + await new MatrixClient(config).ensureUser('@naa.03f5eb:matrix.test', 'naa'); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +}); + describe('MatrixClient rate limiting', () => { it('retries a 429 after the delay Synapse asks for', async () => { // The dev outage: a reconcile is a burst of writes, Synapse throttled an 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 ec58fe7ce..4da85b339 100644 --- a/apps/edr-freight-api/src/modules/chat/matrix.client.ts +++ b/apps/edr-freight-api/src/modules/chat/matrix.client.ts @@ -366,11 +366,18 @@ export class MatrixClient implements OnApplicationBootstrap { * ("User not found") on an account that doesn't exist yet. */ async ensureUser(userId: string, displayName?: string): Promise { - const existing = await this.requestOrNull<{ name: string }>( + const existing = await this.requestOrNull<{ name: string; locked?: boolean }>( 'GET', `/_synapse/admin/v2/users/${encodeURIComponent(userId)}`, ); - if (existing) return; + if (existing) { + // A returning employee is still locked from the reconcile that saw them + // leave. Force-joining them into rooms while they cannot log in is a + // silent half-restore, and this is the one call that already knows the + // flag — so undo it here rather than making the caller ask again. + if (existing.locked) await this.setLocked(userId, false); + return; + } await this.request( 'PUT', `/_synapse/admin/v2/users/${encodeURIComponent(userId)}`, @@ -410,12 +417,30 @@ export class MatrixClient implements OnApplicationBootstrap { ); } - /** Deactivating (rather than just kicking) a leaver's account revokes all their sessions. */ - deactivateUser(userId: string): Promise { + /** + * Lock a departed employee out of chat — reversible, unlike deactivation. + * + * This used to call `/_synapse/admin/v1/deactivate`. That revokes sessions + * the same way but cannot be undone in any useful sense on this deployment: + * reactivation wants a password, and `password_config.enabled: false` means + * there is none to set. Room memberships do not come back either. One bad + * reconcile — a half-applied IAM migration, a renamed org key — would have + * destroyed every staff account that way, permanently. + * + * Locking blocks exactly the same access (Synapse rejects the account's + * tokens with M_USER_LOCKED and refuses new logins) and is undone with a + * single PUT — see {@link ensureUser}, which lifts it automatically when + * someone comes back. + */ + lockUser(userId: string): Promise { + return this.setLocked(userId, true); + } + + private setLocked(userId: string, locked: boolean): Promise { return this.request( - 'POST', - `/_synapse/admin/v1/deactivate/${encodeURIComponent(userId)}`, - { erase: false }, + 'PUT', + `/_synapse/admin/v2/users/${encodeURIComponent(userId)}`, + { locked }, ); }