From c50794eceb37df666096b1bb67412e4a9a811e87 Mon Sep 17 00:00:00 2001 From: Julien Gautier Date: Tue, 12 May 2026 12:16:39 +0200 Subject: [PATCH] =?UTF-8?q?feat(portal-bff):=20/auth/callback=20route=20?= =?UTF-8?q?=E2=80=94=20token=20exchange=20+=20amr=20check=20(#107)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fourth step of ADR-0009 wiring. Closes the OIDC round-trip on the BFF side (modulo session persistence — that's the next PR per ADR-0010). Entra now redirects the user back to `GET /api/auth/callback`; the BFF verifies the state, exchanges the code for tokens via MSAL's `acquireTokenByCode`, runs the ADR-0011 `amr` sanity-check, logs the resolved identity to Pino, clears the single-use pre-auth cookie, and 302s the user back to the SPA. ## What lands - **[`auth.errors.ts`](apps/portal-bff/src/auth/auth.errors.ts)** — discriminated-union `AuthCodeFlowError` (`state-mismatch` / `flow-expired` / `amr-missing` / `token-exchange-failed`) + `AuthCodeFlowException` wrapper. The `kind` field doubles as the `?auth_error=` query param on the SPA-bound redirect so the front-end can render an exact message without duplicating the string set. - **[`AuthService.completeAuthCodeFlow(code, state, preAuth, now?)`](apps/portal-bff/src/auth/auth.service.ts)** — verifies state binding, refuses cookies older than the 5-minute flow TTL, calls MSAL Node's `acquireTokenByCode` with the stored verifier, validates `amr` is non-empty (the BFF sanity-check per ADR-0011 — Entra Conditional Access on the org side does the real enforcement), extracts `oid` / `tid` / `preferred_username` / `name` / `amr` into an `AuthenticatedUser` shape. - **[`auth.cookie.ts`](apps/portal-bff/src/auth/auth.cookie.ts)** gains `clearPreAuthCookieOptions()` mirroring the set-options minus `maxAge` so the browser actually drops the cookie. (Cookies match by name + path + secure; getting any of those wrong leaves the old cookie in place.) - **[`AuthController.callback()`](apps/portal-bff/src/auth/auth.controller.ts)** — `@Get('callback')`. Always clears the cookie first (single-use). Bails on Entra-side errors (`?error=`), missing query params, missing or malformed cookie — each branch logs a structured Pino warning and redirects with the right `auth_error` code. On `AuthCodeFlowException`, logs + redirects with the typed `kind`. On success, logs an `auth.signed_in` event with `oid`, `tid`, `username`, `amr` (PII-sensitive bits only; no tokens), then 302s to `entra.postLogoutRedirectUri`. ## Decisions worth flagging - **`postLogoutRedirectUri` reused as the SPA root URL.** Semantically a tiny stretch (its OIDC role is the post-logout destination) but the value is the same. Avoids one more env var until / unless the two URLs need to diverge. - **Cookie cleared FIRST**, before any branching. Single-use is a property we want guaranteed regardless of which path exits the handler — overlap with a parallel /login from the same browser session would otherwise leak a usable cookie. - **`auth.signed_in` logged via Pino, not via the audit module.** ADR-0013 wants this in the audit table; pairing audit with the session that ships in the next PR keeps the audit row carrying a `session_id` (otherwise it'd reference a "phantom" auth event with no follow-up). - **`amr` non-empty is the BFF's check; the Conditional Access policy is what enforces "MFA happened".** ADR-0011 explicitly factors it this way — empty `amr` would indicate a policy misconfiguration where MFA never fired. ## Verification - `nx run-many -t lint test build --projects=portal-bff` — green. - **52 / 52 specs** (was 39; +13 across the new completeFlow branches and callback branches). - Service spec covers happy path + 6 failure modes (state mismatch, flow expired, amr missing, MSAL throws, MSAL returns null, oid claim missing). - Controller spec covers happy redirect, Entra error, missing cookie, AuthCodeFlowException branch, missing query, malformed cookie. ## Manual smoke test (end-to-end) 1. `apps/portal-bff/.env` carries real `ENTRA_*` + `SESSION_SECRET`. 2. `nx serve portal-bff` and `nx serve portal-shell`. 3. Open `http://localhost:3000/api/auth/login` → redirects to Entra. 4. Authenticate. Entra redirects to `http://localhost:3000/api/auth/callback?code=…&state=…`. 5. BFF processes; redirects to `http://localhost:4200/`. Pino log shows `auth.signed_in` with the user's `oid`, `tid`, `username`, `amr`. 6. Tamper test: open the link again, hand-edit the `state=` in the callback URL → BFF redirects with `?auth_error=state-mismatch`. ## What this PR explicitly does NOT do - **Persist a session.** The user is "authenticated" from the BFF's point of view (identity resolved + logged) but the next request lands anonymous. Closes in the Redis sessions PR per ADR-0010. - **Audit log entry.** Pairs with sessions so the row carries a `session_id`. - **Logout / `/me`.** Land after sessions. --------- Co-authored-by: Julien Gautier Reviewed-on: https://git.unespace.com/julien/apf_portal/pulls/107 --- .../src/auth/auth.controller.spec.ts | 185 ++++++++++++++++-- apps/portal-bff/src/auth/auth.controller.ts | 140 ++++++++++++- apps/portal-bff/src/auth/auth.cookie.ts | 18 ++ apps/portal-bff/src/auth/auth.errors.ts | 29 +++ apps/portal-bff/src/auth/auth.service.spec.ts | 153 +++++++++++++-- apps/portal-bff/src/auth/auth.service.ts | 124 +++++++++++- 6 files changed, 604 insertions(+), 45 deletions(-) create mode 100644 apps/portal-bff/src/auth/auth.errors.ts diff --git a/apps/portal-bff/src/auth/auth.controller.spec.ts b/apps/portal-bff/src/auth/auth.controller.spec.ts index 90d0cef..b24c67f 100644 --- a/apps/portal-bff/src/auth/auth.controller.spec.ts +++ b/apps/portal-bff/src/auth/auth.controller.spec.ts @@ -1,7 +1,20 @@ -import type { Response } from 'express'; +import type { Request, Response } from 'express'; +import type { Logger } from 'nestjs-pino'; import { AuthController } from './auth.controller'; import { PRE_AUTH_COOKIE_NAME, PRE_AUTH_COOKIE_TTL_MS } from './auth.cookie'; -import type { AuthService, PreAuthPayload } from './auth.service'; +import { AuthCodeFlowException } from './auth.errors'; +import type { AuthService, AuthenticatedUser, PreAuthPayload } from './auth.service'; +import type { EntraConfig } from './entra-config.token'; + +const ENTRA: EntraConfig = { + instanceUrl: 'https://login.microsoftonline.com/', + tenantId: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee', + clientId: '11111111-2222-3333-4444-555555555555', + clientSecret: 's3cret', + redirectUri: 'http://localhost:3000/api/auth/callback', + postLogoutRedirectUri: 'http://localhost:4200/', + authority: 'https://login.microsoftonline.com/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee', +}; const PRE_AUTH: PreAuthPayload = { state: 'state-nonce', @@ -9,28 +22,65 @@ const PRE_AUTH: PreAuthPayload = { createdAt: 1_000, }; +const USER: AuthenticatedUser = { + oid: 'user-oid', + tid: ENTRA.tenantId, + username: 'jane.doe@apf.example', + displayName: 'Jane Doe', + amr: ['pwd', 'mfa'], +}; + function makeResStub() { return { cookie: jest.fn().mockReturnThis(), + clearCookie: jest.fn().mockReturnThis(), redirect: jest.fn().mockReturnThis(), - } as unknown as Response & { cookie: jest.Mock; redirect: jest.Mock }; + } as unknown as Response & { + cookie: jest.Mock; + clearCookie: jest.Mock; + redirect: jest.Mock; + }; } -describe('AuthController', () => { - let beginAuthCodeFlow: jest.Mock; - let service: AuthService; - let controller: AuthController; +function makeReqStub(signedCookies: Record = {}): Request { + return { signedCookies } as unknown as Request; +} - beforeEach(() => { - beginAuthCodeFlow = jest.fn().mockResolvedValue({ - authUrl: 'https://entra.example/authorize?state=state-nonce', - preAuthPayload: PRE_AUTH, - }); - service = { beginAuthCodeFlow } as unknown as AuthService; - controller = new AuthController(service); +function makeLoggerStub() { + return { + log: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn(), + } as unknown as Logger & { log: jest.Mock; warn: jest.Mock }; +} + +interface ControllerFixture { + controller: AuthController; + beginAuthCodeFlow: jest.Mock; + completeAuthCodeFlow: jest.Mock; + logger: ReturnType; +} + +function makeController(opts?: { completeAuthCodeFlow?: jest.Mock }): ControllerFixture { + const beginAuthCodeFlow = jest.fn().mockResolvedValue({ + authUrl: 'https://entra.example/authorize?state=state-nonce', + preAuthPayload: PRE_AUTH, }); + const completeAuthCodeFlow = opts?.completeAuthCodeFlow ?? jest.fn().mockResolvedValue(USER); + const service = { beginAuthCodeFlow, completeAuthCodeFlow } as unknown as AuthService; + const logger = makeLoggerStub(); + return { + controller: new AuthController(service, logger as unknown as Logger, ENTRA), + beginAuthCodeFlow, + completeAuthCodeFlow, + logger, + }; +} +describe('AuthController.login', () => { it('writes the pre-auth cookie and 302s to the Entra auth URL', async () => { + const { controller } = makeController(); const res = makeResStub(); await controller.login(res); @@ -57,6 +107,7 @@ describe('AuthController', () => { const originalNodeEnv = process.env['NODE_ENV']; try { process.env['NODE_ENV'] = 'production'; + const { controller } = makeController(); const res = makeResStub(); await controller.login(res); const call = res.cookie.mock.calls[0] ?? []; @@ -71,3 +122,109 @@ describe('AuthController', () => { } }); }); + +describe('AuthController.callback', () => { + it('clears the pre-auth cookie, logs success, redirects to the SPA on the happy path', async () => { + const { controller, completeAuthCodeFlow, logger } = makeController(); + const res = makeResStub(); + const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) }); + + await controller.callback(req, res, 'auth-code', PRE_AUTH.state); + + expect(res.clearCookie).toHaveBeenCalledWith(PRE_AUTH_COOKIE_NAME, expect.any(Object)); + expect(completeAuthCodeFlow).toHaveBeenCalledWith('auth-code', PRE_AUTH.state, PRE_AUTH); + expect(logger.log).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'auth.signed_in', + oid: USER.oid, + username: USER.username, + amr: USER.amr, + }), + 'AuthCallback', + ); + expect(res.redirect).toHaveBeenCalledWith(302, ENTRA.postLogoutRedirectUri); + }); + + it('redirects with ?auth_error=token-exchange-failed when Entra returns error', async () => { + const { controller, completeAuthCodeFlow } = makeController(); + const res = makeResStub(); + const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) }); + + await controller.callback(req, res, undefined, undefined, 'access_denied', 'user cancelled'); + + expect(completeAuthCodeFlow).not.toHaveBeenCalled(); + expect(res.clearCookie).toHaveBeenCalled(); + expect(res.redirect).toHaveBeenCalledWith( + 302, + expect.stringContaining('auth_error=token-exchange-failed'), + ); + }); + + it('redirects with ?auth_error=flow-expired when the pre-auth cookie is missing', async () => { + const { controller, completeAuthCodeFlow, logger } = makeController(); + const res = makeResStub(); + const req = makeReqStub({}); + + await controller.callback(req, res, 'code', 'state'); + + expect(completeAuthCodeFlow).not.toHaveBeenCalled(); + expect(logger.warn).toHaveBeenCalledWith( + expect.objectContaining({ event: 'auth.no_pre_auth_cookie' }), + 'AuthCallback', + ); + expect(res.redirect).toHaveBeenCalledWith( + 302, + expect.stringContaining('auth_error=flow-expired'), + ); + }); + + it('redirects with the typed error from AuthCodeFlowException', async () => { + const completeAuthCodeFlow = jest + .fn() + .mockRejectedValue(new AuthCodeFlowException({ kind: 'state-mismatch' })); + const { controller, logger } = makeController({ completeAuthCodeFlow }); + const res = makeResStub(); + const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) }); + + await controller.callback(req, res, 'code', 'state'); + + expect(logger.warn).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'auth.flow_error', + failure: { kind: 'state-mismatch' }, + }), + 'AuthCallback', + ); + expect(res.redirect).toHaveBeenCalledWith( + 302, + expect.stringContaining('auth_error=state-mismatch'), + ); + }); + + it('redirects with ?auth_error=token-exchange-failed when query is missing code/state', async () => { + const { controller } = makeController(); + const res = makeResStub(); + const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) }); + + await controller.callback(req, res); // no code, no state + + expect(res.redirect).toHaveBeenCalledWith( + 302, + expect.stringContaining('auth_error=token-exchange-failed'), + ); + }); + + it('rejects a malformed pre-auth cookie as flow-expired', async () => { + const { controller, completeAuthCodeFlow } = makeController(); + const res = makeResStub(); + const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: 'not-json' }); + + await controller.callback(req, res, 'code', 'state'); + + expect(completeAuthCodeFlow).not.toHaveBeenCalled(); + expect(res.redirect).toHaveBeenCalledWith( + 302, + expect.stringContaining('auth_error=flow-expired'), + ); + }); +}); diff --git a/apps/portal-bff/src/auth/auth.controller.ts b/apps/portal-bff/src/auth/auth.controller.ts index a3deeda..4552ee7 100644 --- a/apps/portal-bff/src/auth/auth.controller.ts +++ b/apps/portal-bff/src/auth/auth.controller.ts @@ -1,19 +1,29 @@ -import { Controller, Get, Res } from '@nestjs/common'; -import type { Response } from 'express'; -import { AuthService } from './auth.service'; -import { PRE_AUTH_COOKIE_NAME, preAuthCookieOptions } from './auth.cookie'; +import { Controller, Get, Inject, Query, Req, Res } from '@nestjs/common'; +import type { Request, Response } from 'express'; +import { Logger } from 'nestjs-pino'; +import { + PRE_AUTH_COOKIE_NAME, + clearPreAuthCookieOptions, + preAuthCookieOptions, +} from './auth.cookie'; +import { AuthCodeFlowException, type AuthCodeFlowError, authErrorCode } from './auth.errors'; +import { AuthService, type PreAuthPayload } from './auth.service'; +import { ENTRA_CONFIG, type EntraConfig } from './entra-config.token'; /** * OIDC routes mounted under `/api/auth/` per ADR-0009. * - * v1 ships one route: `GET /login`, the entry point of the - * Authorization Code + PKCE flow. The follow-up PR adds - * `/callback`, then `/me` and `/logout` once Redis-backed sessions - * land (ADR-0010). + * v1 ships two routes — `GET /login` (PR #105) and `GET /callback` + * (this PR). The next PR adds session persistence (Redis, + * ADR-0010); after that, `/me` and `/logout` close the loop. */ @Controller('auth') export class AuthController { - constructor(private readonly authService: AuthService) {} + constructor( + private readonly authService: AuthService, + private readonly logger: Logger, + @Inject(ENTRA_CONFIG) private readonly entra: EntraConfig, + ) {} /** * Starts the auth flow. Generates a fresh state + PKCE pair via @@ -31,4 +41,116 @@ export class AuthController { res.cookie(PRE_AUTH_COOKIE_NAME, JSON.stringify(preAuthPayload), preAuthCookieOptions()); res.redirect(302, authUrl); } + + /** + * Second leg of the OIDC flow. Entra redirects the user here with + * `?code=…&state=…` (success) or `?error=…&error_description=…` + * (failure). The pre-auth cookie set by `/login` carries the + * matching state + PKCE verifier so we can verify the round-trip + * and finish the token exchange. + * + * On success, the user identity is logged via Pino and the + * browser is redirected to the SPA. No session is persisted yet — + * that's the next PR (Redis-backed sessions per ADR-0010); until + * then the SPA cannot tell the user is "logged in". + * + * On failure (any branch — state mismatch, expired cookie, + * missing `amr` per ADR-0011, MSAL token-exchange error, or + * Entra-side error from the query) the user lands on the SPA + * with `?auth_error=` so the front-end can surface a + * specific message. The pre-auth cookie is cleared on every + * exit path: it is single-use by design. + */ + @Get('callback') + async callback( + @Req() req: Request, + @Res() res: Response, + @Query('code') code?: string, + @Query('state') state?: string, + @Query('error') entraError?: string, + @Query('error_description') entraErrorDescription?: string, + ): Promise { + // Drop the single-use cookie regardless of outcome. + res.clearCookie(PRE_AUTH_COOKIE_NAME, clearPreAuthCookieOptions()); + + // Entra signalled a failure (e.g. user cancelled the consent + // screen, MFA challenge failed, app-registration mismatch). + if (entraError) { + this.logger.warn( + { + event: 'auth.entra_error', + entraError, + entraErrorDescription, + }, + 'AuthCallback', + ); + return this.redirectWithError(res, 'token-exchange-failed'); + } + + if (typeof code !== 'string' || typeof state !== 'string') { + return this.redirectWithError(res, 'token-exchange-failed'); + } + + const preAuth = readPreAuthCookie(req); + if (!preAuth) { + // No cookie → either the user opened /callback directly, or + // 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'); + return this.redirectWithError(res, 'flow-expired'); + } + + try { + const user = await this.authService.completeAuthCodeFlow(code, state, preAuth); + this.logger.log( + { + event: 'auth.signed_in', + oid: user.oid, + tid: user.tid, + username: user.username, + amr: user.amr, + }, + 'AuthCallback', + ); + // No session persistence yet — next PR. SPA will see the user + // as anonymous on the landing page. + res.redirect(302, this.entra.postLogoutRedirectUri); + } catch (err) { + if (err instanceof AuthCodeFlowException) { + this.logger.warn({ event: 'auth.flow_error', failure: err.failure }, 'AuthCallback'); + return this.redirectWithError(res, err.failure.kind); + } + throw err; + } + } + + private redirectWithError(res: Response, kind: AuthCodeFlowError['kind']): void { + const url = new URL(this.entra.postLogoutRedirectUri); + url.searchParams.set('auth_error', authErrorCode({ kind } as AuthCodeFlowError)); + res.redirect(302, url.toString()); + } +} + +function readPreAuthCookie(req: Request): PreAuthPayload | null { + const raw = (req.signedCookies as Record)[PRE_AUTH_COOKIE_NAME]; + if (typeof raw !== 'string') { + return null; + } + try { + const parsed = JSON.parse(raw) as Partial; + if ( + typeof parsed.state === 'string' && + typeof parsed.codeVerifier === 'string' && + typeof parsed.createdAt === 'number' + ) { + return { + state: parsed.state, + codeVerifier: parsed.codeVerifier, + createdAt: parsed.createdAt, + }; + } + return null; + } catch { + return null; + } } diff --git a/apps/portal-bff/src/auth/auth.cookie.ts b/apps/portal-bff/src/auth/auth.cookie.ts index a9c8c18..330489e 100644 --- a/apps/portal-bff/src/auth/auth.cookie.ts +++ b/apps/portal-bff/src/auth/auth.cookie.ts @@ -43,3 +43,21 @@ export function preAuthCookieOptions(): CookieOptions { maxAge: PRE_AUTH_COOKIE_TTL_MS, }; } + +/** + * Options for `res.clearCookie(PRE_AUTH_COOKIE_NAME, …)` — must + * mirror everything but `maxAge` from `preAuthCookieOptions()` so + * the browser actually drops the cookie. Browsers match cookies by + * (name, domain, path) — getting path / sameSite / secure wrong + * leaves the old cookie in place. + */ +export function clearPreAuthCookieOptions(): CookieOptions { + const isProduction = process.env['NODE_ENV'] === 'production'; + return { + signed: true, + httpOnly: true, + sameSite: 'lax', + secure: isProduction, + path: '/', + }; +} diff --git a/apps/portal-bff/src/auth/auth.errors.ts b/apps/portal-bff/src/auth/auth.errors.ts new file mode 100644 index 0000000..3a11501 --- /dev/null +++ b/apps/portal-bff/src/auth/auth.errors.ts @@ -0,0 +1,29 @@ +/** + * Typed failure modes of the OIDC callback. The controller maps each + * to a query-param identifier on the SPA redirect so the front-end + * can render an appropriate message without inventing strings. + * + * Keep this list flat — the controller's `switch` is exhaustive + * thanks to TypeScript's narrowing on `kind`. + */ +export type AuthCodeFlowError = + | { kind: 'state-mismatch' } + | { kind: 'flow-expired' } + | { kind: 'amr-missing' } + | { kind: 'token-exchange-failed'; cause: string }; + +export class AuthCodeFlowException extends Error { + constructor(readonly failure: AuthCodeFlowError) { + super(`auth code flow failed: ${failure.kind}`); + this.name = 'AuthCodeFlowException'; + } +} + +/** + * Stable short codes used as `?auth_error=` on the SPA-bound + * redirect. Same identifiers as the discriminant `kind` so log + * grepping correlates trivially. + */ +export function authErrorCode(failure: AuthCodeFlowError): string { + return failure.kind; +} diff --git a/apps/portal-bff/src/auth/auth.service.spec.ts b/apps/portal-bff/src/auth/auth.service.spec.ts index c4b414d..74e3010 100644 --- a/apps/portal-bff/src/auth/auth.service.spec.ts +++ b/apps/portal-bff/src/auth/auth.service.spec.ts @@ -1,5 +1,7 @@ -import type { ConfidentialClientApplication } from '@azure/msal-node'; -import { AuthService } from './auth.service'; +import type { AuthenticationResult, ConfidentialClientApplication } from '@azure/msal-node'; +import { PRE_AUTH_COOKIE_TTL_MS } from './auth.cookie'; +import { AuthCodeFlowException } from './auth.errors'; +import { AuthService, type PreAuthPayload } from './auth.service'; import type { EntraConfig } from './entra-config.token'; const ENTRA: EntraConfig = { @@ -12,17 +14,54 @@ const ENTRA: EntraConfig = { authority: 'https://login.microsoftonline.com/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee', }; -describe('AuthService', () => { - let getAuthCodeUrl: jest.Mock, [unknown]>; - let service: AuthService; +interface ServiceFixture { + service: AuthService; + getAuthCodeUrl: jest.Mock; + acquireTokenByCode: jest.Mock; +} - beforeEach(() => { - getAuthCodeUrl = jest.fn().mockResolvedValue('https://entra.example/authorize?…'); - const msalStub = { getAuthCodeUrl } as unknown as ConfidentialClientApplication; - service = new AuthService(msalStub, ENTRA); - }); +function makeService(overrides?: { acquireTokenByCode?: jest.Mock }): ServiceFixture { + const getAuthCodeUrl = jest.fn().mockResolvedValue('https://entra.example/authorize?…'); + const acquireTokenByCode = + overrides?.acquireTokenByCode ?? + jest.fn().mockResolvedValue(makeAuthResult({ amr: ['pwd', 'mfa'] })); + const msalStub = { + getAuthCodeUrl, + acquireTokenByCode, + } as unknown as ConfidentialClientApplication; + return { service: new AuthService(msalStub, ENTRA), getAuthCodeUrl, acquireTokenByCode }; +} +function makeAuthResult( + claims: Partial<{ + amr: string[]; + oid: string; + tid: string; + name: string; + preferred_username: string; + }>, +): AuthenticationResult { + return { + idTokenClaims: { + oid: claims.oid ?? 'user-oid', + tid: claims.tid ?? ENTRA.tenantId, + name: claims.name ?? 'Jane Doe', + preferred_username: claims.preferred_username ?? 'jane.doe@apf.example', + amr: claims.amr ?? ['pwd', 'mfa'], + }, + account: { username: 'jane.doe@apf.example', name: 'Jane Doe' }, + } as unknown as AuthenticationResult; +} + +const PRE_AUTH_OK: PreAuthPayload = { + state: 'state-nonce', + codeVerifier: 'verifier-secret', + createdAt: 1_000_000, +}; + +describe('AuthService.beginAuthCodeFlow', () => { it('builds the auth URL with the configured redirect, OIDC scopes, S256 challenge', async () => { + const { service, getAuthCodeUrl } = makeService(); await service.beginAuthCodeFlow(); expect(getAuthCodeUrl).toHaveBeenCalledTimes(1); const arg = getAuthCodeUrl.mock.calls[0]?.[0] as Record; @@ -30,30 +69,102 @@ describe('AuthService', () => { expect(arg['scopes']).toEqual(['openid', 'profile', 'email']); expect(arg['codeChallengeMethod']).toBe('S256'); expect(typeof arg['codeChallenge']).toBe('string'); - expect((arg['codeChallenge'] as string).length).toBeGreaterThan(20); expect(typeof arg['state']).toBe('string'); - expect((arg['state'] as string).length).toBeGreaterThan(8); }); it('returns the pre-auth payload with state + codeVerifier matching what MSAL was called with', async () => { - const { authUrl, preAuthPayload } = await service.beginAuthCodeFlow(); - expect(authUrl).toBe('https://entra.example/authorize?…'); - + const { service, getAuthCodeUrl } = makeService(); + const { preAuthPayload } = await service.beginAuthCodeFlow(); const arg = getAuthCodeUrl.mock.calls[0]?.[0] as Record; expect(preAuthPayload.state).toBe(arg['state']); - // The verifier returned to the caller is the secret the callback - // will send back to Entra; the challenge sent to Entra is its - // SHA-256-of-verifier transform, so the two must differ. expect(preAuthPayload.codeVerifier).not.toBe(arg['codeChallenge']); - expect(typeof preAuthPayload.codeVerifier).toBe('string'); - expect(preAuthPayload.codeVerifier.length).toBeGreaterThan(40); expect(preAuthPayload.createdAt).toBeLessThanOrEqual(Date.now()); }); - it('generates a fresh state + verifier on every call (replay protection)', async () => { + it('generates a fresh state + verifier on every call', async () => { + const { service } = makeService(); const a = await service.beginAuthCodeFlow(); const b = await service.beginAuthCodeFlow(); expect(a.preAuthPayload.state).not.toBe(b.preAuthPayload.state); expect(a.preAuthPayload.codeVerifier).not.toBe(b.preAuthPayload.codeVerifier); }); }); + +describe('AuthService.completeAuthCodeFlow', () => { + it('returns the authenticated user on the happy path', async () => { + const { service, acquireTokenByCode } = makeService(); + const user = await service.completeAuthCodeFlow( + 'auth-code', + PRE_AUTH_OK.state, + PRE_AUTH_OK, + PRE_AUTH_OK.createdAt + 1_000, + ); + expect(acquireTokenByCode).toHaveBeenCalledWith({ + code: 'auth-code', + codeVerifier: PRE_AUTH_OK.codeVerifier, + redirectUri: ENTRA.redirectUri, + scopes: ['openid', 'profile', 'email'], + }); + expect(user).toEqual({ + oid: 'user-oid', + tid: ENTRA.tenantId, + username: 'jane.doe@apf.example', + displayName: 'Jane Doe', + amr: ['pwd', 'mfa'], + }); + }); + + it('throws state-mismatch when the query state differs from the cookie state', async () => { + const { service } = makeService(); + await expect( + service.completeAuthCodeFlow('code', 'other-state', PRE_AUTH_OK, PRE_AUTH_OK.createdAt), + ).rejects.toMatchObject({ failure: { kind: 'state-mismatch' } }); + }); + + it('throws flow-expired when the cookie is older than the TTL', async () => { + const { service } = makeService(); + const now = PRE_AUTH_OK.createdAt + PRE_AUTH_COOKIE_TTL_MS + 1; + await expect( + service.completeAuthCodeFlow('code', PRE_AUTH_OK.state, PRE_AUTH_OK, now), + ).rejects.toMatchObject({ failure: { kind: 'flow-expired' } }); + }); + + it('throws amr-missing when the ID token has no amr claim', async () => { + const acquireTokenByCode = jest.fn().mockResolvedValue(makeAuthResult({ amr: [] })); + const { service } = makeService({ acquireTokenByCode }); + await expect( + service.completeAuthCodeFlow('code', PRE_AUTH_OK.state, PRE_AUTH_OK, PRE_AUTH_OK.createdAt), + ).rejects.toMatchObject({ failure: { kind: 'amr-missing' } }); + }); + + it('throws token-exchange-failed when MSAL throws', async () => { + const acquireTokenByCode = jest.fn().mockRejectedValue(new Error('AADSTS70008')); + const { service } = makeService({ acquireTokenByCode }); + await expect( + service.completeAuthCodeFlow('code', PRE_AUTH_OK.state, PRE_AUTH_OK, PRE_AUTH_OK.createdAt), + ).rejects.toMatchObject({ + failure: { kind: 'token-exchange-failed', cause: 'AADSTS70008' }, + }); + }); + + it('throws token-exchange-failed when MSAL returns null', async () => { + const acquireTokenByCode = jest.fn().mockResolvedValue(null); + const { service } = makeService({ acquireTokenByCode }); + await expect( + service.completeAuthCodeFlow('code', PRE_AUTH_OK.state, PRE_AUTH_OK, PRE_AUTH_OK.createdAt), + ).rejects.toBeInstanceOf(AuthCodeFlowException); + }); + + it('throws token-exchange-failed when oid claim is missing', async () => { + const acquireTokenByCode = jest.fn().mockResolvedValue({ + idTokenClaims: { tid: ENTRA.tenantId, amr: ['mfa'] }, + account: { username: '', name: '' }, + } as unknown as AuthenticationResult); + const { service } = makeService({ acquireTokenByCode }); + await expect( + service.completeAuthCodeFlow('code', PRE_AUTH_OK.state, PRE_AUTH_OK, PRE_AUTH_OK.createdAt), + ).rejects.toMatchObject({ + failure: { kind: 'token-exchange-failed', cause: /oid/ }, + }); + }); +}); diff --git a/apps/portal-bff/src/auth/auth.service.ts b/apps/portal-bff/src/auth/auth.service.ts index 2e082b5..6d00e35 100644 --- a/apps/portal-bff/src/auth/auth.service.ts +++ b/apps/portal-bff/src/auth/auth.service.ts @@ -1,5 +1,11 @@ -import { ConfidentialClientApplication, CryptoProvider } from '@azure/msal-node'; +import { + ConfidentialClientApplication, + CryptoProvider, + type AuthenticationResult, +} from '@azure/msal-node'; import { Inject, Injectable } from '@nestjs/common'; +import { PRE_AUTH_COOKIE_TTL_MS } from './auth.cookie'; +import { AuthCodeFlowException } from './auth.errors'; import { ENTRA_CONFIG, type EntraConfig } from './entra-config.token'; import { MSAL_CLIENT } from './msal-client.token'; @@ -21,6 +27,25 @@ export interface AuthCodeFlowStart { readonly preAuthPayload: PreAuthPayload; } +/** + * Identity resolved by `completeAuthCodeFlow`. Carries the claims + * the rest of the BFF / SPA care about post-authentication: + * - `oid`: stable per-user object id inside the Entra tenant. + * - `tid`: tenant id the user authenticated against (for the + * dual-audience / multi-tenant logic to come). + * - `username` / `displayName`: surfaced in the UI. + * - `amr`: authentication methods reference — the array used by + * the BFF's MFA sanity-check (ADR-0011) and by `@RequireMfa` + * freshness checks once the session lands. + */ +export interface AuthenticatedUser { + readonly oid: string; + readonly tid: string; + readonly username: string; + readonly displayName: string; + readonly amr: readonly string[]; +} + /** * The minimum OIDC scopes — `openid` to get an ID token, `profile` * for the user's display name / preferred_username, `email` for the @@ -71,4 +96,101 @@ export class AuthService { }, }; } + + /** + * Second leg. Verifies the state round-trip, refuses cookies + * older than the flow TTL, asks MSAL to exchange the auth code + * for tokens using the stored PKCE verifier, then runs the + * BFF-side sanity-checks ADR-0009 mandates (state, expiry, `amr`). + * + * MSAL Node performs the heavy ID-token validation itself + * (signature against Entra's JWKS, issuer + audience, exp, nbf). + * What this method adds on top is the application-layer policy: + * - anti-CSRF state binding, + * - flow-TTL replay protection, + * - MFA presence (`amr`) per ADR-0011. + */ + async completeAuthCodeFlow( + code: string, + state: string, + preAuth: PreAuthPayload, + now: number = Date.now(), + ): Promise { + if (state !== preAuth.state) { + throw new AuthCodeFlowException({ kind: 'state-mismatch' }); + } + if (now - preAuth.createdAt > PRE_AUTH_COOKIE_TTL_MS) { + throw new AuthCodeFlowException({ kind: 'flow-expired' }); + } + + let result: AuthenticationResult | null; + try { + result = await this.msal.acquireTokenByCode({ + code, + codeVerifier: preAuth.codeVerifier, + redirectUri: this.config.redirectUri, + scopes: [...SCOPES], + }); + } catch (err) { + throw new AuthCodeFlowException({ + kind: 'token-exchange-failed', + cause: errorCause(err), + }); + } + + if (!result) { + throw new AuthCodeFlowException({ + kind: 'token-exchange-failed', + cause: 'msal returned null result', + }); + } + + return this.toAuthenticatedUser(result); + } + + private toAuthenticatedUser(result: AuthenticationResult): AuthenticatedUser { + const claims = result.idTokenClaims as Record; + const amr = Array.isArray(claims['amr']) + ? (claims['amr'] as unknown[]).filter((v): v is string => typeof v === 'string') + : []; + + // ADR-0011 sanity-check: Conditional Access on the org side is + // the actual enforcement layer; the BFF refuses tokens that + // lack any `amr` evidence (an empty array would indicate a + // policy misconfiguration where MFA never fired). + if (amr.length === 0) { + throw new AuthCodeFlowException({ kind: 'amr-missing' }); + } + + return { + oid: requireString(claims['oid'], 'oid'), + tid: requireString(claims['tid'], 'tid'), + username: + typeof claims['preferred_username'] === 'string' + ? (claims['preferred_username'] as string) + : (result.account?.username ?? ''), + displayName: + typeof claims['name'] === 'string' + ? (claims['name'] as string) + : (result.account?.name ?? ''), + amr, + }; + } +} + +function requireString(value: unknown, claim: string): string { + if (typeof value !== 'string' || value === '') { + throw new AuthCodeFlowException({ + kind: 'token-exchange-failed', + cause: `id token missing required string claim: ${claim}`, + }); + } + return value; +} + +function errorCause(err: unknown): string { + if (err instanceof Error) { + return err.message; + } + return String(err); }