From 0d1720a99b5116bd8c133d9848fea06a2c52a26f Mon Sep 17 00:00:00 2001 From: Nathnael Date: Tue, 1 Sep 2026 05:51:48 +0000 Subject: [PATCH 1/5] fix: matrix --- .../src/modules/chat/matrix.client.spec.ts | 33 ++++++++++++++++ .../src/modules/chat/matrix.client.ts | 39 +++++++++++++++++-- 2 files changed, 69 insertions(+), 3 deletions(-) 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 73798bce9..3fd203543 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,39 @@ describe('MatrixClient.verifyServerAdmin', () => { }); }); +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 + // m.space.child PUT, and one un-retried 429 threw the whole run away. + fetchMock + .mockResolvedValueOnce(response(200, { user_id: '@edrbot:matrix.test' })) + .mockResolvedValueOnce( + response(429, { + errcode: 'M_LIMIT_EXCEEDED', + error: 'Too Many Requests', + retry_after_ms: 1, + }), + ) + .mockResolvedValueOnce(response(200, { users: [] })); + + const check = await new MatrixClient(config).verifyServerAdmin(); + + expect(check.ok).toBe(true); + expect(fetchMock).toHaveBeenCalledTimes(3); + }); + + it('gives up rather than hanging on a homeserver that only ever 429s', async () => { + fetchMock.mockResolvedValue( + response(429, { errcode: 'M_LIMIT_EXCEEDED', retry_after_ms: 1 }), + ); + + const check = await new MatrixClient(config).verifyServerAdmin(); + + expect(check.ok).toBe(false); + expect(check.error).toContain('429'); + }); +}); + describe('MatrixClient.adminCheck', () => { it('does not re-hit Synapse on every readiness probe', async () => { fetchMock 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 fdfab052d..ec58fe7ce 100644 --- a/apps/edr-freight-api/src/modules/chat/matrix.client.ts +++ b/apps/edr-freight-api/src/modules/chat/matrix.client.ts @@ -57,6 +57,9 @@ export class MatrixClient implements OnApplicationBootstrap { /** 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; + /** Enough to ride out Synapse's limiter; short enough that a genuinely + * wedged homeserver still fails the run rather than hanging it. */ + private static readonly MAX_RATE_LIMIT_RETRIES = 5; private adminCheckCache?: { at: number; result: AdminCheck }; constructor( @@ -95,13 +98,43 @@ export class MatrixClient implements OnApplicationBootstrap { return this.config.enabled; } + /** + * Synapse answers a burst of writes with 429 + `retry_after_ms`, and a + * reconcile is nothing but a burst of writes — one run creates the space, + * #general and a room per position, then force-joins every holder into each. + * The first run against dev tripped the limiter on an `m.space.child` PUT, + * and because nothing retried, that single 429 threw the whole reconcile + * away mid-flight. On the sign-in path ChatSsoService swallows the throw, so + * the only visible symptom was an empty Element. + * + * Honour the delay Synapse asks for rather than guessing at one. + */ + private async fetchWithRetry( + url: string, + init: Parameters[1], + ): Promise>> { + for (let attempt = 0; ; attempt++) { + const res = await fetch(url, init); + if (res.status !== 429 || attempt >= MatrixClient.MAX_RATE_LIMIT_RETRIES) { + return res; + } + // Body is discarded either way — this response is being retried. + const body = (await res.json().catch(() => ({}))) as { + retry_after_ms?: number; + }; + await new Promise((resolve) => + setTimeout(resolve, (Number(body.retry_after_ms) || 1000) + 100), + ); + } + } + private async request( method: string, path: string, body?: unknown, token: string = this.config.adminToken, ): Promise { - const res = await fetch(`${this.config.baseUrl}${path}`, { + const res = await this.fetchWithRetry(`${this.config.baseUrl}${path}`, { method, headers: { 'Content-Type': 'application/json', @@ -125,7 +158,7 @@ export class MatrixClient implements OnApplicationBootstrap { path: string, body: unknown, ): Promise { - const res = await fetch(`${this.config.baseUrl}${path}`, { + const res = await this.fetchWithRetry(`${this.config.baseUrl}${path}`, { method, headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(body), @@ -145,7 +178,7 @@ export class MatrixClient implements OnApplicationBootstrap { path: string, token?: string, ): Promise { - const res = await fetch(`${this.config.baseUrl}${path}`, { + const res = await this.fetchWithRetry(`${this.config.baseUrl}${path}`, { method, headers: { Authorization: `Bearer ${token ?? this.config.adminToken}` }, }); From 2dfea96cb8e6ca23b55a09665874d5d25664570e Mon Sep 17 00:00:00 2001 From: Nathnael Date: Tue, 1 Sep 2026 06:28:55 +0000 Subject: [PATCH 2/5] fix(chat): retry rate limits, lock instead of deactivate, guard reconcile Provisioning created nothing on dev. Synapse answers a burst of writes with 429 + retry_after_ms, and a reconcile is nothing but a burst of writes, so the first throttled m.space.child PUT threw and took the whole run with it. On the sign-in path ChatSsoService catches that by design, so every employee got a working sign-in into an Element with no rooms in it. fetchWithRetry now sits behind all three request wrappers and honours the delay Synapse asks for, capped at 5 attempts so a wedged homeserver still fails rather than hangs. Two ways the reconcile could destroy state, both now blocked: - Zero position holders meant "remove everyone": every member kicked from every room, then every account deactivated. It never means that - it means the IAM query failed, the org/unit keys drifted, or a migration is mid-flight. reconcile() aborts, and syncMembership() refuses to empty a populated room, as a per-room backstop. - Deactivation could not be undone here. Reactivation wants a password and password_config.enabled is false, and room memberships do not come back. Departed accounts are locked instead - same access block, one PUT to reverse - and ensureUser lifts the lock when someone returns in IAM. Also: join the space itself, not only the rooms inside it, or Element leaves every dept room loose in Home and never shows the space at all. And drop the bridge's per-type routing - it pointed at a hardcoded dept-operation alias while the reconcile derives dept-${positionKey} from IAM, so bridged notifications went to a room the bridge created and nobody was in. Co-Authored-By: Claude Opus 5 (1M context) --- .../modules/chat/chat-bridge.service.spec.ts | 78 ++++++++ .../src/modules/chat/chat-bridge.service.ts | 35 ++-- .../chat/chat-provisioning.service.spec.ts | 174 ++++++++++++++---- .../modules/chat/chat-provisioning.service.ts | 70 +++++-- .../src/modules/chat/matrix.client.spec.ts | 29 +++ .../src/modules/chat/matrix.client.ts | 39 +++- 6 files changed, 350 insertions(+), 75 deletions(-) create mode 100644 apps/edr-freight-api/src/modules/chat/chat-bridge.service.spec.ts 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 }, ); } From 3f2bcf1a1acf558fae4fc7c208706f742f19bca7 Mon Sep 17 00:00:00 2001 From: Nathnael Date: Tue, 1 Sep 2026 07:01:27 +0000 Subject: [PATCH 3/5] fix(customers): drop fake before-state in history diff rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DiffRow rendered onboarding's initial field values as '— → value', implying a prior state that never existed. Show the value alone when there is no real before. Co-Authored-By: Claude Sonnet 5 --- .../customers/ChangeRequestReview.tsx | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/apps/edr-freight-web/backoffice/src/components/customers/ChangeRequestReview.tsx b/apps/edr-freight-web/backoffice/src/components/customers/ChangeRequestReview.tsx index 7f4cf30ce..88cbc1769 100644 --- a/apps/edr-freight-web/backoffice/src/components/customers/ChangeRequestReview.tsx +++ b/apps/edr-freight-web/backoffice/src/components/customers/ChangeRequestReview.tsx @@ -174,6 +174,10 @@ export function DiffRow({ from: string; to: string; }) { + // No real "before" (field went from unset straight to a value, e.g. the + // onboarding wizard's first save) — show the value alone rather than a + // fake "— → value" that implies a prior state that never existed. + const hadBefore = from !== "—"; const changed = from !== to; return ( @@ -181,24 +185,24 @@ export function DiffRow({ {label} - - {from} - - {changed && ( - <> - - → - - - {to} - - + {hadBefore && ( + + {from} + )} + {hadBefore && changed && ( + + → + + )} + + {to} + ); From db9d6e49c748b70438d87878ff5f510fd11dc688 Mon Sep 17 00:00:00 2001 From: Nathnael Date: Tue, 1 Sep 2026 07:03:58 +0000 Subject: [PATCH 4/5] fix(auth): load every position a user holds Staff given a post in Smart Office and another in freight only ever loaded one of them. Two causes, both in how the IAM guard collapses the login snapshot: - `x-current-position-id` is read two ways inside one function: the employee row is matched on `position.id`, the position on `employeePositionId`. Freight sends the latter, Smart Office the former, so whichever value arrives one lookup matches nothing and falls back to `positions[0]`. FreightJwtGuard now matches both fields. - IAM keeps one employee row per organization, and EDR and EDR Freight are separate organizations, so a user holding a post in each owns two rows. Only the active row reached `collectPermissionKeys`, so the freight post's permissions disappeared whenever the other row won the active slot. `employee.positions` now unions every row, which is what the util already does for the array shape. `delegatedPositions` stays scoped to the active row on purpose: yard scope widens on it, and someone standing in on another organization's row is not this desk's stand-in. /auth/me now returns every employee row, active row first, so the position picker can offer a desk that is not on the active row. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/common/freight-jwt.guard.spec.ts | 148 +++++++++++++ .../src/common/freight-jwt.guard.ts | 205 ++++++++++++++---- .../src/modules/auth/freight-me.service.ts | 55 +++-- 3 files changed, 355 insertions(+), 53 deletions(-) create mode 100644 apps/edr-freight-api/src/common/freight-jwt.guard.spec.ts diff --git a/apps/edr-freight-api/src/common/freight-jwt.guard.spec.ts b/apps/edr-freight-api/src/common/freight-jwt.guard.spec.ts new file mode 100644 index 000000000..817e967f8 --- /dev/null +++ b/apps/edr-freight-api/src/common/freight-jwt.guard.spec.ts @@ -0,0 +1,148 @@ +import { + collectAllPositions, + resolveActiveEmployee, + type SnapshotEmployee, +} from './freight-jwt.guard'; + +// Shapes and ids taken from the real dev session for `test_dj_gl_director` +// (iam.sessions 800ad793-…), an employee holding two posts on one row. +const CHIEF = { + id: '990189f1-e872-4b8c-9f6a-36259a0df480', + employeePositionId: 'd0d527f6-f344-49aa-ab8b-25a448a770b6', + name: { en: 'Djibouti GL Chief' }, + isDelegate: false, +}; +const DIRECTOR = { + id: '258a8d82-28c4-401f-bf88-78f58bb6bd0e', + employeePositionId: 'b97aa265-5de8-4ffe-95bf-01f94d38a2df', + name: { en: 'Djibouti GL Director' }, + isDelegate: false, +}; + +const EMPLOYEE_ID = '70545ee5-c7d7-4196-af7e-a7eb7e76b21b'; +const oneRow: SnapshotEmployee[] = [ + { id: EMPLOYEE_ID, positions: [CHIEF, DIRECTOR] }, +]; + +describe('resolveActiveEmployee', () => { + it('leaves the parent guard alone when no position header is sent', () => { + const { owner, active } = resolveActiveEmployee( + oneRow, + undefined, + EMPLOYEE_ID, + ); + + expect(owner).toBe(oneRow[0]); + expect(active).toBeUndefined(); + }); + + it("resolves freight's header value (employeePositionId)", () => { + const { active } = resolveActiveEmployee( + oneRow, + DIRECTOR.employeePositionId, + EMPLOYEE_ID, + ); + + expect(active).toBe(DIRECTOR); + }); + + // The regression this guard exists for: the stock IAM guard matches the + // header against employeePositionId only, so Smart Office's position.id + // matched nothing and every request silently ran as positions[0]. + it("resolves Smart Office's header value (position.id)", () => { + const { active } = resolveActiveEmployee(oneRow, DIRECTOR.id, EMPLOYEE_ID); + + expect(active).toBe(DIRECTOR); + expect(active).not.toBe(CHIEF); + }); + + it('falls back to the parent row when the header names nothing', () => { + const { owner, active } = resolveActiveEmployee( + oneRow, + 'not-a-position-id', + EMPLOYEE_ID, + ); + + expect(owner).toBe(oneRow[0]); + expect(active).toBeUndefined(); + }); + + describe('when the two posts sit on different employee rows', () => { + const smartOfficeRow: SnapshotEmployee = { + id: 'emp-smart-office', + positions: [CHIEF], + }; + const freightRow: SnapshotEmployee = { + id: 'emp-freight', + positions: [DIRECTOR], + }; + const twoRows = [smartOfficeRow, freightRow]; + + it('selects the row that owns the requested position', () => { + const { owner, active } = resolveActiveEmployee( + twoRows, + DIRECTOR.employeePositionId, + // The parent guard matches the header against position.id only, so it + // matched neither row and fell through to the first. + smartOfficeRow.id, + ); + + expect(owner).toBe(freightRow); + expect(active).toBe(DIRECTOR); + }); + + it('keeps the parent row when no header is sent', () => { + const { owner } = resolveActiveEmployee(twoRows, undefined, freightRow.id); + + expect(owner).toBe(freightRow); + }); + + it('falls back to the first row when the parent row is unknown', () => { + const { owner } = resolveActiveEmployee(twoRows, undefined, undefined); + + expect(owner).toBe(smartOfficeRow); + }); + }); +}); + +describe('collectAllPositions', () => { + it('unions posts held across separate employee rows', () => { + // The real shape: IAM keeps one employee row per organization, and "EDR" + // and "EDR Freight" are separate orgs, so a user holding a Smart Office + // post and a freight post owns one row each. + const smartOfficeRow: SnapshotEmployee = { + id: 'emp-edr', + organizationId: 'org-edr', + positions: [CHIEF], + }; + const freightRow: SnapshotEmployee = { + id: 'emp-edr-freight', + organizationId: 'org-edr-freight', + positions: [DIRECTOR], + }; + + expect(collectAllPositions([smartOfficeRow, freightRow])).toEqual([ + CHIEF, + DIRECTOR, + ]); + }); + + it('keeps every post when they share one row', () => { + expect(collectAllPositions(oneRow)).toEqual([CHIEF, DIRECTOR]); + }); + + it('de-duplicates a post repeated across rows', () => { + const rows: SnapshotEmployee[] = [ + { id: 'a', positions: [CHIEF] }, + { id: 'b', positions: [CHIEF, DIRECTOR] }, + ]; + + expect(collectAllPositions(rows)).toEqual([CHIEF, DIRECTOR]); + }); + + it('tolerates rows carrying no positions', () => { + const rows: SnapshotEmployee[] = [{ id: 'a' }, { id: 'b', positions: [] }]; + + expect(collectAllPositions(rows)).toEqual([]); + }); +}); diff --git a/apps/edr-freight-api/src/common/freight-jwt.guard.ts b/apps/edr-freight-api/src/common/freight-jwt.guard.ts index 68b842217..9dbc36b73 100644 --- a/apps/edr-freight-api/src/common/freight-jwt.guard.ts +++ b/apps/edr-freight-api/src/common/freight-jwt.guard.ts @@ -3,29 +3,124 @@ import { Reflector } from '@nestjs/core'; import { InjectDataSource } from '@nestjs/typeorm'; import { JwtGuard as IamJwtGuard } from '@tria-plc/api-common/modules/auth/services/jwt.guard'; import type { TCurrentUser } from '@tria-plc/api-common/modules/auth/types/current-user.type'; +import { CURRENT_POSITION_ID } from '@tria-plc/api-common/utils/constants/tenant.constant'; import { DataSource } from 'typeorm'; /** One position as the login snapshot stores it (`iam.sessions.userInfo`). */ -type SnapshotPosition = { id?: string; [key: string]: unknown }; +export type SnapshotPosition = { + id?: string; + employeePositionId?: string; + isDelegate?: boolean; + [key: string]: unknown; +}; -type SessionUserInfo = { - employee?: { id?: string; positions?: SnapshotPosition[] }[]; +/** One employee row as the snapshot stores it. A user may hold several. */ +export type SnapshotEmployee = { + id?: string; + positions?: SnapshotPosition[]; + [key: string]: unknown; +}; + +type SessionUserInfo = { employee?: SnapshotEmployee[] }; + +/** + * `x-current-position-id` is sent with two different meanings by two different + * frontends, and the IAM guard reads it both ways in the same function: it + * picks the EMPLOYEE row by `position.id` but the POSITION by + * `employeePositionId`. Freight sends `employeePositionId`, Smart Office sends + * `position.id` — so whichever value arrives, one of the two lookups silently + * matches nothing and falls back to the first entry. + * + * Matching both fields is what makes the header mean one thing again. + */ +const identifies = (position: SnapshotPosition, id: string): boolean => + position?.id === id || position?.employeePositionId === id; + +/** + * Every post the user holds, across every employee row, first occurrence kept. + * + * IAM keeps one employee row per ORGANIZATION, and "EDR" and "EDR Freight" are + * separate organizations — so a user given a freight post and a Smart Office + * post owns two rows, one post on each. Only one row can be the active one, and + * a permission check that reads only that row cannot see the other post at all. + */ +export const collectAllPositions = ( + employees: SnapshotEmployee[], +): SnapshotPosition[] => { + const seen = new Set(); + const all: SnapshotPosition[] = []; + + for (const employee of employees) { + for (const position of employee.positions ?? []) { + const key = position.employeePositionId ?? position.id; + if (key) { + if (seen.has(key)) continue; + seen.add(key); + } + all.push(position); + } + } + + return all; }; /** - * Like the IAM JwtGuard, but keeps the caller's SECONDARY positions. + * Which employee row the caller is acting as, and which of its positions the + * request selected. Pure so it can be tested without a session or a token. * - * IAM models an employee as holding many positions, and the login snapshot in - * `iam.sessions.userInfo` carries all of them. `JwtGuard.parseToken` then - * collapses that to a single `employee.position` — whichever the request - * headers select, else `positions[0]` — and drops the rest. Non-delegate - * secondary positions vanish entirely, so staff holding two posts resolve to - * only one post's permissions and every check on the other one rejects them. + * `owner` is the row holding the requested position; failing that the row the + * parent guard already picked; failing that the first. `active` is undefined + * when no header was sent or it names nothing — the caller then leaves the + * parent's choice of `employee.position` alone. + */ +export const resolveActiveEmployee = ( + employees: SnapshotEmployee[], + requestedId: string | undefined, + parentEmployeeId: string | undefined, +): { owner: SnapshotEmployee | undefined; active: SnapshotPosition | undefined } => { + const owner = + (requestedId && + employees.find((candidate) => + (candidate.positions ?? []).some((position) => + identifies(position, requestedId), + ), + )) || + employees.find( + (candidate) => candidate.id && candidate.id === parentEmployeeId, + ) || + employees[0]; + + const active = requestedId + ? (owner?.positions ?? []).find((position) => + identifies(position, requestedId), + ) + : undefined; + + return { owner, active }; +}; + +/** + * Like the IAM JwtGuard, but resolves the caller's position honestly. * - * This re-attaches the full list as `employee.positions`. `employee.position` - * is left exactly as the parent set it, so everything reading the single - * position today (audit log, delegation deadline) is unaffected; only the - * permission utils, which prefer the array, see the difference. + * IAM models an employee as holding many positions — and a user as possibly + * holding several employee rows — and the login snapshot in + * `iam.sessions.userInfo` carries all of them. `JwtGuard.parseToken` collapses + * that to a single `employee.position` and drops the rest, so staff holding two + * posts resolve to one post's permissions and every check on the other one + * rejects them. + * + * This guard re-reads the snapshot and fixes three things the parent gets wrong: + * + * 1. re-attaches the full position list as `employee.positions`, which is what + * the permission utils union over; + * 2. selects the employee row that actually owns the requested position, so a + * post held on a second employee row is reachable at all; + * 3. sets `employee.position` to the requested position when the parent's + * one-sided id match missed it, keeping `auditUser` in step. + * + * Every correction is skipped unless the snapshot positively resolves it, so an + * unreadable session degrades to the parent's single-position behaviour rather + * than to no position at all. */ @Injectable() export class FreightJwtGuard extends IamJwtGuard implements CanActivate { @@ -35,7 +130,7 @@ export class FreightJwtGuard extends IamJwtGuard implements CanActivate { private static readonly CACHE_MAX_ENTRIES = 5_000; private readonly cache = new Map< string, - { positions: SnapshotPosition[]; expiresAt: number } + { employees: SnapshotEmployee[]; expiresAt: number } >(); constructor( @@ -48,44 +143,78 @@ export class FreightJwtGuard extends IamJwtGuard implements CanActivate { async canActivate(context: ExecutionContext): Promise { if (!(await super.canActivate(context))) return false; - const user = context.switchToHttp().getRequest().user as - | TCurrentUser - | undefined; - const employee = user?.employee; + const request = context.switchToHttp().getRequest(); + const user = request.user as TCurrentUser | undefined; + const employee = user?.employee as SnapshotEmployee | undefined; if (!employee || !user?.sessionId) return true; - const positions = await this.positionsForSession( - user.sessionId, + const employees = await this.employeesForSession(user.sessionId); + if (!employees.length) return true; + + const requestedId = request.headers?.[CURRENT_POSITION_ID] as + | string + | undefined; + + const { owner, active } = resolveActiveEmployee( + employees, + requestedId, employee.id, ); - // Never blank out what the parent resolved: an unreadable session or a - // snapshot without positions must degrade to the single-position - // behaviour, not to no positions at all. - if (positions.length) { - (employee as { positions?: SnapshotPosition[] }).positions = positions; + + const ownerPositions = owner?.positions ?? []; + // Never blank out what the parent resolved: a snapshot without positions + // must degrade to the single-position behaviour, not to no positions. + if (!ownerPositions.length) return true; + + // Carries the owning row's id / unitId / organizationId too, which unit + // scoping downstream reads — a swapped row must be swapped whole. + Object.assign(employee, owner); + + // `collectPermissionKeys` / `collectPositionTypeKeys` union over this, and + // a user's posts can span several employee rows (one per organization), so + // it carries every row's — otherwise a freight post is invisible whenever + // another organization's row wins the active slot. + employee.positions = collectAllPositions(employees); + + // Delegation stays scoped to the active desk: yard scope widens on + // `delegatedPositions`, and someone standing in on another organization's + // row is not this desk's stand-in. + employee.delegatedPositions = ownerPositions.filter( + (position) => position.isDelegate, + ); + + // The full set, for `/auth/me` — the position picker has to be able to + // offer a desk on a row that is not the active one. + (user as { employeeRows?: SnapshotEmployee[] }).employeeRows = employees; + + if (active) { + employee.position = active; + // The parent already built `auditUser` from the position it guessed. + if (request.auditUser) { + request.auditUser.employeeId = employee.id; + request.auditUser.positionId = active.id; + request.auditUser.employeePositionId = active.employeePositionId; + } } + return true; } - /** Every position the login snapshot holds for this employee. */ - private async positionsForSession( + /** Every employee row the login snapshot holds for this session. */ + private async employeesForSession( sessionId: string, - employeeId: string | undefined, - ): Promise { + ): Promise { const now = Date.now(); const hit = this.cache.get(sessionId); - if (hit && hit.expiresAt > now) return hit.positions; + if (hit && hit.expiresAt > now) return hit.employees; - let positions: SnapshotPosition[] = []; + let employees: SnapshotEmployee[] = []; try { const rows: { userInfo: SessionUserInfo | null }[] = await this.ds.query( `SELECT "userInfo" FROM iam.sessions WHERE id = $1`, [sessionId], ); - const employees = rows[0]?.userInfo?.employee ?? []; - const match = - employees.find((e) => e?.id && e.id === employeeId) ?? employees[0]; - positions = match?.positions ?? []; + employees = rows[0]?.userInfo?.employee ?? []; } catch { return []; // iam unreachable — caller keeps the parent's single position } @@ -93,9 +222,9 @@ export class FreightJwtGuard extends IamJwtGuard implements CanActivate { if (this.cache.size >= FreightJwtGuard.CACHE_MAX_ENTRIES) this.cache.clear(); this.cache.set(sessionId, { - positions, + employees, expiresAt: now + FreightJwtGuard.CACHE_TTL_MS, }); - return positions; + return employees; } } diff --git a/apps/edr-freight-api/src/modules/auth/freight-me.service.ts b/apps/edr-freight-api/src/modules/auth/freight-me.service.ts index b897c5fef..0195bba47 100644 --- a/apps/edr-freight-api/src/modules/auth/freight-me.service.ts +++ b/apps/edr-freight-api/src/modules/auth/freight-me.service.ts @@ -3,6 +3,7 @@ import { InjectDataSource } from '@nestjs/typeorm'; import type { TCurrentUser } from '@tria-plc/api-common/modules/auth/types/current-user.type'; import { DataSource } from 'typeorm'; +import type { SnapshotEmployee } from '../../common/freight-jwt.guard'; import { collectPermissionKeys, isSuperAdmin, @@ -82,8 +83,26 @@ export class FreightMeService { ? [employeeRecord.position] : []; - const enrichedPositions = await Promise.all( - rawPositions.map(async (position) => { + // IAM keeps one employee row per organization, so a user holding a freight + // post and a Smart Office post owns two rows. The backoffice reads + // `employee` as an array and the position picker lists what it finds there + // — returning only the active row hides the other desk and makes it + // unselectable. `FreightJwtGuard` leaves the full set here. + const employeeRows = (user as { employeeRows?: SnapshotEmployee[] }) + .employeeRows; + + // Active row first: the backoffice reads `employee[0]` for + // unitId/organizationId, so the desk the caller is acting as must lead. + const rows: SnapshotEmployee[] = employeeRows?.length + ? [ + ...employeeRows.filter((row) => row.id === employeeRecord?.id), + ...employeeRows.filter((row) => row.id !== employeeRecord?.id), + ] + : employeeRecord + ? [{ ...employeeRecord, positions: rawPositions } as SnapshotEmployee] + : []; + + const enrichPosition = async (position: TokenPosition) => { const [positionType, positionTypePermissionKeys] = await Promise.all([ this.lookupPositionType(position.id), this.lookupPositionTypePermissions(position.id), @@ -114,20 +133,24 @@ export class FreightMeService { positionType, }, }; - }), + }; + + const enrichedRows = await Promise.all( + rows.map(async (row) => ({ + row, + positions: await Promise.all( + ((row.positions ?? []) as TokenPosition[]).map(enrichPosition), + ), + })), ); - const employee = employeeRecord - ? [ - { - id: employeeRecord.id, - organizationId: employeeRecord.organizationId, - unitId: employeeRecord.unitId, - name: employeeRecord.name, - positions: enrichedPositions.map((p) => p.position), - }, - ] - : []; + const employee = enrichedRows.map(({ row, positions }) => ({ + id: row.id as string, + organizationId: row.organizationId as string, + unitId: row.unitId as string, + name: row.name, + positions: positions.map((p) => p.position), + })); // `collectPermissionKeys` reads the raw token (position-level only), so // union the type-level grants in — the backoffice prefers this flat list @@ -135,7 +158,9 @@ export class FreightMeService { const permissionKeys = [ ...new Set([ ...collectPermissionKeys(user), - ...enrichedPositions.flatMap((p) => p.positionTypePermissionKeys), + ...enrichedRows.flatMap(({ positions }) => + positions.flatMap((p) => p.positionTypePermissionKeys), + ), ]), ]; From 7fd56161885ca3709e45a2fe3f70f3b20d7b4c28 Mon Sep 17 00:00:00 2001 From: Nathnael Date: Tue, 1 Sep 2026 07:04:10 +0000 Subject: [PATCH 5/5] fix(backoffice): stop sharing the position cookie with Smart Office MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both apps wrote a cookie named `current-position-id` but stored different ids in it — freight the `employeePositionId`, Smart Office the `position.id`. On a shared domain each login overwrote the other's desk selection, and the loser silently fell back to the first position. Freight now uses `freight-current-position-id` through a small helper that reads the old name once, so a session live across the deploy keeps its desk, and clears it on every write. Also mounts the position switcher in the freight dashboard header. It only existed under /performance-management, so on every other page a two-desk user had no way to switch and was stuck on whatever `useAuthUser` defaulted to. It now hides below two positions rather than showing a one-option dropdown to the single-desk majority. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/modules/audit/audit.interceptor.ts | 43 +++++++------------ .../backoffice/src/auth/cookies.ts | 4 ++ .../layout/FreightDashboardHeader.tsx | 5 +++ .../components/positionSelection.tsx | 17 +++++--- .../services/api/signatureAndTeeterService.ts | 4 +- .../services/api/withHeaders.tsx | 4 +- .../src/shared/hooks/useAuthUser.ts | 15 ++++--- .../src/shared/utils/faydaAuthSession.ts | 3 +- .../src/shared/utils/positionCookie.ts | 39 +++++++++++++++++ 9 files changed, 92 insertions(+), 42 deletions(-) create mode 100644 apps/edr-freight-web/backoffice/src/shared/utils/positionCookie.ts diff --git a/apps/edr-freight-api/src/modules/audit/audit.interceptor.ts b/apps/edr-freight-api/src/modules/audit/audit.interceptor.ts index cf12fd4d3..8b5c82e9c 100644 --- a/apps/edr-freight-api/src/modules/audit/audit.interceptor.ts +++ b/apps/edr-freight-api/src/modules/audit/audit.interceptor.ts @@ -4,24 +4,17 @@ import { HttpException, Injectable, NestInterceptor, -} from '@nestjs/common'; -import { Observable, tap } from 'rxjs'; -import type { Request, Response } from 'express'; +} from "@nestjs/common"; +import { Observable, tap } from "rxjs"; +import type { Request, Response } from "express"; -import { AuditService } from './audit.service'; -import { - auditEndpointMatcher, - type MatchedAuditEndpoint, -} from './audit-endpoint-matcher'; -import { - isAuditableActor, - resolveAuditActor, - type AuditActorSource, -} from './audit-actor'; -import { redactUrlQuery, sanitizeRequestPayload } from './audit.sanitizer'; +import { AuditService } from "./audit.service"; +import { auditEndpointMatcher, type MatchedAuditEndpoint } from "./audit-endpoint-matcher"; +import { isAuditableActor, resolveAuditActor, type AuditActorSource } from "./audit-actor"; +import { redactUrlQuery, sanitizeRequestPayload } from "./audit.sanitizer"; /** Methods that can change state. Everything else is never audited. */ -const AUDITED_METHODS = new Set(['POST', 'PUT', 'PATCH', 'DELETE']); +const AUDITED_METHODS = new Set(["POST", "PUT", "PATCH", "DELETE"]); /** `error_message` ceiling — stack traces do not belong in this column. */ const MAX_ERROR_LENGTH = 2_000; @@ -52,7 +45,7 @@ export class AuditInterceptor implements NestInterceptor { intercept(context: ExecutionContext, next: CallHandler): Observable { // Non-HTTP contexts (the RabbitMQ microservice transport) have no request. - if (context.getType() !== 'http') return next.handle(); + if (context.getType() !== "http") return next.handle(); const httpContext = context.switchToHttp(); const request = httpContext.getRequest(); @@ -72,10 +65,7 @@ export class AuditInterceptor implements NestInterceptor { const startedAt = Date.now(); // The body is captured up front: handlers are free to mutate the DTO they // are given, so reading it after the fact can record post-mutation values. - const requestPayload = sanitizeRequestPayload( - request.body, - request.files ?? request.file, - ); + const requestPayload = sanitizeRequestPayload(request.body, request.files ?? request.file); return next.handle().pipe( tap({ @@ -137,7 +127,6 @@ export class AuditInterceptor implements NestInterceptor { resourceId: matched.resourceId, request: requestPayload, ipAddress: resolveIp(request), - userAgent: request.headers['user-agent'] ?? null, requestId: resolveRequestId(request), durationMs: Date.now() - startedAt, }); @@ -154,10 +143,10 @@ function resolveErrorMessage(error: unknown): string | null { if (error instanceof HttpException) { const response = error.getResponse(); const message = - typeof response === 'string' + typeof response === "string" ? response : ((response as { message?: unknown })?.message ?? error.message); - const text = Array.isArray(message) ? message.join('; ') : String(message); + const text = Array.isArray(message) ? message.join("; ") : String(message); return text.slice(0, MAX_ERROR_LENGTH); } @@ -171,19 +160,19 @@ function resolveErrorMessage(error: unknown): string | null { * entry (the original client) taken. */ function resolveIp(request: Request): string | null { - const forwarded = request.headers['x-forwarded-for']; + const forwarded = request.headers["x-forwarded-for"]; const raw = Array.isArray(forwarded) ? forwarded[0] : forwarded; - const candidate = raw?.split(',')[0]?.trim() || request.ip; + const candidate = raw?.split(",")[0]?.trim() || request.ip; if (!candidate) return null; // Normalize IPv4-mapped IPv6 (`::ffff:10.0.0.1`), which the `inet` column // accepts but which reads badly and breaks grouping by address. - return candidate.startsWith('::ffff:') ? candidate.slice(7) : candidate; + return candidate.startsWith("::ffff:") ? candidate.slice(7) : candidate; } /** Correlation id from the proxy/tracing layer, when present. */ function resolveRequestId(request: RequestWithUser): string | null { - const header = request.headers['x-request-id'] ?? request.headers['x-correlation-id']; + const header = request.headers["x-request-id"] ?? request.headers["x-correlation-id"]; const value = Array.isArray(header) ? header[0] : header; return (value ?? request.id ?? null)?.toString().slice(0, 64) ?? null; } diff --git a/apps/edr-freight-web/backoffice/src/auth/cookies.ts b/apps/edr-freight-web/backoffice/src/auth/cookies.ts index 0292f7f38..dd7edc5a4 100644 --- a/apps/edr-freight-web/backoffice/src/auth/cookies.ts +++ b/apps/edr-freight-web/backoffice/src/auth/cookies.ts @@ -1,3 +1,5 @@ +import { POSITION_COOKIE } from "@/shared/utils/positionCookie"; + const DEFAULT_PATH = "/"; const SEVEN_DAYS_IN_SECONDS = 60 * 60 * 24 * 7; @@ -32,6 +34,8 @@ export const clearSessionCookies = () => { AUTH_TOKEN_COOKIE, REFRESH_TOKEN_COOKIE, AUTH_USER_COOKIE, + POSITION_COOKIE, + // Pre-rename name, still cleared so a stale value cannot outlive logout. "current-position-id", "selected-position-id", ].forEach(clearCookie); diff --git a/apps/edr-freight-web/backoffice/src/components/layout/FreightDashboardHeader.tsx b/apps/edr-freight-web/backoffice/src/components/layout/FreightDashboardHeader.tsx index 0144db306..45c2f36f1 100644 --- a/apps/edr-freight-web/backoffice/src/components/layout/FreightDashboardHeader.tsx +++ b/apps/edr-freight-web/backoffice/src/components/layout/FreightDashboardHeader.tsx @@ -22,6 +22,7 @@ import { type ReactNode } from "react"; import { useNavigate } from "react-router-dom"; import DocReviewAlertButton from "@/features/bookingWindows/DocReviewAlertButton"; +import { PositionSelect } from "@/record-management/components/positionSelection"; import NotificationBellContainer from "@/features/notifications/NotificationBellContainer"; import type { PageMeta } from "./types"; @@ -111,6 +112,10 @@ const FreightDashboardHeader = ({ renders only during a review phase that still has undecided requests, so it never competes for space otherwise. */} + {/* Staff holding two posts switch desks here. Renders nothing for the + single-position majority, so it costs the header no space. */} + + diff --git a/apps/edr-freight-web/backoffice/src/record-management/components/positionSelection.tsx b/apps/edr-freight-web/backoffice/src/record-management/components/positionSelection.tsx index b014425e5..6e904066a 100644 --- a/apps/edr-freight-web/backoffice/src/record-management/components/positionSelection.tsx +++ b/apps/edr-freight-web/backoffice/src/record-management/components/positionSelection.tsx @@ -18,6 +18,10 @@ import { TooltipTrigger, } from "@/shared/common/ui/tooltip"; import { PositionName } from "../dto/delegation/delegationDto"; +import { + getPositionCookie, + setPositionCookie, +} from "@/shared/utils/positionCookie"; interface BasePosition { id: string; employeePositionId: string; @@ -55,7 +59,7 @@ export const PositionSelect = () => { ? unFilteredUserDetails.employee.flatMap((emp) => emp?.positions ?? []) : []; const selectablePositions = allPositions ?? []; - const currentPositionCookie = Cookies.get("current-position-id"); + const currentPositionCookie = getPositionCookie(); const delegatedPositionCookie = Cookies.get("delegatedPositionId"); const activePositionId = selectedPositionId || currentPositionCookie || delegatedPositionCookie; @@ -85,15 +89,18 @@ export const PositionSelect = () => { // (useAuthUser already self-heals this cookie for the same reason.) if ( currentPosition.employeePositionId && - Cookies.get("current-position-id") !== currentPosition.employeePositionId + getPositionCookie() !== currentPosition.employeePositionId ) { - Cookies.set("current-position-id", currentPosition.employeePositionId); + setPositionCookie(currentPosition.employeePositionId); } applyDelegationCookie(currentPosition); }, [currentPosition, isLoading, selectedPositionId, setSelectedPositionId]); - if (isLoading || selectablePositions.length === 0) return null; + // Below two desks there is nothing to switch between. This now sits in the + // main freight header, so a one-option dropdown would show for every + // single-desk staff member. + if (isLoading || selectablePositions.length < 2) return null; const handleChange = (value: string) => { const selected = selectablePositions.find((pos) => pos.id === value); @@ -102,7 +109,7 @@ export const PositionSelect = () => { // dropdown's own value is position.id, which the API does not match on. setSelectedPositionId(selected?.employeePositionId ?? value); if (selected?.employeePositionId) { - Cookies.set("current-position-id", selected.employeePositionId); + setPositionCookie(selected.employeePositionId); } applyDelegationCookie(selected); diff --git a/apps/edr-freight-web/backoffice/src/record-management/services/api/signatureAndTeeterService.ts b/apps/edr-freight-web/backoffice/src/record-management/services/api/signatureAndTeeterService.ts index bb9d8b274..cefeb94ef 100644 --- a/apps/edr-freight-web/backoffice/src/record-management/services/api/signatureAndTeeterService.ts +++ b/apps/edr-freight-web/backoffice/src/record-management/services/api/signatureAndTeeterService.ts @@ -12,9 +12,11 @@ import { } from "@/record-management/dto/userRecords/teetersAndSignatureDto"; import Cookies from "js-cookie"; +import { getPositionCookie } from "@/shared/utils/positionCookie"; + export const withHeaders = (passPosId: boolean = false) => { const unitId = Cookies.get("unit-id"); - const positionId = Cookies.get("current-position-id"); + const positionId = getPositionCookie(); const projectId = Cookies.get("current-project-id"); const delegatedPositionId = Cookies.get("delegatedPositionId"); diff --git a/apps/edr-freight-web/backoffice/src/record-management/services/api/withHeaders.tsx b/apps/edr-freight-web/backoffice/src/record-management/services/api/withHeaders.tsx index b735ce611..e00325c24 100644 --- a/apps/edr-freight-web/backoffice/src/record-management/services/api/withHeaders.tsx +++ b/apps/edr-freight-web/backoffice/src/record-management/services/api/withHeaders.tsx @@ -1,9 +1,11 @@ import Cookies from "js-cookie"; +import { getPositionCookie } from "@/shared/utils/positionCookie"; + export const withHeaders = () => { const tenantKey = Cookies.get("tenant-key"); const unitId = Cookies.get("unit-id"); - const positionId = Cookies.get("current-position-id"); + const positionId = getPositionCookie(); const projectId = Cookies.get("current-project-id"); const delegatedPositionId = Cookies.get("delegatedPositionId"); const headers: Record = {}; diff --git a/apps/edr-freight-web/backoffice/src/shared/hooks/useAuthUser.ts b/apps/edr-freight-web/backoffice/src/shared/hooks/useAuthUser.ts index af41dc2b7..bd589d51b 100644 --- a/apps/edr-freight-web/backoffice/src/shared/hooks/useAuthUser.ts +++ b/apps/edr-freight-web/backoffice/src/shared/hooks/useAuthUser.ts @@ -30,6 +30,10 @@ import { persistRememberMePreference, setAuthCookies, } from "@/shared/utils/authPersistence"; +import { + getPositionCookie, + setPositionCookie, +} from "@/shared/utils/positionCookie"; import { clearComplaintVerification } from "@/complaints/utils/complaintVerificationStorage"; interface LoginPayload { @@ -46,7 +50,7 @@ export const useAuthUser = () => { const { t } = useTranslation(); const { handleError } = useErrorHandler(t); const delegatedPositionId = Cookies.get("delegatedPositionId"); - const currentPositionId = Cookies.get("current-position-id"); + const currentPositionId = getPositionCookie(); const { setUser, @@ -96,7 +100,7 @@ export const useAuthUser = () => { userDetails.employee?.[0]?.positions?.[0]?.employeePositionId; if (firstPositionId) { setSelectedPositionId(firstPositionId); - Cookies.set("current-position-id", firstPositionId, cookieOptions); + setPositionCookie(firstPositionId, cookieOptions); } } @@ -141,7 +145,7 @@ export const useAuthUser = () => { if (fallbackId) { setSelectedPositionId(fallbackId); - Cookies.set("current-position-id", fallbackId); + setPositionCookie(fallbackId); } } else if (selectedPositionId) { // Self-heal stale cookies that were set to position.id instead of @@ -157,10 +161,7 @@ export const useAuthUser = () => { if (matchingPosition?.employeePositionId) { setSelectedPositionId(matchingPosition.employeePositionId); - Cookies.set( - "current-position-id", - matchingPosition.employeePositionId, - ); + setPositionCookie(matchingPosition.employeePositionId); } } } diff --git a/apps/edr-freight-web/backoffice/src/shared/utils/faydaAuthSession.ts b/apps/edr-freight-web/backoffice/src/shared/utils/faydaAuthSession.ts index 8200f6aba..8dce901b3 100644 --- a/apps/edr-freight-web/backoffice/src/shared/utils/faydaAuthSession.ts +++ b/apps/edr-freight-web/backoffice/src/shared/utils/faydaAuthSession.ts @@ -8,6 +8,7 @@ import { getAuthCookieOptions, setAuthCookies, } from "@/shared/utils/authPersistence"; +import { setPositionCookie } from "@/shared/utils/positionCookie"; import type { VerifiedCitizen } from "@/complaints/types/complaint.types"; function unwrapApiData(payload: T | { data?: T }): T { @@ -138,7 +139,7 @@ export async function persistFaydaRegistrationAuth( const firstPositionId = userDetails.employee?.[0]?.positions?.[0]?.employeePositionId; if (firstPositionId) { - Cookies.set("current-position-id", firstPositionId, cookieOptions); + setPositionCookie(firstPositionId, cookieOptions); } return userDetails; diff --git a/apps/edr-freight-web/backoffice/src/shared/utils/positionCookie.ts b/apps/edr-freight-web/backoffice/src/shared/utils/positionCookie.ts new file mode 100644 index 000000000..60c97c6ae --- /dev/null +++ b/apps/edr-freight-web/backoffice/src/shared/utils/positionCookie.ts @@ -0,0 +1,39 @@ +import Cookies from "js-cookie"; + +type CookieOptions = NonNullable[2]>; + +/** + * Freight's own active-position cookie. + * + * Smart Office is a separate app on the same IAM and it also writes a cookie + * named `current-position-id` — but it stores `position.id` where freight + * stores `employeePositionId`. The two are not interchangeable, so on a shared + * domain each app's login silently overwrote the other's desk selection and the + * loser fell back to `positions[0]`. Freight keeps its own cookie name so both + * can hold a selection at once. + */ +export const POSITION_COOKIE = "freight-current-position-id"; + +/** + * The pre-rename, shared-with-Smart-Office name. Still read so a session that + * is live across the deploy keeps its desk, and cleared on every write so the + * colliding cookie does not linger. + */ +const LEGACY_POSITION_COOKIE = "current-position-id"; + +/** The active `employeePositionId`, or undefined when no desk is selected. */ +export const getPositionCookie = (): string | undefined => + Cookies.get(POSITION_COOKIE) ?? Cookies.get(LEGACY_POSITION_COOKIE); + +export const setPositionCookie = ( + value: string, + options?: CookieOptions, +): void => { + Cookies.set(POSITION_COOKIE, value, options); + Cookies.remove(LEGACY_POSITION_COOKIE); +}; + +export const clearPositionCookie = (): void => { + Cookies.remove(POSITION_COOKIE); + Cookies.remove(LEGACY_POSITION_COOKIE); +};