From 39689d27f6d8f84f4ee3414c071f25f5577be673 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 21 Sep 2022 18:05:45 +0100 Subject: [PATCH] Updating user API to user a single bulk endpoint rather than case sensitive named endpoints. --- packages/frontend-core/src/api/user.js | 17 +++-- packages/types/src/api/web/user.ts | 33 +++++---- .../src/api/controllers/global/users.ts | 68 ++++++++++--------- .../src/api/routes/global/tests/users.spec.ts | 65 +++++++++--------- .../worker/src/api/routes/global/users.js | 7 +- .../worker/src/api/routes/validation/users.ts | 15 ++-- packages/worker/src/sdk/users/users.ts | 9 ++- packages/worker/src/tests/api/users.ts | 20 +++--- 8 files changed, 124 insertions(+), 110 deletions(-) diff --git a/packages/frontend-core/src/api/user.js b/packages/frontend-core/src/api/user.js index accf1f430b..6ba801da34 100644 --- a/packages/frontend-core/src/api/user.js +++ b/packages/frontend-core/src/api/user.js @@ -86,13 +86,16 @@ export const buildUserEndpoints = API => ({ /** * Creates multiple users. * @param users the array of user objects to create + * @param groups the array of group ids to add all users to */ createUsers: async ({ users, groups }) => { return await API.post({ - url: "/api/global/users/bulkCreate", + url: "/api/global/users/bulk", body: { - users, - groups, + create: { + users, + groups, + }, }, }) }, @@ -109,13 +112,15 @@ export const buildUserEndpoints = API => ({ /** * Deletes multiple users - * @param userId the ID of the user to delete + * @param userIds the ID of the user to delete */ deleteUsers: async userIds => { return await API.post({ - url: `/api/global/users/bulkDelete`, + url: `/api/global/users/bulk`, body: { - userIds, + delete: { + userIds, + }, }, }) }, diff --git a/packages/types/src/api/web/user.ts b/packages/types/src/api/web/user.ts index b2c17575c2..c66d3203e8 100644 --- a/packages/types/src/api/web/user.ts +++ b/packages/types/src/api/web/user.ts @@ -6,28 +6,31 @@ export interface CreateUserResponse { email: string } -export interface BulkCreateUsersRequest { - users: User[] - groups: any[] -} - export interface UserDetails { _id: string email: string } -export interface BulkCreateUsersResponse { - successful: UserDetails[] - unsuccessful: { email: string; reason: string }[] +export interface BulkUserRequest { + delete?: { + userIds: string[] + } + create?: { + users: User[] + groups: any[] + } } -export interface BulkDeleteUsersRequest { - userIds: string[] -} - -export interface BulkDeleteUsersResponse { - successful: UserDetails[] - unsuccessful: { _id: string; email: string; reason: string }[] +export interface BulkUserResponse { + created?: { + successful: UserDetails[] + unsuccessful: { email: string; reason: string }[] + } + deleted?: { + successful: UserDetails[] + unsuccessful: { _id: string; email: string; reason: string }[] + } + message?: string } export interface InviteUserRequest { diff --git a/packages/worker/src/api/controllers/global/users.ts b/packages/worker/src/api/controllers/global/users.ts index 639aab5794..77d44ef4d2 100644 --- a/packages/worker/src/api/controllers/global/users.ts +++ b/packages/worker/src/api/controllers/global/users.ts @@ -1,8 +1,9 @@ import { checkInviteCode } from "../../../utilities/redis" -import { users } from "../../../sdk" +import { users as userSdk } from "../../../sdk" import env from "../../../environment" import { - BulkDeleteUsersRequest, + BulkUserRequest, + BulkUserResponse, CloudAccount, InviteUserRequest, InviteUsersRequest, @@ -21,27 +22,43 @@ const MAX_USERS_UPLOAD_LIMIT = 1000 export const save = async (ctx: any) => { try { - ctx.body = await users.save(ctx.request.body) + ctx.body = await userSdk.save(ctx.request.body) } catch (err: any) { ctx.throw(err.status || 400, err) } } -export const bulkCreate = async (ctx: any) => { - let { users: newUsersRequested, groups } = ctx.request.body +const bulkDelete = async (userIds: string[], currentUserId: string) => { + if (userIds?.indexOf(currentUserId) !== -1) { + throw new Error("Unable to delete self.") + } + return await userSdk.bulkDelete(userIds) +} - if (!env.SELF_HOSTED && newUsersRequested.length > MAX_USERS_UPLOAD_LIMIT) { - ctx.throw( - 400, +const bulkCreate = async (users: User[], groupIds: string[]) => { + if (!env.SELF_HOSTED && users.length > MAX_USERS_UPLOAD_LIMIT) { + throw new Error( "Max limit for upload is 1000 users. Please reduce file size and try again." ) } + return await userSdk.bulkCreate(users, groupIds) +} +export const bulkUpdate = async (ctx: any) => { + const currentUserId = ctx.user._id + const input = ctx.request.body as BulkUserRequest + let created, deleted try { - ctx.body = await users.bulkCreate(newUsersRequested, groups) + if (input.create) { + created = await bulkCreate(input.create.users, input.create.groups) + } + if (input.delete) { + deleted = await bulkDelete(input.delete.userIds, currentUserId) + } } catch (err: any) { - ctx.throw(err.status || 400, err) + ctx.throw(400, err?.message || err) } + ctx.body = { created, deleted } as BulkUserResponse } const parseBooleanParam = (param: any) => { @@ -85,7 +102,7 @@ export const adminUser = async (ctx: any) => { // always bust checklist beforehand, if an error occurs but can proceed, don't get // stuck in a cycle await cache.bustCache(cache.CacheKeys.CHECKLIST) - const finalUser = await users.save(user, { + const finalUser = await userSdk.save(user, { hashPassword, requirePassword, }) @@ -107,7 +124,7 @@ export const adminUser = async (ctx: any) => { export const countByApp = async (ctx: any) => { const appId = ctx.params.appId try { - ctx.body = await users.countUsersByApp(appId) + ctx.body = await userSdk.countUsersByApp(appId) } catch (err: any) { ctx.throw(err.status || 400, err) } @@ -119,28 +136,15 @@ export const destroy = async (ctx: any) => { ctx.throw(400, "Unable to delete self.") } - await users.destroy(id, ctx.user) + await userSdk.destroy(id, ctx.user) ctx.body = { message: `User ${id} deleted.`, } } -export const bulkDelete = async (ctx: any) => { - const { userIds } = ctx.request.body as BulkDeleteUsersRequest - if (userIds?.indexOf(ctx.user._id) !== -1) { - ctx.throw(400, "Unable to delete self.") - } - - try { - ctx.body = await users.bulkDelete(userIds) - } catch (err) { - ctx.throw(err) - } -} - export const search = async (ctx: any) => { - const paginated = await users.paginatedUsers(ctx.request.body) + const paginated = await userSdk.paginatedUsers(ctx.request.body) // user hashed password shouldn't ever be returned for (let user of paginated.data) { if (user) { @@ -152,7 +156,7 @@ export const search = async (ctx: any) => { // called internally by app server user fetch export const fetch = async (ctx: any) => { - const all = await users.allUsers() + const all = await userSdk.allUsers() // user hashed password shouldn't ever be returned for (let user of all) { if (user) { @@ -164,7 +168,7 @@ export const fetch = async (ctx: any) => { // called internally by app server user find export const find = async (ctx: any) => { - ctx.body = await users.getUser(ctx.params.id) + ctx.body = await userSdk.getUser(ctx.params.id) } export const tenantUserLookup = async (ctx: any) => { @@ -179,7 +183,7 @@ export const tenantUserLookup = async (ctx: any) => { export const invite = async (ctx: any) => { const request = ctx.request.body as InviteUserRequest - const response = await users.invite([request]) + const response = await userSdk.invite([request]) // explicitly throw for single user invite if (response.unsuccessful.length) { @@ -198,7 +202,7 @@ export const invite = async (ctx: any) => { export const inviteMultiple = async (ctx: any) => { const request = ctx.request.body as InviteUsersRequest - ctx.body = await users.invite(request) + ctx.body = await userSdk.invite(request) } export const inviteAccept = async (ctx: any) => { @@ -207,7 +211,7 @@ export const inviteAccept = async (ctx: any) => { // info is an extension of the user object that was stored by global const { email, info }: any = await checkInviteCode(inviteCode) ctx.body = await tenancy.doInTenant(info.tenantId, async () => { - const saved = await users.save({ + const saved = await userSdk.save({ firstName, lastName, password, diff --git a/packages/worker/src/api/routes/global/tests/users.spec.ts b/packages/worker/src/api/routes/global/tests/users.spec.ts index fd9ef7ff9f..218bc60800 100644 --- a/packages/worker/src/api/routes/global/tests/users.spec.ts +++ b/packages/worker/src/api/routes/global/tests/users.spec.ts @@ -97,16 +97,16 @@ describe("/api/global/users", () => { }) }) - describe("bulkCreate", () => { + describe("bulk (create)", () => { it("should ignore users existing in the same tenant", async () => { const user = await config.createUser() jest.clearAllMocks() const response = await api.users.bulkCreateUsers([user]) - expect(response.successful.length).toBe(0) - expect(response.unsuccessful.length).toBe(1) - expect(response.unsuccessful[0].email).toBe(user.email) + expect(response.created?.successful.length).toBe(0) + expect(response.created?.unsuccessful.length).toBe(1) + expect(response.created?.unsuccessful[0].email).toBe(user.email) expect(events.user.created).toBeCalledTimes(0) }) @@ -117,9 +117,9 @@ describe("/api/global/users", () => { await tenancy.doInTenant(TENANT_1, async () => { const response = await api.users.bulkCreateUsers([user]) - expect(response.successful.length).toBe(0) - expect(response.unsuccessful.length).toBe(1) - expect(response.unsuccessful[0].email).toBe(user.email) + expect(response.created?.successful.length).toBe(0) + expect(response.created?.unsuccessful.length).toBe(1) + expect(response.created?.unsuccessful[0].email).toBe(user.email) expect(events.user.created).toBeCalledTimes(0) }) }) @@ -132,24 +132,24 @@ describe("/api/global/users", () => { const response = await api.users.bulkCreateUsers([user]) - expect(response.successful.length).toBe(0) - expect(response.unsuccessful.length).toBe(1) - expect(response.unsuccessful[0].email).toBe(user.email) + expect(response.created?.successful.length).toBe(0) + expect(response.created?.unsuccessful.length).toBe(1) + expect(response.created?.unsuccessful[0].email).toBe(user.email) expect(events.user.created).toBeCalledTimes(0) }) - it("should be able to bulkCreate users", async () => { + it("should be able to bulk create users", async () => { const builder = structures.users.builderUser() const admin = structures.users.adminUser() const user = structures.users.user() const response = await api.users.bulkCreateUsers([builder, admin, user]) - expect(response.successful.length).toBe(3) - expect(response.successful[0].email).toBe(builder.email) - expect(response.successful[1].email).toBe(admin.email) - expect(response.successful[2].email).toBe(user.email) - expect(response.unsuccessful.length).toBe(0) + expect(response.created?.successful.length).toBe(3) + expect(response.created?.successful[0].email).toBe(builder.email) + expect(response.created?.successful[1].email).toBe(admin.email) + expect(response.created?.successful[2].email).toBe(user.email) + expect(response.created?.unsuccessful.length).toBe(0) expect(events.user.created).toBeCalledTimes(3) expect(events.user.permissionAdminAssigned).toBeCalledTimes(1) expect(events.user.permissionBuilderAssigned).toBeCalledTimes(2) @@ -420,33 +420,30 @@ describe("/api/global/users", () => { }) }) - describe("bulkDelete", () => { - it("should not be able to bulkDelete current user", async () => { + describe("bulk (delete)", () => { + it("should not be able to bulk delete current user", async () => { const user = await config.defaultUser! - const request = { userIds: [user._id!] } - const response = await api.users.bulkDeleteUsers(request, 400) + const response = await api.users.bulkDeleteUsers([user._id!], 400) - expect(response.body.message).toBe("Unable to delete self.") + expect(response.message).toBe("Unable to delete self.") expect(events.user.deleted).not.toBeCalled() }) - it("should not be able to bulkDelete account owner", async () => { + it("should not be able to bulk delete account owner", async () => { const user = await config.createUser() const account = structures.accounts.cloudAccount() account.budibaseUserId = user._id! mocks.accounts.getAccountByTenantId.mockReturnValue(account) - const request = { userIds: [user._id!] } + const response = await api.users.bulkDeleteUsers([user._id!]) - const response = await api.users.bulkDeleteUsers(request) - - expect(response.body.successful.length).toBe(0) - expect(response.body.unsuccessful.length).toBe(1) - expect(response.body.unsuccessful[0].reason).toBe( + expect(response.deleted?.successful.length).toBe(0) + expect(response.deleted?.unsuccessful.length).toBe(1) + expect(response.deleted?.unsuccessful[0].reason).toBe( "Account holder cannot be deleted" ) - expect(response.body.unsuccessful[0]._id).toBe(user._id) + expect(response.deleted?.unsuccessful[0]._id).toBe(user._id) expect(events.user.deleted).not.toBeCalled() }) @@ -462,12 +459,14 @@ describe("/api/global/users", () => { admin, user, ]) - const request = { userIds: createdUsers.successful.map(u => u._id!) } - const response = await api.users.bulkDeleteUsers(request) + const toDelete = createdUsers.created?.successful.map( + u => u._id! + ) as string[] + const response = await api.users.bulkDeleteUsers(toDelete) - expect(response.body.successful.length).toBe(3) - expect(response.body.unsuccessful.length).toBe(0) + expect(response.deleted?.successful.length).toBe(3) + expect(response.deleted?.unsuccessful.length).toBe(0) expect(events.user.deleted).toBeCalledTimes(3) expect(events.user.permissionAdminRemoved).toBeCalledTimes(1) expect(events.user.permissionBuilderRemoved).toBeCalledTimes(2) diff --git a/packages/worker/src/api/routes/global/users.js b/packages/worker/src/api/routes/global/users.js index 2b442d13a0..3047b4096c 100644 --- a/packages/worker/src/api/routes/global/users.js +++ b/packages/worker/src/api/routes/global/users.js @@ -56,16 +56,15 @@ router controller.save ) .post( - "/api/global/users/bulkCreate", + "/api/global/users/bulk", adminOnly, - users.buildUserBulkSaveValidation(), - controller.bulkCreate + users.buildUserBulkUserValidation(), + controller.bulkUpdate ) .get("/api/global/users", builderOrAdmin, controller.fetch) .post("/api/global/users/search", builderOrAdmin, controller.search) .delete("/api/global/users/:id", adminOnly, controller.destroy) - .post("/api/global/users/bulkDelete", adminOnly, controller.bulkDelete) .get("/api/global/users/count/:appId", builderOrAdmin, controller.countByApp) .get("/api/global/roles/:appId") .post( diff --git a/packages/worker/src/api/routes/validation/users.ts b/packages/worker/src/api/routes/validation/users.ts index d84ae94ee6..0cb14c047e 100644 --- a/packages/worker/src/api/routes/validation/users.ts +++ b/packages/worker/src/api/routes/validation/users.ts @@ -28,7 +28,7 @@ export const buildUserSaveValidation = (isSelf = false) => { return joiValidator.body(Joi.object(schema).required().unknown(true)) } -export const buildUserBulkSaveValidation = (isSelf = false) => { +export const buildUserBulkUserValidation = (isSelf = false) => { if (!isSelf) { schema = { ...schema, @@ -36,10 +36,15 @@ export const buildUserBulkSaveValidation = (isSelf = false) => { _rev: Joi.string(), } } - let bulkSaveSchema = { - groups: Joi.array().optional(), - users: Joi.array().items(Joi.object(schema).required().unknown(true)), + let bulkSchema = { + create: Joi.object({ + groups: Joi.array().optional(), + users: Joi.array().items(Joi.object(schema).required().unknown(true)), + }), + delete: Joi.object({ + userIds: Joi.array().items(Joi.string()), + }), } - return joiValidator.body(Joi.object(bulkSaveSchema).required().unknown(true)) + return joiValidator.body(Joi.object(bulkSchema).required().unknown(true)) } diff --git a/packages/worker/src/sdk/users/users.ts b/packages/worker/src/sdk/users/users.ts index 222dccea01..ece2d76082 100644 --- a/packages/worker/src/sdk/users/users.ts +++ b/packages/worker/src/sdk/users/users.ts @@ -19,8 +19,7 @@ import { import { AccountMetadata, AllDocsResponse, - BulkCreateUsersResponse, - BulkDeleteUsersResponse, + BulkUserResponse, CloudAccount, CreateUserResponse, InviteUsersRequest, @@ -347,7 +346,7 @@ const searchExistingEmails = async (emails: string[]) => { export const bulkCreate = async ( newUsersRequested: User[], groups: string[] -): Promise => { +): Promise => { const db = tenancy.getGlobalDB() const tenantId = tenancy.getTenantId() @@ -436,10 +435,10 @@ const getAccountHolderFromUserIds = async ( export const bulkDelete = async ( userIds: string[] -): Promise => { +): Promise => { const db = tenancy.getGlobalDB() - const response: BulkDeleteUsersResponse = { + const response: BulkUserResponse["deleted"] = { successful: [], unsuccessful: [], } diff --git a/packages/worker/src/tests/api/users.ts b/packages/worker/src/tests/api/users.ts index 986a26ad5f..3677bfffc6 100644 --- a/packages/worker/src/tests/api/users.ts +++ b/packages/worker/src/tests/api/users.ts @@ -1,8 +1,6 @@ import { - BulkCreateUsersRequest, - BulkCreateUsersResponse, - BulkDeleteUsersRequest, - BulkDeleteUsersResponse, + BulkUserResponse, + BulkUserRequest, InviteUsersRequest, User, } from "@budibase/types" @@ -69,24 +67,26 @@ export class UserAPI { // BULK bulkCreateUsers = async (users: User[], groups: any[] = []) => { - const body: BulkCreateUsersRequest = { users, groups } + const body: BulkUserRequest = { create: { users, groups } } const res = await this.request - .post(`/api/global/users/bulkCreate`) + .post(`/api/global/users/bulk`) .send(body) .set(this.config.defaultHeaders()) .expect("Content-Type", /json/) .expect(200) - return res.body as BulkCreateUsersResponse + return res.body as BulkUserResponse } - bulkDeleteUsers = (body: BulkDeleteUsersRequest, status?: number) => { - return this.request - .post(`/api/global/users/bulkDelete`) + bulkDeleteUsers = async (userIds: string[], status?: number) => { + const body: BulkUserRequest = { delete: { userIds } } + const res = await this.request + .post(`/api/global/users/bulk`) .send(body) .set(this.config.defaultHeaders()) .expect("Content-Type", /json/) .expect(status ? status : 200) + return res.body as BulkUserResponse } // USER