2480d0dd6d
## 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
266 lines
10 KiB
TypeScript
266 lines
10 KiB
TypeScript
import { Injectable } from '@nestjs/common';
|
|
import { trace } from '@opentelemetry/api';
|
|
import { ClsService } from 'nestjs-cls';
|
|
import { PrismaService } from 'nestjs-prisma';
|
|
import type {
|
|
AdminAccessDeniedInput,
|
|
AdminAuditQueryInput,
|
|
AdminUsersQueryInput,
|
|
AuditEventInput,
|
|
MfaRequiredInput,
|
|
SignInActor,
|
|
SignInFailedInput,
|
|
SignOutInput,
|
|
SessionExpiredInput,
|
|
} from './audit.types';
|
|
import { HashUserIdService } from './hash-user-id.service';
|
|
|
|
/**
|
|
* AuditWriter — single entry point for ADR-0013 audit-log writes.
|
|
*
|
|
* Contract
|
|
* --------
|
|
* - **Append-only at the database level.** Every write runs inside a
|
|
* transaction whose first statement is `SET LOCAL ROLE
|
|
* audit_writer`. That role only has `INSERT` on `audit.events`
|
|
* (per the migration that created the table); `UPDATE`, `DELETE`,
|
|
* `TRUNCATE` all fail at the Postgres level even if the BFF
|
|
* connection is otherwise privileged. The role is reset
|
|
* automatically at transaction end.
|
|
*
|
|
* - **Fail loud, never swallow.** Per ADR-0013 §"Blocking writes":
|
|
* no audit ⇒ no action. Callers must propagate the rejection up
|
|
* so the requested action does not proceed when its audit trail
|
|
* cannot be written. The service throws the underlying Prisma
|
|
* error unchanged; do not wrap it in a catch-and-log block.
|
|
*
|
|
* - **trace_id and actor_id_hash are auto-resolved.** trace_id is
|
|
* read from the active OTel span context (so the audit row joins
|
|
* with the BFF request span and the Pino log lines on the same
|
|
* request). actor_id_hash is read from the CLS context populated
|
|
* by future auth guards (ADR-0009 / ADR-0010); v1 stores `null`
|
|
* when no actor is established. Callers can override either by
|
|
* passing them on `AuditEventInput`.
|
|
*/
|
|
@Injectable()
|
|
export class AuditWriter {
|
|
constructor(
|
|
private readonly prisma: PrismaService,
|
|
private readonly cls: ClsService,
|
|
private readonly hashUserId: HashUserIdService,
|
|
) {}
|
|
|
|
/**
|
|
* Typed event: successful sign-in via the OIDC callback. Per
|
|
* ADR-0013's v1 catalogue (`auth.sign_in`).
|
|
*
|
|
* Hashes the user id internally — callers pass the raw Entra
|
|
* `oid`, never the hash, so the salt stays inside the audit
|
|
* module.
|
|
*/
|
|
async signIn(input: { actor: SignInActor; sessionId: string }): Promise<void> {
|
|
await this.recordEvent({
|
|
eventType: 'auth.sign_in',
|
|
audience: 'workforce',
|
|
outcome: 'success',
|
|
actorIdHash: this.hashUserId.hash(input.actor.oid),
|
|
subject: `session:${input.sessionId}`,
|
|
payload: { amr: input.actor.amr },
|
|
});
|
|
}
|
|
|
|
/**
|
|
* Typed event: failed sign-in at the callback. The `failureKind`
|
|
* mirrors the discriminator on `AuthCodeFlowError` so the audit
|
|
* row is self-describing without joining anything.
|
|
*
|
|
* `actorIdHash` is left null on purpose: at the moment of
|
|
* failure we may not have resolved an identity yet (state
|
|
* mismatch, expired flow, token-exchange error before any user
|
|
* claim was parsed). Callers can pass an explicit hash when the
|
|
* identity *was* resolved before rejection.
|
|
*/
|
|
async signInFailed(input: SignInFailedInput): Promise<void> {
|
|
await this.recordEvent({
|
|
eventType: 'auth.sign_in.failed',
|
|
audience: 'workforce',
|
|
outcome: 'failure',
|
|
...(input.actor !== undefined ? { actorIdHash: this.hashUserId.hash(input.actor.oid) } : {}),
|
|
payload: { failureKind: input.failureKind, ...(input.payload ?? {}) },
|
|
});
|
|
}
|
|
|
|
async signOut(input: SignOutInput): Promise<void> {
|
|
await this.recordEvent({
|
|
eventType: 'auth.sign_out',
|
|
audience: 'workforce',
|
|
outcome: 'success',
|
|
actorIdHash: this.hashUserId.hash(input.actor.oid),
|
|
subject: `session:${input.sessionId}`,
|
|
});
|
|
}
|
|
|
|
/**
|
|
* Typed event: session destroyed by the absolute-timeout
|
|
* middleware (12 h hard ceiling, ADR-0010 §"TTL policy"). The
|
|
* idle-TTL expiry is *not* surfaced through this method — it
|
|
* happens silently inside Redis with no BFF observation point.
|
|
*/
|
|
async sessionExpired(input: SessionExpiredInput): Promise<void> {
|
|
await this.recordEvent({
|
|
eventType: 'auth.session.expired',
|
|
audience: 'workforce',
|
|
outcome: 'success',
|
|
actorIdHash: this.hashUserId.hash(input.actor.oid),
|
|
subject: `session:${input.sessionId}`,
|
|
payload: { reason: input.reason, ageMs: input.ageMs },
|
|
});
|
|
}
|
|
|
|
/**
|
|
* Typed event: a request was rejected by `RequireMfaGuard` because
|
|
* the session did not satisfy the route's MFA freshness gate.
|
|
* Per ADR-0011 §"Step-up MFA — designed-in, dormant", every
|
|
* step-up challenge emitted by the BFF lands here so auditors can
|
|
* track the distribution of step-up prompts (and the rate of stale
|
|
* `mfaVerifiedAt` rejections that would suggest the default
|
|
* freshness is too tight). `outcome=denied` matches the
|
|
* `AdminRoleGuard` posture: the user *is* authenticated, the
|
|
* action is just refused.
|
|
*/
|
|
async mfaRequired(input: MfaRequiredInput): Promise<void> {
|
|
await this.recordEvent({
|
|
eventType: 'auth.mfa_required',
|
|
audience: 'workforce',
|
|
outcome: 'denied',
|
|
actorIdHash: this.hashUserId.hash(input.actor.oid),
|
|
subject: input.attemptedRoute,
|
|
payload: {
|
|
reason: input.reason,
|
|
freshnessSeconds: input.freshnessSeconds,
|
|
...(input.mfaAgeMs !== undefined ? { mfaAgeMs: input.mfaAgeMs } : {}),
|
|
},
|
|
});
|
|
}
|
|
|
|
/**
|
|
* Typed event: an admin queried the audit-log viewer. Recorded at
|
|
* **coarser granularity** per ADR-0020 §"Audit log captures every
|
|
* admin write action … Read actions (audit log viewing, user list
|
|
* browsing) are also captured … to deter fishing expeditions".
|
|
*
|
|
* The filter object the admin used is serialised into the payload
|
|
* so a reviewer can see what was searched (event type, time range,
|
|
* actor hash, etc.). `resultCount` is the number of rows the SPA
|
|
* received in this trip — paged calls each emit one event; an
|
|
* auditor spotting a sequence of `admin.audit.query` with growing
|
|
* offsets is the v1 detection signal for a sweep.
|
|
*
|
|
* `outcome=success` regardless of whether the query returned any
|
|
* rows: the event captures "the admin ran a query", not "the
|
|
* query matched something". An `outcome=failure` audit row from
|
|
* this surface is reserved for the day we add per-row authZ.
|
|
*/
|
|
async adminAuditQuery(input: AdminAuditQueryInput): Promise<void> {
|
|
await this.recordEvent({
|
|
eventType: 'admin.audit.query',
|
|
audience: 'workforce',
|
|
outcome: 'success',
|
|
actorIdHash: this.hashUserId.hash(input.actor.oid),
|
|
payload: {
|
|
filters: input.filters,
|
|
resultCount: input.resultCount,
|
|
},
|
|
});
|
|
}
|
|
|
|
/**
|
|
* Typed event: an admin queried the user-directory viewer. Same
|
|
* deterrent posture as {@link adminAuditQuery} per ADR-0020
|
|
* §"Read actions ... to deter fishing expeditions" — captures the
|
|
* filter and the result count so a reviewer can spot a sweep
|
|
* (admin paging through everyone) without joining anything.
|
|
*/
|
|
async adminUsersQuery(input: AdminUsersQueryInput): Promise<void> {
|
|
await this.recordEvent({
|
|
eventType: 'admin.users.query',
|
|
audience: 'workforce',
|
|
outcome: 'success',
|
|
actorIdHash: this.hashUserId.hash(input.actor.oid),
|
|
payload: {
|
|
filters: input.filters,
|
|
resultCount: input.resultCount,
|
|
},
|
|
});
|
|
}
|
|
|
|
/**
|
|
* Typed event: `/api/admin/*` request rejected by `AdminRoleGuard`
|
|
* because the session's `roles` claim does not include `Portal.Admin`.
|
|
* Per ADR-0020 §"Auth — same Entra ID … `Portal.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<void> {
|
|
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<void> {
|
|
const traceId = trace.getActiveSpan()?.spanContext().traceId ?? null;
|
|
const actorIdHash =
|
|
input.actorIdHash ?? this.cls.get<string | undefined>('actorIdHash') ?? null;
|
|
const payloadJson = input.payload === undefined ? null : JSON.stringify(input.payload);
|
|
|
|
await this.prisma.$transaction(async (tx) => {
|
|
// Lock the connection to audit_writer for the duration of this
|
|
// transaction. SET LOCAL is reset at COMMIT/ROLLBACK so the
|
|
// pool's next consumer sees the original role.
|
|
await tx.$executeRawUnsafe(`SET LOCAL ROLE audit_writer`);
|
|
|
|
// Deliberately NOT `tx.auditEvent.create(...)`. The Prisma ORM
|
|
// create() emits `INSERT … RETURNING *` to hydrate the entity
|
|
// it returns, and Postgres requires SELECT on every column
|
|
// listed in RETURNING. `audit_writer` is granted INSERT only
|
|
// (ADR-0013 §"Append-only by role grants"); RETURNING fails
|
|
// with the deeply misleading "permission denied for table
|
|
// events" error code 42501. Raw parameterised INSERT keeps
|
|
// the role contract strict — audit_writer never needs SELECT.
|
|
//
|
|
// `gen_random_uuid()` is built into Postgres 13+ (the dev /
|
|
// prod target is 17). The enum + jsonb casts are needed
|
|
// because the parameter values are sent as TEXT over the
|
|
// wire.
|
|
await tx.$executeRawUnsafe(
|
|
`INSERT INTO "audit"."events"
|
|
(id, event_type, audience, outcome, subject, actor_id_hash, trace_id, payload)
|
|
VALUES (
|
|
gen_random_uuid(),
|
|
$1,
|
|
$2::"audit"."AuditAudience",
|
|
$3::"audit"."AuditOutcome",
|
|
$4,
|
|
$5,
|
|
$6,
|
|
$7::jsonb
|
|
)`,
|
|
input.eventType,
|
|
input.audience,
|
|
input.outcome,
|
|
input.subject ?? null,
|
|
actorIdHash,
|
|
traceId,
|
|
payloadJson,
|
|
);
|
|
});
|
|
}
|
|
}
|