fix(portal-bff): drop strict amr check — flow blocked when Entra omits the claim (#108)
## 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 <julien.gautier@apf.asso.fr>
Reviewed-on: #108
This commit was merged in pull request #108.
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -150,18 +150,24 @@ export class AuthService {
|
||||
|
||||
private toAuthenticatedUser(result: AuthenticationResult): AuthenticatedUser {
|
||||
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'])
|
||||
? (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'),
|
||||
|
||||
Reference in New Issue
Block a user