feat(portal-bff): wire ADR-0013 audit pipeline to the auth lifecycle (#120)
## Summary Wires the audit pipeline (ADR-0013) to the auth lifecycle. The foundation was already in place (Prisma `AuditEvent` model, Postgres roles + grants, `AuditWriter.recordEvent` with `SET LOCAL ROLE audit_writer`); this PR layers a typed event surface and emits the first four events on real code paths. ### What lands - **Typed methods on `AuditWriter`**: `signIn`, `signInFailed`, `signOut`, `sessionExpired`. Callers pass the raw Entra `oid`; hashing happens inside the writer so the salt never leaves the audit module. ADR-0013 explicitly defers adding these typed methods "as the matching feature ships" — auth has shipped, so we add the four events tied to code paths that exist today. - **`HashUserIdService`** — reads `LOG_USER_ID_SALT` once at injection, exposes `hash(userId)` → 16-hex-char digest used by both `audit_events.actor_id_hash` (ADR-0013) and the future Pino `user_id_hash` (ADR-0012). Same salt + same input ⇒ same output ⇒ join key between the two streams. - **`LOG_USER_ID_SALT` env var** promoted from the "future vars" block in `.env.example` to the active section, with the same boot-time validator pattern as `SESSION_SECRET` / `SESSION_ENCRYPTION_KEY`: mandatory, base64url, ≥ 32 bytes decoded, placeholder rejected. Wired in `main.ts`. - **`AuditModule` is now `@Global()`** and also provides `HashUserIdService`. The previous in-line comment said "imported globally by AppModule" but the decorator was missing — without it, AuthController and the absolute-timeout middleware couldn't inject `AuditWriter` without re-importing AuditModule. - **Emission points**: - `/auth/callback` happy path → `auth.sign_in` after `session.save()` (blocking per ADR-0013 §"Blocking writes": a failed audit fails the sign-in). - `/auth/callback` failure paths → `auth.sign_in.failed` with a discriminator `failureKind` (`entra-error`, `missing-code-or-state`, `no-pre-auth-cookie`, or any of the `AuthCodeFlowError` kinds — `state-mismatch`, `flow-expired`, `token-exchange-failed`). - `/auth/logout` (authenticated only) → `auth.sign_out` before `session.destroy()` — once destroy runs we lose the actor id. - Absolute-timeout middleware → `auth.session.expired` with `reason: 'absolute'` and `ageMs` for forensic granularity. ### Out of scope (next PRs) - The other four v1 events from ADR-0013's catalogue (`auth.session.revoked`, `auth.token.validation.failed`, `auth.mfa.assertion.failed`, `authz.deny`) — no triggering code path exists today. They land with the admin "logout everywhere" route, downstream API access (ADR-0014), and the eventual `@RequireMfa()` / `@RequireAdmin` guards. - Idle-timeout expiry is intentionally silent — Redis lets the key disappear with no BFF observation point. Per ADR-0010. - Separate `AUDIT_DATABASE_URL` connection pool with `audit_writer`-only credentials — ADR-0013 marks it as the production hardening step, deferred behind `SET LOCAL ROLE` in v1. - Retention purge job + startup self-test probe — deferred to the on-prem infrastructure ADR per ADR-0013. ### Notable choices - **No CLS-populating middleware.** ADR-0013 anticipates an interceptor that puts `actorIdHash` on the request CLS so `AuditWriter.recordEvent` can pick it up automatically. For the four call sites in this PR, every emission path already has the user object in hand, so we pass `actorIdHash` explicitly via the typed methods and skip the middleware. It can land later when more routes need it. - **Blocking on the happy path = strict ADR posture.** `audit.signIn` is awaited before the 302; a Postgres outage makes the sign-in fail (5xx) rather than silently producing an un-audited session. That's "no audit ⇒ no action" applied to authentication itself. Matches ADR-0013 §"Blocking writes" verbatim. - **`signInFailed` skips the actor hash by default.** Most failure paths reject before any claim is parsed (state mismatch, expired flow). The interface accepts an optional `actor` for the rare identity-after-rejection case (future MFA assertion failure, etc.). ### Test plan - [x] `pnpm nx test portal-bff` (clean env) → **142/142 pass** (was 123; +19 new specs across `check-log-user-id-salt`, `hash-user-id.service`, `audit.service` typed-methods, `auth.controller`, `absolute-timeout.middleware`). - [x] `pnpm nx lint portal-bff` → clean. - [x] `pnpm nx build portal-bff` → clean. - [x] **CI clean-env repro** (lesson from #115/#116/#117): every env var unset → tests still 142/142. The two module specs that previously sat on the boundary (`auth.module`, `session.module`) now bootstrap their own `@Global()` stub providers for `PrismaService` + `ClsService` so AuditWriter's transitive resolution works without booting Prisma for real. - [ ] Manual smoke against running BFF + Postgres: - [ ] Sign in → `select * from audit.events where event_type = 'auth.sign_in'` returns one row with `actor_id_hash`, `subject = 'session:…'`, `payload.amr` populated. - [ ] Sign out → matching `auth.sign_out` row. - [ ] Force `SESSION_ABSOLUTE_TIMEOUT_SECONDS=5` + wait → `auth.session.expired` row with `payload.reason = 'absolute'` and `ageMs > 5000`. - [ ] Manual `UPDATE audit.events SET event_type = 'x' WHERE id = ...` as the BFF role → fails with "permission denied" (the role contract holds even when the migrator runs as a privileged login). --------- Co-authored-by: Julien Gautier <julien.gautier@apf.asso.fr> Reviewed-on: #120
This commit was merged in pull request #120.
This commit is contained in:
@@ -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<typeof makeLoggerStub>;
|
||||
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 },
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user