From bfa35d3283b58b73f05acfa2f40dca1f7c3cde8d Mon Sep 17 00:00:00 2001 From: Julien Gautier Date: Tue, 12 May 2026 15:31:57 +0200 Subject: [PATCH] =?UTF-8?q?fix(portal-bff):=20drop=20strict=20amr=20check?= =?UTF-8?q?=20=E2=80=94=20flow=20blocked=20when=20Entra=20omits=20the=20cl?= =?UTF-8?q?aim=20(#108)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Bug After a real sign-in against the Entra tenant, the callback rejected the flow with: ``` {"context":"AuthCallback","event":"auth.flow_error","failure":{"kind":"amr-missing"}} ``` The user landed on the SPA with `?auth_error=amr-missing` instead of authenticated. Every dev sign-in is blocked. ## Root cause PR #107's `amr-missing` guard misread ADR-0011's intent. `amr` is an **optional** claim in Entra ID tokens: it's populated for fresh interactive sign-ins where Conditional Access asked for an MFA method, and frequently absent for SSO / refresh flows or in tenants where no CA policy is configured on the app registration. Rejecting tokens on empty `amr` blocks every legitimate sign-in against such a tenant. ADR-0011 actually specifies: - **Conditional Access** (org-side) is the enforcement layer for "MFA happened". - The **`@RequireMfa({ freshness: 600 })`** decorator (designed-in, no v1 consumer) is what guards sensitive routes. - The BFF surfaces `amr` through the audit log and the future guard, not as a callback precondition. ## Fix - **[`auth.errors.ts`](apps/portal-bff/src/auth/auth.errors.ts)**: drop the `amr-missing` variant from the `AuthCodeFlowError` discriminator. Three failure modes left: `state-mismatch`, `flow-expired`, `token-exchange-failed`. MSAL's ID-token validation (signature, issuer, audience, exp, nbf) is the real gate at this stage. - **[`auth.service.ts`](apps/portal-bff/src/auth/auth.service.ts)**: `toAuthenticatedUser` keeps extracting `amr` and passing it through (as a possibly-empty string array) so the structured log line and the future `@RequireMfa` guard still see it. The strict `if (amr.length === 0) throw` is replaced by a comment explaining the new shape. - **[`auth.service.spec.ts`](apps/portal-bff/src/auth/auth.service.spec.ts)**: the `'throws amr-missing'` test becomes `'returns the user even when the ID token has no amr claim'` — asserts the array passes through empty rather than blocking the flow. ## Verification - `nx run-many -t lint test build --projects=portal-bff` — green. **52/52 specs**. - Manual smoke: end-to-end sign-in against the live tenant now lands cleanly on the SPA; Pino's `auth.signed_in` log shows the resolved identity with `amr` (often `[]` until CA is configured on the org side). --------- Co-authored-by: Julien Gautier Reviewed-on: https://git.unespace.com/julien/apf_portal/pulls/108 --- apps/portal-bff/src/auth/auth.errors.ts | 1 - apps/portal-bff/src/auth/auth.service.spec.ts | 15 +++++++++---- apps/portal-bff/src/auth/auth.service.ts | 22 ++++++++++++------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/apps/portal-bff/src/auth/auth.errors.ts b/apps/portal-bff/src/auth/auth.errors.ts index 3a11501..63738c3 100644 --- a/apps/portal-bff/src/auth/auth.errors.ts +++ b/apps/portal-bff/src/auth/auth.errors.ts @@ -9,7 +9,6 @@ export type AuthCodeFlowError = | { kind: 'state-mismatch' } | { kind: 'flow-expired' } - | { kind: 'amr-missing' } | { kind: 'token-exchange-failed'; cause: string }; export class AuthCodeFlowException extends Error { diff --git a/apps/portal-bff/src/auth/auth.service.spec.ts b/apps/portal-bff/src/auth/auth.service.spec.ts index 74e3010..109ad7e 100644 --- a/apps/portal-bff/src/auth/auth.service.spec.ts +++ b/apps/portal-bff/src/auth/auth.service.spec.ts @@ -129,12 +129,19 @@ describe('AuthService.completeAuthCodeFlow', () => { ).rejects.toMatchObject({ failure: { kind: 'flow-expired' } }); }); - it('throws amr-missing when the ID token has no amr claim', async () => { + it('returns the user even when the ID token has no `amr` claim (Entra optional)', 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' } }); + const user = await service.completeAuthCodeFlow( + 'code', + PRE_AUTH_OK.state, + PRE_AUTH_OK, + PRE_AUTH_OK.createdAt, + ); + // `amr` flows through as an empty array; MFA enforcement is + // Conditional Access's job per ADR-0011, not this code path. + expect(user.amr).toEqual([]); + expect(user.oid).toBe('user-oid'); }); it('throws token-exchange-failed when MSAL throws', async () => { diff --git a/apps/portal-bff/src/auth/auth.service.ts b/apps/portal-bff/src/auth/auth.service.ts index 6d00e35..eb8ad72 100644 --- a/apps/portal-bff/src/auth/auth.service.ts +++ b/apps/portal-bff/src/auth/auth.service.ts @@ -150,18 +150,24 @@ export class AuthService { private toAuthenticatedUser(result: AuthenticationResult): AuthenticatedUser { const claims = result.idTokenClaims as Record; + + // `amr` is an optional claim. Entra includes it when it has + // explicit auth-method tracking to report (fresh interactive + // sign-in with MFA, etc.) and may omit it for SSO / refresh + // flows or when no Conditional Access policy requires the + // method to be surfaced. The BFF surfaces whatever is there in + // the audit log / future `@RequireMfa` guard — but it does NOT + // reject the token on empty / missing `amr`. Per ADR-0011, MFA + // enforcement is org-side via Conditional Access, and the + // BFF's "sanity check" is that MSAL accepted the token at all + // (signature + issuer + audience + exp validated). Sensitive + // routes that need a fresh MFA event will assert it explicitly + // through the `@RequireMfa({ freshness: 600 })` decorator once + // it lands. 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'),