EDFI-2772 Generalize OIDC logout

[analiliagutierrez]

Summary

Generalizes Admin App logout to the standard OpenID Connect RP-Initiated Logout flow for any configured OIDC provider, replacing the Keycloak-specific implementation. Logout selects the provider the user logged in with, builds the end-session URL from the provider's discovered end_session_endpoint (with id_token_hint), and degrades to a local-only logout for providers that don't expose one. Keycloak keeps working with no regression.

• Commit 1 stops treating logout as "a Keycloak thing": it retains each provider's discovered client (with its real end_session_endpoint), exposes getEndSessionUrl, captures the id_token at login and registers providers in onModuleInit.
• Commit 2 is the behavioral change: it records oidcId/idToken on the session at login and rewrites logout to build the end-session URL from discovery (full RP-Initiated Logout), with a local-only fallback for providers without an end_session_endpoint (Google).
• Commit 3 is isolated backward-compat: sessions created before the upgrade have no oidcId, so on a single-provider deployment logout falls back to the sole registered provider.

If you want to test these changes locally, you can follow this internal guide to switch providers in a dev environment:
Switching OIDC Providers on a Local Instance

Ticket

EDFI-2772 — Generalize logout to support any OIDC provider

Type of Change

• New feature

What Changed

• Track the login provider on the session. After login the session stores oidcId and idToken (used as id_token_hint at logout).
• Build the logout URL from discovery. getEndSessionUrl uses the discovered end_session_endpoint plus id_token_hint, post_logout_redirect_uri, and client_id. Removed constructKeycloakLogoutUrl and the SAMPLE_OIDC_CONFIG-based selection.
• Local-only fallback. When a provider exposes no end_session_endpoint (e.g. Google), logout destroys the local session and redirects to /unauthenticated?msg=… instead of a broken IdP redirect.
• Provider registration moved to onModuleInit (see Architectural Decisions).
• Sole-provider fallback (backward-compat, optional — see note below). Sessions created before this PR carry no oidcId; when exactly one provider is registered, logout falls back to it so those legacy sessions still get a full IdP logout.

Optional — commit 7877d3b (sole-provider fallback), reviewer's call.
Handles one edge case: sessions created before this PR have no oidcId, so on their first post-upgrade logout the IdP logout is skipped. With a single configured provider, this infers it and performs a full IdP logout.
Dropping it: the local session is still destroyed (never crashes), but those legacy sessions keep the IdP session alive and get no message — self-heals on next login (new sessions carry oidcId); provider switching is unaffected.

Architectural Decisions

1. OIDC registration moved from the constructor to onModuleInit. Discovery previously ran as a fire-and-forget promise in the constructor, which Nest doesn't await — a login arriving in that window failed with "Unknown authentication strategy" (startup race). onModuleInit is awaited during bootstrap, so the app serves requests only after all providers register. Trade-off: ready-time now includes one discovery round-trip per provider. Safe: each provider registers in its own try/catch, so an unreachable IdP is skipped (logged, non-fatal) while the others register.

2. id_token retained server-side in the session as the id_token_hint for logout; never exposed to the browser.

3. post_logout_redirect_uri built from MY_URL_API_PATH — the same base used for the OIDC redirect_uris, so it matches what an OIDC client is configured with per the standard Admin App OIDC docs; providers that validate it accept it with no extra registration.

Testing

Manual (E2E, all three providers, DevTools Network):

1. Keycloak: 3-hop chain to the discovered end_session_endpoint; no confirmation page; re-login required credentials (IdP session ended). PASS.
2. Entra ID: same chain to …/oauth2/v2.0/logout; valid v2.0 id_token (email claim present); re-login forced MFA (IdP session ended). Microsoft shows its own account-selection/confirmation screens (provider UX, not app-controlled). PASS.
3. Google: no end_session_endpoint; local session destroyed, redirect to /unauthenticated?msg=…, no hop to Google. Silent re-login expected by design. PASS.

Keycloak

Google WS

Entra ID
See comment on Jira's ticket (file too large to paste here)

Checklist

• Automated tests added or updated
• Manually tested (happy path + error path)
• Branch up to date with target branch (rebased on latest main)
• Commit history clean and includes ticket ID (4 atomic commits)
• No commented-out code or TODO comments
• PR focused on this ticket only

[Copilot]

Pull request overview

Generalizes Admin App logout to support standard OIDC RP-Initiated Logout across any configured OIDC provider by tracking the login provider in the session and building the IdP end-session redirect URL from discovery metadata, with a local-only fallback when an IdP doesn’t advertise an end_session_endpoint.

Changes:

• Registers OIDC providers on Nest bootstrap (onModuleInit), keeps discovered clients, and exposes getEndSessionUrl/getSoleOidcId.
• Captures oidcId and id_token at login and uses them at logout to perform RP-Initiated Logout when supported (otherwise local-only logout + message).
• Adds/updates unit tests and documentation to reflect the new generalized logout behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/fe/src/app/Layout/AppBar.tsx	Updates logout UI comment to be IdP-agnostic.
packages/api/src/auth/login/README.md	Documents RP-Initiated Logout + local-only fallback behavior.
packages/api/src/auth/login/oidc.strategy.ts	Moves provider registration to onModuleInit, stores clients, adds end-session URL builder and session typing for oidcId/idToken.
packages/api/src/auth/login/oidc.strategy.spec.ts	Adds tests for multi-provider registration, end-session URL building, and discovery failure behavior.
packages/api/src/auth/auth.controller.ts	Stores oidcId/idToken on session after login; rewrites logout to use discovered end-session URL and fallback behavior.
packages/api/src/auth/auth.controller.spec.ts	Adds tests for logout behavior and session tracking during login callback.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Test Results

  5 files  ± 0  153 suites  +2   3m 35s ⏱️ -3s
796 tests +17  796 ✅ +17  0 💤 ±0  0 ❌ ±0 
872 runs  +17  872 ✅ +17  0 💤 ±0  0 ❌ ±0 

Results for commit a5970ae. ± Comparison against base commit 424a307.

♻️ This comment has been updated with latest results.

[stephenfuqua]

PR #211 Review — EDFI-2772 Generalize OIDC logout

Ticket: Jira EDFI-2772 (not accessible during this review — evaluated against the PR's own stated requirements/description as the baseline)
Reviewed by Copilot+Sonnet 5 with sub-agents for: Security · Functionality · Maintainability · Usability · Test Coverage

Note

Copilot's review comments have been lightly edited by Stephen, including tweaks to the severity.

🔴 Critical

None found.

🟠 High

1. [Maintainability] request.session.save() in the login callback ignores its error argument — oidcId/idToken can silently fail to persist, leaving a logged-in session that can't do provider-aware logout later, with no log or user-visible failure.
   Fix: Handle the save error explicitly, log with user/provider context, and fail the login flow rather than redirecting as if it succeeded.
2. [Security] onModuleInit() awaits OIDC discovery for all providers with no timeout — one slow/unresponsive IdP can stall app bootstrap entirely.
   Fix: Add a per-provider discovery timeout/retry budget; log and skip on timeout so healthy providers still register and the app boots.
3. [Maintainability] Startup registration failures are swallowed (try/catch + log) with no readiness signal — a misconfigured provider looks identical at runtime to "provider legitimately has no end_session_endpoint," conflating failure with expected behavior.
   Fix: Surface failed provider registrations via health/readiness checks; distinguish "unregistered/broken provider" from "provider doesn't support RP logout" in getEndSessionUrl.

🟡 Medium

4. [Test Coverage] oidc.strategy.ts verify callback (id_token→session mapping, USER_NOT_FOUND/NO_ROLE/DB-error outcomes) is never actually invoked in tests — the real login handoff is unverified.
   Fix: Capture the Strategy passed to passport.use() and invoke its verify callback with representative tokenset/userinfo/authService results.
5. [Security] The raw OIDC id_token (a JWT with user claims/PII) is now placed in id_token_hint and redirected via the browser Location header — exposed to browser history/referrer/logs, unlike the old Keycloak-only flow.
   Fix: Minimize token claims/lifetime if kept; consider whether id_token_hint is strictly required for all providers, and document the exposure.
6. [Functionality + Maintainability] getSoleOidcId() reflects "sole successfully registered" provider, not "sole configured" provider — if one of two configured IdPs fails discovery, the survivor is wrongly treated as the only provider, and legacy sessions (no oidcId) get logged out against a provider they may not have used.
   Fix: Track configured provider IDs separately from successfully registered clients; only infer the sole-provider fallback when the configured set (not just the registered set) has exactly one entry.
7. [Test Coverage] Login callback error branches (NO_ROLE, generic errors, session-detail/invalid_grant/DB-error messages, request.logIn() failure) are untested beyond USER_NOT_FOUND and "no user returned."
   Fix: Add table-driven tests for each redirectLoginFailure branch, plus a logIn failure case.
8. [Maintainability] RegisterOidcIdpsService now owns DB loading, discovery, Passport wiring, user validation, provider registry, and logout URL construction — a widening single-responsibility violation; AuthController reaches into its internal registry state.
   Fix: Split into focused collaborators (e.g., bootstrapper, provider registry, logout URL builder) and rename to reflect actual scope.
9. [Usability] The local-only-logout message uses "identity provider," which is jargon many non-technical users (teachers/district admins) won't recognize.
   Fix: Use plainer, ideally language (e.g., "your organization's sign-in").
10. [Usability] When session has no oidcId and multiple providers are configured, logout silently redirects to the frontend root with no explanation that the user was signed out or that the IdP session may persist.
    Fix: Redirect to /unauthenticated with a generic signed-out message instead of /.

🔵 Low

11. [Security] GET /auth/logout is CSRF-able (triggered via window.location.href from a plain click); with RP-Initiated Logout now added, a forced request can also sign the user out at the IdP, not just locally.
    Fix: Consider POST + CSRF protection for logout, or accept this as a low-impact nuisance (forced logout, not forced login) and document the decision.
12. [Test Coverage] getSoleOidcId() with zero registered providers (empty repo or all discovery failures) is untested.
    Fix: Add a spec asserting undefined is returned and passport.use() is never called when no providers exist.
13. [Maintainability] The declare module 'express-session' session-typing augmentation lives inside oidc.strategy.ts, a non-obvious location coupled to that file being compiled/imported.
    Fix: Move to a dedicated ambient declaration file, e.g. packages/api/src/types/express-session.d.ts.

---

Specialist Verdicts

Reviewer	Verdict
Security	No auth bypass/open-redirect/IDOR found, but introduces an id_token confidentiality regression and a startup-availability risk.
Functionality	PARTIAL — main OIDC logout flow is correct, but the sole-provider fallback breaks its own stated "exactly one provider" contract under partial registration failure.
Maintainability	Good functional direction, but new logout flow leans on hidden runtime state and silent failure paths that will make future auth bugs hard to diagnose.
Usability	Safe rendering, no XSS, working recovery path (Log in button) — main gap is jargon-heavy fallback copy.
Test Coverage	ADEQUATE — happy path and major fallbacks are covered, but the actual OIDC verify-callback logic and several login error branches are unprotected.

---

Summary

The core design (provider-aware RP-Initiated Logout via discovery metadata) is sound and well-tested at the controller level, and Keycloak/Google behavior was manually verified. The most important pre-merge fix is #5 (sole-provider fallback conflates "registered" with "configured," which can misdirect legacy-session logout), paired with #2 (silent session-save failures) since together they make provider-tracking failures invisible. The id_token exposure (#3) and unbounded discovery await (#4) are worth a deliberate call before merge, even if accepted as-is, given they're behavioral changes from the old flow. Nothing found rises to Critical/blocking.

---

Generated by an automated 5-specialist team review (Security, Functionality, Maintainability, Usability, Test Coverage).