From e25429632b9d9e306f9e3b5d6287936c725d9882 Mon Sep 17 00:00:00 2001 From: adrinr Date: Wed, 29 Mar 2023 15:35:55 +0100 Subject: [PATCH 01/10] Remove users that should not be there anymore when syncGlobalUsers --- packages/server/src/sdk/users/utils.ts | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/server/src/sdk/users/utils.ts b/packages/server/src/sdk/users/utils.ts index 5c369754a1..82a46acfac 100644 --- a/packages/server/src/sdk/users/utils.ts +++ b/packages/server/src/sdk/users/utils.ts @@ -6,25 +6,33 @@ import { InternalTables, } from "../../db/utils" import { isEqual } from "lodash" +import { ContextUser, UserMetadata } from "@budibase/types" -export function combineMetadataAndUser(user: any, metadata: any) { +export function combineMetadataAndUser( + user: ContextUser, + metadata: UserMetadata | UserMetadata[] +) { + const metadataId = generateUserMetadataID(user._id!) + const found = Array.isArray(metadata) + ? metadata.find(doc => doc._id === metadataId) + : metadata // skip users with no access if ( user.roleId == null || user.roleId === rolesCore.BUILTIN_ROLE_IDS.PUBLIC ) { + // If it exists and it should not, we must remove it + if (found) { + return { ...found, _deleted: true } + } return null } delete user._rev - const metadataId = generateUserMetadataID(user._id) const newDoc = { ...user, _id: metadataId, tableId: InternalTables.USER_METADATA, } - const found = Array.isArray(metadata) - ? metadata.find(doc => doc._id === metadataId) - : metadata // copy rev over for the purposes of equality check if (found) { newDoc._rev = found._rev @@ -58,7 +66,7 @@ export async function syncGlobalUsers() { ]) const toWrite = [] for (let user of users) { - const combined = await combineMetadataAndUser(user, metadata) + const combined = combineMetadataAndUser(user, metadata) if (combined) { toWrite.push(combined) } From b368c143087a8d8068212cd2368c001eeb3be023 Mon Sep 17 00:00:00 2001 From: adrinr Date: Wed, 29 Mar 2023 16:18:32 +0100 Subject: [PATCH 02/10] Add unit tests to test new users --- .../server/src/sdk/users/tests/utils.spec.ts | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 packages/server/src/sdk/users/tests/utils.spec.ts diff --git a/packages/server/src/sdk/users/tests/utils.spec.ts b/packages/server/src/sdk/users/tests/utils.spec.ts new file mode 100644 index 0000000000..3178978c46 --- /dev/null +++ b/packages/server/src/sdk/users/tests/utils.spec.ts @@ -0,0 +1,49 @@ +import { db } from "@budibase/backend-core" +import TestConfiguration from "../../../tests/utilities/TestConfiguration" +import { rawUserMetadata, syncGlobalUsers } from "../utils" + +describe("syncGlobalUsers", () => { + const config = new TestConfiguration() + + beforeAll(async () => { + await config.init() + }) + + afterAll(config.end) + + it("the default user is synced", async () => { + await config.doInContext(config.appId, async () => { + await syncGlobalUsers() + + const metadata = await rawUserMetadata() + expect(metadata).toHaveLength(1) + expect(metadata).toEqual([ + expect.objectContaining({ + _id: db.generateUserMetadataID(config.user._id), + }), + ]) + }) + }) + + it("app users are synced", async () => { + await config.doInContext(config.appId, async () => { + const user1 = await config.createUser() + const user2 = await config.createUser() + + await syncGlobalUsers() + + const metadata = await rawUserMetadata() + expect(metadata).toHaveLength(3) + expect(metadata).toContainEqual( + expect.objectContaining({ + _id: db.generateUserMetadataID(user1._id), + }) + ) + expect(metadata).toContainEqual( + expect.objectContaining({ + _id: db.generateUserMetadataID(user2._id), + }) + ) + }) + }) +}) From 8783f0123d51f0b95490151004c59458eb33d2fe Mon Sep 17 00:00:00 2001 From: adrinr Date: Wed, 29 Mar 2023 16:42:31 +0100 Subject: [PATCH 03/10] Add tests for admins vs app users --- .../server/src/sdk/users/tests/utils.spec.ts | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/server/src/sdk/users/tests/utils.spec.ts b/packages/server/src/sdk/users/tests/utils.spec.ts index 3178978c46..8eba6db5b6 100644 --- a/packages/server/src/sdk/users/tests/utils.spec.ts +++ b/packages/server/src/sdk/users/tests/utils.spec.ts @@ -5,7 +5,7 @@ import { rawUserMetadata, syncGlobalUsers } from "../utils" describe("syncGlobalUsers", () => { const config = new TestConfiguration() - beforeAll(async () => { + beforeEach(async () => { await config.init() }) @@ -25,11 +25,10 @@ describe("syncGlobalUsers", () => { }) }) - it("app users are synced", async () => { + it("admin and builders users are synced", async () => { + const user1 = await config.createUser({ admin: true }) + const user2 = await config.createUser({ admin: false, builder: true }) await config.doInContext(config.appId, async () => { - const user1 = await config.createUser() - const user2 = await config.createUser() - await syncGlobalUsers() const metadata = await rawUserMetadata() @@ -46,4 +45,19 @@ describe("syncGlobalUsers", () => { ) }) }) + + it("app users are not synced if not specified", async () => { + const user = await config.createUser({ admin: false, builder: false }) + await config.doInContext(config.appId, async () => { + await syncGlobalUsers() + + const metadata = await rawUserMetadata() + expect(metadata).toHaveLength(1) + expect(metadata).not.toContainEqual( + expect.objectContaining({ + _id: db.generateUserMetadataID(user._id), + }) + ) + }) + }) }) From 4cb6b99982e6f651d4a9380954f323cf1ee96518 Mon Sep 17 00:00:00 2001 From: adrinr Date: Wed, 29 Mar 2023 16:42:55 +0100 Subject: [PATCH 04/10] Types --- packages/server/src/tests/utilities/TestConfiguration.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index cf337c689f..3e0ea331b8 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.ts +++ b/packages/server/src/tests/utilities/TestConfiguration.ts @@ -47,6 +47,7 @@ import { SourceName, Table, SearchFilters, + UserRoles, } from "@budibase/types" type DefaultUserValues = { @@ -277,7 +278,7 @@ class TestConfiguration { email?: string builder?: boolean admin?: boolean - roles?: any + roles?: UserRoles } = {} ) { let { id, firstName, lastName, email, builder, admin, roles } = user From efff31e181c1b4afb48b58c031b8225451d6ad6a Mon Sep 17 00:00:00 2001 From: adrinr Date: Wed, 29 Mar 2023 17:02:56 +0100 Subject: [PATCH 05/10] Test user groups --- .../tests/utilities/structures/index.ts | 1 + .../tests/utilities/structures/userGroups.ts | 10 +++++ .../server/src/sdk/users/tests/utils.spec.ts | 39 ++++++++++++++++++- 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 packages/backend-core/tests/utilities/structures/userGroups.ts diff --git a/packages/backend-core/tests/utilities/structures/index.ts b/packages/backend-core/tests/utilities/structures/index.ts index ca77f476d0..ff2e5b147f 100644 --- a/packages/backend-core/tests/utilities/structures/index.ts +++ b/packages/backend-core/tests/utilities/structures/index.ts @@ -8,4 +8,5 @@ export * as plugins from "./plugins" export * as sso from "./sso" export * as tenant from "./tenants" export * as users from "./users" +export * as userGroups from "./userGroups" export { generator } from "./generator" diff --git a/packages/backend-core/tests/utilities/structures/userGroups.ts b/packages/backend-core/tests/utilities/structures/userGroups.ts new file mode 100644 index 0000000000..4dc870a00a --- /dev/null +++ b/packages/backend-core/tests/utilities/structures/userGroups.ts @@ -0,0 +1,10 @@ +import { UserGroup } from "@budibase/types" +import { generator } from "./generator" + +export function userGroup(): UserGroup { + return { + name: generator.word(), + icon: generator.word(), + color: generator.word(), + } +} diff --git a/packages/server/src/sdk/users/tests/utils.spec.ts b/packages/server/src/sdk/users/tests/utils.spec.ts index 8eba6db5b6..97381a2628 100644 --- a/packages/server/src/sdk/users/tests/utils.spec.ts +++ b/packages/server/src/sdk/users/tests/utils.spec.ts @@ -1,4 +1,7 @@ -import { db } from "@budibase/backend-core" +import { db, roles } from "@budibase/backend-core" +import { structures } from "@budibase/backend-core/tests" +import { sdk as proSdk } from "@budibase/pro" + import TestConfiguration from "../../../tests/utilities/TestConfiguration" import { rawUserMetadata, syncGlobalUsers } from "../utils" @@ -60,4 +63,38 @@ describe("syncGlobalUsers", () => { ) }) }) + + it("app users are removed when removed from the tenant", async () => { + await config.doInTenant(async () => { + const group = await proSdk.groups.save(structures.userGroups.userGroup()) + const user1 = await config.createUser({ admin: false, builder: false }) + const user2 = await config.createUser({ admin: false, builder: false }) + await proSdk.groups.addUsers(group.id, [user1._id, user2._id]) + + await config.doInContext(config.appId, async () => { + await syncGlobalUsers() + expect(await rawUserMetadata()).toHaveLength(1) + + await proSdk.groups.updateGroupApps(group.id, { + appsToAdd: [ + { appId: config.prodAppId!, roleId: roles.BUILTIN_ROLE_IDS.BASIC }, + ], + }) + await syncGlobalUsers() + + const metadata = await rawUserMetadata() + expect(metadata).toHaveLength(3) + expect(metadata).toContainEqual( + expect.objectContaining({ + _id: db.generateUserMetadataID(user1._id), + }) + ) + expect(metadata).toContainEqual( + expect.objectContaining({ + _id: db.generateUserMetadataID(user2._id), + }) + ) + }) + }) + }) }) From 301ad598ada0e94d05e678d56ba74c933e21c51a Mon Sep 17 00:00:00 2001 From: adrinr Date: Wed, 29 Mar 2023 17:05:32 +0100 Subject: [PATCH 06/10] Test user group deletion --- .../server/src/sdk/users/tests/utils.spec.ts | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/packages/server/src/sdk/users/tests/utils.spec.ts b/packages/server/src/sdk/users/tests/utils.spec.ts index 97381a2628..69f1bdf9e6 100644 --- a/packages/server/src/sdk/users/tests/utils.spec.ts +++ b/packages/server/src/sdk/users/tests/utils.spec.ts @@ -64,7 +64,7 @@ describe("syncGlobalUsers", () => { }) }) - it("app users are removed when removed from the tenant", async () => { + it("app users are added when group is assigned to app", async () => { await config.doInTenant(async () => { const group = await proSdk.groups.save(structures.userGroups.userGroup()) const user1 = await config.createUser({ admin: false, builder: false }) @@ -97,4 +97,31 @@ describe("syncGlobalUsers", () => { }) }) }) + + it("app users are removed when user removed from user group", async () => { + await config.doInTenant(async () => { + const group = await proSdk.groups.save(structures.userGroups.userGroup()) + const user1 = await config.createUser({ admin: false, builder: false }) + const user2 = await config.createUser({ admin: false, builder: false }) + await proSdk.groups.updateGroupApps(group.id, { + appsToAdd: [ + { appId: config.prodAppId!, roleId: roles.BUILTIN_ROLE_IDS.BASIC }, + ], + }) + await proSdk.groups.addUsers(group.id, [user1._id, user2._id]) + + await config.doInContext(config.appId, async () => { + await syncGlobalUsers() + expect(await rawUserMetadata()).toHaveLength(3) + + await proSdk.groups.updateGroupApps(group.id, { + appsToRemove: [{ appId: config.prodAppId! }], + }) + await syncGlobalUsers() + + const metadata = await rawUserMetadata() + expect(metadata).toHaveLength(1) + }) + }) + }) }) From b8fe9671b126d6e2f7579c1c70a23fefec8e725e Mon Sep 17 00:00:00 2001 From: adrinr Date: Thu, 30 Mar 2023 15:12:40 +0100 Subject: [PATCH 07/10] Rename test --- packages/server/src/sdk/users/tests/utils.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/sdk/users/tests/utils.spec.ts b/packages/server/src/sdk/users/tests/utils.spec.ts index 69f1bdf9e6..a31f66752e 100644 --- a/packages/server/src/sdk/users/tests/utils.spec.ts +++ b/packages/server/src/sdk/users/tests/utils.spec.ts @@ -98,7 +98,7 @@ describe("syncGlobalUsers", () => { }) }) - it("app users are removed when user removed from user group", async () => { + it("app users are removed when app is removed from user group", async () => { await config.doInTenant(async () => { const group = await proSdk.groups.save(structures.userGroups.userGroup()) const user1 = await config.createUser({ admin: false, builder: false }) From 2dd8b078e577d1ddde66c9a056daeb1391f3b639 Mon Sep 17 00:00:00 2001 From: adrinr Date: Thu, 30 Mar 2023 15:14:16 +0100 Subject: [PATCH 08/10] Test removing users from group --- .../server/src/sdk/users/tests/utils.spec.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/packages/server/src/sdk/users/tests/utils.spec.ts b/packages/server/src/sdk/users/tests/utils.spec.ts index a31f66752e..d5d43a0c04 100644 --- a/packages/server/src/sdk/users/tests/utils.spec.ts +++ b/packages/server/src/sdk/users/tests/utils.spec.ts @@ -124,4 +124,35 @@ describe("syncGlobalUsers", () => { }) }) }) + + it("app users are removed when app is removed from user group", async () => { + await config.doInTenant(async () => { + const group = await proSdk.groups.save(structures.userGroups.userGroup()) + const user1 = await config.createUser({ admin: false, builder: false }) + const user2 = await config.createUser({ admin: false, builder: false }) + await proSdk.groups.updateGroupApps(group.id, { + appsToAdd: [ + { appId: config.prodAppId!, roleId: roles.BUILTIN_ROLE_IDS.BASIC }, + ], + }) + await proSdk.groups.addUsers(group.id, [user1._id, user2._id]) + + await config.doInContext(config.appId, async () => { + await syncGlobalUsers() + expect(await rawUserMetadata()).toHaveLength(3) + + await proSdk.groups.removeUsers(group.id, [user1._id]) + await syncGlobalUsers() + + const metadata = await rawUserMetadata() + expect(metadata).toHaveLength(2) + + expect(metadata).not.toContainEqual( + expect.objectContaining({ + _id: db.generateUserMetadataID(user1._id), + }) + ) + }) + }) + }) }) From 8e8f4ac02d55d3d33bb10601ae96d390b4858333 Mon Sep 17 00:00:00 2001 From: adrinr Date: Thu, 30 Mar 2023 15:16:45 +0100 Subject: [PATCH 09/10] Add extra assertion --- packages/server/src/sdk/users/tests/utils.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/server/src/sdk/users/tests/utils.spec.ts b/packages/server/src/sdk/users/tests/utils.spec.ts index d5d43a0c04..11c2c53643 100644 --- a/packages/server/src/sdk/users/tests/utils.spec.ts +++ b/packages/server/src/sdk/users/tests/utils.spec.ts @@ -32,6 +32,7 @@ describe("syncGlobalUsers", () => { const user1 = await config.createUser({ admin: true }) const user2 = await config.createUser({ admin: false, builder: true }) await config.doInContext(config.appId, async () => { + expect(await rawUserMetadata()).toHaveLength(1) await syncGlobalUsers() const metadata = await rawUserMetadata() From 4ca6982d279ae8ee8ea037a93b78b0c8adeb31d2 Mon Sep 17 00:00:00 2001 From: adrinr Date: Fri, 31 Mar 2023 11:25:51 +0100 Subject: [PATCH 10/10] Prevent double deletions --- packages/server/src/sdk/users/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/sdk/users/utils.ts b/packages/server/src/sdk/users/utils.ts index 82a46acfac..9b9ea04c56 100644 --- a/packages/server/src/sdk/users/utils.ts +++ b/packages/server/src/sdk/users/utils.ts @@ -22,7 +22,7 @@ export function combineMetadataAndUser( user.roleId === rolesCore.BUILTIN_ROLE_IDS.PUBLIC ) { // If it exists and it should not, we must remove it - if (found) { + if (found?._id) { return { ...found, _deleted: true } } return null