From 6510a47c06fcffa826d419bac3b24c040617d695 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 11:10:51 +0200 Subject: [PATCH 01/12] Add basic userDB.save test --- .../backend-core/src/users/test/db.spec.ts | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 packages/backend-core/src/users/test/db.spec.ts diff --git a/packages/backend-core/src/users/test/db.spec.ts b/packages/backend-core/src/users/test/db.spec.ts new file mode 100644 index 0000000000..7e811c9597 --- /dev/null +++ b/packages/backend-core/src/users/test/db.spec.ts @@ -0,0 +1,56 @@ +import { User, UserStatus } from "@budibase/types" +import { DBTestConfiguration, generator, structures } from "../../../tests" +import { UserDB } from "../db" + +const db = UserDB + +const config = new DBTestConfiguration() + +const quotas = { + addUsers: jest + .fn() + .mockImplementation( + (_change: number, _creatorsChange: number, cb?: () => Promise) => + cb && cb() + ), + removeUsers: jest + .fn() + .mockImplementation( + (_change: number, _creatorsChange: number, cb?: () => Promise) => + 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) + }) + + 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(), + }) + }) + }) +}) From 8372632579f3e6d1a8011686fd37837e4b47df41 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 11:14:30 +0200 Subject: [PATCH 02/12] Add extra tests --- packages/backend-core/src/users/test/db.spec.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/backend-core/src/users/test/db.spec.ts b/packages/backend-core/src/users/test/db.spec.ts index 7e811c9597..2820dee7cc 100644 --- a/packages/backend-core/src/users/test/db.spec.ts +++ b/packages/backend-core/src/users/test/db.spec.ts @@ -38,6 +38,7 @@ describe("UserDB", () => { email, tenantId: config.getTenantId(), }) + await config.doInTenant(async () => { const saveUserResponse = await db.save(user) @@ -53,4 +54,18 @@ describe("UserDB", () => { }) }) }) + + 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}'`) + ) + }) }) From 4ddd450a897fe8b2ecc21e57bbbdeddae1307a05 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 11:59:12 +0200 Subject: [PATCH 03/12] More tests --- packages/backend-core/src/users/test/db.spec.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/backend-core/src/users/test/db.spec.ts b/packages/backend-core/src/users/test/db.spec.ts index 2820dee7cc..94219b99b5 100644 --- a/packages/backend-core/src/users/test/db.spec.ts +++ b/packages/backend-core/src/users/test/db.spec.ts @@ -68,4 +68,19 @@ describe("UserDB", () => { 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}'`) + ) + }) }) From 88e054c3660ae472b916a128cc7b73af912fed63 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 12:06:37 +0200 Subject: [PATCH 04/12] Basic update test --- .../backend-core/src/users/test/db.spec.ts | 132 ++++++++++++------ 1 file changed, 87 insertions(+), 45 deletions(-) diff --git a/packages/backend-core/src/users/test/db.spec.ts b/packages/backend-core/src/users/test/db.spec.ts index 94219b99b5..bc72c8d683 100644 --- a/packages/backend-core/src/users/test/db.spec.ts +++ b/packages/backend-core/src/users/test/db.spec.ts @@ -32,55 +32,97 @@ describe("UserDB", () => { db.init(quotas, groups, features) }) - it("creating a new user will persist it", async () => { - const email = generator.email({}) - const user: User = structures.users.user({ - email, - tenantId: config.getTenantId(), + 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}'` + ) + ) + }) }) - await config.doInTenant(async () => { - const saveUserResponse = await db.save(user) + describe("update", () => { + let user: 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(), + 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("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}'`) - ) - }) }) From 3ed9c9a4a5f7c698504b7f087b5d34844c2b271f Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 12:15:56 +0200 Subject: [PATCH 05/12] Test --- packages/backend-core/src/users/test/db.spec.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/backend-core/src/users/test/db.spec.ts b/packages/backend-core/src/users/test/db.spec.ts index bc72c8d683..736422f653 100644 --- a/packages/backend-core/src/users/test/db.spec.ts +++ b/packages/backend-core/src/users/test/db.spec.ts @@ -123,6 +123,14 @@ describe("UserDB", () => { ) }) }) + + 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") + }) + }) }) }) }) From 19e4e8fdb43d508ff77c7e8364cdf439bbbece32 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 12:18:06 +0200 Subject: [PATCH 06/12] Allow updating email --- packages/backend-core/src/users/db.ts | 4 ++-- packages/backend-core/src/users/test/db.spec.ts | 17 +++++++++++++++++ packages/types/src/sdk/user.ts | 1 + 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/backend-core/src/users/db.ts b/packages/backend-core/src/users/db.ts index 37547573bd..b1c7a25c8f 100644 --- a/packages/backend-core/src/users/db.ts +++ b/packages/backend-core/src/users/db.ts @@ -232,8 +232,8 @@ export class UserDB { // 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" + if (email && dbUser.email !== email && !opts.allowChangingEmail) { + throw new Error("Email address cannot be changed") } email = dbUser.email } catch (e: any) { diff --git a/packages/backend-core/src/users/test/db.spec.ts b/packages/backend-core/src/users/test/db.spec.ts index 736422f653..0a103e1064 100644 --- a/packages/backend-core/src/users/test/db.spec.ts +++ b/packages/backend-core/src/users/test/db.spec.ts @@ -131,6 +131,23 @@ describe("UserDB", () => { ).rejects.toThrow("Email address cannot be changed") }) }) + + it("email can be updated if specified", async () => { + await config.doInTenant(async () => { + user.email = generator.email({}) + + await db.save(user, { allowChangingEmail: true }) + + const persistedUser = await db.getUserByEmail(user.email) + expect(persistedUser).toEqual( + expect.objectContaining({ + _id: user._id, + email: user.email, + lastName: user.lastName, + }) + ) + }) + }) }) }) }) diff --git a/packages/types/src/sdk/user.ts b/packages/types/src/sdk/user.ts index 3f6f69d2d1..c0e2f80297 100644 --- a/packages/types/src/sdk/user.ts +++ b/packages/types/src/sdk/user.ts @@ -3,4 +3,5 @@ export interface SaveUserOpts { requirePassword?: boolean currentUserId?: string skipPasswordValidation?: boolean + allowChangingEmail?: boolean } From 8fd2cce093b121286ee3afdbde7dd701e31154a5 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 15:52:48 +0200 Subject: [PATCH 07/12] Remove platform user on remove --- packages/backend-core/src/users/db.ts | 18 +++++--- .../backend-core/src/users/test/db.spec.ts | 43 +++++++++++++++++-- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/packages/backend-core/src/users/db.ts b/packages/backend-core/src/users/db.ts index b1c7a25c8f..4865ebb5bc 100644 --- a/packages/backend-core/src/users/db.ts +++ b/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 + 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!, diff --git a/packages/backend-core/src/users/test/db.spec.ts b/packages/backend-core/src/users/test/db.spec.ts index 0a103e1064..3e29d6673c 100644 --- a/packages/backend-core/src/users/test/db.spec.ts +++ b/packages/backend-core/src/users/test/db.spec.ts @@ -1,6 +1,7 @@ import { User, UserStatus } from "@budibase/types" import { DBTestConfiguration, generator, structures } from "../../../tests" import { UserDB } from "../db" +import { searchExistingEmails } from "../lookup" const db = UserDB @@ -134,20 +135,54 @@ describe("UserDB", () => { it("email can be updated if specified", async () => { await config.doInTenant(async () => { - user.email = generator.email({}) + const newEmail = generator.email({}) - await db.save(user, { allowChangingEmail: true }) + await db.save( + { ...user, email: newEmail }, + { allowChangingEmail: true } + ) - const persistedUser = await db.getUserByEmail(user.email) + const persistedUser = await db.getUserByEmail(newEmail) expect(persistedUser).toEqual( expect.objectContaining({ _id: user._id, - email: user.email, + 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] + ) + }) + }) }) }) }) From 8f086259604a9eb91bf5f7020d43e37d395fa184 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 15:53:11 +0200 Subject: [PATCH 08/12] Allow changing email for scim users --- packages/pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pro b/packages/pro index e8f2c5a147..dbb78c8737 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit e8f2c5a14780e1f61ec3896821ba5f93d486eb72 +Subproject commit dbb78c8737c291871500bc655e30f331f6ffccbf From 3ba3b18c2d91ee2382a398ce07f1a6f3b3a177eb Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 16:57:22 +0200 Subject: [PATCH 09/12] Add scim test --- .../src/api/routes/global/tests/scim.spec.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/worker/src/api/routes/global/tests/scim.spec.ts b/packages/worker/src/api/routes/global/tests/scim.spec.ts index 81dcc4b7c9..1b8039eac1 100644 --- a/packages/worker/src/api/routes/global/tests/scim.spec.ts +++ b/packages/worker/src/api/routes/global/tests/scim.spec.ts @@ -574,6 +574,38 @@ 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) + }) }) describe("DELETE /api/global/scim/v2/users/:id", () => { From 427ccbbb65f7d3d1892ec70c923447a6dd3656d4 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 16:58:56 +0200 Subject: [PATCH 10/12] Test --- packages/worker/src/api/routes/global/tests/scim.spec.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/worker/src/api/routes/global/tests/scim.spec.ts b/packages/worker/src/api/routes/global/tests/scim.spec.ts index 1b8039eac1..7f23bc9460 100644 --- a/packages/worker/src/api/routes/global/tests/scim.spec.ts +++ b/packages/worker/src/api/routes/global/tests/scim.spec.ts @@ -577,7 +577,6 @@ describe("scim", () => { 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: [ @@ -605,6 +604,10 @@ describe("scim", () => { 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 }) + ) }) }) From 7da589a9d66df33df0ad076021e63ee157e8f23a Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 17:24:23 +0200 Subject: [PATCH 11/12] Fix --- .github/workflows/budibase_ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index 86fd4f6799..af3e34c82d 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.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" From 91fa90eeb79dc5c1bc1dd52063e9ffd748973aac Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 3 Jul 2024 17:30:35 +0200 Subject: [PATCH 12/12] Use base_commit_excluding_merges --- .github/workflows/budibase_ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/budibase_ci.yml b/.github/workflows/budibase_ci.yml index af3e34c82d..288a0462e7 100644 --- a/.github/workflows/budibase_ci.yml +++ b/.github/workflows/budibase_ci.yml @@ -231,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