From 3ed6dae3a590cf1cc5c36f3b879a88f257721c30 Mon Sep 17 00:00:00 2001 From: Julien Gautier Date: Thu, 14 May 2026 01:17:30 +0200 Subject: [PATCH] feat(portal-bff): admin module + role guard + /api/admin/me self-test (#127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Lays the foundation for the `/api/admin/*` surface per [ADR-0020](docs/decisions/0020-portal-admin-app.md). This PR ships the role guard, the `@RequireAdmin()` decorator, and a self-test endpoint — no business routes yet. The next consumer (audit log viewer) lands in a later PR once the distinct admin session is in place. ## What ships - **[AdminRoleGuard](apps/portal-bff/src/admin/admin-role.guard.ts)** — three branches: - No session at all → **401**. No audit; unauthenticated probes are normal traffic, not a privilege-escalation signal. - Session but `roles` lacks `admin` → **403** + `admin.access_denied` audit row with actor hash, attempted route (`${METHOD} ${originalUrl}`), and the roles the user did hold. - Session with `admin` role → pass through. - Audit-write failures propagate (no audit ⇒ no action, consistent with the existing call sites in [AuthController](apps/portal-bff/src/auth/auth.controller.ts)). - **[`@RequireAdmin()`](apps/portal-bff/src/admin/require-admin.decorator.ts)** — semantic sugar for `@UseGuards(AdminRoleGuard)` built with `applyDecorators` so future composition (e.g. with the upcoming `@RequireMfa({ freshness })` for the admin entry route) is mechanical. - **`GET /api/admin/me`** — self-test endpoint named in ADR-0020 §"Confirmation" step 3. Returns the public user payload + `roles` so ops can `curl` the gate with three sessions (no cookie / non-admin cookie / admin cookie) and observe `401` / `403` + audit / `200` respectively. - **[`AuditWriter.adminAccessDenied()`](apps/portal-bff/src/audit/audit.service.ts)** — new typed method using the pre-existing `denied` outcome enum value. Keeps the salt inside the audit module, matches the pattern of `signIn` / `signOut` / `sessionExpired`. ## Why the shared portal-shell session (for now) ADR-0020 mandates a distinct `__Host-portal_admin_session` cookie + Redis namespace `session:admin:*` for the admin app. **That is not in this PR.** The chantier sequence splits it out: this PR proves the guard semantics + audit integration on the existing session; the next PR introduces the distinct session middleware + admin-specific auth flow. Rationale: the guard logic is independent of the session implementation — `session.user.roles` is the only field it reads. Landing it first means a smaller diff to review, a faster opportunity to validate the audit emission on a real Postgres, and a clean baseline to layer the session split onto. ## Notes for the reviewer - The non-null assertion on `req.session.user!` in [admin.controller.ts:27](apps/portal-bff/src/admin/admin.controller.ts#L27) is explicitly disabled with an inline comment pointing at the guard's contract. The alternative (a defensive runtime check) duplicates the guard's logic without adding safety. The spec for the guard covers every branch including the absent-user path. - `AdminController` does not depend on `AuthService`'s `toPublicUser` projection — that helper is private to the auth module and pulls `displayName` / `username` with extra account-object fallbacks specific to the OIDC callback. The admin response is built from the already-populated session, so a duplicated projection here is the simpler shape. ## Open questions (out of scope) - The Entra app role `admin` must be **declared on the app registration manifest** and **assigned to at least one test user** before this gate can be exercised end-to-end. That's an Entra Admin Center operation, not code. The guard's behaviour under all three branches is covered by unit tests; e2e validation waits until the role is assigned. ## Test plan - [x] `pnpm nx test portal-bff` — **214 specs pass** (was 203; +11 covering guard branches, controller projection, audit method). - [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] Audit row schema verified — `admin.access_denied` events use `outcome=denied`, store the route in `subject`, and persist `{ rolesHeld: [...] }` in the JSONB payload. - [ ] e2e — pending Entra role declaration + assignment. Will be covered by manual ops curl checks once the role exists. --------- Co-authored-by: Julien Gautier Reviewed-on: https://git.unespace.com/julien/apf_portal/pulls/127 --- .../src/admin/admin-role.guard.spec.ts | 178 ++++++++++++++++++ apps/portal-bff/src/admin/admin-role.guard.ts | 68 +++++++ .../src/admin/admin.controller.spec.ts | 51 +++++ apps/portal-bff/src/admin/admin.controller.ts | 40 ++++ apps/portal-bff/src/admin/admin.module.ts | 21 +++ .../src/admin/require-admin.decorator.ts | 20 ++ apps/portal-bff/src/app/app.module.ts | 2 + .../src/audit/audit.service.spec.ts | 29 +++ apps/portal-bff/src/audit/audit.service.ts | 21 +++ apps/portal-bff/src/audit/audit.types.ts | 18 ++ 10 files changed, 448 insertions(+) create mode 100644 apps/portal-bff/src/admin/admin-role.guard.spec.ts create mode 100644 apps/portal-bff/src/admin/admin-role.guard.ts create mode 100644 apps/portal-bff/src/admin/admin.controller.spec.ts create mode 100644 apps/portal-bff/src/admin/admin.controller.ts create mode 100644 apps/portal-bff/src/admin/admin.module.ts create mode 100644 apps/portal-bff/src/admin/require-admin.decorator.ts diff --git a/apps/portal-bff/src/admin/admin-role.guard.spec.ts b/apps/portal-bff/src/admin/admin-role.guard.spec.ts new file mode 100644 index 0000000..fd1880d --- /dev/null +++ b/apps/portal-bff/src/admin/admin-role.guard.spec.ts @@ -0,0 +1,178 @@ +import { ExecutionContext, ForbiddenException, UnauthorizedException } from '@nestjs/common'; +import type { Request } from 'express'; +import { AuditWriter } from '../audit/audit.service'; +import { ADMIN_ROLE, AdminRoleGuard } from './admin-role.guard'; + +interface SessionStub { + user?: { + oid: string; + tid: string; + username: string; + displayName: string; + amr: readonly string[]; + roles: readonly string[]; + }; +} + +function makeRequest(opts: { + session?: SessionStub; + method?: string; + originalUrl?: string; +}): Request { + return { + session: opts.session, + method: opts.method ?? 'GET', + originalUrl: opts.originalUrl ?? '/api/admin/me', + } as unknown as Request; +} + +function makeContext(req: Request): ExecutionContext { + return { + switchToHttp: () => ({ getRequest: () => req as T }), + } as unknown as ExecutionContext; +} + +function makeAuditStub(): jest.Mocked> { + return { + adminAccessDenied: jest.fn().mockResolvedValue(undefined), + }; +} + +describe('AdminRoleGuard', () => { + it('throws 401 when the request has no session at all', async () => { + const audit = makeAuditStub(); + const guard = new AdminRoleGuard(audit as unknown as AuditWriter); + const ctx = makeContext(makeRequest({ session: undefined })); + await expect(guard.canActivate(ctx)).rejects.toBeInstanceOf(UnauthorizedException); + expect(audit.adminAccessDenied).not.toHaveBeenCalled(); + }); + + it('throws 401 when the session exists but no user is bound to it', async () => { + const audit = makeAuditStub(); + const guard = new AdminRoleGuard(audit as unknown as AuditWriter); + const ctx = makeContext(makeRequest({ session: {} })); + await expect(guard.canActivate(ctx)).rejects.toBeInstanceOf(UnauthorizedException); + expect(audit.adminAccessDenied).not.toHaveBeenCalled(); + }); + + it('throws 403 + records admin.access_denied when the user has no roles at all', async () => { + const audit = makeAuditStub(); + const guard = new AdminRoleGuard(audit as unknown as AuditWriter); + const ctx = makeContext( + makeRequest({ + session: { + user: { + oid: 'user-oid', + tid: 't', + username: 'jane@example', + displayName: 'Jane', + amr: ['pwd'], + roles: [], + }, + }, + method: 'GET', + originalUrl: '/api/admin/me', + }), + ); + await expect(guard.canActivate(ctx)).rejects.toBeInstanceOf(ForbiddenException); + expect(audit.adminAccessDenied).toHaveBeenCalledWith({ + actor: { oid: 'user-oid' }, + attemptedRoute: 'GET /api/admin/me', + rolesHeld: [], + }); + }); + + it('throws 403 + records admin.access_denied when the user has unrelated roles only', async () => { + const audit = makeAuditStub(); + const guard = new AdminRoleGuard(audit as unknown as AuditWriter); + const ctx = makeContext( + makeRequest({ + session: { + user: { + oid: 'user-oid', + tid: 't', + username: 'jane@example', + displayName: 'Jane', + amr: ['pwd'], + roles: ['editor', 'auditor'], + }, + }, + method: 'POST', + originalUrl: '/api/admin/cms/pages', + }), + ); + await expect(guard.canActivate(ctx)).rejects.toBeInstanceOf(ForbiddenException); + expect(audit.adminAccessDenied).toHaveBeenCalledWith({ + actor: { oid: 'user-oid' }, + attemptedRoute: 'POST /api/admin/cms/pages', + rolesHeld: ['editor', 'auditor'], + }); + }); + + it('returns true and does not audit when the user has the admin role', async () => { + const audit = makeAuditStub(); + const guard = new AdminRoleGuard(audit as unknown as AuditWriter); + const ctx = makeContext( + makeRequest({ + session: { + user: { + oid: 'user-oid', + tid: 't', + username: 'jane@example', + displayName: 'Jane', + amr: ['pwd', 'mfa'], + roles: [ADMIN_ROLE], + }, + }, + }), + ); + await expect(guard.canActivate(ctx)).resolves.toBe(true); + expect(audit.adminAccessDenied).not.toHaveBeenCalled(); + }); + + it('returns true when admin is one of several roles', async () => { + const audit = makeAuditStub(); + const guard = new AdminRoleGuard(audit as unknown as AuditWriter); + const ctx = makeContext( + makeRequest({ + session: { + user: { + oid: 'user-oid', + tid: 't', + username: 'jane@example', + displayName: 'Jane', + amr: ['pwd', 'mfa'], + roles: ['editor', ADMIN_ROLE, 'auditor'], + }, + }, + }), + ); + await expect(guard.canActivate(ctx)).resolves.toBe(true); + }); + + it('propagates audit write failures (no audit ⇒ no action, per ADR-0013)', async () => { + const audit = { + adminAccessDenied: jest.fn().mockRejectedValue(new Error('audit_writer denied')), + }; + const guard = new AdminRoleGuard(audit as unknown as AuditWriter); + const ctx = makeContext( + makeRequest({ + session: { + user: { + oid: 'user-oid', + tid: 't', + username: 'jane@example', + displayName: 'Jane', + amr: ['pwd'], + roles: [], + }, + }, + }), + ); + // The 403 path goes through audit first; if audit throws, the + // guard surfaces the underlying error rather than the + // ForbiddenException — the caller sees a 500 and the audit + // invariant is preserved. + await expect(guard.canActivate(ctx)).rejects.toThrow('audit_writer denied'); + }); +}); diff --git a/apps/portal-bff/src/admin/admin-role.guard.ts b/apps/portal-bff/src/admin/admin-role.guard.ts new file mode 100644 index 0000000..71ac5c1 --- /dev/null +++ b/apps/portal-bff/src/admin/admin-role.guard.ts @@ -0,0 +1,68 @@ +import { + CanActivate, + ExecutionContext, + ForbiddenException, + Injectable, + UnauthorizedException, +} from '@nestjs/common'; +import type { Request } from 'express'; +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 + * registration manifest. Defined as a constant so spec assertions + * can refer to the same source of truth. + */ +export const ADMIN_ROLE = 'admin'; + +/** + * `AdminRoleGuard` — enforces ADR-0020's role-based admin gate. + * + * Contract + * -------- + * - **No session → 401.** The user is not authenticated at all; the + * SPA should kick off the login flow. We do not audit anonymous + * probes here because the route family is not a target — any + * unauthenticated 401 is normal traffic the absolute-timeout + * middleware would surface anyway. + * + * - **Session but missing `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 + * attempted route as `subject`, and the roles the user did hold + * in the payload. Per ADR-0013 §"Blocking writes": no audit ⇒ no + * action — if the audit write fails, the request fails too + * (consistent with the existing audit call sites in + * `AuthController`). + * + * - **Session with `admin` role → pass through.** Downstream + * controllers see `req.session.user` populated and can rely on + * the role check having happened. + */ +@Injectable() +export class AdminRoleGuard implements CanActivate { + constructor(private readonly audit: AuditWriter) {} + + async canActivate(context: ExecutionContext): Promise { + const req = context.switchToHttp().getRequest(); + const user = req.session?.user; + + if (!user) { + throw new UnauthorizedException(); + } + + if (!user.roles.includes(ADMIN_ROLE)) { + await this.audit.adminAccessDenied({ + actor: { oid: user.oid }, + attemptedRoute: `${req.method} ${req.originalUrl}`, + rolesHeld: user.roles, + }); + throw new ForbiddenException(); + } + + return true; + } +} diff --git a/apps/portal-bff/src/admin/admin.controller.spec.ts b/apps/portal-bff/src/admin/admin.controller.spec.ts new file mode 100644 index 0000000..6e5a282 --- /dev/null +++ b/apps/portal-bff/src/admin/admin.controller.spec.ts @@ -0,0 +1,51 @@ +import type { Request } from 'express'; +import { AdminController } from './admin.controller'; + +function makeReq(user: { + oid: string; + tid: string; + username: string; + displayName: string; + amr: readonly string[]; + roles: readonly string[]; +}): Request { + return { session: { user } } as unknown as Request; +} + +describe('AdminController', () => { + it('GET /me returns the public payload of the admin user, including roles', () => { + const controller = new AdminController(); + const res = controller.me( + makeReq({ + oid: 'admin-oid', + tid: 'tenant-1', + username: 'admin@apf.example', + displayName: 'Admin Smith', + amr: ['pwd', 'mfa'], + roles: ['admin'], + }), + ); + expect(res).toEqual({ + oid: 'admin-oid', + tid: 'tenant-1', + username: 'admin@apf.example', + displayName: 'Admin Smith', + roles: ['admin'], + }); + }); + + it('does not leak server-internal claims (`amr`) to the response', () => { + const controller = new AdminController(); + const res = controller.me( + makeReq({ + oid: 'admin-oid', + tid: 'tenant-1', + username: 'admin@apf.example', + displayName: 'Admin Smith', + amr: ['pwd', 'mfa'], + roles: ['admin'], + }), + ); + expect(res).not.toHaveProperty('amr'); + }); +}); diff --git a/apps/portal-bff/src/admin/admin.controller.ts b/apps/portal-bff/src/admin/admin.controller.ts new file mode 100644 index 0000000..14fdf8d --- /dev/null +++ b/apps/portal-bff/src/admin/admin.controller.ts @@ -0,0 +1,40 @@ +import { Controller, Get, Req } from '@nestjs/common'; +import type { Request } from 'express'; +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: … })` + * for the entry route. + * + * `GET /api/admin/me` is the self-test endpoint named in ADR-0020's + * "Confirmation" §3 ("AdminModule with AdminRoleGuard + first + * endpoint (`GET /api/admin/me` returning the session for + * self-test)"). It lets ops verify the guard is wired correctly + * (curl with no cookies → 401; curl with a non-admin session → 403 + * with an `admin.access_denied` audit row; curl with an admin + * session → 200 + the public user payload) before any real admin + * route ships. + */ +@Controller('admin') +@RequireAdmin() +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 + // 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. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- guaranteed by AdminRoleGuard + const user = req.session.user!; + return { + oid: user.oid, + tid: user.tid, + username: user.username, + displayName: user.displayName, + roles: user.roles, + }; + } +} diff --git a/apps/portal-bff/src/admin/admin.module.ts b/apps/portal-bff/src/admin/admin.module.ts new file mode 100644 index 0000000..d5c765f --- /dev/null +++ b/apps/portal-bff/src/admin/admin.module.ts @@ -0,0 +1,21 @@ +import { Module } from '@nestjs/common'; +import { AdminController } from './admin.controller'; +import { AdminRoleGuard } from './admin-role.guard'; + +/** + * `AdminModule` — root of the `/api/admin/*` surface per ADR-0020. + * + * v1 ships the self-test endpoint only (`GET /api/admin/me`). The + * functional admin modules (audit log viewer, CMS, menu management, + * user list) land as separate sub-modules consumed from here once + * the distinct admin session + the audit query endpoint are in + * place — see the chantier sequence in `notes/handoff.md`. + * + * `AuditWriter` (required by `AdminRoleGuard`) is provided globally + * by `AuditModule`, so no extra import is needed here. + */ +@Module({ + controllers: [AdminController], + providers: [AdminRoleGuard], +}) +export class AdminModule {} diff --git a/apps/portal-bff/src/admin/require-admin.decorator.ts b/apps/portal-bff/src/admin/require-admin.decorator.ts new file mode 100644 index 0000000..2f31ddb --- /dev/null +++ b/apps/portal-bff/src/admin/require-admin.decorator.ts @@ -0,0 +1,20 @@ +import { UseGuards, applyDecorators } from '@nestjs/common'; +import { AdminRoleGuard } from './admin-role.guard'; + +/** + * `@RequireAdmin()` — semantic sugar for `@UseGuards(AdminRoleGuard)`. + * + * Applied at the controller (class) level so every route in an admin + * controller is gated uniformly, or at the method level on the rare + * route inside a mixed controller that needs admin. ADR-0020 §"Auth" + * names this decorator alongside `@RequireMfa()`; we expose the + * decorator API so both gates compose the same way in upcoming PRs. + * + * Using `applyDecorators` (instead of a one-line factory function) + * leaves room for additional decorators in the same wrapper later — + * e.g. an `@SetMetadata('rbac:requires', ['admin'])` for OpenAPI / + * route-introspection tooling — without changing call sites. + */ +export function RequireAdmin(): ClassDecorator & MethodDecorator { + return applyDecorators(UseGuards(AdminRoleGuard)); +} diff --git a/apps/portal-bff/src/app/app.module.ts b/apps/portal-bff/src/app/app.module.ts index 626a5a3..30d3233 100644 --- a/apps/portal-bff/src/app/app.module.ts +++ b/apps/portal-bff/src/app/app.module.ts @@ -4,6 +4,7 @@ import { AppController } from './app.controller'; import { AppService } from './app.service'; import { ObservabilityModule } from '../observability/observability.module'; import { HealthModule } from '../health/health.module'; +import { AdminModule } from '../admin/admin.module'; import { AuditModule } from '../audit/audit.module'; import { AuthModule } from '../auth/auth.module'; import { RedisModule } from '../redis/redis.module'; @@ -20,6 +21,7 @@ import { SessionModule } from '../session/session.module'; SessionModule, SecurityModule, HealthModule, + AdminModule, ], controllers: [AppController], providers: [AppService], diff --git a/apps/portal-bff/src/audit/audit.service.spec.ts b/apps/portal-bff/src/audit/audit.service.spec.ts index 82cd86a..9082ced 100644 --- a/apps/portal-bff/src/audit/audit.service.spec.ts +++ b/apps/portal-bff/src/audit/audit.service.spec.ts @@ -315,4 +315,33 @@ describe('AuditWriter — typed event methods', () => { ); }); }); + + describe('adminAccessDenied()', () => { + it('records admin.access_denied with outcome=denied, attempted route as subject, and rolesHeld payload', async () => { + const { writer, prisma, hashUserId } = await createSubject(); + await writer.adminAccessDenied({ + actor: { oid: 'user-oid' }, + attemptedRoute: 'GET /api/admin/audit', + rolesHeld: ['editor'], + }); + expect(hashUserId.hash).toHaveBeenCalledWith('user-oid'); + const row = extractInsertedRow(prisma); + expect(row.eventType).toBe('admin.access_denied'); + expect(row.audience).toBe('workforce'); + expect(row.outcome).toBe('denied'); + expect(row.actorIdHash).toBe('hash(user-oid)'); + expect(row.subject).toBe('GET /api/admin/audit'); + expect(row.payloadJson).toBe(JSON.stringify({ rolesHeld: ['editor'] })); + }); + + it('persists an empty rolesHeld array verbatim (distinguishable from missing payload)', async () => { + const { writer, prisma } = await createSubject(); + await writer.adminAccessDenied({ + actor: { oid: 'user-oid' }, + attemptedRoute: 'GET /api/admin/me', + rolesHeld: [], + }); + expect(extractInsertedRow(prisma).payloadJson).toBe(JSON.stringify({ rolesHeld: [] })); + }); + }); }); diff --git a/apps/portal-bff/src/audit/audit.service.ts b/apps/portal-bff/src/audit/audit.service.ts index 288b664..6ed8046 100644 --- a/apps/portal-bff/src/audit/audit.service.ts +++ b/apps/portal-bff/src/audit/audit.service.ts @@ -3,6 +3,7 @@ import { trace } from '@opentelemetry/api'; import { ClsService } from 'nestjs-cls'; import { PrismaService } from 'nestjs-prisma'; import type { + AdminAccessDeniedInput, AuditEventInput, SignInActor, SignInFailedInput, @@ -113,6 +114,26 @@ export class AuditWriter { }); } + /** + * Typed event: `/api/admin/*` request rejected by `AdminRoleGuard` + * because the session's `roles` claim does not include `admin`. + * Per ADR-0020 §"Auth — same Entra ID … `admin` role claim", every + * 403 from the admin surface is captured here with the attempted + * route and the roles the user actually held — auditors looking + * for privilege-escalation attempts pivot on `subject` (the route) + * and `outcome=denied`. + */ + async adminAccessDenied(input: AdminAccessDeniedInput): Promise { + await this.recordEvent({ + eventType: 'admin.access_denied', + audience: 'workforce', + outcome: 'denied', + actorIdHash: this.hashUserId.hash(input.actor.oid), + subject: input.attemptedRoute, + payload: { rolesHeld: input.rolesHeld }, + }); + } + async recordEvent(input: AuditEventInput): Promise { const traceId = trace.getActiveSpan()?.spanContext().traceId ?? null; const actorIdHash = diff --git a/apps/portal-bff/src/audit/audit.types.ts b/apps/portal-bff/src/audit/audit.types.ts index 78bab7f..3f9fefa 100644 --- a/apps/portal-bff/src/audit/audit.types.ts +++ b/apps/portal-bff/src/audit/audit.types.ts @@ -82,3 +82,21 @@ export interface SessionExpiredInput { /** Age of the session at the moment of expiry, in milliseconds. */ ageMs: number; } + +export interface AdminAccessDeniedInput { + actor: { oid: string }; + /** + * Canonical "{METHOD} {originalUrl}" of the rejected request, e.g. + * `GET /api/admin/audit`. Persisted into the audit row's `subject` + * column so auditors can pivot on it without joining anything. + */ + attemptedRoute: string; + /** + * The roles the user *did* hold at the moment of denial — empty + * array when no app role was assigned. Stored in the payload so + * an auditor can tell apart "tried admin while logged in as a + * regular user" from "tried admin with an unrelated role like + * `auditor` (future)" without parsing the deny pattern. + */ + rolesHeld: readonly string[]; +}