From f10297953f6c790dc53fda3c2bd480701aa05647 Mon Sep 17 00:00:00 2001 From: Rory Powell Date: Wed, 6 Oct 2021 15:15:46 +0100 Subject: [PATCH 1/2] Don't let user exist in multiple tenants when using custom sso --- .../middleware/passport/third-party-common.js | 9 +- packages/auth/src/tenancy/tenancy.js | 10 ++ packages/auth/src/utils.js | 110 ++++++++++++++++- .../src/api/controllers/global/users.js | 111 +----------------- 4 files changed, 128 insertions(+), 112 deletions(-) diff --git a/packages/auth/src/middleware/passport/third-party-common.js b/packages/auth/src/middleware/passport/third-party-common.js index c25aa3e0b0..03a9cb31f1 100644 --- a/packages/auth/src/middleware/passport/third-party-common.js +++ b/packages/auth/src/middleware/passport/third-party-common.js @@ -1,6 +1,7 @@ const env = require("../../environment") const jwt = require("jsonwebtoken") const { generateGlobalUserID } = require("../../db/utils") +const { saveUser } = require("../../utils") const { authError } = require("./utils") const { newid } = require("../../hashing") const { createASession } = require("../../security/sessions") @@ -71,7 +72,13 @@ exports.authenticateThirdParty = async function ( dbUser = await syncUser(dbUser, thirdPartyUser) // create or sync the user - const response = await db.put(dbUser) + let response + try { + response = await saveUser(dbUser, getTenantId(), false, false) + } catch (err) { + return authError(done, err) + } + dbUser._rev = response.rev // authenticate diff --git a/packages/auth/src/tenancy/tenancy.js b/packages/auth/src/tenancy/tenancy.js index 668bc010ba..67dbfd5619 100644 --- a/packages/auth/src/tenancy/tenancy.js +++ b/packages/auth/src/tenancy/tenancy.js @@ -107,3 +107,13 @@ exports.lookupTenantId = async userId => { } return tenantId } + +// lookup, could be email or userId, either will return a doc +exports.getTenantUser = async identifier => { + const db = getDB(PLATFORM_INFO_DB) + try { + return await db.get(identifier) + } catch (err) { + return null + } +} diff --git a/packages/auth/src/utils.js b/packages/auth/src/utils.js index 93b483c6be..f509a626c1 100644 --- a/packages/auth/src/utils.js +++ b/packages/auth/src/utils.js @@ -1,10 +1,24 @@ -const { DocumentTypes, SEPARATOR, ViewNames } = require("./db/utils") +const { + DocumentTypes, + SEPARATOR, + ViewNames, + generateGlobalUserID, +} = require("./db/utils") const jwt = require("jsonwebtoken") const { options } = require("./middleware/passport/jwt") const { createUserEmailView } = require("./db/views") -const { Headers } = require("./constants") -const { getGlobalDB } = require("./tenancy") +const { Headers, UserStatus } = require("./constants") +const { + getGlobalDB, + updateTenantId, + getTenantUser, + tryAddTenant, +} = require("./tenancy") const environment = require("./environment") +const accounts = require("./cloud/accounts") +const { hash } = require("./hashing") +const userCache = require("./cache/user") +const env = require("./environment") const APP_PREFIX = DocumentTypes.APP + SEPARATOR @@ -131,3 +145,93 @@ exports.getGlobalUserByEmail = async email => { } } } + +exports.saveUser = async ( + user, + tenantId, + hashPassword = true, + requirePassword = true +) => { + if (!tenantId) { + throw "No tenancy specified." + } + // need to set the context for this request, as specified + updateTenantId(tenantId) + // specify the tenancy incase we're making a new admin user (public) + const db = getGlobalDB(tenantId) + let { email, password, _id } = user + // make sure another user isn't using the same email + let dbUser + if (email) { + // check budibase users inside the tenant + dbUser = await exports.getGlobalUserByEmail(email) + if (dbUser != null && (dbUser._id !== _id || Array.isArray(dbUser))) { + throw `Email address ${email} already in use.` + } + + // check budibase users in other tenants + if (env.MULTI_TENANCY) { + dbUser = await getTenantUser(email) + if (dbUser != null && dbUser.tenantId !== tenantId) { + throw `Email address ${email} already in use.` + } + } + + // check root account users in account portal + if (!env.SELF_HOSTED && !env.DISABLE_ACCOUNT_PORTAL) { + const account = await accounts.getAccount(email) + if (account && account.verified && account.tenantId !== tenantId) { + throw `Email address ${email} already in use.` + } + } + } else { + dbUser = await db.get(_id) + } + + // get the password, make sure one is defined + let hashedPassword + if (password) { + hashedPassword = hashPassword ? await hash(password) : password + } else if (dbUser) { + hashedPassword = dbUser.password + } else if (requirePassword) { + throw "Password must be specified." + } + + _id = _id || generateGlobalUserID() + user = { + createdAt: Date.now(), + ...dbUser, + ...user, + _id, + password: hashedPassword, + tenantId, + } + // make sure the roles object is always present + if (!user.roles) { + user.roles = {} + } + // add the active status to a user if its not provided + if (user.status == null) { + user.status = UserStatus.ACTIVE + } + try { + const response = await db.put({ + password: hashedPassword, + ...user, + }) + await tryAddTenant(tenantId, _id, email) + await userCache.invalidateUser(response.id) + return { + _id: response.id, + _rev: response.rev, + email, + } + } catch (err) { + if (err.status === 409) { + throw "User exists already" + } else { + throw err + } + } +} diff --git a/packages/worker/src/api/controllers/global/users.js b/packages/worker/src/api/controllers/global/users.js index 732ae0a152..38a814f465 100644 --- a/packages/worker/src/api/controllers/global/users.js +++ b/packages/worker/src/api/controllers/global/users.js @@ -1,29 +1,24 @@ const { - generateGlobalUserID, getGlobalUserParams, StaticDatabases, generateNewUsageQuotaDoc, } = require("@budibase/auth/db") -const { hash, getGlobalUserByEmail } = require("@budibase/auth").utils -const { UserStatus, EmailTemplatePurpose } = require("../../../constants") +const { hash, getGlobalUserByEmail, saveUser } = require("@budibase/auth").utils +const { EmailTemplatePurpose } = require("../../../constants") const { checkInviteCode } = require("../../../utilities/redis") const { sendEmail } = require("../../../utilities/email") const { user: userCache } = require("@budibase/auth/cache") const { invalidateSessions } = require("@budibase/auth/sessions") -const CouchDB = require("../../../db") const accounts = require("@budibase/auth/accounts") const { getGlobalDB, getTenantId, + getTenantUser, doesTenantExist, - tryAddTenant, - updateTenantId, } = require("@budibase/auth/tenancy") const { removeUserFromInfoDB } = require("@budibase/auth/deprovision") const env = require("../../../environment") -const PLATFORM_INFO_DB = StaticDatabases.PLATFORM_INFO.name - async function allUsers() { const db = getGlobalDB() const response = await db.allDocs( @@ -34,96 +29,6 @@ async function allUsers() { return response.rows.map(row => row.doc) } -async function saveUser( - user, - tenantId, - hashPassword = true, - requirePassword = true -) { - if (!tenantId) { - throw "No tenancy specified." - } - // need to set the context for this request, as specified - updateTenantId(tenantId) - // specify the tenancy incase we're making a new admin user (public) - const db = getGlobalDB(tenantId) - let { email, password, _id } = user - // make sure another user isn't using the same email - let dbUser - if (email) { - // check budibase users inside the tenant - dbUser = await getGlobalUserByEmail(email) - if (dbUser != null && (dbUser._id !== _id || Array.isArray(dbUser))) { - throw `Email address ${email} already in use.` - } - - // check budibase users in other tenants - if (env.MULTI_TENANCY) { - dbUser = await getTenantUser(email) - if (dbUser != null && dbUser.tenantId !== tenantId) { - throw `Email address ${email} already in use.` - } - } - - // check root account users in account portal - if (!env.SELF_HOSTED && !env.DISABLE_ACCOUNT_PORTAL) { - const account = await accounts.getAccount(email) - if (account && account.verified && account.tenantId !== tenantId) { - throw `Email address ${email} already in use.` - } - } - } else { - dbUser = await db.get(_id) - } - - // get the password, make sure one is defined - let hashedPassword - if (password) { - hashedPassword = hashPassword ? await hash(password) : password - } else if (dbUser) { - hashedPassword = dbUser.password - } else if (requirePassword) { - throw "Password must be specified." - } - - _id = _id || generateGlobalUserID() - user = { - createdAt: Date.now(), - ...dbUser, - ...user, - _id, - password: hashedPassword, - tenantId, - } - // make sure the roles object is always present - if (!user.roles) { - user.roles = {} - } - // add the active status to a user if its not provided - if (user.status == null) { - user.status = UserStatus.ACTIVE - } - try { - const response = await db.put({ - password: hashedPassword, - ...user, - }) - await tryAddTenant(tenantId, _id, email) - await userCache.invalidateUser(response.id) - return { - _id: response.id, - _rev: response.rev, - email, - } - } catch (err) { - if (err.status === 409) { - throw "User exists already" - } else { - throw err - } - } -} - exports.save = async ctx => { try { ctx.body = await saveUser(ctx.request.body, getTenantId()) @@ -310,16 +215,6 @@ exports.find = async ctx => { ctx.body = user } -// lookup, could be email or userId, either will return a doc -const getTenantUser = async identifier => { - const db = new CouchDB(PLATFORM_INFO_DB) - try { - return await db.get(identifier) - } catch (err) { - return null - } -} - exports.tenantUserLookup = async ctx => { const id = ctx.params.id const user = await getTenantUser(id) From 13a2770c2604c2df4988d034b45f9b7d7d7e6dbf Mon Sep 17 00:00:00 2001 From: Rory Powell Date: Wed, 6 Oct 2021 15:57:13 +0100 Subject: [PATCH 2/2] Fix tests --- .../passport/tests/third-party-common.spec.js | 14 +++++++++----- .../src/middleware/passport/third-party-common.js | 5 +++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/auth/src/middleware/passport/tests/third-party-common.spec.js b/packages/auth/src/middleware/passport/tests/third-party-common.spec.js index 1ace65ba40..e2ad9a9300 100644 --- a/packages/auth/src/middleware/passport/tests/third-party-common.spec.js +++ b/packages/auth/src/middleware/passport/tests/third-party-common.spec.js @@ -20,6 +20,10 @@ const getErrorMessage = () => { return done.mock.calls[0][2].message } +const saveUser = async (user) => { + return await db.put(user) +} + describe("third party common", () => { describe("authenticateThirdParty", () => { let thirdPartyUser @@ -36,7 +40,7 @@ describe("third party common", () => { describe("validation", () => { const testValidation = async (message) => { - await authenticateThirdParty(thirdPartyUser, false, done) + await authenticateThirdParty(thirdPartyUser, false, done, saveUser) expect(done.mock.calls.length).toBe(1) expect(getErrorMessage()).toContain(message) } @@ -78,7 +82,7 @@ describe("third party common", () => { describe("when the user doesn't exist", () => { describe("when a local account is required", () => { it("returns an error message", async () => { - await authenticateThirdParty(thirdPartyUser, true, done) + await authenticateThirdParty(thirdPartyUser, true, done, saveUser) expect(done.mock.calls.length).toBe(1) expect(getErrorMessage()).toContain("Email does not yet exist. You must set up your local budibase account first.") }) @@ -86,7 +90,7 @@ describe("third party common", () => { describe("when a local account isn't required", () => { it("creates and authenticates the user", async () => { - await authenticateThirdParty(thirdPartyUser, false, done) + await authenticateThirdParty(thirdPartyUser, false, done, saveUser) const user = expectUserIsAuthenticated() expectUserIsSynced(user, thirdPartyUser) expect(user.roles).toStrictEqual({}) @@ -123,7 +127,7 @@ describe("third party common", () => { }) it("syncs and authenticates the user", async () => { - await authenticateThirdParty(thirdPartyUser, true, done) + await authenticateThirdParty(thirdPartyUser, true, done, saveUser) const user = expectUserIsAuthenticated() expectUserIsSynced(user, thirdPartyUser) @@ -139,7 +143,7 @@ describe("third party common", () => { }) it("syncs and authenticates the user", async () => { - await authenticateThirdParty(thirdPartyUser, true, done) + await authenticateThirdParty(thirdPartyUser, true, done, saveUser) const user = expectUserIsAuthenticated() expectUserIsSynced(user, thirdPartyUser) diff --git a/packages/auth/src/middleware/passport/third-party-common.js b/packages/auth/src/middleware/passport/third-party-common.js index 03a9cb31f1..54a5504712 100644 --- a/packages/auth/src/middleware/passport/third-party-common.js +++ b/packages/auth/src/middleware/passport/third-party-common.js @@ -15,7 +15,8 @@ const fetch = require("node-fetch") exports.authenticateThirdParty = async function ( thirdPartyUser, requireLocalAccount = true, - done + done, + saveUserFn = saveUser ) { if (!thirdPartyUser.provider) { return authError(done, "third party user provider required") @@ -74,7 +75,7 @@ exports.authenticateThirdParty = async function ( // create or sync the user let response try { - response = await saveUser(dbUser, getTenantId(), false, false) + response = await saveUserFn(dbUser, getTenantId(), false, false) } catch (err) { return authError(done, err) }