feat(portal-bff): /auth/callback route — token exchange + amr check (#107)
## 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=<code>` 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 <julien.gautier@apf.asso.fr>
Reviewed-on: #107
This commit was merged in pull request #107.
This commit is contained in:
@@ -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<string, unknown> = {}): 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<typeof makeLoggerStub>;
|
||||
}
|
||||
|
||||
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'),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user