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

fix(portal-bff): align admin entra role name with Portal.Admin #145

Merged
julien merged 1 commits from fix/portal-bff-portal-admin-role-name into main 2026-05-15 10:33:55 +02:00
Conversation 0 Commits 1 Files Changed 6
+18 -18
julien commented 2026-05-15 10:33:17 +02:00
Owner
Copy Link
Copy Source

Summary

The AdminRoleGuard 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 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:

-export const ADMIN_ROLE = 'admin';
+export const ADMIN_ROLE = 'Portal.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, admin.controller.spec.ts, admin-auth.controller.spec.ts, 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:

Documentation

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

  • 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.
  • 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).
## 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. - [x] 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).
julien added 1 commit 2026-05-15 10:33:17 +02:00
fix(portal-bff): align admin entra role name with Portal.Admin
CI / scan (pull_request) Successful in 2m47s
Details
CI / commits (pull_request) Successful in 2m47s
Details
CI / check (pull_request) Successful in 2m56s
Details
CI / a11y (pull_request) Successful in 2m40s
Details
CI / perf (pull_request) Successful in 5m51s
Details
ad369d872a 
The AdminRoleGuard was matching on `admin`, but the Entra app
registration declares the role with value `Portal.Admin`. Sessions
were established with `roles: []`, so every authenticated user got
a 403 on `/api/admin/audit` and `/api/admin/users`.

Single source of truth: the `ADMIN_ROLE` constant in
admin-role.guard.ts. The spec already imports the constant, so the
behaviour rolls through unchanged.

Doc-comment touches in admin-role.guard.ts, admin.controller.ts and
audit.service.ts keep the inline references accurate; ADR-0020,
docs/architecture.md and CLAUDE.md updated to the new name.
julien merged commit 2480d0dd6d into main 2026-05-15 10:33:55 +02:00
julien deleted branch fix/portal-bff-portal-admin-role-name 2026-05-15 10:33:56 +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#145
Delete Branch "fix/portal-bff-portal-admin-role-name"

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?