feat(portal-bff): close the auth loop — callback persists session, /me, RP-initiated /logout (#112)
## Summary
Closes the OIDC loop end-to-end on the BFF side:
- `/auth/callback` now writes the resolved `AuthenticatedUser` into `req.session.user` and waits for `req.session.save()` before redirecting, so the SPA reaches the landing page with a populated session.
- `GET /auth/me` returns the curated public view of the session user (`oid`, `tid`, `username`, `displayName`) or `401 {"error": "unauthenticated"}`. `amr` and other internal claims stay server-side.
- `GET /auth/logout` destroys the BFF session (Redis `DEL`), clears the session cookie, and 302s to Entra's `/oauth2/v2.0/logout` so the IdP-side session is killed too — RP-initiated logout per ADR-0009.
Scope intentionally stops here: the absolute-timeout interceptor (12 h hard ceiling) and the `user_sessions:{userId}` secondary index land in dedicated follow-ups.
## Notable choices
**`req.session.save()` is awaited before the redirect.** Express-session writes to its store on response end; emitting the 302 closes the response before `connect-redis` finishes the write, so without an explicit await the browser can race the SPA into requesting `/me` against a missing key. Awaiting `save()` is the documented fix.
**Logout via `GET`.** Matches `/login` (also `GET`) and keeps the UX a plain anchor / top-level navigation. The CSRF surface is mitigated by `SameSite=Lax` on the session cookie — cross-site subresource requests (`<img src>`, `fetch`) don't carry it. A dedicated CSRF middleware lands with phase-2 security; if we want POST-only logout earlier, easy follow-up.
**`/me` strips `amr`.** The session payload mirrors `AuthenticatedUser` (used internally by the future `@RequireMfa()` guard, ADR-0011), but the SPA only ever needs the curated subset. Mapping happens in the controller — no leak by default.
**Logout URL skips `id_token_hint`.** ADR-0009 mentions it for single-account logout UX, but v1 doesn't persist the `id_token` in the session yet (the encrypted `tokens` blob lands with downstream API support per ADR-0014). Without `id_token_hint`, Entra shows an account picker — the conservative default until token persistence ships.
**Cookie name in logout.** Uses `sessionCookieName()` from `session/session-cookie.ts` so logout clears the same cookie the middleware sets — `__Host-portal_session` in prod, `portal_session` in dev.
## Out of scope (next PRs)
- Absolute-timeout interceptor (12 h hard ceiling, ADR-0010).
- `user_sessions:{userId}` secondary index for admin "logout everywhere".
- Persisting the `id_token` / `access_token` / `refresh_token` blob in the encrypted session (ADR-0014 dependency).
- CSRF middleware (phase-2 security).
- Renaming `ENTRA_POST_LOGOUT_REDIRECT_URI` if we want a distinct post-login redirect target — for now both flows land on the same SPA URL.
## Test plan
- [x] `pnpm nx test portal-bff` → **110/110 pass** (was 99 before this PR; +11 specs across `auth.controller.spec.ts` and `auth.service.spec.ts`).
- [x] `pnpm nx lint portal-bff` → clean.
- [x] `pnpm nx build portal-bff` → webpack compiled successfully.
- [x] Prettier-clean on all touched files.
- [ ] Manual end-to-end smoke test:
- [ ] `/api/auth/login` → Entra → back at `/api/auth/callback` → session cookie set, redirect to SPA.
- [ ] `/api/auth/me` → 200 JSON when authenticated, 401 when anonymous.
- [ ] `/api/auth/logout` → Redis key gone, cookie cleared, lands at SPA via Entra logout.
---------
Co-authored-by: Julien Gautier <julien.gautier@apf.asso.fr>
Reviewed-on: #112
This commit was merged in pull request #112.
This commit is contained in:
@@ -31,19 +31,50 @@ const USER: AuthenticatedUser = {
|
||||
};
|
||||
|
||||
function makeResStub() {
|
||||
const res = {
|
||||
cookie: jest.fn(),
|
||||
clearCookie: jest.fn(),
|
||||
redirect: jest.fn(),
|
||||
status: jest.fn(),
|
||||
json: jest.fn(),
|
||||
};
|
||||
// status/json/cookie/clearCookie/redirect all return the response
|
||||
// in Express for chaining. Wire `mockReturnThis` after the object
|
||||
// exists so `this` resolves to the same stub.
|
||||
res.cookie.mockReturnValue(res);
|
||||
res.clearCookie.mockReturnValue(res);
|
||||
res.redirect.mockReturnValue(res);
|
||||
res.status.mockReturnValue(res);
|
||||
res.json.mockReturnValue(res);
|
||||
return res as unknown as Response & typeof res;
|
||||
}
|
||||
|
||||
interface SessionStub {
|
||||
user?: AuthenticatedUser;
|
||||
save: jest.Mock;
|
||||
destroy: jest.Mock;
|
||||
}
|
||||
|
||||
function makeSessionStub(opts?: {
|
||||
user?: AuthenticatedUser;
|
||||
saveError?: Error;
|
||||
destroyError?: Error;
|
||||
}): SessionStub {
|
||||
return {
|
||||
cookie: jest.fn().mockReturnThis(),
|
||||
clearCookie: jest.fn().mockReturnThis(),
|
||||
redirect: jest.fn().mockReturnThis(),
|
||||
} as unknown as Response & {
|
||||
cookie: jest.Mock;
|
||||
clearCookie: jest.Mock;
|
||||
redirect: jest.Mock;
|
||||
...(opts?.user !== undefined ? { user: opts.user } : {}),
|
||||
save: jest.fn((cb: (err?: Error) => void) => cb(opts?.saveError)),
|
||||
destroy: jest.fn((cb: (err?: Error) => void) => cb(opts?.destroyError)),
|
||||
};
|
||||
}
|
||||
|
||||
function makeReqStub(signedCookies: Record<string, unknown> = {}): Request {
|
||||
return { signedCookies } as unknown as Request;
|
||||
function makeReqStub(opts?: {
|
||||
signedCookies?: Record<string, unknown>;
|
||||
session?: SessionStub;
|
||||
}): Request & { session: SessionStub } {
|
||||
return {
|
||||
signedCookies: opts?.signedCookies ?? {},
|
||||
session: opts?.session ?? makeSessionStub(),
|
||||
} as unknown as Request & { session: SessionStub };
|
||||
}
|
||||
|
||||
function makeLoggerStub() {
|
||||
@@ -52,7 +83,12 @@ function makeLoggerStub() {
|
||||
warn: jest.fn(),
|
||||
error: jest.fn(),
|
||||
debug: jest.fn(),
|
||||
} as unknown as Logger & { log: jest.Mock; warn: jest.Mock };
|
||||
} as unknown as Logger & {
|
||||
log: jest.Mock;
|
||||
warn: jest.Mock;
|
||||
error: jest.Mock;
|
||||
debug: jest.Mock;
|
||||
};
|
||||
}
|
||||
|
||||
interface ControllerFixture {
|
||||
@@ -62,13 +98,20 @@ interface ControllerFixture {
|
||||
logger: ReturnType<typeof makeLoggerStub>;
|
||||
}
|
||||
|
||||
const LOGOUT_URL = `${ENTRA.authority}/oauth2/v2.0/logout?post_logout_redirect_uri=${encodeURIComponent(ENTRA.postLogoutRedirectUri)}`;
|
||||
|
||||
function makeController(opts?: { completeAuthCodeFlow?: jest.Mock }): ControllerFixture {
|
||||
const beginAuthCodeFlow = jest.fn().mockResolvedValue({
|
||||
authUrl: 'https://entra.example/authorize?state=state-nonce',
|
||||
preAuthPayload: PRE_AUTH,
|
||||
});
|
||||
const completeAuthCodeFlow = opts?.completeAuthCodeFlow ?? jest.fn().mockResolvedValue(USER);
|
||||
const service = { beginAuthCodeFlow, completeAuthCodeFlow } as unknown as AuthService;
|
||||
const buildLogoutUrl = jest.fn().mockReturnValue(LOGOUT_URL);
|
||||
const service = {
|
||||
beginAuthCodeFlow,
|
||||
completeAuthCodeFlow,
|
||||
buildLogoutUrl,
|
||||
} as unknown as AuthService;
|
||||
const logger = makeLoggerStub();
|
||||
return {
|
||||
controller: new AuthController(service, logger as unknown as Logger, ENTRA),
|
||||
@@ -124,15 +167,23 @@ describe('AuthController.login', () => {
|
||||
});
|
||||
|
||||
describe('AuthController.callback', () => {
|
||||
it('clears the pre-auth cookie, logs success, redirects to the SPA on the happy path', async () => {
|
||||
it('clears the pre-auth cookie, persists the user in the session, logs success, redirects to the SPA on the happy path', async () => {
|
||||
const { controller, completeAuthCodeFlow, logger } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) });
|
||||
const session = makeSessionStub();
|
||||
const req = makeReqStub({
|
||||
signedCookies: { [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) },
|
||||
session,
|
||||
});
|
||||
|
||||
await controller.callback(req, res, 'auth-code', PRE_AUTH.state);
|
||||
|
||||
expect(res.clearCookie).toHaveBeenCalledWith(PRE_AUTH_COOKIE_NAME, expect.any(Object));
|
||||
expect(completeAuthCodeFlow).toHaveBeenCalledWith('auth-code', PRE_AUTH.state, PRE_AUTH);
|
||||
// The resolved user must be on the session before the redirect
|
||||
// so the subsequent /me call sees a populated payload.
|
||||
expect(session.user).toEqual(USER);
|
||||
expect(session.save).toHaveBeenCalledTimes(1);
|
||||
expect(logger.log).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
event: 'auth.signed_in',
|
||||
@@ -145,10 +196,27 @@ describe('AuthController.callback', () => {
|
||||
expect(res.redirect).toHaveBeenCalledWith(302, ENTRA.postLogoutRedirectUri);
|
||||
});
|
||||
|
||||
it('propagates a session.save() error as an exception (so the SPA never sees a successful redirect with no session)', async () => {
|
||||
const { controller } = makeController();
|
||||
const res = makeResStub();
|
||||
const session = makeSessionStub({ saveError: new Error('redis down') });
|
||||
const req = makeReqStub({
|
||||
signedCookies: { [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) },
|
||||
session,
|
||||
});
|
||||
|
||||
await expect(controller.callback(req, res, 'auth-code', PRE_AUTH.state)).rejects.toThrow(
|
||||
/redis down/,
|
||||
);
|
||||
expect(res.redirect).not.toHaveBeenCalledWith(302, ENTRA.postLogoutRedirectUri);
|
||||
});
|
||||
|
||||
it('redirects with ?auth_error=token-exchange-failed when Entra returns error', async () => {
|
||||
const { controller, completeAuthCodeFlow } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) });
|
||||
const req = makeReqStub({
|
||||
signedCookies: { [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) },
|
||||
});
|
||||
|
||||
await controller.callback(req, res, undefined, undefined, 'access_denied', 'user cancelled');
|
||||
|
||||
@@ -163,7 +231,7 @@ describe('AuthController.callback', () => {
|
||||
it('redirects with ?auth_error=flow-expired when the pre-auth cookie is missing', async () => {
|
||||
const { controller, completeAuthCodeFlow, logger } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({});
|
||||
const req = makeReqStub();
|
||||
|
||||
await controller.callback(req, res, 'code', 'state');
|
||||
|
||||
@@ -184,7 +252,9 @@ describe('AuthController.callback', () => {
|
||||
.mockRejectedValue(new AuthCodeFlowException({ kind: 'state-mismatch' }));
|
||||
const { controller, logger } = makeController({ completeAuthCodeFlow });
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) });
|
||||
const req = makeReqStub({
|
||||
signedCookies: { [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) },
|
||||
});
|
||||
|
||||
await controller.callback(req, res, 'code', 'state');
|
||||
|
||||
@@ -204,7 +274,9 @@ describe('AuthController.callback', () => {
|
||||
it('redirects with ?auth_error=token-exchange-failed when query is missing code/state', async () => {
|
||||
const { controller } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) });
|
||||
const req = makeReqStub({
|
||||
signedCookies: { [PRE_AUTH_COOKIE_NAME]: JSON.stringify(PRE_AUTH) },
|
||||
});
|
||||
|
||||
await controller.callback(req, res); // no code, no state
|
||||
|
||||
@@ -217,7 +289,7 @@ describe('AuthController.callback', () => {
|
||||
it('rejects a malformed pre-auth cookie as flow-expired', async () => {
|
||||
const { controller, completeAuthCodeFlow } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({ [PRE_AUTH_COOKIE_NAME]: 'not-json' });
|
||||
const req = makeReqStub({ signedCookies: { [PRE_AUTH_COOKIE_NAME]: 'not-json' } });
|
||||
|
||||
await controller.callback(req, res, 'code', 'state');
|
||||
|
||||
@@ -228,3 +300,127 @@ describe('AuthController.callback', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('AuthController.me', () => {
|
||||
it('returns the curated public user when the session is populated', () => {
|
||||
const { controller } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({ session: makeSessionStub({ user: USER }) });
|
||||
|
||||
controller.me(req, res);
|
||||
|
||||
expect(res.status).not.toHaveBeenCalledWith(401);
|
||||
expect(res.json).toHaveBeenCalledWith({
|
||||
oid: USER.oid,
|
||||
tid: USER.tid,
|
||||
username: USER.username,
|
||||
displayName: USER.displayName,
|
||||
});
|
||||
});
|
||||
|
||||
it('strips amr (internal claim) from the response payload', () => {
|
||||
const { controller } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({ session: makeSessionStub({ user: USER }) });
|
||||
|
||||
controller.me(req, res);
|
||||
|
||||
const payload = res.json.mock.calls[0]?.[0] as Record<string, unknown>;
|
||||
expect(payload).not.toHaveProperty('amr');
|
||||
});
|
||||
|
||||
it('returns 401 when no user is on the session', () => {
|
||||
const { controller } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub();
|
||||
|
||||
controller.me(req, res);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(401);
|
||||
expect(res.json).toHaveBeenCalledWith({ error: 'unauthenticated' });
|
||||
});
|
||||
});
|
||||
|
||||
describe('AuthController.logout', () => {
|
||||
it('destroys the session, clears the session cookie, logs success, redirects to Entra logout', async () => {
|
||||
const { controller, logger } = makeController();
|
||||
const res = makeResStub();
|
||||
const session = makeSessionStub({ user: USER });
|
||||
const req = makeReqStub({ session });
|
||||
|
||||
await controller.logout(req, res);
|
||||
|
||||
expect(session.destroy).toHaveBeenCalledTimes(1);
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('portal_session', { path: '/' });
|
||||
expect(logger.log).toHaveBeenCalledWith(
|
||||
{ event: 'auth.signed_out', wasAuthenticated: true },
|
||||
'AuthLogout',
|
||||
);
|
||||
// Authority + /oauth2/v2.0/logout?post_logout_redirect_uri=…
|
||||
expect(res.redirect).toHaveBeenCalledWith(
|
||||
302,
|
||||
expect.stringContaining(`${ENTRA.authority}/oauth2/v2.0/logout`),
|
||||
);
|
||||
expect(res.redirect).toHaveBeenCalledWith(
|
||||
302,
|
||||
expect.stringContaining(
|
||||
`post_logout_redirect_uri=${encodeURIComponent(ENTRA.postLogoutRedirectUri)}`,
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
it('uses the __Host- prefixed cookie name in production', async () => {
|
||||
const originalNodeEnv = process.env['NODE_ENV'];
|
||||
try {
|
||||
process.env['NODE_ENV'] = 'production';
|
||||
const { controller } = makeController();
|
||||
const res = makeResStub();
|
||||
const req = makeReqStub({ session: makeSessionStub({ user: USER }) });
|
||||
|
||||
await controller.logout(req, res);
|
||||
|
||||
expect(res.clearCookie).toHaveBeenCalledWith('__Host-portal_session', { path: '/' });
|
||||
} finally {
|
||||
if (originalNodeEnv === undefined) {
|
||||
delete process.env['NODE_ENV'];
|
||||
} else {
|
||||
process.env['NODE_ENV'] = originalNodeEnv;
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
it('still clears the cookie and redirects when the user was already anonymous', async () => {
|
||||
const { controller, logger } = makeController();
|
||||
const res = makeResStub();
|
||||
const session = makeSessionStub();
|
||||
const req = makeReqStub({ session });
|
||||
|
||||
await controller.logout(req, res);
|
||||
|
||||
expect(session.destroy).toHaveBeenCalledTimes(1);
|
||||
expect(logger.log).toHaveBeenCalledWith(
|
||||
{ event: 'auth.signed_out', wasAuthenticated: false },
|
||||
'AuthLogout',
|
||||
);
|
||||
expect(res.redirect).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('logs but does not throw when session.destroy fails — still clears cookie and redirects', async () => {
|
||||
const { controller, logger } = makeController();
|
||||
const res = makeResStub();
|
||||
const session = makeSessionStub({ user: USER, destroyError: new Error('redis down') });
|
||||
const req = makeReqStub({ session });
|
||||
|
||||
await controller.logout(req, res);
|
||||
|
||||
expect(logger.error).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
event: 'session.destroy_failed',
|
||||
message: 'redis down',
|
||||
}),
|
||||
'AuthLogout',
|
||||
);
|
||||
expect(res.clearCookie).toHaveBeenCalled();
|
||||
expect(res.redirect).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user