From cf00650c6d3bd74ddb9fae92138c00f808511150 Mon Sep 17 00:00:00 2001
From: Denis Arnst <git@sapd.eu>
Date: Tue, 5 Dec 2023 09:43:06 +0100
Subject: [PATCH] SSO/OpenID: Also fix possible race condition

- We need to define redirect_uri in the callback again, because the global params of passport can change between calls to the first route (ie. if multiple users log in at same time)
- Removed is_rest parameter as requirement for mobile flow (to maximise compatibility with possible oauth libraries)
- Also renamed some variables for clarity
---
 server/Auth.js | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/server/Auth.js b/server/Auth.js
index b5bc7d40..0a282c9c 100644
--- a/server/Auth.js
+++ b/server/Auth.js
@@ -190,9 +190,10 @@ class Auth {
    * @param {import('express').Response} res
    */
   paramsToCookies(req, res) {
-    if (req.query.isRest?.toLowerCase() == 'true') {
+    // Set if isRest flag is set or if mobile oauth flow is used
+    if (req.query.isRest?.toLowerCase() == 'true' || req.query.redirect_uri) {
       // store the isRest flag to the is_rest cookie 
-      res.cookie('is_rest', req.query.isRest.toLowerCase(), {
+      res.cookie('is_rest', 'true', {
         maxAge: 120000, // 2 min
         httpOnly: true
       })
@@ -287,7 +288,7 @@ class Auth {
         const oidcStrategy = passport._strategy('openid-client')
         const protocol = (req.secure || req.get('x-forwarded-proto') === 'https') ? 'https' : 'http'
 
-        let redirect_uri = null
+        let mobile_redirect_uri = null
 
         // The client wishes a different redirect_uri
         // We will allow if it is in the whitelist, by saving it into this.openIdAuthSession and setting the redirect uri to /auth/openid/mobile-redirect
@@ -297,7 +298,7 @@ class Auth {
           if (Database.serverSettings.authOpenIDMobileRedirectURIs.includes(req.query.redirect_uri) ||
            (Database.serverSettings.authOpenIDMobileRedirectURIs.length === 1 && Database.serverSettings.authOpenIDMobileRedirectURIs[0] === '*')) {
             oidcStrategy._params.redirect_uri = new URL(`${protocol}://${req.get('host')}/auth/openid/mobile-redirect`).toString()
-            redirect_uri = req.query.redirect_uri
+            mobile_redirect_uri = req.query.redirect_uri
           } else {
             Logger.debug(`[Auth] Invalid redirect_uri=${req.query.redirect_uri} - not in whitelist`)
             return res.status(400).send('Invalid redirect_uri')
@@ -306,7 +307,7 @@ class Auth {
           oidcStrategy._params.redirect_uri = new URL(`${protocol}://${req.get('host')}/auth/openid/callback`).toString()
         }
 
-        Logger.debug(`[Auth] Set oidc redirect_uri=${oidcStrategy._params.redirect_uri}`)
+        Logger.debug(`[Auth] Oidc redirect_uri=${oidcStrategy._params.redirect_uri}`)
         const client = oidcStrategy._client
         const sessionKey = oidcStrategy._key
 
@@ -346,12 +347,13 @@ class Auth {
         req.session[sessionKey] = {
           ...req.session[sessionKey],
           ...pick(params, 'nonce', 'state', 'max_age', 'response_type'),
-          mobile: req.query.isRest?.toLowerCase() === 'true' // Used in the abs callback later
+          mobile: req.query.redirect_uri, // Used in the abs callback later, set mobile if redirect_uri is filled out
+          sso_redirect_uri: oidcStrategy._params.redirect_uri // Save the redirect_uri (for the SSO Provider) for the callback
         }
 
         // We cannot save redirect_uri in the session, because it the mobile client uses browser instead of the API
         //   for the request to mobile-redirect and as such the session is not shared
-        this.openIdAuthSession.set(params.state, { redirect_uri: redirect_uri })
+        this.openIdAuthSession.set(params.state, { mobile_redirect_uri: mobile_redirect_uri })
 
         // Now get the URL to direct to
         const authorizationUrl = client.authorizationUrl({
@@ -386,16 +388,16 @@ class Auth {
           return res.status(400).send('State parameter mismatch')
         }
 
-        let redirect_uri = this.openIdAuthSession.get(state).redirect_uri
+        let mobile_redirect_uri = this.openIdAuthSession.get(state).mobile_redirect_uri
 
-        if (!redirect_uri) {
+        if (!mobile_redirect_uri) {
           Logger.error('[Auth] No redirect URI')
           return res.status(400).send('No redirect URI')
         }
 
         this.openIdAuthSession.delete(state)
 
-        const redirectUri = `${redirect_uri}?code=${encodeURIComponent(code)}&state=${encodeURIComponent(state)}`
+        const redirectUri = `${mobile_redirect_uri}?code=${encodeURIComponent(code)}&state=${encodeURIComponent(state)}`
         // Redirect to the overwrite URI saved in the map
         res.redirect(redirectUri)
       } catch (error) {
@@ -460,8 +462,8 @@ class Auth {
 
       // While not required by the standard, the passport plugin re-sends the original redirect_uri in the token request
       // We need to set it correctly, as some SSO providers (e.g. keycloak) check that parameter when it is provided
-      // This is already done in the strategy in the route to /auth/openid using oidcStrategy._params.redirect_uri 
-      return passport.authenticate('openid-client', passportCallback(req, res, next))(req, res, next)
+      // We set it here again because the passport param can change between requests
+      return passport.authenticate('openid-client', { redirect_uri: req.session[sessionKey].sso_redirect_uri }, passportCallback(req, res, next))(req, res, next)
     },
       // on a successfull login: read the cookies and react like the client requested (callback or json)
       this.handleLoginSuccessBasedOnCookie.bind(this))