APIGW: Refactor SAML-specifics out of session module

- Instead of having anything specifically SAML-related in the session module, take the logout token in as just a string and move the parsing and nameID+sessionIndex logic into more SAML-specific modules
- This also allows dropping unnecessary arguments from all other routes than logout callback
    - But requires the login callback route to have logic for generating the token initially before saving it

apigw/src/shared/auth/index.ts

@@ -4,10 +4,12 @@
 
 import { concat } from 'lodash'
 import { NextFunction, Request, Response } from 'express'
-import { logAuditEvent } from '../logging'
+import { logAuditEvent, logDebug } from '../logging'
 import { gatewayRole } from '../config'
 import { createJwt } from './jwt'
 import { SamlUser } from '../routes/auth/saml/types'
+import { Profile, SAML } from 'passport-saml'
+import { fromCallback } from '../promise-utils'
 
 const auditEventGatewayId =
   (gatewayRole === 'enduser' && 'eugw') ||
@@ -65,3 +67,50 @@ function createJwtToken(user: SamlUser): string {
 export function createAuthHeader(user: SamlUser): string {
   return `Bearer ${createJwtToken(user)}`
 }
+
+export function createLogoutToken(
+  nameID: Required<Profile>['nameID'],
+  sessionIndex: Profile['sessionIndex']
+) {
+  return `${nameID}:::${sessionIndex}`
+}
+
+/**
+ * If request is a SAMLRequest, parse, validate and return the Profile from it.
+ * @param saml Config must match active strategy's config
+ */
+export async function tryParseProfile(
+  req: Request,
+  saml?: SAML
+): Promise<Profile | undefined> {
+  if (!saml) {
+    logDebug('No SAML parser provided, skipping profile parsing from request')
+    return
+  }
+
+  let profile: Profile | undefined
+
+  // NOTE: This duplicate parsing can be removed if passport-saml ever exposes
+  // an alternative for passport.authenticate() that either lets us hook into
+  // it before any redirects or separate XML parsing and authentication methods.
+  if (req.query?.SAMLRequest) {
+    // Redirects have signatures in the original query parameter
+    const dummyOrigin = 'http://evaka'
+    const originalQuery = new URL(req.url, dummyOrigin).search.replace(
+      /^\?/,
+      ''
+    )
+    profile =
+      (await fromCallback<Profile | null | undefined>((cb) =>
+        saml.validateRedirect(req.query, originalQuery, cb)
+      )) || undefined
+  } else if (req.body?.SAMLRequest) {
+    // POST logout callbacks have the signature in the message body directly
+    profile =
+      (await fromCallback<Profile | null | undefined>((cb) =>
+        saml.validatePostRequest(req.body, cb)
+      )) || undefined
+  }
+
+  return profile
+}

apigw/src/shared/routes/auth/saml/index.ts

@@ -14,6 +14,7 @@ import { fromCallback } from '../../../promise-utils'
 import { getEmployees } from '../../../dev-api'
 import _ from 'lodash'
 import { AuthenticateOptions, SAML } from 'passport-saml'
+import { createLogoutToken, tryParseProfile } from '../../../auth'
 
 const urlencodedParser = urlencoded({ extended: false })
 
@@ -92,7 +93,15 @@ function createLoginHandler({
             req,
             'User logged in successfully'
           )
-          await saveLogoutToken(req, strategyName)
+
+          if (!user.nameID) {
+            throw new Error('User unexpectedly missing nameID property')
+          }
+          await saveLogoutToken(
+            req,
+            strategyName,
+            createLogoutToken(user.nameID, user.sessionIndex)
+          )
           const redirectUrl = getRedirectUrl(req)
           logDebug(`Redirecting to ${redirectUrl}`, req, { redirectUrl })
           return res.redirect(redirectUrl)
@@ -114,14 +123,10 @@ function createLoginHandler({
 }
 
 function createLogoutHandler({
-  samlConfig,
   strategy,
   strategyName,
   sessionType
 }: SamlEndpointConfig): express.RequestHandler {
-  // For parsing SAML messages outside the strategy
-  const saml = new SAML(samlConfig)
-
   return toRequestHandler(async (req, res) => {
     logAuditEvent(
       `evaka.saml.${strategyName}.sign_out_requested`,
@@ -134,7 +139,7 @@ function createLogoutHandler({
           strategy.logout(req, cb)
         )
         logDebug('Logging user out from passport.', req)
-        await logoutExpress(req, res, sessionType, saml)
+        await logoutExpress(req, res, sessionType)
         return res.redirect(redirectUrl)
       } catch (err) {
         logAuditEvent(
@@ -151,7 +156,7 @@ function createLogoutHandler({
         'User signed out'
       )
       logDebug('Logging user out from passport.', req)
-      await logoutExpress(req, res, sessionType, saml)
+      await logoutExpress(req, res, sessionType)
       res.redirect(getRedirectUrl(req))
     }
   })
@@ -178,7 +183,13 @@ export default function createSamlRouter(config: SamlEndpointConfig): Router {
       req,
       'Logout callback called'
     )
-    await logoutExpress(req, res, config.sessionType, saml)
+    const profile = await tryParseProfile(req, saml)
+    await logoutExpress(
+      req,
+      res,
+      config.sessionType,
+      profile?.nameID && createLogoutToken(profile.nameID, profile.sessionIndex)
+    )
   })
 
   const router = Router()

apigw/src/shared/session.ts

@@ -2,7 +2,7 @@
 //
 // SPDX-License-Identifier: LGPL-2.1-or-later
 
-import express, { CookieOptions, Request } from 'express'
+import express, { CookieOptions } from 'express'
 import session from 'express-session'
 import connectRedis from 'connect-redis'
 import {
@@ -24,10 +24,9 @@ import {
   differenceInSeconds,
   isDate
 } from 'date-fns'
-import { toMiddleware } from './express'
+import { LogoutToken, toMiddleware } from './express'
 import AsyncRedisClient from './async-redis-client'
 import { fromCallback } from './promise-utils'
-import { SAML, Profile } from 'passport-saml'
 
 export type SessionType = 'enduser' | 'employee'
 
@@ -71,8 +70,8 @@ function sessionKey(id: string) {
   return `sess:${id}`
 }
 
-function logoutKey(nameID: string, sessionIndex?: string) {
-  return `slo:${nameID}:::${sessionIndex}`
+function logoutKey(token: string) {
+  return `slo:${token}`
 }
 
 export function sessionCookie(sessionType: SessionType) {
@@ -93,68 +92,44 @@ export function refreshLogoutToken() {
   })
 }
 
-async function tryParseProfile(
-  req: Request,
-  saml?: SAML
-): Promise<Profile | null | undefined> {
-  if (!saml) {
-    logDebug('No SAML parser provided, skipping profile parsing from request')
-    return
-  }
-
-  // NOTE: This duplicate parsing can be removed if passport-saml ever exposes
-  // an alternative for passport.authenticate() that either lets us hook into
-  // it before any redirects or separate XML parsing and authentication methods.
-  if (req.query?.SAMLRequest) {
-    // Redirects have signatures in the original query parameteru
-    const dummyOrigin = 'http://evaka'
-    const originalQuery = new URL(req.url, dummyOrigin).search.replace(
-      /^\?/,
-      ''
-    )
-    return await fromCallback<Profile | null | undefined>((cb) =>
-      saml.validateRedirect(req.query, originalQuery, cb)
-    )
-  } else if (req.body?.SAMLRequest) {
-    // POST logout callbacks have the signature in the message body directly
-    return await fromCallback<Profile | null | undefined>((cb) =>
-      saml.validatePostRequest(req.body, cb)
-    )
-  }
-}
-
 /**
+ * TODO: move this comment
  * Save a logout token for a user session to be consumed during logout.
  *
- * The token is generated by creating an effective secondary
- * index in Redis from SAML session identifiers (nameID and sessionIndex).
- * This token can then be used with LogoutRequests without relying
- * on 3rd party cookies which are starting to be disabled by default on many
- * browsers, enabling Single Logout.
+ * The token must be a value generated from values available in a logout request
+ * without any cookies, as many browsers will disable 3rd party cookies by
+ * default, breaking Single Logout. For example, SAML nameID and sessionIndex.
+ *
+ * The token is used as an effective secondary index in Redis for the session,
+ * which would normally be recognized from the session cookie.
  *
- * This token can be removed if this passport-saml issue is ever fixed:
+ * This token can be removed if this passport-saml issue is ever fixed and this
+ * logic is directly implemented in the library:
  * https://github.com/node-saml/passport-saml/issues/419
  */
 export async function saveLogoutToken(
   req: express.Request,
-  strategyName: string | null | undefined
+  strategyName: string | null | undefined,
+  logoutToken?: string
 ): Promise<void> {
-  if (!req.session || !req.user?.nameID) return
+  if (!req.session) return
+
+  const token = logoutToken || req.session.logoutToken?.value
+  if (!token) return
 
   // Persist in session to allow custom logic per strategy
   req.session.idpProvider = strategyName
 
   if (!asyncRedisClient) return
-  const key = logoutKey(req.user.nameID, req.user.sessionIndex)
-
   const now = new Date()
   const expires = addMinutes(now, sessionTimeoutMinutes + 60)
-  const logoutToken = {
+  const newToken = {
     expiresAt: expires.valueOf(),
-    value: req.session.logoutToken?.value || key
+    value: token
   }
-  req.session.logoutToken = logoutToken
+  req.session.logoutToken = newToken
 
+  const key = logoutKey(token)
   const ttlSeconds = differenceInSeconds(expires, now)
   // https://redis.io/commands/expire - Set a timeout on key
   // Return value:
@@ -170,25 +145,20 @@ export async function saveLogoutToken(
 
 async function consumeLogoutRequest(
   req: express.Request,
-  saml?: SAML
+  logoutToken?: LogoutToken['value']
 ): Promise<void> {
   if (!asyncRedisClient) return
 
-  const profile = await tryParseProfile(req, saml)
-
-  if (!profile?.nameID || !req.session?.logoutToken?.value) {
-    logDebug(
-      "Can't consume logout request without a SAMLRequest or session cookie, ignoring"
-    )
+  const token = logoutToken || req.session?.logoutToken?.value
+  if (!token) {
+    logDebug("Can't consume logout request without a logout token, ignoring")
     return
   }
 
   // Prefer details from the SAML message (profile) but fall back to details
   // from the session in case a) this wasn't a SAMLRequest b) it's malformed
   // to ensure the logout token is deleted from the store even in non-SLO cases.
-  const key = profile.nameID
-    ? logoutKey(profile.nameID, profile.sessionIndex)
-    : req.session.logoutToken.value
+  const key = logoutKey(token)
   const sid = await asyncRedisClient.get(key)
   if (sid) {
     // Ensure both session and logout keys are cleared in case no cookies were
@@ -201,10 +171,10 @@ export async function logoutExpress(
   req: express.Request,
   res: express.Response,
   sessionType: SessionType,
-  saml?: SAML
+  logoutToken?: LogoutToken['value']
 ) {
   req.logout()
-  await consumeLogoutRequest(req, saml)
+  await consumeLogoutRequest(req, logoutToken)
   if (req.session) {
     const session = req.session
     await fromCallback((cb) => session.destroy(cb))