fix(portal-bff): align admin entra role name with Portal.Admin (#145)
CI / check (push) Successful in 2m47s
CI / commits (push) Has been skipped
CI / scan (push) Successful in 1m47s
CI / a11y (push) Successful in 1m30s
CI / perf (push) Successful in 3m57s

## Summary

The [`AdminRoleGuard`](apps/portal-bff/src/admin/admin-role.guard.ts) was matching on the literal `'admin'`, but the Entra app registration declares the admin app role with `value: "Portal.Admin"`. End result: an authenticated user with the role assigned in Entra still landed with `roles: []` in their session (claim simply not present in the id token), and every request to `/api/admin/audit` and `/api/admin/users` returned a **403**.

Caught manually in the portal-admin SPA: login succeeded, sidebar links to "Audit log" / "User list" returned 403. The [`/api/admin/auth/me`](apps/portal-bff/src/admin/admin-auth.controller.ts) self-test confirmed the missing claim was the cause.

## What lands

### Constant value — single source of truth

[`apps/portal-bff/src/admin/admin-role.guard.ts:18`](apps/portal-bff/src/admin/admin-role.guard.ts#L18):

```diff
-export const ADMIN_ROLE = 'admin';
+export const ADMIN_ROLE = 'Portal.Admin';
```

[`admin-role.guard.spec.ts`](apps/portal-bff/src/admin/admin-role.guard.spec.ts) already imports `ADMIN_ROLE` from the source rather than hardcoding the literal, so the guard contract spec rolls through unchanged. The fixtures elsewhere ([`auth.service.spec.ts`](apps/portal-bff/src/auth/auth.service.spec.ts), [`admin.controller.spec.ts`](apps/portal-bff/src/admin/admin.controller.spec.ts), [`admin-auth.controller.spec.ts`](apps/portal-bff/src/admin/admin-auth.controller.spec.ts), [`require-mfa.guard.spec.ts`](apps/portal-bff/src/auth/require-mfa.guard.spec.ts)) keep `roles: ['admin']` as fixture data — those tests exercise the extraction / serialization pipeline, which is role-value-agnostic; touching them would be incidental cleanup with no behaviour signal.

### Doc-comment refresh

Inline references to the role name updated so future readers don't grep `'admin'` and find a phantom value:

- [`admin-role.guard.ts`](apps/portal-bff/src/admin/admin-role.guard.ts) — class doc-block (3 mentions).
- [`admin.controller.ts`](apps/portal-bff/src/admin/admin.controller.ts) — class doc-block + inline guard-contract comment.
- [`audit.service.ts`](apps/portal-bff/src/audit/audit.service.ts) — `adminAccessDenied` doc-block (2 mentions).

### Documentation

- [`docs/decisions/0020-portal-admin-app.md`](docs/decisions/0020-portal-admin-app.md) — 5 references to the role across §"How is admin access enforced", §"Auth — same Entra ID …", and the Consequences §.
- [`docs/architecture.md`](docs/architecture.md) — note next to the C4 container diagram describing the admin entry gate.
- [`CLAUDE.md`](CLAUDE.md) — "Admin application" project rule.

## Notes for the reviewer

- **Why `Portal.Admin` rather than `admin`?** Operator's call on the Entra side. The `<Application>.<Role>` namespace is the conventional Entra App Role pattern when the directory may host roles for multiple applications, and `admin` alone is ambiguous in a directory shared across products.
- **Why no migration / backfill?** The role value lives only in two places: Entra app-registration manifest (operator-managed) and the BFF constant (this PR). Existing Redis sessions captured `roles: []` (claim absent) — they'll naturally pick up the correct value on next sign-in. No persisted data references the old value.
- **No ADR.** ADR-0020 §"How is admin access enforced" already commits to "Entra ID role claim + BFF guard"; the literal role string is an implementation detail the ADR happened to spell. Updated the ADR's prose to the new value to keep the doc honest, but the decision is unchanged.

## Test plan

- [x] `pnpm nx test portal-bff` — **396 specs pass**, unchanged from `main`. The `AdminRoleGuard` contract spec (covers 401-on-no-session, 403-on-missing-role + audit emission, pass-through-on-role-present) imports `ADMIN_ROLE` and re-exercises with the new value.
- [x] `pnpm exec nx affected -t format:check lint test build --base=origin/main` — clean.
- [ ] Manual verification — pending Entra-side App Role declaration with `value: "Portal.Admin"` + assignment to the test user. Once both exist: sign out + sign in on portal-admin, hit `/api/admin/auth/me` and confirm `roles: ["Portal.Admin"]`, then click "Audit log" + "User list" and confirm both render. An `admin.access_denied` row in `audit.events` is the negative-test signal (still emitted for any user without the role).

---------

Co-authored-by: Julien Gautier <julien.gautier@apf.asso.fr>
Reviewed-on: #145
This commit was merged in pull request #145.
This commit is contained in:
2026-05-15 10:33:54 +02:00
parent 0335d992d9
commit 2480d0dd6d
6 changed files with 18 additions and 18 deletions
@@ -10,12 +10,12 @@ import { AuditWriter } from '../audit/audit.service';
/**
* Single Entra app role that gates the entire `/api/admin/*` surface
* per ADR-0020 §"Auth — `admin` role claim". The value matches the
* `value` field declared on the app role in the Entra app
* per ADR-0020 §"Auth — `Portal.Admin` role claim". The value matches
* the `value` field declared on the app role in the Entra app
* registration manifest. Defined as a constant so spec assertions
* can refer to the same source of truth.
*/
export const ADMIN_ROLE = 'admin';
export const ADMIN_ROLE = 'Portal.Admin';
/**
* `AdminRoleGuard` — enforces ADR-0020's role-based admin gate.
@@ -28,7 +28,7 @@ export const ADMIN_ROLE = 'admin';
* unauthenticated 401 is normal traffic the absolute-timeout
* middleware would surface anyway.
*
* - **Session but missing `admin` role → 403 + audit.** The user
* - **Session but missing `Portal.Admin` role → 403 + audit.** The user
* *is* authenticated; they just are not authorised for admin.
* This is the privilege-escalation attempt audit signal — every
* denial lands in `audit.events` with `outcome=denied`, the
@@ -38,7 +38,7 @@ export const ADMIN_ROLE = 'admin';
* (consistent with the existing audit call sites in
* `AuthController`).
*
* - **Session with `admin` role → pass through.** Downstream
* - **Session with `Portal.Admin` role → pass through.** Downstream
* controllers see `req.session.user` populated and can rely on
* the role check having happened.
*/
@@ -5,8 +5,8 @@ import { RequireAdmin } from './require-admin.decorator';
/**
* `/api/admin/*` controllers per ADR-0020. The `@RequireAdmin()`
* class-level decorator gates every route on the Entra `admin` role;
* downstream PRs will add per-method `@RequireMfa({ freshness: … })`
* class-level decorator gates every route on the Entra `Portal.Admin`
* role; downstream PRs will add per-method `@RequireMfa({ freshness: … })`
* for the entry route.
*
* `GET /api/admin/me` is the self-test endpoint named in ADR-0020's
@@ -27,7 +27,7 @@ export class AdminController {
@Get('me')
me(@Req() req: Request) {
// `AdminRoleGuard` has already established that `req.session.user`
// is present and has the `admin` role — the handler can read the
// is present and has the `Portal.Admin` role — the handler can read the
// field without re-checking. Using the non-null assertion keeps
// the dead-branch noise out of the controller while leaving the
// safety contract anchored in the guard's spec.