From 5eecb6e686f21b99d7534e8325400d575907a86d Mon Sep 17 00:00:00 2001 From: Rory Powell Date: Tue, 4 Apr 2023 10:14:20 +0100 Subject: [PATCH] Remove loop for get account during user bulk import (#10203) --- .../src/api/routes/global/tests/auth.spec.ts | 10 ++-- .../worker/src/sdk/users/tests/users.spec.ts | 52 +++++++++++++------ packages/worker/src/sdk/users/users.ts | 19 ++++--- 3 files changed, 52 insertions(+), 29 deletions(-) diff --git a/packages/worker/src/api/routes/global/tests/auth.spec.ts b/packages/worker/src/api/routes/global/tests/auth.spec.ts index 6c133df652..5e62b2123f 100644 --- a/packages/worker/src/api/routes/global/tests/auth.spec.ts +++ b/packages/worker/src/api/routes/global/tests/auth.spec.ts @@ -126,9 +126,8 @@ describe("/api/global/auth", () => { it("should prevent user from logging in", async () => { user = await config.createUser() const account = structures.accounts.ssoAccount() as CloudAccount - mocks.accounts.getAccount.mockReturnValueOnce( - Promise.resolve(account) - ) + account.email = user.email + mocks.accounts.getAccountByTenantId.mockResolvedValueOnce(account) await testSSOUser() }) @@ -186,9 +185,8 @@ describe("/api/global/auth", () => { it("should prevent user from generating password reset email", async () => { user = await config.createUser(structures.users.user()) const account = structures.accounts.ssoAccount() as CloudAccount - mocks.accounts.getAccount.mockReturnValueOnce( - Promise.resolve(account) - ) + account.email = user.email + mocks.accounts.getAccountByTenantId.mockResolvedValueOnce(account) await testSSOUser() }) diff --git a/packages/worker/src/sdk/users/tests/users.spec.ts b/packages/worker/src/sdk/users/tests/users.spec.ts index 77f02eec7a..a24f074512 100644 --- a/packages/worker/src/sdk/users/tests/users.spec.ts +++ b/packages/worker/src/sdk/users/tests/users.spec.ts @@ -1,6 +1,6 @@ import { structures } from "../../../tests" import { mocks } from "@budibase/backend-core/tests" -import { env } from "@budibase/backend-core" +import { env, context } from "@budibase/backend-core" import * as users from "../users" import { CloudAccount } from "@budibase/types" import { isPreventPasswordActions } from "../users" @@ -16,32 +16,50 @@ describe("users", () => { describe("isPreventPasswordActions", () => { it("returns false for non sso user", async () => { - const user = structures.users.user() - const result = await users.isPreventPasswordActions(user) - expect(result).toBe(false) + await context.doInTenant(structures.tenant.id(), async () => { + const user = structures.users.user() + const result = await users.isPreventPasswordActions(user) + expect(result).toBe(false) + }) }) it("returns true for sso account user", async () => { - const user = structures.users.user() - mocks.accounts.getAccount.mockReturnValue( - Promise.resolve(structures.accounts.ssoAccount() as CloudAccount) - ) - const result = await users.isPreventPasswordActions(user) - expect(result).toBe(true) + await context.doInTenant(structures.tenant.id(), async () => { + const user = structures.users.user() + const account = structures.accounts.ssoAccount() as CloudAccount + account.email = user.email + mocks.accounts.getAccountByTenantId.mockResolvedValueOnce(account) + const result = await users.isPreventPasswordActions(user) + expect(result).toBe(true) + }) + }) + + it("returns false when account doesn't match user email", async () => { + await context.doInTenant(structures.tenant.id(), async () => { + const user = structures.users.user() + const account = structures.accounts.ssoAccount() as CloudAccount + mocks.accounts.getAccountByTenantId.mockResolvedValueOnce(account) + const result = await users.isPreventPasswordActions(user) + expect(result).toBe(false) + }) }) it("returns true for sso user", async () => { - const user = structures.users.ssoUser() - const result = await users.isPreventPasswordActions(user) - expect(result).toBe(true) + await context.doInTenant(structures.tenant.id(), async () => { + const user = structures.users.ssoUser() + const result = await users.isPreventPasswordActions(user) + expect(result).toBe(true) + }) }) describe("enforced sso", () => { it("returns true for all users when sso is enforced", async () => { - const user = structures.users.user() - pro.features.isSSOEnforced.mockReturnValue(Promise.resolve(true)) - const result = await users.isPreventPasswordActions(user) - expect(result).toBe(true) + await context.doInTenant(structures.tenant.id(), async () => { + const user = structures.users.user() + pro.features.isSSOEnforced.mockResolvedValueOnce(true) + const result = await users.isPreventPasswordActions(user) + expect(result).toBe(true) + }) }) }) diff --git a/packages/worker/src/sdk/users/users.ts b/packages/worker/src/sdk/users/users.ts index 135128d816..c520ea2fd1 100644 --- a/packages/worker/src/sdk/users/users.ts +++ b/packages/worker/src/sdk/users/users.ts @@ -31,6 +31,7 @@ import { SearchUsersRequest, User, SaveUserOpts, + Account, } from "@budibase/types" import { sendEmail } from "../../utilities/email" import { EmailTemplatePurpose } from "../../constants" @@ -127,7 +128,8 @@ const buildUser = async ( requirePassword: true, }, tenantId: string, - dbUser?: any + dbUser?: any, + account?: Account ): Promise => { let { password, _id } = user @@ -138,7 +140,7 @@ const buildUser = async ( let hashedPassword if (password) { - if (await isPreventPasswordActions(user)) { + if (await isPreventPasswordActions(user, account)) { throw new HTTPError("Password change is disabled for this user", 400) } hashedPassword = opts.hashPassword ? await utils.hash(password) : password @@ -209,7 +211,7 @@ const validateUniqueUser = async (email: string, tenantId: string) => { } } -export async function isPreventPasswordActions(user: User) { +export async function isPreventPasswordActions(user: User, account?: Account) { // when in maintenance mode we allow sso users with the admin role // to perform any password action - this prevents lockout if (coreEnv.ENABLE_SSO_MAINTENANCE_MODE && user.admin?.global) { @@ -227,8 +229,10 @@ export async function isPreventPasswordActions(user: User) { } // Check account sso - const account = await accountSdk.api.getAccount(user.email) - return !!(account && isSSOAccount(account)) + if (!account) { + account = await accountSdk.api.getAccountByTenantId(tenancy.getTenantId()) + } + return !!(account && account.email === user.email && isSSOAccount(account)) } export const save = async ( @@ -439,6 +443,7 @@ export const bulkCreate = async ( newUsers.push(newUser) } + const account = await accountSdk.api.getAccountByTenantId(tenantId) // create the promises array that will be called by bulkDocs newUsers.forEach((user: any) => { usersToSave.push( @@ -448,7 +453,9 @@ export const bulkCreate = async ( hashPassword: true, requirePassword: user.requirePassword, }, - tenantId + tenantId, + undefined, // no dbUser + account ) ) })