From 25f1368227c47ecd6dfcd875543e886fe3546a6f Mon Sep 17 00:00:00 2001 From: Rory Powell Date: Thu, 8 Jul 2021 11:12:34 +0100 Subject: [PATCH] Merge google/oidc user authentication and surface user relevant error messages during authentication --- .../auth/src/middleware/passport/google.js | 81 +++--------- packages/auth/src/middleware/passport/jwt.js | 3 +- .../auth/src/middleware/passport/local.js | 11 +- packages/auth/src/middleware/passport/oidc.js | 91 +++---------- .../middleware/passport/third-party-common.js | 124 ++++++++++++++++++ .../auth/src/middleware/passport/utils.js | 14 ++ .../worker/src/api/controllers/admin/auth.js | 9 +- 7 files changed, 184 insertions(+), 149 deletions(-) create mode 100644 packages/auth/src/middleware/passport/third-party-common.js create mode 100644 packages/auth/src/middleware/passport/utils.js diff --git a/packages/auth/src/middleware/passport/google.js b/packages/auth/src/middleware/passport/google.js index f84da95e73..a20912d973 100644 --- a/packages/auth/src/middleware/passport/google.js +++ b/packages/auth/src/middleware/passport/google.js @@ -1,75 +1,24 @@ -const env = require("../../environment") -const jwt = require("jsonwebtoken") -const database = require("../../db") const GoogleStrategy = require("passport-google-oauth").OAuth2Strategy -const { - StaticDatabases, - generateGlobalUserID, - ViewNames, -} = require("../../db/utils") -async function authenticate(token, tokenSecret, profile, done) { - // Check the user exists in the instance DB by email - const db = database.getDB(StaticDatabases.GLOBAL.name) +const { authenticateThirdParty } = require("./third-party-common") - let dbUser - - const userId = generateGlobalUserID(profile.id) - - try { - // use the google profile id - dbUser = await db.get(userId) - } catch (err) { - const user = { - _id: userId, - provider: profile.provider, - roles: {}, - ...profile._json, - } - - // check if an account with the google email address exists locally - const users = await db.query(`database/${ViewNames.USER_BY_EMAIL}`, { - key: profile._json.email, - include_docs: true, - }) - - // Google user already exists by email - if (users.rows.length > 0) { - const existing = users.rows[0].doc - - // remove the local account to avoid conflicts - await db.remove(existing._id, existing._rev) - - // merge with existing account - user.roles = existing.roles - user.builder = existing.builder - user.admin = existing.admin - - const response = await db.post(user) - dbUser = user - dbUser._rev = response.rev - } else { - return done( - new Error( - "email does not yet exist. You must set up your local budibase account first." - ), - false - ) +async function authenticate(accessToken, refreshToken, profile, done) { + const thirdPartyUser = { + provider: profile.provider, // should always be 'google' + providerType: "google", + userId: profile.id, + profile: profile, + email: profile._json.email, + oauth2: { + accessToken: accessToken, + refreshToken: refreshToken } } - // authenticate - const payload = { - userId: dbUser._id, - builder: dbUser.builder, - email: dbUser.email, - } - - dbUser.token = jwt.sign(payload, env.JWT_SECRET, { - expiresIn: "1 day", - }) - - return done(null, dbUser) + return authenticateThirdParty( + thirdPartyUser, + true, // require local accounts to exist + done) } /** diff --git a/packages/auth/src/middleware/passport/jwt.js b/packages/auth/src/middleware/passport/jwt.js index ed7d179482..690c2ac8a1 100644 --- a/packages/auth/src/middleware/passport/jwt.js +++ b/packages/auth/src/middleware/passport/jwt.js @@ -1,5 +1,6 @@ const { Cookies } = require("../../constants") const env = require("../../environment") +const { authError } = require("./utils") exports.options = { secretOrKey: env.JWT_SECRET, @@ -12,6 +13,6 @@ exports.authenticate = async function (jwt, done) { try { return done(null, jwt) } catch (err) { - return done(new Error("JWT invalid."), false) + return authError(done, "JWT invalid", err) } } diff --git a/packages/auth/src/middleware/passport/local.js b/packages/auth/src/middleware/passport/local.js index 0f5cb82606..01d1e00934 100644 --- a/packages/auth/src/middleware/passport/local.js +++ b/packages/auth/src/middleware/passport/local.js @@ -3,6 +3,7 @@ const { UserStatus } = require("../../constants") const { compare } = require("../../hashing") const env = require("../../environment") const { getGlobalUserByEmail } = require("../../utils") +const { authError } = require("./utils") const INVALID_ERR = "Invalid Credentials" @@ -16,17 +17,17 @@ exports.options = {} * @returns The authenticated user, or errors if they occur */ exports.authenticate = async function (email, password, done) { - if (!email) return done(null, false, "Email Required.") - if (!password) return done(null, false, "Password Required.") + if (!email) return authError(done, "Email Required") + if (!password) return authError(done, "Password Required") const dbUser = await getGlobalUserByEmail(email) if (dbUser == null) { - return done(null, false, { message: "User not found" }) + return authError(done, "User not found") } // check that the user is currently inactive, if this is the case throw invalid if (dbUser.status === UserStatus.INACTIVE) { - return done(null, false, { message: INVALID_ERR }) + return authError(done, INVALID_ERR) } // authenticate @@ -43,6 +44,6 @@ exports.authenticate = async function (email, password, done) { return done(null, dbUser) } else { - done(new Error(INVALID_ERR), false) + return authError(done, INVALID_ERR) } } diff --git a/packages/auth/src/middleware/passport/oidc.js b/packages/auth/src/middleware/passport/oidc.js index f28aa030f9..e695e085bf 100644 --- a/packages/auth/src/middleware/passport/oidc.js +++ b/packages/auth/src/middleware/passport/oidc.js @@ -1,28 +1,18 @@ -const env = require("../../environment") -const jwt = require("jsonwebtoken") -const database = require("../../db") const fetch = require("node-fetch") const OIDCStrategy = require("@techpass/passport-openidconnect").Strategy -const { - StaticDatabases, - generateGlobalUserID, - ViewNames, -} = require("../../db/utils") +const { authenticateThirdParty } = require("./third-party-common") /** - * Attempt to parse the users email address. - * - * It is not guaranteed that the email will be returned by the user info endpoint (e.g. github connected account used in azure ad). - * Fallback to the id token where possible. - * * @param {*} profile The structured profile created by passport using the user info endpoint - * @param {*} jwtClaims The raw claims returned in the id token + * @param {*} jwtClaims The claims returned in the id token */ function getEmail(profile, jwtClaims) { + // profile not guaranteed to contain email e.g. github connected azure ad account if (profile._json.email) { return profile._json.email } + // fallback to id token if (jwtClaims.email) { return jwtClaims.email } @@ -31,7 +21,6 @@ function getEmail(profile, jwtClaims) { } /** - * * @param {*} issuer The identity provider base URL * @param {*} sub The user ID * @param {*} profile The user profile information. Created by passport from the /userinfo response @@ -54,67 +43,23 @@ async function authenticate( params, done ) { - // Check the user exists in the instance DB by email - const db = database.getDB(StaticDatabases.GLOBAL.name) - - let dbUser - - const userId = generateGlobalUserID(profile.id) - - try { - // use the OIDC profile id - dbUser = await db.get(userId) - } catch (err) { - const user = { - _id: userId, - provider: profile.provider, - roles: {}, - ...profile._json, - } - - // check if an account with the OIDC email address exists locally - const email = getEmail(profile, jwtClaims) - if (!email) { - return done(null, false, { message: "No email address found" }) - } - - const users = await db.query(`database/${ViewNames.USER_BY_EMAIL}`, { - key: email, - include_docs: true, - }) - - // OIDC user already exists by email - if (users.rows.length > 0) { - const existing = users.rows[0].doc - - // remove the local account to avoid conflicts - await db.remove(existing._id, existing._rev) - - // merge with existing account - user.roles = existing.roles - user.builder = existing.builder - user.admin = existing.admin - - const response = await db.post(user) - dbUser = user - dbUser._rev = response.rev - } else { - return done(null, false, { message: "Email does not yet exist. You must set up your local budibase account first." }) + const thirdPartyUser = { + // store the issuer info to enable sync in future + provider: issuer, + providerType: "oidc", + userId: profile.id, + profile: profile, + email: getEmail(profile, jwtClaims), + oauth2: { + accessToken: accessToken, + refreshToken: refreshToken } } - // authenticate - const payload = { - userId: dbUser._id, - builder: dbUser.builder, - email: dbUser.email, - } - - dbUser.token = jwt.sign(payload, env.JWT_SECRET, { - expiresIn: "1 day", - }) - - return done(null, dbUser) + return authenticateThirdParty( + thirdPartyUser, + false, // don't require local accounts to exist + done) } /** diff --git a/packages/auth/src/middleware/passport/third-party-common.js b/packages/auth/src/middleware/passport/third-party-common.js new file mode 100644 index 0000000000..55212e3304 --- /dev/null +++ b/packages/auth/src/middleware/passport/third-party-common.js @@ -0,0 +1,124 @@ +const env = require("../../environment") +const jwt = require("jsonwebtoken") +const database = require("../../db") +const { + StaticDatabases, + generateGlobalUserID, + ViewNames, +} = require("../../db/utils") +const { authError } = require("./utils") + +/** + * Common authentication logic for third parties. e.g. OAuth, OIDC. + */ +exports.authenticateThirdParty = async function ( + thirdPartyUser, + requireLocalAccount = true, + done + ) { + if (!thirdPartyUser.provider) return authError(done, "third party user provider required") + if (!thirdPartyUser.userId) return authError(done, "third party user id required") + if (!thirdPartyUser.email) return authError(done, "third party user email required") + + const db = database.getDB(StaticDatabases.GLOBAL.name) + + let dbUser + + // use the third party id + const userId = generateGlobalUserID(thirdPartyUser.userId) + + try { + dbUser = await db.get(userId) + } catch (err) { + // abort when not 404 error + if (!err.status || err.status !== 404) { + return authError(done, "Unexpected error when retrieving existing user", err) + } + + // check user already exists by email + const users = await db.query(`database/${ViewNames.USER_BY_EMAIL}`, { + key: thirdPartyUser.email, + include_docs: true, + }) + let userExists = users.rows.length > 0 + + if (requireLocalAccount && !userExists) { + return authError(done, "Email does not yet exist. You must set up your local budibase account first.") + } + + // create the user to save + let user + if (userExists) { + const existing = users.rows[0].doc + user = constructMergedUser(userId, existing, thirdPartyUser) + + // remove the local account to avoid conflicts + await db.remove(existing._id, existing._rev) + } else { + user = constructNewUser(userId, thirdPartyUser) + } + + // save the user + const response = await db.post(user) + dbUser = user + dbUser._rev = response.rev + } + + // authenticate + const payload = { + userId: dbUser._id, + builder: dbUser.builder, + email: dbUser.email, + } + + dbUser.token = jwt.sign(payload, env.JWT_SECRET, { + expiresIn: "1 day", + }) + + return done(null, dbUser) +} + +/** + * @returns a user object constructed from existing and third party information + */ +function constructMergedUser(userId, existing, thirdPartyUser) { + const user = constructNewUser(userId, thirdPartyUser) + + // merge with existing account + user.roles = existing.roles + user.builder = existing.builder + user.admin = existing.admin + + return user +} + +/** + * @returns a user object constructed from third party information + */ +function constructNewUser(userId, thirdPartyUser) { + const user = { + _id: userId, + provider: thirdPartyUser.provider, + providerType: thirdPartyUser.providerType, + roles: {} + } + + // persist profile information + // @reviewers: Historically stored at the root level of the user + // Nest to prevent conflicts with future fields + // Is this okay to change? + if (thirdPartyUser.profile) { + user.thirdPartyProfile = { + ...thirdPartyUser.profile._json + } + } + + // persist oauth tokens for future use + if (thirdPartyUser.oauth2) { + user.oauth2 = { + ...thirdPartyUser.oauth2 + } + } + + return user +} diff --git a/packages/auth/src/middleware/passport/utils.js b/packages/auth/src/middleware/passport/utils.js new file mode 100644 index 0000000000..1f7b4437bd --- /dev/null +++ b/packages/auth/src/middleware/passport/utils.js @@ -0,0 +1,14 @@ +/** + * Utility to handle authentication errors. + * + * @param {*} done The passport callback. + * @param {*} message Message that will be returned in the response body + * @param {*} err (Optional) error that will be logged + */ +exports.authError = function (done, message, err = null) { + return done( + err, + null, // never return a user + { message: message } + ) +} \ No newline at end of file diff --git a/packages/worker/src/api/controllers/admin/auth.js b/packages/worker/src/api/controllers/admin/auth.js index 01717bffe0..e6d553bc62 100644 --- a/packages/worker/src/api/controllers/admin/auth.js +++ b/packages/worker/src/api/controllers/admin/auth.js @@ -13,6 +13,7 @@ const GLOBAL_DB = authPkg.StaticDatabases.GLOBAL.name function authInternal(ctx, user, err = null, info = null) { if (err) { + console.error("Authentication error", err) return ctx.throw(403, info? info : "Unauthorized") } @@ -32,8 +33,8 @@ function authInternal(ctx, user, err = null, info = null) { } exports.authenticate = async (ctx, next) => { - return passport.authenticate("local", async (err, user) => { - authInternal(ctx, user, err) + return passport.authenticate("local", async (err, user, info) => { + authInternal(ctx, user, err, info) delete user.token @@ -123,8 +124,8 @@ exports.googleAuth = async (ctx, next) => { return passport.authenticate( strategy, { successRedirect: "/", failureRedirect: "/error" }, - async (err, user) => { - authInternal(ctx, user, err) + async (err, user, info) => { + authInternal(ctx, user, err, info) ctx.redirect("/") }