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
This commit is contained in:
Denis Arnst 2023-12-05 09:43:06 +01:00
parent e6ab28365f
commit cf00650c6d

View File

@ -190,9 +190,10 @@ class Auth {
* @param {import('express').Response} res * @param {import('express').Response} res
*/ */
paramsToCookies(req, 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 // 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 maxAge: 120000, // 2 min
httpOnly: true httpOnly: true
}) })
@ -287,7 +288,7 @@ class Auth {
const oidcStrategy = passport._strategy('openid-client') const oidcStrategy = passport._strategy('openid-client')
const protocol = (req.secure || req.get('x-forwarded-proto') === 'https') ? 'https' : 'http' 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 // 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 // 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) || if (Database.serverSettings.authOpenIDMobileRedirectURIs.includes(req.query.redirect_uri) ||
(Database.serverSettings.authOpenIDMobileRedirectURIs.length === 1 && Database.serverSettings.authOpenIDMobileRedirectURIs[0] === '*')) { (Database.serverSettings.authOpenIDMobileRedirectURIs.length === 1 && Database.serverSettings.authOpenIDMobileRedirectURIs[0] === '*')) {
oidcStrategy._params.redirect_uri = new URL(`${protocol}://${req.get('host')}/auth/openid/mobile-redirect`).toString() 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 { } else {
Logger.debug(`[Auth] Invalid redirect_uri=${req.query.redirect_uri} - not in whitelist`) Logger.debug(`[Auth] Invalid redirect_uri=${req.query.redirect_uri} - not in whitelist`)
return res.status(400).send('Invalid redirect_uri') 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() 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 client = oidcStrategy._client
const sessionKey = oidcStrategy._key const sessionKey = oidcStrategy._key
@ -346,12 +347,13 @@ class Auth {
req.session[sessionKey] = { req.session[sessionKey] = {
...req.session[sessionKey], ...req.session[sessionKey],
...pick(params, 'nonce', 'state', 'max_age', 'response_type'), ...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 // 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 // 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 // Now get the URL to direct to
const authorizationUrl = client.authorizationUrl({ const authorizationUrl = client.authorizationUrl({
@ -386,16 +388,16 @@ class Auth {
return res.status(400).send('State parameter mismatch') 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') Logger.error('[Auth] No redirect URI')
return res.status(400).send('No redirect URI') return res.status(400).send('No redirect URI')
} }
this.openIdAuthSession.delete(state) 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 // Redirect to the overwrite URI saved in the map
res.redirect(redirectUri) res.redirect(redirectUri)
} catch (error) { } 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 // 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 // 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 // We set it here again because the passport param can change between requests
return passport.authenticate('openid-client', passportCallback(req, res, next))(req, res, next) 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) // on a successfull login: read the cookies and react like the client requested (callback or json)
this.handleLoginSuccessBasedOnCookie.bind(this)) this.handleLoginSuccessBasedOnCookie.bind(this))