Merge branch 'master' of github.com:budibase/budibase into budi-8417-check-error-responses-in-middleware-for-environment

.github/workflows/budibase_ci.yml

@@ -214,6 +214,7 @@ jobs:
             echo "pro_commit=$pro_commit"
             echo "pro_commit=$pro_commit" >> "$GITHUB_OUTPUT"
             echo "base_commit=$base_commit"
+            echo "base_commit=$base_commit" >> "$GITHUB_OUTPUT"
 
             base_commit_excluding_merges=$(git log --no-merges -n 1 --format=format:%H $base_commit)
             echo "base_commit_excluding_merges=$base_commit_excluding_merges"
@@ -230,7 +231,7 @@ jobs:
           base_commit_excluding_merges='${{ steps.get_pro_commits.outputs.base_commit_excluding_merges }}'
           pro_commit='${{ steps.get_pro_commits.outputs.pro_commit }}'
 
-          any_commit=$(git log --no-merges $base_commit...$pro_commit)
+          any_commit=$(git log --no-merges $base_commit_excluding_merges...$pro_commit)
 
           if [ -n "$any_commit" ]; then
             echo $any_commit

packages/backend-core/src/users/db.ts

@@ -221,7 +221,7 @@ export class UserDB {
     const tenantId = getTenantId()
     const db = getGlobalDB()
 
-    let { email, _id, userGroups = [], roles } = user
+    const { email, _id, userGroups = [], roles } = user
 
     if (!email && !_id) {
       throw new Error("_id or email is required")
@@ -231,11 +231,10 @@ export class UserDB {
     if (_id) {
       // try to get existing user from db
       try {
-        dbUser = (await db.get(_id)) as User
-        if (email && dbUser.email !== email) {
-          throw "Email address cannot be changed"
+        dbUser = await usersCore.getById(_id)
+        if (email && dbUser.email !== email && !opts.allowChangingEmail) {
+          throw new Error("Email address cannot be changed")
         }
-        email = dbUser.email
       } catch (e: any) {
         if (e.status === 404) {
           // do nothing, save this new user with the id specified - required for SSO auth
@@ -271,13 +270,13 @@ export class UserDB {
 
       // make sure we set the _id field for a new user
       // Also if this is a new user, associate groups with them
-      let groupPromises = []
+      const groupPromises = []
       if (!_id) {
-        _id = builtUser._id!
-
         if (userGroups.length > 0) {
           for (let groupId of userGroups) {
-            groupPromises.push(UserDB.groups.addUsers(groupId, [_id!]))
+            groupPromises.push(
+              UserDB.groups.addUsers(groupId, [builtUser._id!])
+            )
           }
         }
       }
@@ -288,6 +287,11 @@ export class UserDB {
         builtUser._rev = response.rev
 
         await eventHelpers.handleSaveEvents(builtUser, dbUser)
+        if (dbUser && builtUser.email !== dbUser.email) {
+          // Remove the plaform email reference if the email changed
+          await platform.users.removeUser({ email: dbUser.email } as User)
+        }
+
         await platform.users.addUser(
           tenantId,
           builtUser._id!,

packages/backend-core/src/users/test/db.spec.ts

@@ -0,0 +1,188 @@
+import { User, UserStatus } from "@budibase/types"
+import { DBTestConfiguration, generator, structures } from "../../../tests"
+import { UserDB } from "../db"
+import { searchExistingEmails } from "../lookup"
+
+const db = UserDB
+
+const config = new DBTestConfiguration()
+
+const quotas = {
+  addUsers: jest
+    .fn()
+    .mockImplementation(
+      (_change: number, _creatorsChange: number, cb?: () => Promise<any>) =>
+        cb && cb()
+    ),
+  removeUsers: jest
+    .fn()
+    .mockImplementation(
+      (_change: number, _creatorsChange: number, cb?: () => Promise<any>) =>
+        cb && cb()
+    ),
+}
+const groups = {
+  addUsers: jest.fn(),
+  getBulk: jest.fn(),
+  getGroupBuilderAppIds: jest.fn(),
+}
+const features = { isSSOEnforced: jest.fn(), isAppBuildersEnabled: jest.fn() }
+
+describe("UserDB", () => {
+  beforeAll(() => {
+    db.init(quotas, groups, features)
+  })
+
+  describe("save", () => {
+    describe("create", () => {
+      it("creating a new user will persist it", async () => {
+        const email = generator.email({})
+        const user: User = structures.users.user({
+          email,
+          tenantId: config.getTenantId(),
+        })
+
+        await config.doInTenant(async () => {
+          const saveUserResponse = await db.save(user)
+
+          const persistedUser = await db.getUserByEmail(email)
+          expect(persistedUser).toEqual({
+            ...user,
+            _id: saveUserResponse._id,
+            _rev: expect.stringMatching(/^1-\w+/),
+            password: expect.not.stringMatching(user.password!),
+            status: UserStatus.ACTIVE,
+            createdAt: Date.now(),
+            updatedAt: new Date().toISOString(),
+          })
+        })
+      })
+
+      it("the same email cannot be used twice in the same tenant", async () => {
+        const email = generator.email({})
+        const user: User = structures.users.user({
+          email,
+          tenantId: config.getTenantId(),
+        })
+
+        await config.doInTenant(() => db.save(user))
+
+        await config.doInTenant(() =>
+          expect(db.save(user)).rejects.toThrow(
+            `Email already in use: '${email}'`
+          )
+        )
+      })
+
+      it("the same email cannot be used twice in different tenants", async () => {
+        const email = generator.email({})
+        const user: User = structures.users.user({
+          email,
+          tenantId: config.getTenantId(),
+        })
+
+        await config.doInTenant(() => db.save(user))
+
+        config.newTenant()
+        await config.doInTenant(() =>
+          expect(db.save(user)).rejects.toThrow(
+            `Email already in use: '${email}'`
+          )
+        )
+      })
+    })
+
+    describe("update", () => {
+      let user: User
+
+      beforeEach(async () => {
+        user = await config.doInTenant(() =>
+          db.save(
+            structures.users.user({
+              email: generator.email({}),
+              tenantId: config.getTenantId(),
+            })
+          )
+        )
+      })
+
+      it("can update user properties", async () => {
+        await config.doInTenant(async () => {
+          const updatedName = generator.first()
+          user.firstName = updatedName
+
+          await db.save(user)
+
+          const persistedUser = await db.getUserByEmail(user.email)
+          expect(persistedUser).toEqual(
+            expect.objectContaining({
+              _id: user._id,
+              email: user.email,
+              firstName: updatedName,
+              lastName: user.lastName,
+            })
+          )
+        })
+      })
+
+      it("email cannot be updated by default", async () => {
+        await config.doInTenant(async () => {
+          await expect(
+            db.save({ ...user, email: generator.email({}) })
+          ).rejects.toThrow("Email address cannot be changed")
+        })
+      })
+
+      it("email can be updated if specified", async () => {
+        await config.doInTenant(async () => {
+          const newEmail = generator.email({})
+
+          await db.save(
+            { ...user, email: newEmail },
+            { allowChangingEmail: true }
+          )
+
+          const persistedUser = await db.getUserByEmail(newEmail)
+          expect(persistedUser).toEqual(
+            expect.objectContaining({
+              _id: user._id,
+              email: newEmail,
+              lastName: user.lastName,
+              _rev: expect.stringMatching(/^2-\w+/),
+            })
+          )
+        })
+      })
+
+      it("updating emails frees previous emails", async () => {
+        await config.doInTenant(async () => {
+          const previousEmail = user.email
+          const newEmail = generator.email({})
+          expect(await searchExistingEmails([previousEmail, newEmail])).toEqual(
+            [previousEmail]
+          )
+
+          await db.save(
+            { ...user, email: newEmail },
+            { allowChangingEmail: true }
+          )
+
+          expect(await searchExistingEmails([previousEmail, newEmail])).toEqual(
+            [newEmail]
+          )
+
+          await db.save(
+            structures.users.user({
+              email: previousEmail,
+              tenantId: config.getTenantId(),
+            })
+          )
+
+          expect(await searchExistingEmails([previousEmail, newEmail])).toEqual(
+            [previousEmail, newEmail]
+          )
+        })
+      })
+    })
+  })
+})

packages/pro

@@ -1 +1 @@
-Subproject commit e8f2c5a14780e1f61ec3896821ba5f93d486eb72
+Subproject commit dbb78c8737c291871500bc655e30f331f6ffccbf

packages/server/__mocks__/@sendgrid/mail.ts

@@ -1,23 +0,0 @@
-// eslint-disable-next-line @typescript-eslint/no-unused-vars
-module SendgridMock {
-  class Email {
-    constructor() {
-      // @ts-ignore
-      this.apiKey = null
-    }
-
-    setApiKey(apiKey: any) {
-      // @ts-ignore
-      this.apiKey = apiKey
-    }
-
-    async send(msg: any) {
-      if (msg.to === "invalid@example.com") {
-        throw "Invalid"
-      }
-      return msg
-    }
-  }
-
-  module.exports = new Email()
-}

packages/server/src/automations/tests/openai.spec.ts

@@ -43,9 +43,7 @@ describe("test the openai action", () => {
 
   it("should present the correct error message when the OPENAI_API_KEY variable isn't set", async () => {
     await config.withCoreEnv({ OPENAI_API_KEY: "" }, async () => {
-      let res = await runStep("OPENAI", {
-        prompt: OPENAI_PROMPT,
-      })
+      let res = await runStep("OPENAI", { prompt: OPENAI_PROMPT })
       expect(res.response).toEqual(
         "OpenAI API Key not configured - please add the OPENAI_API_KEY environment variable."
       )
@@ -54,17 +52,13 @@ describe("test the openai action", () => {
   })
 
   it("should be able to receive a response from ChatGPT given a prompt", async () => {
-    const res = await runStep("OPENAI", {
-      prompt: OPENAI_PROMPT,
-    })
+    const res = await runStep("OPENAI", { prompt: OPENAI_PROMPT })
     expect(res.response).toEqual("This is a test")
     expect(res.success).toBeTruthy()
   })
 
   it("should present the correct error message when a prompt is not provided", async () => {
-    const res = await runStep("OPENAI", {
-      prompt: null,
-    })
+    const res = await runStep("OPENAI", { prompt: null })
     expect(res.response).toEqual(
       "Budibase OpenAI Automation Failed: No prompt supplied"
     )

packages/server/src/environment.ts

@@ -75,7 +75,6 @@ const environment = {
   AUTOMATION_MAX_ITERATIONS:
     parseIntSafe(process.env.AUTOMATION_MAX_ITERATIONS) ||
     DEFAULTS.AUTOMATION_MAX_ITERATIONS,
-  SENDGRID_API_KEY: process.env.SENDGRID_API_KEY,
   DYNAMO_ENDPOINT: process.env.DYNAMO_ENDPOINT,
   QUERY_THREAD_TIMEOUT: QUERY_THREAD_TIMEOUT,
   AUTOMATION_THREAD_TIMEOUT:

packages/server/src/tests/utilities/TestConfiguration.ts

@@ -290,7 +290,7 @@ export default class TestConfiguration {
    * that can be called to reset the environment variables to their original values.
    */
   setCoreEnv(newEnvVars: Partial<typeof coreEnv>): () => void {
-    const oldEnv = cloneDeep(env)
+    const oldEnv = cloneDeep(coreEnv)
 
     let key: keyof typeof newEnvVars
     for (key in newEnvVars) {

packages/types/src/sdk/user.ts

@@ -3,4 +3,5 @@ export interface SaveUserOpts {
   requirePassword?: boolean
   currentUserId?: string
   skipPasswordValidation?: boolean
+  allowChangingEmail?: boolean
 }

packages/worker/src/api/routes/global/tests/scim.spec.ts

@@ -574,6 +574,41 @@ describe("scim", () => {
 
         expect(events.user.updated).toHaveBeenCalledTimes(1)
       })
+
+      it("an existing user's email can be updated", async () => {
+        const newEmail = structures.generator.email()
+        const body: ScimUpdateRequest = {
+          schemas: ["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
+          Operations: [
+            {
+              op: "Replace",
+              path: 'emails[type eq "work"].value',
+              value: newEmail,
+            },
+          ],
+        }
+
+        const response = await patchScimUser({ id: user.id, body })
+
+        const expectedScimUser: ScimUserResponse = {
+          ...user,
+          emails: [
+            {
+              value: newEmail,
+              type: "work",
+              primary: true,
+            },
+          ],
+        }
+        expect(response).toEqual(expectedScimUser)
+
+        const persistedUser = await config.api.scimUsersAPI.find(user.id)
+        expect(persistedUser).toEqual(expectedScimUser)
+
+        expect((await config.api.users.getUser(user.id)).body).toEqual(
+          expect.objectContaining({ _id: user.id, email: newEmail })
+        )
+      })
     })
 
     describe("DELETE /api/global/scim/v2/users/:id", () => {