feat(portal-bff): extract Entra roles claim onto AuthenticatedUser (#126)
## 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 <julien.gautier@apf.asso.fr>
Reviewed-on: #126
This commit was merged in pull request #126.
This commit is contained in:
@@ -30,6 +30,7 @@ const USER: AuthenticatedUser = {
|
||||
username: 'jane.doe@apf.example',
|
||||
displayName: 'Jane Doe',
|
||||
amr: ['pwd', 'mfa'],
|
||||
roles: [],
|
||||
};
|
||||
|
||||
function makeResStub() {
|
||||
|
||||
@@ -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<string, unknown> = {
|
||||
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 });
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -68,6 +68,7 @@ const USER = {
|
||||
username: 'jane@apf.example',
|
||||
displayName: 'Jane Doe',
|
||||
amr: ['pwd', 'mfa'],
|
||||
roles: [],
|
||||
};
|
||||
|
||||
describe('absoluteTimeout middleware', () => {
|
||||
|
||||
Reference in New Issue
Block a user