fix(portal-bff): drop strict amr check — flow blocked when Entra omits the claim
CI / commits (pull_request) Successful in 2m5s
CI / scan (pull_request) Successful in 2m17s
CI / check (pull_request) Successful in 2m19s
CI / a11y (pull_request) Successful in 1m12s
CI / perf (pull_request) Successful in 3m48s

The original `amr-missing` guard in PR #107 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 without a CA policy on the app. Rejecting
tokens on empty `amr` blocks every legitimate dev sign-in against a
tenant that hasn't yet had CA configured — observed in practice:

    {"context":"AuthCallback","event":"auth.flow_error",
     "failure":{"kind":"amr-missing"}}

ADR-0011 specifies that MFA enforcement is the Conditional Access
policy's job (org-side) and the `@RequireMfa({ freshness: 600 })`
decorator's job on sensitive routes (designed-in, no v1 consumer).
The BFF's part is to surface `amr` through the audit log and the
future guard, NOT to gate the callback on its presence.

Changes:

- `AuthCodeFlowError` discriminator drops the `amr-missing` variant.
  Three failure modes left: state-mismatch, flow-expired,
  token-exchange-failed. MSAL's own ID-token validation (signature,
  issuer, audience, exp, nbf) is the real gate at this stage.
- `AuthService.toAuthenticatedUser` keeps extracting `amr` and
  passing it through (as a possibly-empty string array) so the
  log line and future `@RequireMfa` guard still see it. The strict
  `if (amr.length === 0) throw` is gone, replaced by a comment
  block explaining the new shape.
- Spec test `'throws amr-missing'` becomes `'returns the user even
  when the ID token has no amr claim'` — asserts the array
  passes through empty rather than blocking the flow.

Verified: 52/52 specs green, lint clean, build green. Smoke test
against the live tenant — sign-in now lands cleanly on the SPA;
Pino's `auth.signed_in` log shows the resolved identity with the
`amr` value (often `[]` until CA is configured on the org side).
This commit is contained in:
Julien Gautier
2026-05-12 15:25:17 +02:00
parent c50794eceb
commit afd9f175bf
3 changed files with 25 additions and 13 deletions
-1
View File
@@ -9,7 +9,6 @@
export type AuthCodeFlowError = export type AuthCodeFlowError =
| { kind: 'state-mismatch' } | { kind: 'state-mismatch' }
| { kind: 'flow-expired' } | { kind: 'flow-expired' }
| { kind: 'amr-missing' }
| { kind: 'token-exchange-failed'; cause: string }; | { kind: 'token-exchange-failed'; cause: string };
export class AuthCodeFlowException extends Error { export class AuthCodeFlowException extends Error {
+11 -4
View File
@@ -129,12 +129,19 @@ describe('AuthService.completeAuthCodeFlow', () => {
).rejects.toMatchObject({ failure: { kind: 'flow-expired' } }); ).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 acquireTokenByCode = jest.fn().mockResolvedValue(makeAuthResult({ amr: [] }));
const { service } = makeService({ acquireTokenByCode }); const { service } = makeService({ acquireTokenByCode });
await expect( const user = await service.completeAuthCodeFlow(
service.completeAuthCodeFlow('code', PRE_AUTH_OK.state, PRE_AUTH_OK, PRE_AUTH_OK.createdAt), 'code',
).rejects.toMatchObject({ failure: { kind: 'amr-missing' } }); 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 () => { it('throws token-exchange-failed when MSAL throws', async () => {
+14 -8
View File
@@ -150,18 +150,24 @@ export class AuthService {
private toAuthenticatedUser(result: AuthenticationResult): AuthenticatedUser { private toAuthenticatedUser(result: AuthenticationResult): AuthenticatedUser {
const claims = result.idTokenClaims as Record<string, unknown>; const claims = result.idTokenClaims as Record<string, unknown>;
// `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']) const amr = Array.isArray(claims['amr'])
? (claims['amr'] as unknown[]).filter((v): v is string => typeof v === 'string') ? (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 { return {
oid: requireString(claims['oid'], 'oid'), oid: requireString(claims['oid'], 'oid'),
tid: requireString(claims['tid'], 'tid'), tid: requireString(claims['tid'], 'tid'),