From f9f0151717401d6fc7d3eb1dc0f9fe85cd960052 Mon Sep 17 00:00:00 2001 From: Julien Gautier Date: Thu, 14 May 2026 00:30:37 +0200 Subject: [PATCH] feat(portal-bff): extract Entra `roles` claim onto AuthenticatedUser (#126) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary First step in the `portal-admin` audit-log-viewer workstream (per [ADR-0020](docs/decisions/0020-portal-admin-app.md)). The BFF's `AdminRoleGuard` (next PR) needs to read `session.user.roles` to enforce admin-only access to `/api/admin/*`. Today the session carries `{ oid, tid, username, displayName, amr }` — the `roles` claim is dropped on the floor when the ID token comes back from Entra. This PR closes that gap: - Adds `roles: readonly string[]` to [AuthenticatedUser](apps/portal-bff/src/auth/auth.service.ts) and threads it through `toAuthenticatedUser()`. - The field flows onto `req.session.user` automatically via the existing module-augmentation chain in [session.types.ts](apps/portal-bff/src/session/session.types.ts) — no extra wiring. ## Defensive parsing Mirrors the existing `amr` extraction pattern: | Input claim shape | Result | | --- | --- | | `["admin", "editor"]` | `["admin", "editor"]` | | Claim absent | `[]` | | Non-array (e.g. `"admin"`) | `[]` | | Mixed types (e.g. `["admin", 42, null, "editor"]`) | `["admin", "editor"]` | Empty array means **"user has no app role assigned"**, not **"claim was unparseable"** — both collapse to the same value because both are equally non-authoritative for the admin guard. ## Why this is its own PR The `AdminRoleGuard` + `@RequireAdmin()` decorator + first `/api/admin/me` self-test endpoint will follow in the next PR. Splitting the claim extraction out makes both diffs trivial to read and lets the second PR focus on guard semantics + audit emission without the mechanical fixture updates that came with adding a new `AuthenticatedUser` field. ## Surface impact — none yet - `PublicUser` (the SPA-facing shape returned by `GET /api/auth/me`) is **deliberately unchanged**. Exposing `roles` to the SPA happens in the next PR alongside the conditional admin-link rendering — without a consumer in this PR it would be dead code. - Audit pipeline unchanged. `SignInActor` carries `{ oid, amr }` only; the audit log doesn't need `roles` and won't get it. - No new env vars, no new dependencies. ## Test plan - [x] `pnpm nx test portal-bff` — **203 specs pass** (was 199; +4 new specs covering the four parsing cases above). - [x] `pnpm exec nx affected -t format:check lint test build --base=origin/main` — clean (the pre-existing `_res` / `_next` warnings in `rate-limit.middleware.ts` are unrelated). - [x] Existing fixtures in [auth.controller.spec.ts](apps/portal-bff/src/auth/auth.controller.spec.ts), [auth.service.spec.ts](apps/portal-bff/src/auth/auth.service.spec.ts), [absolute-timeout.middleware.spec.ts](apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts) updated with `roles: []`. - [ ] e2e — would require the `admin` app role to be declared on the Entra registration and assigned to a test user. Out of scope for this PR; will be validated when the `AdminRoleGuard` lands and there is a 403 to observe. --------- Co-authored-by: Julien Gautier Reviewed-on: https://git.unespace.com/julien/apf_portal/pulls/126 --- .../src/auth/auth.controller.spec.ts | 1 + apps/portal-bff/src/auth/auth.service.spec.ts | 76 +++++++++++++++++-- apps/portal-bff/src/auth/auth.service.ts | 21 +++++ .../absolute-timeout.middleware.spec.ts | 1 + 4 files changed, 92 insertions(+), 7 deletions(-) diff --git a/apps/portal-bff/src/auth/auth.controller.spec.ts b/apps/portal-bff/src/auth/auth.controller.spec.ts index 98d4d01..e7591ad 100644 --- a/apps/portal-bff/src/auth/auth.controller.spec.ts +++ b/apps/portal-bff/src/auth/auth.controller.spec.ts @@ -30,6 +30,7 @@ const USER: AuthenticatedUser = { username: 'jane.doe@apf.example', displayName: 'Jane Doe', amr: ['pwd', 'mfa'], + roles: [], }; function makeResStub() { diff --git a/apps/portal-bff/src/auth/auth.service.spec.ts b/apps/portal-bff/src/auth/auth.service.spec.ts index 73f82b4..46c57ab 100644 --- a/apps/portal-bff/src/auth/auth.service.spec.ts +++ b/apps/portal-bff/src/auth/auth.service.spec.ts @@ -35,20 +35,29 @@ function makeService(overrides?: { acquireTokenByCode?: jest.Mock }): ServiceFix function makeAuthResult( claims: Partial<{ amr: string[]; + roles: unknown; oid: string; tid: string; name: string; preferred_username: string; }>, ): AuthenticationResult { + const idTokenClaims: Record = { + oid: claims.oid ?? 'user-oid', + tid: claims.tid ?? ENTRA.tenantId, + name: claims.name ?? 'Jane Doe', + preferred_username: claims.preferred_username ?? 'jane.doe@apf.example', + amr: claims.amr ?? ['pwd', 'mfa'], + }; + // `roles` is optional in Entra ID tokens — only present when the + // user has at least one app role assigned. The fixture omits it + // unless the caller explicitly opts in (including with falsy + // values to exercise the empty-array fallback). + if (Object.prototype.hasOwnProperty.call(claims, 'roles')) { + idTokenClaims['roles'] = claims.roles; + } return { - idTokenClaims: { - oid: claims.oid ?? 'user-oid', - tid: claims.tid ?? ENTRA.tenantId, - name: claims.name ?? 'Jane Doe', - preferred_username: claims.preferred_username ?? 'jane.doe@apf.example', - amr: claims.amr ?? ['pwd', 'mfa'], - }, + idTokenClaims, account: { username: 'jane.doe@apf.example', name: 'Jane Doe' }, } as unknown as AuthenticationResult; } @@ -111,6 +120,7 @@ describe('AuthService.completeAuthCodeFlow', () => { username: 'jane.doe@apf.example', displayName: 'Jane Doe', amr: ['pwd', 'mfa'], + roles: [], }); }); @@ -144,6 +154,58 @@ describe('AuthService.completeAuthCodeFlow', () => { expect(user.oid).toBe('user-oid'); }); + it('surfaces the `roles` claim when Entra includes it (app-role-assigned user)', async () => { + const acquireTokenByCode = jest + .fn() + .mockResolvedValue(makeAuthResult({ roles: ['admin', 'editor'] })); + const { service } = makeService({ acquireTokenByCode }); + const user = await service.completeAuthCodeFlow( + 'code', + PRE_AUTH_OK.state, + PRE_AUTH_OK, + PRE_AUTH_OK.createdAt, + ); + expect(user.roles).toEqual(['admin', 'editor']); + }); + + it('returns an empty `roles` array when the claim is absent (no app role assigned)', async () => { + const acquireTokenByCode = jest.fn().mockResolvedValue(makeAuthResult({})); + const { service } = makeService({ acquireTokenByCode }); + const user = await service.completeAuthCodeFlow( + 'code', + PRE_AUTH_OK.state, + PRE_AUTH_OK, + PRE_AUTH_OK.createdAt, + ); + expect(user.roles).toEqual([]); + }); + + it('returns an empty `roles` array when the claim is non-array (defensive)', async () => { + const acquireTokenByCode = jest.fn().mockResolvedValue(makeAuthResult({ roles: 'admin' })); + const { service } = makeService({ acquireTokenByCode }); + const user = await service.completeAuthCodeFlow( + 'code', + PRE_AUTH_OK.state, + PRE_AUTH_OK, + PRE_AUTH_OK.createdAt, + ); + expect(user.roles).toEqual([]); + }); + + it('drops non-string entries from `roles` (defensive against unexpected claim shapes)', async () => { + const acquireTokenByCode = jest + .fn() + .mockResolvedValue(makeAuthResult({ roles: ['admin', 42, null, 'editor'] })); + const { service } = makeService({ acquireTokenByCode }); + const user = await service.completeAuthCodeFlow( + 'code', + PRE_AUTH_OK.state, + PRE_AUTH_OK, + PRE_AUTH_OK.createdAt, + ); + expect(user.roles).toEqual(['admin', 'editor']); + }); + it('throws token-exchange-failed when MSAL throws', async () => { const acquireTokenByCode = jest.fn().mockRejectedValue(new Error('AADSTS70008')); const { service } = makeService({ acquireTokenByCode }); diff --git a/apps/portal-bff/src/auth/auth.service.ts b/apps/portal-bff/src/auth/auth.service.ts index 6f4e9ef..9c068b3 100644 --- a/apps/portal-bff/src/auth/auth.service.ts +++ b/apps/portal-bff/src/auth/auth.service.ts @@ -37,6 +37,12 @@ export interface AuthCodeFlowStart { * - `amr`: authentication methods reference — the array used by * the BFF's MFA sanity-check (ADR-0011) and by `@RequireMfa` * freshness checks once the session lands. + * - `roles`: Entra app roles assigned to this user on the BFF's + * app registration. Surfaced for the future `@RequireAdmin` / + * `@RequireRole(...)` guards (ADR-0020 admin module). Always + * present in the shape — an empty array means the user has no + * app role on this app registration, not that the claim was + * unparseable. */ export interface AuthenticatedUser { readonly oid: string; @@ -44,6 +50,7 @@ export interface AuthenticatedUser { readonly username: string; readonly displayName: string; readonly amr: readonly string[]; + readonly roles: readonly string[]; } /** @@ -186,6 +193,19 @@ export class AuthService { ? (claims['amr'] as unknown[]).filter((v): v is string => typeof v === 'string') : []; + // `roles` is an optional claim. Entra includes it when the user + // has at least one app role assigned on the BFF's app + // registration. The value is an array of the role `value` + // strings declared in the registration manifest (e.g. `admin`). + // No role assignment → claim is omitted entirely; we normalise + // to an empty array so consumers can call `.includes('admin')` + // unconditionally. Per ADR-0020, role-based authorisation is the + // single source of authority for admin access; the AdminRoleGuard + // will read this field on every `/api/admin/*` request. + const roles = Array.isArray(claims['roles']) + ? (claims['roles'] as unknown[]).filter((v): v is string => typeof v === 'string') + : []; + return { oid: requireString(claims['oid'], 'oid'), tid: requireString(claims['tid'], 'tid'), @@ -198,6 +218,7 @@ export class AuthService { ? (claims['name'] as string) : (result.account?.name ?? ''), amr, + roles, }; } } diff --git a/apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts b/apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts index 2aa27d4..4fd2d6b 100644 --- a/apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts +++ b/apps/portal-bff/src/session/absolute-timeout.middleware.spec.ts @@ -68,6 +68,7 @@ const USER = { username: 'jane@apf.example', displayName: 'Jane Doe', amr: ['pwd', 'mfa'], + roles: [], }; describe('absoluteTimeout middleware', () => {