diff --git a/apps/portal-bff/.env.example b/apps/portal-bff/.env.example index db2d790..943ac57 100644 --- a/apps/portal-bff/.env.example +++ b/apps/portal-bff/.env.example @@ -105,6 +105,18 @@ SESSION_ENCRYPTION_KEY=replace_with_32_random_bytes_base64url # SESSION_IDLE_TIMEOUT_SECONDS=1800 # SESSION_ABSOLUTE_TIMEOUT_SECONDS=43200 +# Per-environment salt used to pseudonymise the user id before it +# lands in audit rows (per ADR-0013 §"Schema") and in Pino app log +# lines (per ADR-0012 §"User id hashing"). Same value must be used +# on both sides so audit and app logs join on `actor_id_hash`. +# +# Rotation invalidates the join key — old rows / log lines can no +# longer be correlated with the new hash. Treat as long-lived per +# environment. Mandatory at boot. +# +# node -e "console.log(require('crypto').randomBytes(32).toString('base64url'))" +LOG_USER_ID_SALT=replace_with_32_random_bytes_base64url + # Future env vars introduced by upcoming phases / ADRs: # # Auth flow (ADR-0009) — additional keys wired as the routes land: @@ -121,10 +133,6 @@ SESSION_ENCRYPTION_KEY=replace_with_32_random_bytes_base64url # MFA (ADR-0011): # MFA_FRESHNESS_SECONDS (default 600) # -# Observability — additional keys to be wired as features land: -# LOG_USER_ID_SALT (per-environment salt for hashing user_id -# in CLS context — needed once auth lands) -# # Audit trail (ADR-0013): # AUDIT_DATABASE_URL (separate creds, role 'audit_writer') # AUDIT_ARCHIVER_DATABASE_URL (role 'audit_archiver', for the retention purge job) diff --git a/apps/portal-bff/src/audit/audit.module.ts b/apps/portal-bff/src/audit/audit.module.ts index 4653b54..2e705b8 100644 --- a/apps/portal-bff/src/audit/audit.module.ts +++ b/apps/portal-bff/src/audit/audit.module.ts @@ -1,15 +1,19 @@ -import { Module } from '@nestjs/common'; +import { Global, Module } from '@nestjs/common'; import { AuditWriter } from './audit.service'; +import { HashUserIdService } from './hash-user-id.service'; /** - * Provides the AuditWriter to the rest of the BFF. Imported globally - * by AppModule so any feature module can inject it without an extra - * import. The actual append-only contract is enforced by the - * Postgres role grants set up in the audit-schema migration — see + * Provides the AuditWriter (and the salt-bound HashUserIdService it + * depends on) to the rest of the BFF. Marked `@Global()` so a feature + * module can inject `AuditWriter` by writing it in a constructor — + * no need to re-import AuditModule in every feature. The append-only + * contract is enforced by the Postgres role grants set up in the + * audit-schema migration — see * apps/portal-bff/prisma/migrations/*_init_audit_schema. */ +@Global() @Module({ - providers: [AuditWriter], - exports: [AuditWriter], + providers: [HashUserIdService, AuditWriter], + exports: [HashUserIdService, AuditWriter], }) export class AuditModule {} diff --git a/apps/portal-bff/src/audit/audit.service.spec.ts b/apps/portal-bff/src/audit/audit.service.spec.ts index c861395..960686f 100644 --- a/apps/portal-bff/src/audit/audit.service.spec.ts +++ b/apps/portal-bff/src/audit/audit.service.spec.ts @@ -5,6 +5,7 @@ import { PrismaService } from 'nestjs-prisma'; import { Prisma } from '@prisma/client'; import { AuditWriter } from './audit.service'; import type { AuditEventInput } from './audit.types'; +import { HashUserIdService } from './hash-user-id.service'; interface AuditEventCreateCall { data: { @@ -45,17 +46,20 @@ async function createSubject(): Promise<{ writer: AuditWriter; prisma: MockPrisma; cls: { get: jest.Mock }; + hashUserId: { hash: jest.Mock }; }> { const { prisma, cls } = buildMocks(); + const hashUserId = { hash: jest.fn((id: string) => `hash(${id})`) }; const moduleRef = await Test.createTestingModule({ providers: [ AuditWriter, { provide: PrismaService, useValue: prisma }, { provide: ClsService, useValue: cls }, + { provide: HashUserIdService, useValue: hashUserId }, ], }).compile(); - return { writer: moduleRef.get(AuditWriter), prisma, cls }; + return { writer: moduleRef.get(AuditWriter), prisma, cls, hashUserId }; } const baseInput: AuditEventInput = { @@ -172,3 +176,89 @@ describe('AuditWriter', () => { ); }); }); + +describe('AuditWriter — typed event methods', () => { + describe('signIn()', () => { + it('hashes the actor oid and records auth.sign_in (success / workforce)', async () => { + const { writer, prisma, hashUserId } = await createSubject(); + await writer.signIn({ + actor: { oid: 'user-oid', amr: ['pwd', 'mfa'] }, + sessionId: 'sid-1', + }); + expect(hashUserId.hash).toHaveBeenCalledWith('user-oid'); + const call = prisma.tx.auditEvent.create.mock.calls[0]?.[0] as AuditEventCreateCall; + expect(call.data.eventType).toBe('auth.sign_in'); + expect(call.data.audience).toBe('workforce'); + expect(call.data.outcome).toBe('success'); + expect(call.data.actorIdHash).toBe('hash(user-oid)'); + expect(call.data.subject).toBe('session:sid-1'); + expect(call.data.payload).toEqual({ amr: ['pwd', 'mfa'] }); + }); + }); + + describe('signInFailed()', () => { + it('records auth.sign_in.failed with the failureKind and no actor by default', async () => { + const { writer, prisma, hashUserId } = await createSubject(); + await writer.signInFailed({ failureKind: 'state-mismatch' }); + expect(hashUserId.hash).not.toHaveBeenCalled(); + const call = prisma.tx.auditEvent.create.mock.calls[0]?.[0] as AuditEventCreateCall; + expect(call.data.eventType).toBe('auth.sign_in.failed'); + expect(call.data.outcome).toBe('failure'); + expect(call.data.actorIdHash).toBeNull(); + expect(call.data.payload).toEqual({ failureKind: 'state-mismatch' }); + }); + + it('merges payload with failureKind under the same key', async () => { + const { writer, prisma } = await createSubject(); + await writer.signInFailed({ + failureKind: 'entra-error', + payload: { entraError: 'access_denied', entraErrorDescription: 'user cancelled' }, + }); + const call = prisma.tx.auditEvent.create.mock.calls[0]?.[0] as AuditEventCreateCall; + expect(call.data.payload).toEqual({ + failureKind: 'entra-error', + entraError: 'access_denied', + entraErrorDescription: 'user cancelled', + }); + }); + + it('hashes the actor oid when one is provided (rare — identity-after-rejection path)', async () => { + const { writer, prisma, hashUserId } = await createSubject(); + await writer.signInFailed({ failureKind: 'amr-missing', actor: { oid: 'user-oid' } }); + expect(hashUserId.hash).toHaveBeenCalledWith('user-oid'); + const call = prisma.tx.auditEvent.create.mock.calls[0]?.[0] as AuditEventCreateCall; + expect(call.data.actorIdHash).toBe('hash(user-oid)'); + }); + }); + + describe('signOut()', () => { + it('records auth.sign_out (success) with the hashed actor + session subject', async () => { + const { writer, prisma, hashUserId } = await createSubject(); + await writer.signOut({ actor: { oid: 'user-oid' }, sessionId: 'sid-2' }); + expect(hashUserId.hash).toHaveBeenCalledWith('user-oid'); + const call = prisma.tx.auditEvent.create.mock.calls[0]?.[0] as AuditEventCreateCall; + expect(call.data.eventType).toBe('auth.sign_out'); + expect(call.data.outcome).toBe('success'); + expect(call.data.actorIdHash).toBe('hash(user-oid)'); + expect(call.data.subject).toBe('session:sid-2'); + }); + }); + + describe('sessionExpired()', () => { + it('records auth.session.expired with reason + ageMs', async () => { + const { writer, prisma, hashUserId } = await createSubject(); + await writer.sessionExpired({ + actor: { oid: 'user-oid' }, + sessionId: 'sid-3', + reason: 'absolute', + ageMs: 13 * 60 * 60 * 1000, + }); + expect(hashUserId.hash).toHaveBeenCalledWith('user-oid'); + const call = prisma.tx.auditEvent.create.mock.calls[0]?.[0] as AuditEventCreateCall; + expect(call.data.eventType).toBe('auth.session.expired'); + expect(call.data.outcome).toBe('success'); + expect(call.data.subject).toBe('session:sid-3'); + expect(call.data.payload).toEqual({ reason: 'absolute', ageMs: 13 * 60 * 60 * 1000 }); + }); + }); +}); diff --git a/apps/portal-bff/src/audit/audit.service.ts b/apps/portal-bff/src/audit/audit.service.ts index aeb5762..a8d83be 100644 --- a/apps/portal-bff/src/audit/audit.service.ts +++ b/apps/portal-bff/src/audit/audit.service.ts @@ -3,7 +3,14 @@ import { trace } from '@opentelemetry/api'; import { ClsService } from 'nestjs-cls'; import { PrismaService } from 'nestjs-prisma'; import { Prisma } from '@prisma/client'; -import type { AuditEventInput } from './audit.types'; +import type { + AuditEventInput, + SignInActor, + SignInFailedInput, + SignOutInput, + SessionExpiredInput, +} from './audit.types'; +import { HashUserIdService } from './hash-user-id.service'; /** * AuditWriter — single entry point for ADR-0013 audit-log writes. @@ -37,8 +44,76 @@ export class AuditWriter { constructor( private readonly prisma: PrismaService, private readonly cls: ClsService, + private readonly hashUserId: HashUserIdService, ) {} + /** + * Typed event: successful sign-in via the OIDC callback. Per + * ADR-0013's v1 catalogue (`auth.sign_in`). + * + * Hashes the user id internally — callers pass the raw Entra + * `oid`, never the hash, so the salt stays inside the audit + * module. + */ + async signIn(input: { actor: SignInActor; sessionId: string }): Promise { + await this.recordEvent({ + eventType: 'auth.sign_in', + audience: 'workforce', + outcome: 'success', + actorIdHash: this.hashUserId.hash(input.actor.oid), + subject: `session:${input.sessionId}`, + payload: { amr: input.actor.amr }, + }); + } + + /** + * Typed event: failed sign-in at the callback. The `failureKind` + * mirrors the discriminator on `AuthCodeFlowError` so the audit + * row is self-describing without joining anything. + * + * `actorIdHash` is left null on purpose: at the moment of + * failure we may not have resolved an identity yet (state + * mismatch, expired flow, token-exchange error before any user + * claim was parsed). Callers can pass an explicit hash when the + * identity *was* resolved before rejection. + */ + async signInFailed(input: SignInFailedInput): Promise { + await this.recordEvent({ + eventType: 'auth.sign_in.failed', + audience: 'workforce', + outcome: 'failure', + ...(input.actor !== undefined ? { actorIdHash: this.hashUserId.hash(input.actor.oid) } : {}), + payload: { failureKind: input.failureKind, ...(input.payload ?? {}) }, + }); + } + + async signOut(input: SignOutInput): Promise { + await this.recordEvent({ + eventType: 'auth.sign_out', + audience: 'workforce', + outcome: 'success', + actorIdHash: this.hashUserId.hash(input.actor.oid), + subject: `session:${input.sessionId}`, + }); + } + + /** + * Typed event: session destroyed by the absolute-timeout + * middleware (12 h hard ceiling, ADR-0010 §"TTL policy"). The + * idle-TTL expiry is *not* surfaced through this method — it + * happens silently inside Redis with no BFF observation point. + */ + async sessionExpired(input: SessionExpiredInput): Promise { + await this.recordEvent({ + eventType: 'auth.session.expired', + audience: 'workforce', + outcome: 'success', + actorIdHash: this.hashUserId.hash(input.actor.oid), + subject: `session:${input.sessionId}`, + payload: { reason: input.reason, ageMs: input.ageMs }, + }); + } + async recordEvent(input: AuditEventInput): Promise { const traceId = trace.getActiveSpan()?.spanContext().traceId ?? null; const actorIdHash = diff --git a/apps/portal-bff/src/audit/audit.types.ts b/apps/portal-bff/src/audit/audit.types.ts index bc2302a..78bab7f 100644 --- a/apps/portal-bff/src/audit/audit.types.ts +++ b/apps/portal-bff/src/audit/audit.types.ts @@ -41,3 +41,44 @@ export interface AuditEventInput { */ actorIdHash?: string; } + +/** + * Identity payload accepted by the typed audit methods. The raw + * Entra `oid` is hashed by `AuditWriter` itself so the salt stays + * inside the module. `amr` is captured for the sign-in record. + */ +export interface SignInActor { + oid: string; + amr: readonly string[]; +} + +export interface SignInFailedInput { + failureKind: string; + /** + * Optional — pass only when the identity was actually resolved + * before the rejection (rare). Most failure paths (state + * mismatch, expired flow, token-exchange error) reject before any + * user claim is parsed and should leave `actor` undefined. + */ + actor?: { oid: string }; + /** + * Extra fields to merge into the audit row's payload alongside + * `failureKind`. PII is the caller's responsibility, same posture + * as `AuditEventInput.payload`. + */ + payload?: Record; +} + +export interface SignOutInput { + actor: { oid: string }; + sessionId: string; +} + +export interface SessionExpiredInput { + actor: { oid: string }; + sessionId: string; + /** `absolute` is the only reason emitted in v1; `idle` is silent. */ + reason: 'absolute'; + /** Age of the session at the moment of expiry, in milliseconds. */ + ageMs: number; +} diff --git a/apps/portal-bff/src/audit/hash-user-id.service.spec.ts b/apps/portal-bff/src/audit/hash-user-id.service.spec.ts new file mode 100644 index 0000000..26de611 --- /dev/null +++ b/apps/portal-bff/src/audit/hash-user-id.service.spec.ts @@ -0,0 +1,62 @@ +import { randomBytes } from 'node:crypto'; +import { HashUserIdService } from './hash-user-id.service'; + +const STRONG_SALT = randomBytes(32).toString('base64url'); + +function withSalt(salt: string, fn: () => T): T { + const original = process.env['LOG_USER_ID_SALT']; + process.env['LOG_USER_ID_SALT'] = salt; + try { + return fn(); + } finally { + if (original === undefined) { + delete process.env['LOG_USER_ID_SALT']; + } else { + process.env['LOG_USER_ID_SALT'] = original; + } + } +} + +describe('HashUserIdService', () => { + it('hashes the same input to the same output (stable join key)', () => { + withSalt(STRONG_SALT, () => { + const service = new HashUserIdService(); + expect(service.hash('user-oid-42')).toBe(service.hash('user-oid-42')); + }); + }); + + it('returns a 16-hex-char digest', () => { + withSalt(STRONG_SALT, () => { + const service = new HashUserIdService(); + const hash = service.hash('user-oid-42'); + expect(hash).toMatch(/^[0-9a-f]{16}$/); + }); + }); + + it('produces different outputs for different inputs', () => { + withSalt(STRONG_SALT, () => { + const service = new HashUserIdService(); + expect(service.hash('alice')).not.toBe(service.hash('bob')); + }); + }); + + it('produces different outputs across salts (rotation invalidates the join key)', () => { + const a = withSalt(STRONG_SALT, () => new HashUserIdService().hash('alice')); + const b = withSalt(randomBytes(32).toString('base64url'), () => + new HashUserIdService().hash('alice'), + ); + expect(a).not.toBe(b); + }); + + it('refuses to construct when LOG_USER_ID_SALT is unset', () => { + const original = process.env['LOG_USER_ID_SALT']; + delete process.env['LOG_USER_ID_SALT']; + try { + expect(() => new HashUserIdService()).toThrow(/LOG_USER_ID_SALT is not set/); + } finally { + if (original !== undefined) { + process.env['LOG_USER_ID_SALT'] = original; + } + } + }); +}); diff --git a/apps/portal-bff/src/audit/hash-user-id.service.ts b/apps/portal-bff/src/audit/hash-user-id.service.ts new file mode 100644 index 0000000..b06c9ad --- /dev/null +++ b/apps/portal-bff/src/audit/hash-user-id.service.ts @@ -0,0 +1,42 @@ +import { createHash } from 'node:crypto'; +import { Injectable } from '@nestjs/common'; +import { assertLogUserIdSalt } from '../config/check-log-user-id-salt'; + +/** + * Pseudonymises a user id (Entra `oid`) into the 16-hex-char hash + * that both ADR-0013 (`audit_events.actor_id_hash`) and ADR-0012 + * (Pino `user_id_hash`) store. The salt is read once at injection + * time so the per-request hot path runs purely on `node:crypto`. + * + * Same salt + same input ⇒ same output, on both writers, across + * application restarts within a given environment. That stability + * is the *whole point*: an investigator joins audit rows with app + * log lines on this hash, then re-hydrates the join key to the + * cleartext user id only inside the live operational DB. + * + * Rotating the salt invalidates the join key for everything that + * was written before the rotation. Treat as long-lived per + * environment. + * + * Hash construction: + * sha256(`${salt}:${userId}`).hex.slice(0, 16) + * + * SHA-256 is overkill for collision resistance at 16 hex chars + * (64 bits of output), but truncation is intentional — log lines + * and audit rows want a compact identifier, and 64 bits of + * randomness is well above the birthday bound for any realistic + * user count. The salt prevents rainbow attacks even on the + * truncated form. + */ +@Injectable() +export class HashUserIdService { + private readonly salt: string; + + constructor() { + this.salt = assertLogUserIdSalt(); + } + + hash(userId: string): string { + return createHash('sha256').update(`${this.salt}:${userId}`).digest('hex').slice(0, 16); + } +} diff --git a/apps/portal-bff/src/auth/auth.controller.spec.ts b/apps/portal-bff/src/auth/auth.controller.spec.ts index 6ee0628..aea05c6 100644 --- a/apps/portal-bff/src/auth/auth.controller.spec.ts +++ b/apps/portal-bff/src/auth/auth.controller.spec.ts @@ -1,5 +1,6 @@ import type { Request, Response } from 'express'; import type { Logger } from 'nestjs-pino'; +import type { AuditWriter } from '../audit/audit.service'; import type { UserSessionIndexService } from '../session/user-session-index.service'; import { AuthController } from './auth.controller'; import { PRE_AUTH_COOKIE_NAME, PRE_AUTH_COOKIE_TTL_MS } from './auth.cookie'; @@ -102,6 +103,12 @@ interface ControllerFixture { completeAuthCodeFlow: jest.Mock; logger: ReturnType; userSessionIndex: { add: jest.Mock; remove: jest.Mock; list: jest.Mock }; + audit: { + signIn: jest.Mock; + signInFailed: jest.Mock; + signOut: jest.Mock; + sessionExpired: jest.Mock; + }; } const LOGOUT_URL = `${ENTRA.authority}/oauth2/v2.0/logout?post_logout_redirect_uri=${encodeURIComponent(ENTRA.postLogoutRedirectUri)}`; @@ -123,6 +130,12 @@ function makeController(opts?: { completeAuthCodeFlow?: jest.Mock }): Controller remove: jest.fn().mockResolvedValue(undefined), list: jest.fn().mockResolvedValue([]), }; + const audit = { + signIn: jest.fn().mockResolvedValue(undefined), + signInFailed: jest.fn().mockResolvedValue(undefined), + signOut: jest.fn().mockResolvedValue(undefined), + sessionExpired: jest.fn().mockResolvedValue(undefined), + }; const logger = makeLoggerStub(); return { controller: new AuthController( @@ -130,11 +143,13 @@ function makeController(opts?: { completeAuthCodeFlow?: jest.Mock }): Controller logger as unknown as Logger, ENTRA, userSessionIndex as unknown as UserSessionIndexService, + audit as unknown as AuditWriter, ), beginAuthCodeFlow, completeAuthCodeFlow, logger, userSessionIndex, + audit, }; } @@ -252,6 +267,65 @@ describe('AuthController.callback', () => { expect(session.save).toHaveBeenCalled(); }); + it('emits an auth.sign_in audit event on the happy path', async () => { + const { controller, audit } = makeController(); + const res = makeResStub(); + const req = makeReqStub({ + signedCookies: { [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) }, + session: makeSessionStub(), + sessionID: 'fresh-sid', + }); + + await controller.callback(req, res, 'auth-code', PRE_AUTH.state); + + expect(audit.signIn).toHaveBeenCalledWith({ + actor: USER, + sessionId: 'fresh-sid', + }); + expect(audit.signInFailed).not.toHaveBeenCalled(); + }); + + it('emits auth.sign_in.failed on the Entra-error branch', async () => { + const { controller, audit } = makeController(); + const res = makeResStub(); + const req = makeReqStub({ + signedCookies: { [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) }, + }); + + await controller.callback(req, res, undefined, undefined, 'access_denied', 'user cancelled'); + + expect(audit.signInFailed).toHaveBeenCalledWith({ + failureKind: 'entra-error', + payload: { entraError: 'access_denied', entraErrorDescription: 'user cancelled' }, + }); + expect(audit.signIn).not.toHaveBeenCalled(); + }); + + it('emits auth.sign_in.failed (no-pre-auth-cookie) when the cookie is missing', async () => { + const { controller, audit } = makeController(); + const res = makeResStub(); + const req = makeReqStub(); + + await controller.callback(req, res, 'code', 'state'); + + expect(audit.signInFailed).toHaveBeenCalledWith({ failureKind: 'no-pre-auth-cookie' }); + }); + + it('emits auth.sign_in.failed with the discriminator kind on AuthCodeFlowException', async () => { + const completeAuthCodeFlow = jest + .fn() + .mockRejectedValue(new AuthCodeFlowException({ kind: 'state-mismatch' })); + const { controller, audit } = makeController({ completeAuthCodeFlow }); + const res = makeResStub(); + const req = makeReqStub({ + signedCookies: { [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) }, + }); + + await controller.callback(req, res, 'code', 'state'); + + expect(audit.signInFailed).toHaveBeenCalledWith({ failureKind: 'state-mismatch' }); + }); + it('propagates a session.save() error as an exception (so the SPA never sees a successful redirect with no session)', async () => { const { controller } = makeController(); const res = makeResStub(); @@ -398,8 +472,8 @@ describe('AuthController.me', () => { }); describe('AuthController.logout', () => { - it('destroys the session, clears the session cookie, logs success, redirects to Entra logout', async () => { - const { controller, logger, userSessionIndex } = makeController(); + it('destroys the session, clears the session cookie, logs + audits sign-out, redirects to Entra logout', async () => { + const { controller, logger, userSessionIndex, audit } = makeController(); const res = makeResStub(); const session = makeSessionStub({ user: USER }); const req = makeReqStub({ session, sessionID: 'logged-out-sid' }); @@ -407,6 +481,10 @@ describe('AuthController.logout', () => { await controller.logout(req, res); expect(userSessionIndex.remove).toHaveBeenCalledWith(USER.oid, 'logged-out-sid'); + expect(audit.signOut).toHaveBeenCalledWith({ + actor: USER, + sessionId: 'logged-out-sid', + }); expect(session.destroy).toHaveBeenCalledTimes(1); expect(res.clearCookie).toHaveBeenCalledWith('portal_session', { path: '/' }); expect(logger.log).toHaveBeenCalledWith( @@ -446,8 +524,8 @@ describe('AuthController.logout', () => { } }); - it('still clears the cookie and redirects when the user was already anonymous', async () => { - const { controller, logger, userSessionIndex } = makeController(); + it('still clears the cookie and redirects when the user was already anonymous (no audit row either)', async () => { + const { controller, logger, userSessionIndex, audit } = makeController(); const res = makeResStub(); const session = makeSessionStub(); const req = makeReqStub({ session }); @@ -456,8 +534,10 @@ describe('AuthController.logout', () => { // Nothing was ever added to the index for an anonymous request — // skip the SREM (cheaper + avoids racing the cleanup of a - // future user's session id collision). + // future user's session id collision). The audit row is also + // skipped: an "anonymous user logged out" event is noise. expect(userSessionIndex.remove).not.toHaveBeenCalled(); + expect(audit.signOut).not.toHaveBeenCalled(); expect(session.destroy).toHaveBeenCalledTimes(1); expect(logger.log).toHaveBeenCalledWith( { event: 'auth.signed_out', wasAuthenticated: false }, diff --git a/apps/portal-bff/src/auth/auth.controller.ts b/apps/portal-bff/src/auth/auth.controller.ts index 156d02e..0579e65 100644 --- a/apps/portal-bff/src/auth/auth.controller.ts +++ b/apps/portal-bff/src/auth/auth.controller.ts @@ -1,6 +1,7 @@ import { Controller, Get, Inject, Query, Req, Res } from '@nestjs/common'; import type { Request, Response } from 'express'; import { Logger } from 'nestjs-pino'; +import { AuditWriter } from '../audit/audit.service'; import { readSessionTimeouts, sessionCookieName } from '../session/session-cookie'; import { UserSessionIndexService } from '../session/user-session-index.service'; import { @@ -29,6 +30,7 @@ export class AuthController { private readonly logger: Logger, @Inject(ENTRA_CONFIG) private readonly entra: EntraConfig, private readonly userSessionIndex: UserSessionIndexService, + private readonly audit: AuditWriter, ) {} /** @@ -90,10 +92,15 @@ export class AuthController { }, 'AuthCallback', ); + await this.audit.signInFailed({ + failureKind: 'entra-error', + payload: { entraError, entraErrorDescription }, + }); return this.redirectWithError(res, 'token-exchange-failed'); } if (typeof code !== 'string' || typeof state !== 'string') { + await this.audit.signInFailed({ failureKind: 'missing-code-or-state' }); return this.redirectWithError(res, 'token-exchange-failed'); } @@ -103,6 +110,7 @@ export class AuthController { // their browser dropped the cookie (TTL elapsed in // a 3rd-party-cookie blocker, etc.). Treat as flow-expired. this.logger.warn({ event: 'auth.no_pre_auth_cookie' }, 'AuthCallback'); + await this.audit.signInFailed({ failureKind: 'no-pre-auth-cookie' }); return this.redirectWithError(res, 'flow-expired'); } @@ -127,6 +135,12 @@ export class AuthController { // and revoke. Best-effort: a Redis hiccup here doesn't fail // sign-in (the service swallows + logs). await this.userSessionIndex.add(user.oid, req.sessionID); + // First write to the audit trail — blocking, per ADR-0013. + // If this throws the user does NOT see a successful sign-in: + // the exception propagates and the controller emits a 5xx via + // Nest's default exception filter. Same posture as the + // session.save() above. + await this.audit.signIn({ actor: user, sessionId: req.sessionID }); this.logger.log( { event: 'auth.signed_in', @@ -141,6 +155,7 @@ export class AuthController { } catch (err) { if (err instanceof AuthCodeFlowException) { this.logger.warn({ event: 'auth.flow_error', failure: err.failure }, 'AuthCallback'); + await this.audit.signInFailed({ failureKind: err.failure.kind }); return this.redirectWithError(res, err.failure.kind); } throw err; @@ -192,6 +207,12 @@ export class AuthController { // requests (nothing was ever added). if (user) { await this.userSessionIndex.remove(user.oid, sessionId); + // Audit the sign-out before tearing the session down — once + // destroy() runs we lose the actor id. Blocking per ADR-0013: + // if the audit row can't be written, the user does NOT get a + // "you're logged out" experience, because we can't certify + // the sign-out happened. + await this.audit.signOut({ actor: user, sessionId }); } try { diff --git a/apps/portal-bff/src/auth/auth.module.spec.ts b/apps/portal-bff/src/auth/auth.module.spec.ts index 64226c9..4a24c6b 100644 --- a/apps/portal-bff/src/auth/auth.module.spec.ts +++ b/apps/portal-bff/src/auth/auth.module.spec.ts @@ -1,18 +1,40 @@ import { randomBytes } from 'node:crypto'; import { ConfidentialClientApplication } from '@azure/msal-node'; +import { Global, Module } from '@nestjs/common'; import { Test } from '@nestjs/testing'; +import { ClsService } from 'nestjs-cls'; import { LoggerModule } from 'nestjs-pino'; +import { PrismaService } from 'nestjs-prisma'; +import { AuditModule } from '../audit/audit.module'; import { AuthModule } from './auth.module'; import { ENTRA_CONFIG, type EntraConfig } from './entra-config.token'; import { MSAL_CLIENT } from './msal-client.token'; -// AuthModule imports SessionModule (so AuthController can inject -// `UserSessionIndexService`), which transitively pulls in -// RedisModule + the session-secret / session-encryption-key -// validators. The spec has to satisfy all of those env vars or the -// test fails as soon as `compile()` walks the import graph — which -// is exactly what bit the CI on PR #115 (CI starts with a clean env; -// locally `nx test` was loading apps/portal-bff/.env and masking it). +// Production app makes PrismaService + ClsService globally available +// via `PrismaModule.forRoot({ isGlobal: true })` and ObservabilityModule. +// In a slice-of-graph spec we stub both via a tiny @Global() module +// so AuditModule's transitive resolution of AuditWriter succeeds +// without booting Prisma or nestjs-cls for real. +@Global() +@Module({ + providers: [ + { provide: PrismaService, useValue: {} }, + { provide: ClsService, useValue: { get: () => undefined } }, + ], + exports: [PrismaService, ClsService], +}) +class TestStubsModule {} + +// AuthModule imports SessionModule + (transitively, via AuditModule +// being @Global) needs the AuditWriter providers. compile() walks +// the full import graph so the spec has to satisfy every validator +// at boot time, including: +// - SessionModule's RedisModule (REDIS_URL) +// - SessionModule's middleware factory (SESSION_SECRET + +// SESSION_ENCRYPTION_KEY) +// - AuditModule's HashUserIdService (LOG_USER_ID_SALT) +// Lesson from PR #115/#116: CI starts with a clean env, masking the +// failure that nx-loaded apps/portal-bff/.env hides locally. const VALID = { ENTRA_INSTANCE_URL: 'https://login.microsoftonline.com/', ENTRA_TENANT_ID: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee', @@ -25,16 +47,26 @@ const VALID = { REDIS_URL: 'redis://default:test-pass@127.0.0.1:65535/0', SESSION_SECRET: randomBytes(32).toString('base64url'), SESSION_ENCRYPTION_KEY: randomBytes(32).toString('base64url'), + LOG_USER_ID_SALT: randomBytes(32).toString('base64url'), + // PrismaModule reads this at boot; an unreachable URL is fine in + // tests because we never issue queries. + DATABASE_URL: 'postgresql://test:test@127.0.0.1:65535/test', }; // AuthModule's MSAL_CLIENT factory injects the Pino-backed `Logger` // (which the production app supplies through `ObservabilityModule`). -// Importing `LoggerModule.forRoot({ pinoHttp: { level: 'silent' } })` -// here registers the same provider for the testing module without -// flooding test stdout. +// `AuditModule` is `@Global()` in production via AppModule; here we +// import it explicitly because we compile a slice of the graph. +// AuditWriter's deps (PrismaService, ClsService) are stubbed — the +// spec doesn't need a working DB, only that the wiring resolves. async function compile() { return Test.createTestingModule({ - imports: [LoggerModule.forRoot({ pinoHttp: { level: 'silent' } }), AuthModule], + imports: [ + LoggerModule.forRoot({ pinoHttp: { level: 'silent' } }), + TestStubsModule, + AuditModule, + AuthModule, + ], }).compile(); } diff --git a/apps/portal-bff/src/config/check-log-user-id-salt.spec.ts b/apps/portal-bff/src/config/check-log-user-id-salt.spec.ts new file mode 100644 index 0000000..6508395 --- /dev/null +++ b/apps/portal-bff/src/config/check-log-user-id-salt.spec.ts @@ -0,0 +1,36 @@ +import { randomBytes } from 'node:crypto'; +import { assertLogUserIdSalt } from './check-log-user-id-salt'; + +const STRONG_SALT = randomBytes(32).toString('base64url'); + +describe('assertLogUserIdSalt', () => { + const original = process.env['LOG_USER_ID_SALT']; + + afterEach(() => { + if (original === undefined) { + delete process.env['LOG_USER_ID_SALT']; + } else { + process.env['LOG_USER_ID_SALT'] = original; + } + }); + + it('returns the value when LOG_USER_ID_SALT decodes to ≥ 32 bytes', () => { + process.env['LOG_USER_ID_SALT'] = STRONG_SALT; + expect(assertLogUserIdSalt()).toBe(STRONG_SALT); + }); + + it('throws when LOG_USER_ID_SALT is unset', () => { + delete process.env['LOG_USER_ID_SALT']; + expect(() => assertLogUserIdSalt()).toThrow(/LOG_USER_ID_SALT is not set/); + }); + + it('throws when LOG_USER_ID_SALT is the .env.example placeholder', () => { + process.env['LOG_USER_ID_SALT'] = 'replace_with_32_random_bytes_base64url'; + expect(() => assertLogUserIdSalt()).toThrow(/placeholder/); + }); + + it('throws when LOG_USER_ID_SALT decodes below 32 bytes', () => { + process.env['LOG_USER_ID_SALT'] = randomBytes(16).toString('base64url'); + expect(() => assertLogUserIdSalt()).toThrow(/decodes to 16 bytes/); + }); +}); diff --git a/apps/portal-bff/src/config/check-log-user-id-salt.ts b/apps/portal-bff/src/config/check-log-user-id-salt.ts new file mode 100644 index 0000000..2916855 --- /dev/null +++ b/apps/portal-bff/src/config/check-log-user-id-salt.ts @@ -0,0 +1,63 @@ +/** + * Sanity-check the `LOG_USER_ID_SALT` env var early in bootstrap so + * a missing or obviously weak value fails fast instead of producing + * predictable hashes at runtime. + * + * Wired in `main.ts` alongside the other `assertX()` validators — + * same pre-flight family per ADR-0018 §"BFF env-var loading". + * + * `LOG_USER_ID_SALT` is the per-environment salt fed into SHA-256 + * to pseudonymise user ids before they land in audit rows + * (`actor_id_hash`, per ADR-0013 §"Schema") and in Pino app log + * lines (`user_id_hash`, per ADR-0012 §"User id hashing"). The + * same value must be in scope on both writers so the two streams + * join on the hash. + * + * Returns the salt as a string so callers (`HashUserIdService`) + * can fold it into the hash without re-reading the env. + */ + +const PLACEHOLDER = 'replace_with_32_random_bytes_base64url'; +const MIN_ENTROPY_BYTES = 32; + +export function assertLogUserIdSalt(): string { + const raw = process.env['LOG_USER_ID_SALT']; + if (!raw || raw === '') { + throw new Error( + `LOG_USER_ID_SALT is not set. Generate one with ` + + `"node -e \\"console.log(require('crypto').randomBytes(32).toString('base64url'))\\"" ` + + `and put it in apps/portal-bff/.env.`, + ); + } + + if (raw === PLACEHOLDER) { + throw new Error( + `LOG_USER_ID_SALT is still set to the .env.example placeholder ` + + `("${PLACEHOLDER}"). Replace with a real random value.`, + ); + } + + // Accept base64url-encoded input (the documented generation + // recipe). Anything below 32 bytes of decoded entropy is weaker + // than the project's AES-256 key family — reject early. + let decoded: Buffer; + try { + decoded = Buffer.from(raw, 'base64url'); + } catch { + throw new Error(`LOG_USER_ID_SALT must be a base64url-encoded string. Got: ${truncate(raw)}`); + } + + if (decoded.length < MIN_ENTROPY_BYTES) { + throw new Error( + `LOG_USER_ID_SALT decodes to ${decoded.length} bytes, ` + + `below the ${MIN_ENTROPY_BYTES}-byte minimum (≈ 256 bits of entropy). ` + + `Generate a longer value.`, + ); + } + + return raw; +} + +function truncate(s: string): string { + return s.length > 16 ? `${s.slice(0, 16)}…` : s; +} diff --git a/apps/portal-bff/src/main.ts b/apps/portal-bff/src/main.ts index a2049f6..07d8eee 100644 --- a/apps/portal-bff/src/main.ts +++ b/apps/portal-bff/src/main.ts @@ -11,6 +11,7 @@ import { AppModule } from './app/app.module'; import { assertDatabaseUrl } from './config/check-database-url'; import { assertEntraConfig } from './config/check-entra-config'; import { assertRedisConfig } from './config/check-redis-config'; +import { assertLogUserIdSalt } from './config/check-log-user-id-salt'; import { assertSessionEncryptionKey } from './config/check-session-encryption-key'; import { assertSessionSecret } from './config/check-session-secret'; import { @@ -44,6 +45,11 @@ assertRedisConfig(); // surface on the first authenticated request otherwise. assertSessionEncryptionKey(); +// LOG_USER_ID_SALT — per-environment SHA-256 salt for the actor-id +// hash that joins audit rows (ADR-0013) and Pino log lines +// (ADR-0012). Mandatory at boot. +assertLogUserIdSalt(); + async function bootstrap() { // `bufferLogs: true` holds early-bootstrap log lines until the // Pino-based Logger is wired in below, so we don't lose anything diff --git a/apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts b/apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts index 1265078..5334b47 100644 --- a/apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts +++ b/apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts @@ -1,5 +1,6 @@ import type { NextFunction, Request, Response } from 'express'; import type { Logger } from 'nestjs-pino'; +import type { AuditWriter } from '../audit/audit.service'; import { createAbsoluteTimeoutMiddleware } from './absolute-timeout.middleware'; import type { UserSessionIndexService } from './user-session-index.service'; @@ -42,6 +43,12 @@ function makeIndex(): UserSessionIndexService & { add: jest.Mock; remove: jest.M } as unknown as UserSessionIndexService & { add: jest.Mock; remove: jest.Mock }; } +function makeAudit(): AuditWriter & { sessionExpired: jest.Mock } { + return { + sessionExpired: jest.fn().mockResolvedValue(undefined), + } as unknown as AuditWriter & { sessionExpired: jest.Mock }; +} + function makeLogger() { return { log: jest.fn(), @@ -65,7 +72,12 @@ const USER = { describe('absoluteTimeout middleware', () => { it('passes through when there is no session at all', async () => { - const middleware = createAbsoluteTimeoutMiddleware(makeIndex(), makeLogger(), () => NOW); + const middleware = createAbsoluteTimeoutMiddleware( + makeIndex(), + makeLogger(), + makeAudit(), + () => NOW, + ); const req = makeReq(undefined); const res = makeRes(); const next: NextFunction = jest.fn(); @@ -78,7 +90,12 @@ describe('absoluteTimeout middleware', () => { it('passes through when the session has no user (anonymous request hit a session-touching route)', async () => { const session = makeSession(); - const middleware = createAbsoluteTimeoutMiddleware(makeIndex(), makeLogger(), () => NOW); + const middleware = createAbsoluteTimeoutMiddleware( + makeIndex(), + makeLogger(), + makeAudit(), + () => NOW, + ); const res = makeRes(); const next: NextFunction = jest.fn(); @@ -90,7 +107,12 @@ describe('absoluteTimeout middleware', () => { it('passes through when absoluteExpiresAt is unset (pre-hardening session)', async () => { const session = makeSession({ user: USER }); - const middleware = createAbsoluteTimeoutMiddleware(makeIndex(), makeLogger(), () => NOW); + const middleware = createAbsoluteTimeoutMiddleware( + makeIndex(), + makeLogger(), + makeAudit(), + () => NOW, + ); const res = makeRes(); const next: NextFunction = jest.fn(); @@ -106,7 +128,12 @@ describe('absoluteTimeout middleware', () => { createdAt: NOW - 60_000, absoluteExpiresAt: NOW + 60_000, }); - const middleware = createAbsoluteTimeoutMiddleware(makeIndex(), makeLogger(), () => NOW); + const middleware = createAbsoluteTimeoutMiddleware( + makeIndex(), + makeLogger(), + makeAudit(), + () => NOW, + ); const res = makeRes(); const next: NextFunction = jest.fn(); @@ -117,7 +144,7 @@ describe('absoluteTimeout middleware', () => { expect(res.clearCookie).not.toHaveBeenCalled(); }); - it('destroys the session, removes from the index, clears the cookie, and still calls next() when past the ceiling', async () => { + it('destroys the session, removes from the index, clears the cookie, audits expiry, and still calls next() when past the ceiling', async () => { const session = makeSession({ user: USER, createdAt: NOW - 13 * 60 * 60 * 1000, @@ -125,7 +152,8 @@ describe('absoluteTimeout middleware', () => { }); const index = makeIndex(); const logger = makeLogger(); - const middleware = createAbsoluteTimeoutMiddleware(index, logger, () => NOW); + const audit = makeAudit(); + const middleware = createAbsoluteTimeoutMiddleware(index, logger, audit, () => NOW); const res = makeRes(); const next: NextFunction = jest.fn(); @@ -134,6 +162,12 @@ describe('absoluteTimeout middleware', () => { expect(session.destroy).toHaveBeenCalledTimes(1); expect(index.remove).toHaveBeenCalledWith(USER.oid, 'session-abc'); expect(res.clearCookie).toHaveBeenCalledWith('portal_session', { path: '/' }); + expect(audit.sessionExpired).toHaveBeenCalledWith({ + actor: { oid: USER.oid }, + sessionId: 'session-abc', + reason: 'absolute', + ageMs: 13 * 60 * 60 * 1000, + }); expect(logger.log).toHaveBeenCalledWith( expect.objectContaining({ event: 'session.absolute_timeout', @@ -145,7 +179,7 @@ describe('absoluteTimeout middleware', () => { expect(next).toHaveBeenCalledTimes(1); }); - it('logs but does not throw when destroy() fails — still clears cookie + index + calls next()', async () => { + it('logs but does not throw when destroy() fails — still clears cookie + index + audits + calls next()', async () => { const session = makeSession({ user: USER, createdAt: NOW - 13 * 60 * 60 * 1000, @@ -156,7 +190,8 @@ describe('absoluteTimeout middleware', () => { ); const index = makeIndex(); const logger = makeLogger(); - const middleware = createAbsoluteTimeoutMiddleware(index, logger, () => NOW); + const audit = makeAudit(); + const middleware = createAbsoluteTimeoutMiddleware(index, logger, audit, () => NOW); const res = makeRes(); const next: NextFunction = jest.fn(); @@ -170,6 +205,7 @@ describe('absoluteTimeout middleware', () => { 'AbsoluteTimeout', ); expect(index.remove).toHaveBeenCalledWith(USER.oid, 'session-abc'); + expect(audit.sessionExpired).toHaveBeenCalled(); expect(res.clearCookie).toHaveBeenCalled(); expect(next).toHaveBeenCalledTimes(1); }); diff --git a/apps/portal-bff/src/session/absolute-timeout.middleware.ts b/apps/portal-bff/src/session/absolute-timeout.middleware.ts index 96bd2a5..78dfb70 100644 --- a/apps/portal-bff/src/session/absolute-timeout.middleware.ts +++ b/apps/portal-bff/src/session/absolute-timeout.middleware.ts @@ -1,5 +1,6 @@ import type { NextFunction, Request, RequestHandler, Response } from 'express'; import { Logger } from 'nestjs-pino'; +import { AuditWriter } from '../audit/audit.service'; import { sessionCookieName } from './session-cookie'; import { UserSessionIndexService } from './user-session-index.service'; @@ -29,6 +30,7 @@ import { UserSessionIndexService } from './user-session-index.service'; export function createAbsoluteTimeoutMiddleware( index: UserSessionIndexService, logger: Logger, + audit: AuditWriter, now: () => number = Date.now, ): RequestHandler { return async function absoluteTimeout( @@ -79,6 +81,18 @@ export function createAbsoluteTimeoutMiddleware( await index.remove(userId, sessionId); res.clearCookie(sessionCookieName(), { path: '/' }); + // Audit the expiry — blocking per ADR-0013: if the audit row + // can't be written, the request fails (the user sees a 5xx, + // ops gets a critical Pino line). The session was already + // destroyed above, so the user is signed out regardless — the + // failure surfaces in the response, not the security posture. + await audit.sessionExpired({ + actor: { oid: userId }, + sessionId, + reason: 'absolute', + ageMs, + }); + logger.log( { event: 'session.absolute_timeout', diff --git a/apps/portal-bff/src/session/session.module.spec.ts b/apps/portal-bff/src/session/session.module.spec.ts index 110d02d..94ccfe9 100644 --- a/apps/portal-bff/src/session/session.module.spec.ts +++ b/apps/portal-bff/src/session/session.module.spec.ts @@ -1,9 +1,23 @@ import { randomBytes } from 'node:crypto'; +import { Global, Module } from '@nestjs/common'; import { Test } from '@nestjs/testing'; +import { ClsService } from 'nestjs-cls'; import { LoggerModule } from 'nestjs-pino'; +import { PrismaService } from 'nestjs-prisma'; +import { AuditModule } from '../audit/audit.module'; import { SessionModule } from './session.module'; import { SESSION_MIDDLEWARE, type RequestHandler } from './session.token'; +@Global() +@Module({ + providers: [ + { provide: PrismaService, useValue: {} }, + { provide: ClsService, useValue: { get: () => undefined } }, + ], + exports: [PrismaService, ClsService], +}) +class TestStubsModule {} + const STRONG_KEY = randomBytes(32).toString('base64url'); const STRONG_SECRET = randomBytes(32).toString('base64url'); @@ -11,12 +25,22 @@ interface OriginalEnv { REDIS_URL: string | undefined; SESSION_SECRET: string | undefined; SESSION_ENCRYPTION_KEY: string | undefined; + LOG_USER_ID_SALT: string | undefined; NODE_ENV: string | undefined; } +// SessionModule's absolute-timeout middleware factory now depends +// on `AuditWriter` (provided by AuditModule via @Global() in +// production). The spec compiles a slice of the graph so we import +// AuditModule explicitly and stub its DB-touching deps. async function compile() { return Test.createTestingModule({ - imports: [LoggerModule.forRoot({ pinoHttp: { level: 'silent' } }), SessionModule], + imports: [ + LoggerModule.forRoot({ pinoHttp: { level: 'silent' } }), + TestStubsModule, + AuditModule, + SessionModule, + ], }).compile(); } @@ -25,6 +49,7 @@ describe('SessionModule', () => { REDIS_URL: process.env['REDIS_URL'], SESSION_SECRET: process.env['SESSION_SECRET'], SESSION_ENCRYPTION_KEY: process.env['SESSION_ENCRYPTION_KEY'], + LOG_USER_ID_SALT: process.env['LOG_USER_ID_SALT'], NODE_ENV: process.env['NODE_ENV'], }; @@ -34,6 +59,7 @@ describe('SessionModule', () => { process.env['REDIS_URL'] = 'redis://default:test-pass@127.0.0.1:65535/0'; process.env['SESSION_SECRET'] = STRONG_SECRET; process.env['SESSION_ENCRYPTION_KEY'] = STRONG_KEY; + process.env['LOG_USER_ID_SALT'] = randomBytes(32).toString('base64url'); process.env['NODE_ENV'] = 'development'; }); @@ -41,6 +67,7 @@ describe('SessionModule', () => { restore('REDIS_URL', original.REDIS_URL); restore('SESSION_SECRET', original.SESSION_SECRET); restore('SESSION_ENCRYPTION_KEY', original.SESSION_ENCRYPTION_KEY); + restore('LOG_USER_ID_SALT', original.LOG_USER_ID_SALT); restore('NODE_ENV', original.NODE_ENV); }); diff --git a/apps/portal-bff/src/session/session.module.ts b/apps/portal-bff/src/session/session.module.ts index 92cce80..2e7343a 100644 --- a/apps/portal-bff/src/session/session.module.ts +++ b/apps/portal-bff/src/session/session.module.ts @@ -7,6 +7,7 @@ import { assertSessionEncryptionKey } from '../config/check-session-encryption-k import { assertSessionSecret } from '../config/check-session-secret'; import { RedisModule } from '../redis/redis.module'; import { REDIS_CLIENT, type Redis } from '../redis/redis.token'; +import { AuditWriter } from '../audit/audit.service'; import { createAbsoluteTimeoutMiddleware } from './absolute-timeout.middleware'; import { adaptIoredisForConnectRedis } from './ioredis-connect-redis-adapter'; import { SessionDecryptError, decrypt, encrypt } from './session-crypto'; @@ -57,9 +58,12 @@ import { UserSessionIndexService } from './user-session-index.service'; UserSessionIndexService, { provide: SESSION_ABSOLUTE_TIMEOUT_MIDDLEWARE, - inject: [UserSessionIndexService, Logger], - useFactory: (index: UserSessionIndexService, logger: Logger): RequestHandler => - createAbsoluteTimeoutMiddleware(index, logger), + inject: [UserSessionIndexService, Logger, AuditWriter], + useFactory: ( + index: UserSessionIndexService, + logger: Logger, + audit: AuditWriter, + ): RequestHandler => createAbsoluteTimeoutMiddleware(index, logger, audit), }, { provide: SESSION_MIDDLEWARE,