From 1ff9785498c883d5cf643d626ba80460d7bbb570 Mon Sep 17 00:00:00 2001 From: Dean Date: Mon, 4 Jul 2022 12:54:26 +0100 Subject: [PATCH] Code review updates --- packages/backend-core/src/auth.js | 5 ++- packages/backend-core/src/db/utils.ts | 18 ++++----- .../src/middleware/authenticated.js | 1 - packages/backend-core/src/middleware/index.js | 3 +- .../src/middleware/passport/google.js | 10 ----- .../src/middleware/passport/local.js | 15 +------- .../src/middleware/passport/oidc.js | 4 +- .../src/middleware/passport/utils.js | 6 +-- .../worker/src/api/controllers/global/auth.ts | 37 +++---------------- 9 files changed, 22 insertions(+), 77 deletions(-) diff --git a/packages/backend-core/src/auth.js b/packages/backend-core/src/auth.js index 88a90e2126..b6d6a2027f 100644 --- a/packages/backend-core/src/auth.js +++ b/packages/backend-core/src/auth.js @@ -15,6 +15,7 @@ const { tenancy, appTenancy, authError, + ssoCallbackUrl, csrf, internalApi, } = require("./middleware") @@ -72,11 +73,10 @@ async function refreshOIDCAccessToken(db, chosenConfig, refreshToken) { async function refreshGoogleAccessToken(db, config, refreshToken) { let callbackUrl = await google.getCallbackUrl(db, config) - const googleConfig = await google.fetchStrategyConfig(config) let strategy try { - strategy = await google.strategyFactory(googleConfig, callbackUrl) + strategy = await google.strategyFactory(config, callbackUrl) } catch (err) { console.error(err) throw new Error("Error constructing OIDC refresh strategy", err) @@ -168,4 +168,5 @@ module.exports = { internalApi, refreshOAuthToken, updateUserOAuth, + ssoCallbackUrl, } diff --git a/packages/backend-core/src/db/utils.ts b/packages/backend-core/src/db/utils.ts index 3ad7f9b4f6..c23b56197b 100644 --- a/packages/backend-core/src/db/utils.ts +++ b/packages/backend-core/src/db/utils.ts @@ -386,10 +386,9 @@ export const getScopedFullConfig = async function ( if (type === Configs.SETTINGS) { if (scopedConfig && scopedConfig.doc) { // overrides affected by environment variables - scopedConfig.doc.config.platformUrl = await getPlatformUrl( - { tenantAware: true }, - db - ) + scopedConfig.doc.config.platformUrl = await getPlatformUrl({ + tenantAware: true, + }) scopedConfig.doc.config.analyticsEnabled = await events.analytics.enabled() } else { @@ -398,7 +397,7 @@ export const getScopedFullConfig = async function ( doc: { _id: generateConfigID({ type, user, workspace }), config: { - platformUrl: await getPlatformUrl({ tenantAware: true }, db), + platformUrl: await getPlatformUrl({ tenantAware: true }), analyticsEnabled: await events.analytics.enabled(), }, }, @@ -409,10 +408,7 @@ export const getScopedFullConfig = async function ( return scopedConfig && scopedConfig.doc } -export const getPlatformUrl = async ( - opts = { tenantAware: true }, - db = null -) => { +export const getPlatformUrl = async (opts = { tenantAware: true }) => { let platformUrl = env.PLATFORM_URL || "http://localhost:10000" if (!env.SELF_HOSTED && env.MULTI_TENANCY && opts.tenantAware) { @@ -422,11 +418,11 @@ export const getPlatformUrl = async ( platformUrl = platformUrl.replace("://", `://${tenantId}.`) } } else if (env.SELF_HOSTED) { - const dbx = db ? db : getGlobalDB() + const db = getGlobalDB() // get the doc directly instead of with getScopedConfig to prevent loop let settings try { - settings = await dbx.get(generateConfigID({ type: Configs.SETTINGS })) + settings = await db.get(generateConfigID({ type: Configs.SETTINGS })) } catch (e: any) { if (e.status !== 404) { throw e diff --git a/packages/backend-core/src/middleware/authenticated.js b/packages/backend-core/src/middleware/authenticated.js index cca80cc511..4e6e0b7ba2 100644 --- a/packages/backend-core/src/middleware/authenticated.js +++ b/packages/backend-core/src/middleware/authenticated.js @@ -94,7 +94,6 @@ module.exports = ( user = await getUser(userId, session.tenantId) } user.csrfToken = session.csrfToken - delete user.password authenticated = true } catch (err) { error = err diff --git a/packages/backend-core/src/middleware/index.js b/packages/backend-core/src/middleware/index.js index 6c4c0d8883..1721d56a3c 100644 --- a/packages/backend-core/src/middleware/index.js +++ b/packages/backend-core/src/middleware/index.js @@ -2,7 +2,7 @@ const jwt = require("./passport/jwt") const local = require("./passport/local") const google = require("./passport/google") const oidc = require("./passport/oidc") -const { authError } = require("./passport/utils") +const { authError, ssoCallbackUrl } = require("./passport/utils") const authenticated = require("./authenticated") const auditLog = require("./auditLog") const tenancy = require("./tenancy") @@ -20,6 +20,7 @@ module.exports = { tenancy, authError, internalApi, + ssoCallbackUrl, datasource: { google: datasourceGoogle, }, diff --git a/packages/backend-core/src/middleware/passport/google.js b/packages/backend-core/src/middleware/passport/google.js index 167e719203..7419974cd7 100644 --- a/packages/backend-core/src/middleware/passport/google.js +++ b/packages/backend-core/src/middleware/passport/google.js @@ -2,7 +2,6 @@ const GoogleStrategy = require("passport-google-oauth").OAuth2Strategy const { ssoCallbackUrl } = require("./utils") const { authenticateThirdParty } = require("./third-party-common") const { Configs } = require("../../../constants") -const environment = require("../../environment") const buildVerifyFn = saveUserFn => { return (accessToken, refreshToken, profile, done) => { @@ -60,15 +59,6 @@ exports.strategyFactory = async function (config, callbackUrl, saveUserFn) { } } -exports.fetchStrategyConfig = async function (googleConfig) { - return ( - googleConfig || { - clientID: environment.GOOGLE_CLIENT_ID, - clientSecret: environment.GOOGLE_CLIENT_SECRET, - } - ) -} - exports.getCallbackUrl = async function (db, config) { return ssoCallbackUrl(db, config, Configs.GOOGLE) } diff --git a/packages/backend-core/src/middleware/passport/local.js b/packages/backend-core/src/middleware/passport/local.js index 3f224e7abd..b955d29102 100644 --- a/packages/backend-core/src/middleware/passport/local.js +++ b/packages/backend-core/src/middleware/passport/local.js @@ -6,7 +6,7 @@ const users = require("../../users") const { authError } = require("./utils") const { newid } = require("../../hashing") const { createASession } = require("../../security/sessions") -const { getTenantId, getGlobalDB } = require("../../tenancy") +const { getTenantId } = require("../../tenancy") const INVALID_ERR = "Invalid credentials" const SSO_NO_PASSWORD = "SSO user does not have a password set" @@ -56,19 +56,6 @@ exports.authenticate = async function (ctx, email, password, done) { const sessionId = newid() const tenantId = getTenantId() - if (dbUser.provider || dbUser.providerType || dbUser.pictureUrl) { - delete dbUser.provider - delete dbUser.providerType - delete dbUser.pictureUrl - - try { - const db = getGlobalDB() - await db.put(dbUser) - } catch (err) { - console.error("OAuth elements could not be purged") - } - } - await createASession(dbUser._id, { sessionId, tenantId }) dbUser.token = jwt.sign( diff --git a/packages/backend-core/src/middleware/passport/oidc.js b/packages/backend-core/src/middleware/passport/oidc.js index 570ce641cf..20dbd4669b 100644 --- a/packages/backend-core/src/middleware/passport/oidc.js +++ b/packages/backend-core/src/middleware/passport/oidc.js @@ -103,9 +103,9 @@ exports.strategyFactory = async function (config, saveUserFn) { } } -exports.fetchStrategyConfig = async function (config, callbackUrl) { +exports.fetchStrategyConfig = async function (enrichedConfig, callbackUrl) { try { - const { clientID, clientSecret, configUrl } = config + const { clientID, clientSecret, configUrl } = enrichedConfig if (!clientID || !clientSecret || !callbackUrl || !configUrl) { //check for remote config and all required elements diff --git a/packages/backend-core/src/middleware/passport/utils.js b/packages/backend-core/src/middleware/passport/utils.js index ddc87c6cd0..217130cd6d 100644 --- a/packages/backend-core/src/middleware/passport/utils.js +++ b/packages/backend-core/src/middleware/passport/utils.js @@ -1,4 +1,4 @@ -const { getGlobalDB, isMultiTenant, getTenantId } = require("../../tenancy") +const { isMultiTenant, getTenantId } = require("../../tenancy") const { getScopedConfig } = require("../../db/utils") const { Configs } = require("../../constants") @@ -23,9 +23,7 @@ exports.ssoCallbackUrl = async (db, config, type) => { if (config && config.callbackURL) { return config.callbackURL } - - const dbx = db ? db : getGlobalDB() - const publicConfig = await getScopedConfig(dbx, { + const publicConfig = await getScopedConfig(db, { type: Configs.SETTINGS, }) diff --git a/packages/worker/src/api/controllers/global/auth.ts b/packages/worker/src/api/controllers/global/auth.ts index 2b738b4900..97b6c4c02f 100644 --- a/packages/worker/src/api/controllers/global/auth.ts +++ b/packages/worker/src/api/controllers/global/auth.ts @@ -1,49 +1,22 @@ const core = require("@budibase/backend-core") -const { getScopedConfig } = require("@budibase/backend-core/db") -const { google } = require("@budibase/backend-core/middleware") -const { oidc } = require("@budibase/backend-core/middleware") const { Configs, EmailTemplatePurpose } = require("../../../constants") const { sendEmail, isEmailConfigured } = require("../../../utilities/email") const { setCookie, getCookie, clearCookie, hash, platformLogout } = core.utils const { Cookies, Headers } = core.constants -const { passport } = core.auth +const { passport, ssoCallbackUrl, google, oidc } = core.auth const { checkResetPasswordCode } = require("../../../utilities/redis") -const { - getGlobalDB, - getTenantId, - isMultiTenant, -} = require("@budibase/backend-core/tenancy") +const { getGlobalDB } = require("@budibase/backend-core/tenancy") const env = require("../../../environment") import { events, users as usersCore, context } from "@budibase/backend-core" import { users } from "../../../sdk" import { User } from "@budibase/types" -const ssoCallbackUrl = async (config: any, type: any) => { - // incase there is a callback URL from before - if (config && config.callbackURL) { - return config.callbackURL - } - - const db = getGlobalDB() - const publicConfig = await getScopedConfig(db, { - type: Configs.SETTINGS, - }) - - let callbackUrl = `/api/global/auth` - if (isMultiTenant()) { - callbackUrl += `/${getTenantId()}` - } - callbackUrl += `/${type}/callback` - - return `${publicConfig.platformUrl}${callbackUrl}` -} - export const googleCallbackUrl = async (config: any) => { - return ssoCallbackUrl(config, "google") + return ssoCallbackUrl(getGlobalDB(), config, "google") } export const oidcCallbackUrl = async (config: any) => { - return ssoCallbackUrl(config, "oidc") + return ssoCallbackUrl(getGlobalDB(), config, "oidc") } async function authInternal(ctx: any, user: any, err = null, info = null) { @@ -70,7 +43,7 @@ async function authInternal(ctx: any, user: any, err = null, info = null) { export const authenticate = async (ctx: any, next: any) => { return passport.authenticate( "local", - async (err: any, user: any, info: any) => { + async (err: any, user: User, info: any) => { await authInternal(ctx, user, err, info) await context.identity.doInUserContext(user, async () => { await events.auth.login("local")