julien/apf_portal
1
0
Fork 0
Code Issues 1 Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(portal-bff): drop strict amr check — flow blocked when Entra omits the claim #108

Merged
julien merged 1 commits from fix/portal-bff/auth-relax-amr-check into main 2026-05-12 15:31:57 +02:00
Conversation 0 Commits 1 Files Changed 3
+25 -13
julien commented 2026-05-12 15:27:19 +02:00
Owner
Copy Link
Copy Source

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: 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: 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: 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).
## 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).
julien added 1 commit 2026-05-12 15:27:19 +02:00
fix(portal-bff): drop strict amr check — flow blocked when Entra omits the claim
CI / commits (pull_request) Successful in 2m5s
Details
CI / scan (pull_request) Successful in 2m17s
Details
CI / check (pull_request) Successful in 2m19s
Details
CI / a11y (pull_request) Successful in 1m12s
Details
CI / perf (pull_request) Successful in 3m48s
Details
afd9f175bf 
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).
julien merged commit bfa35d3283 into main 2026-05-12 15:31:57 +02:00
julien deleted branch fix/portal-bff/auth-relax-amr-check 2026-05-12 15:31:58 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adastra-bot (AdAstra Bot) julien (Julien Gautier)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: julien/apf_portal#108
Delete Branch "fix/portal-bff/auth-relax-amr-check"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?