From ad53894ea1e57ab49c633bed177f512d976c2fdb Mon Sep 17 00:00:00 2001 From: Denis Arnst Date: Tue, 28 Nov 2023 17:29:22 +0100 Subject: [PATCH 1/4] SSO/OpenID: Provide detailed error messages --- server/Auth.js | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/server/Auth.js b/server/Auth.js index dedf32f0..af2b4289 100644 --- a/server/Auth.js +++ b/server/Auth.js @@ -363,12 +363,48 @@ class Auth { req.session[sessionKey].code_verifier = req.query.code_verifier } + function handleAuthError(isMobile, errorCode, errorMessage, logMessage, logMessageDetail) { + Logger.error(logMessage) + if (logMessageDetail) { + Logger.debug(logMessageDetail.toString()) + } + + if (isMobile) { + return res.status(errorCode).send(errorMessage) + } else { + return res.redirect(`/login?error=${encodeURIComponent(errorMessage)}&autoLaunch=0`) + } + } + + function passportCallback(req, res, next) { + return (err, user, info) => { + const isMobile = req.session[sessionKey]?.mobile === true + if (err) { + return handleAuthError(isMobile, 500, 'Error in callback', `[Auth] Error in openid callback - ${err}`, err?.response?.body) + } + + if (!user) { + // Info usually contains the error message from the SSO provider + // Depending on the error, it can also have a body + return handleAuthError(isMobile, 401, 'Unauthorized', `[Auth] No user in openid callback - ${info}`, info?.response?.body) + } + + req.logIn(user, (loginError) => { + if (loginError) { + return handleAuthError(isMobile, 500, 'Error during login', `[Auth] Error in openid callback: ${loginError}`) + } + next() + }) + } + } + + // 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 if (req.session[sessionKey].mobile) { - return passport.authenticate('openid-client', { redirect_uri: 'audiobookshelf://oauth' })(req, res, next) + return passport.authenticate('openid-client', { redirect_uri: 'audiobookshelf://oauth' }, passportCallback(req, res, next))(req, res, next) } else { - return passport.authenticate('openid-client', { failureRedirect: '/login?error=Unauthorized&autoLaunch=0' })(req, res, next) + return passport.authenticate('openid-client', passportCallback(req, res, next))(req, res, next) } }, // on a successfull login: read the cookies and react like the client requested (callback or json) From 618028503bcd4033ef664a0747abd4a80a594635 Mon Sep 17 00:00:00 2001 From: Denis Arnst Date: Tue, 28 Nov 2023 20:07:49 +0100 Subject: [PATCH 2/4] SSO/OpenID: Also Log token header --- server/Auth.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/server/Auth.js b/server/Auth.js index af2b4289..74ccf240 100644 --- a/server/Auth.js +++ b/server/Auth.js @@ -363,10 +363,13 @@ class Auth { req.session[sessionKey].code_verifier = req.query.code_verifier } - function handleAuthError(isMobile, errorCode, errorMessage, logMessage, logMessageDetail) { + function handleAuthError(isMobile, errorCode, errorMessage, logMessage, response) { Logger.error(logMessage) - if (logMessageDetail) { - Logger.debug(logMessageDetail.toString()) + if (response) { + // Depending on the error, it can also have a body + // We also log the request header the passport plugin sents for the URL + const header = response.req?._header.replace(/Authorization: [^\r\n]*/i, 'Authorization: REDACTED') + Logger.debug(header + '\n' + response.body?.toString()) } if (isMobile) { @@ -380,13 +383,12 @@ class Auth { return (err, user, info) => { const isMobile = req.session[sessionKey]?.mobile === true if (err) { - return handleAuthError(isMobile, 500, 'Error in callback', `[Auth] Error in openid callback - ${err}`, err?.response?.body) + return handleAuthError(isMobile, 500, 'Error in callback', `[Auth] Error in openid callback - ${err}`, err?.response) } if (!user) { // Info usually contains the error message from the SSO provider - // Depending on the error, it can also have a body - return handleAuthError(isMobile, 401, 'Unauthorized', `[Auth] No user in openid callback - ${info}`, info?.response?.body) + return handleAuthError(isMobile, 401, 'Unauthorized', `[Auth] No user in openid callback - ${info}`, info?.response) } req.logIn(user, (loginError) => { From 36599a2984c539c3d22d7f058c3101a328427572 Mon Sep 17 00:00:00 2001 From: Denis Arnst Date: Tue, 28 Nov 2023 21:16:39 +0100 Subject: [PATCH 3/4] SSO/OpenID: Rename probably misleading message --- server/Auth.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/Auth.js b/server/Auth.js index 74ccf240..96a2bc9e 100644 --- a/server/Auth.js +++ b/server/Auth.js @@ -388,7 +388,7 @@ class Auth { if (!user) { // Info usually contains the error message from the SSO provider - return handleAuthError(isMobile, 401, 'Unauthorized', `[Auth] No user in openid callback - ${info}`, info?.response) + return handleAuthError(isMobile, 401, 'Unauthorized', `[Auth] No data in openid callback - ${info}`, info?.response) } req.logIn(user, (loginError) => { From a719065b8d8d24c0d5b7c1ce10ea3e112b6e1c39 Mon Sep 17 00:00:00 2001 From: advplyr Date: Tue, 28 Nov 2023 16:37:19 -0600 Subject: [PATCH 4/4] Auto formatting --- server/Auth.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/Auth.js b/server/Auth.js index 96a2bc9e..57792177 100644 --- a/server/Auth.js +++ b/server/Auth.js @@ -379,7 +379,7 @@ class Auth { } } - function passportCallback(req, res, next) { + function passportCallback(req, res, next) { return (err, user, info) => { const isMobile = req.session[sessionKey]?.mobile === true if (err) { @@ -390,7 +390,7 @@ class Auth { // Info usually contains the error message from the SSO provider return handleAuthError(isMobile, 401, 'Unauthorized', `[Auth] No data in openid callback - ${info}`, info?.response) } - + req.logIn(user, (loginError) => { if (loginError) { return handleAuthError(isMobile, 500, 'Error during login', `[Auth] Error in openid callback: ${loginError}`)