From bc68b165267deb7dea7798c115e2c5764fa31ee9 Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Mon, 28 Nov 2022 16:01:27 +0000 Subject: [PATCH 1/9] Allow developers to manage user access --- packages/worker/src/api/routes/global/users.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/worker/src/api/routes/global/users.js b/packages/worker/src/api/routes/global/users.js index 2d9b1d9ac9..c9acaaf068 100644 --- a/packages/worker/src/api/routes/global/users.js +++ b/packages/worker/src/api/routes/global/users.js @@ -1,7 +1,6 @@ const Router = require("@koa/router") const controller = require("../../controllers/global/users") const { joiValidator } = require("@budibase/backend-core/auth") -const { adminOnly } = require("@budibase/backend-core/auth") const Joi = require("joi") const cloudRestricted = require("../../../middleware/cloudRestricted") const { users } = require("../validation") @@ -51,31 +50,31 @@ function buildInviteAcceptValidation() { router .post( "/api/global/users", - adminOnly, + builderOrAdmin, users.buildUserSaveValidation(), controller.save ) .post( "/api/global/users/bulk", - adminOnly, + builderOrAdmin, 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) + .delete("/api/global/users/:id", builderOrAdmin, controller.destroy) .get("/api/global/users/count/:appId", builderOrAdmin, controller.countByApp) .get("/api/global/roles/:appId") .post( "/api/global/users/invite", - adminOnly, + builderOrAdmin, buildInviteValidation(), controller.invite ) .post( "/api/global/users/multi/invite", - adminOnly, + builderOrAdmin, buildInviteMultipleValidation(), controller.inviteMultiple ) From 0b3d84b63d218e75ca4cd6700eac8813f2f9decd Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Mon, 28 Nov 2022 16:02:49 +0000 Subject: [PATCH 2/9] Invite is adminOnly --- packages/worker/src/api/routes/global/users.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/worker/src/api/routes/global/users.js b/packages/worker/src/api/routes/global/users.js index c9acaaf068..99c2f52c6f 100644 --- a/packages/worker/src/api/routes/global/users.js +++ b/packages/worker/src/api/routes/global/users.js @@ -1,6 +1,7 @@ const Router = require("@koa/router") const controller = require("../../controllers/global/users") const { joiValidator } = require("@budibase/backend-core/auth") +const { adminOnly } = require("@budibase/backend-core/auth") const Joi = require("joi") const cloudRestricted = require("../../../middleware/cloudRestricted") const { users } = require("../validation") @@ -68,13 +69,13 @@ router .get("/api/global/roles/:appId") .post( "/api/global/users/invite", - builderOrAdmin, + adminOnly, buildInviteValidation(), controller.invite ) .post( "/api/global/users/multi/invite", - builderOrAdmin, + adminOnly, buildInviteMultipleValidation(), controller.inviteMultiple ) From 376e17cae1907e478c49a64f04cc3ba722dc61b3 Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Tue, 29 Nov 2022 11:36:24 +0000 Subject: [PATCH 3/9] Only allow admin to create new user --- packages/worker/src/api/controllers/global/users.ts | 4 ++++ packages/worker/src/api/routes/global/users.js | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/worker/src/api/controllers/global/users.ts b/packages/worker/src/api/controllers/global/users.ts index ea1df5b45a..d76c1741a3 100644 --- a/packages/worker/src/api/controllers/global/users.ts +++ b/packages/worker/src/api/controllers/global/users.ts @@ -23,6 +23,10 @@ const MAX_USERS_UPLOAD_LIMIT = 1000 export const save = async (ctx: any) => { try { + if (!ctx.request.body._id && !ctx.internal && + (!ctx.user || !ctx.user.admin || !ctx.user.admin.global)) { + ctx.throw(403, "Only admin user can create new user.") + } ctx.body = await sdk.users.save(ctx.request.body) } catch (err: any) { ctx.throw(err.status || 400, err) diff --git a/packages/worker/src/api/routes/global/users.js b/packages/worker/src/api/routes/global/users.js index 99c2f52c6f..7740276dee 100644 --- a/packages/worker/src/api/routes/global/users.js +++ b/packages/worker/src/api/routes/global/users.js @@ -57,14 +57,14 @@ router ) .post( "/api/global/users/bulk", - builderOrAdmin, + adminOnly, users.buildUserBulkUserValidation(), controller.bulkUpdate ) .get("/api/global/users", builderOrAdmin, controller.fetch) .post("/api/global/users/search", builderOrAdmin, controller.search) - .delete("/api/global/users/:id", builderOrAdmin, controller.destroy) + .delete("/api/global/users/:id", adminOnly, controller.destroy) .get("/api/global/users/count/:appId", builderOrAdmin, controller.countByApp) .get("/api/global/roles/:appId") .post( From c476b20ac1e5f6f30a5cd7da745e5512b8ca6bcb Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Tue, 29 Nov 2022 15:13:58 +0000 Subject: [PATCH 4/9] Fix unit tests --- .../src/api/controllers/global/users.ts | 9 ++++-- .../worker/src/tests/TestConfiguration.ts | 29 ++++++++++++++----- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/packages/worker/src/api/controllers/global/users.ts b/packages/worker/src/api/controllers/global/users.ts index d76c1741a3..df1797132e 100644 --- a/packages/worker/src/api/controllers/global/users.ts +++ b/packages/worker/src/api/controllers/global/users.ts @@ -23,9 +23,12 @@ const MAX_USERS_UPLOAD_LIMIT = 1000 export const save = async (ctx: any) => { try { - if (!ctx.request.body._id && !ctx.internal && - (!ctx.user || !ctx.user.admin || !ctx.user.admin.global)) { - ctx.throw(403, "Only admin user can create new user.") + if ( + !ctx.request.body._id && + !ctx.internal && + (!ctx.user || !ctx.user.admin || !ctx.user.admin.global) + ) { + ctx.throw(403, "Only admin user can create new user.") } ctx.body = await sdk.users.save(ctx.request.body) } catch (err: any) { diff --git a/packages/worker/src/tests/TestConfiguration.ts b/packages/worker/src/tests/TestConfiguration.ts index 746e1ccf1b..b6da8693da 100644 --- a/packages/worker/src/tests/TestConfiguration.ts +++ b/packages/worker/src/tests/TestConfiguration.ts @@ -72,12 +72,24 @@ class TestConfiguration { // UTILS - async _req(config: any, params: any, controlFunc: any) { + async _req(config: any, params: any, controlFunc: any, opts: { force?: boolean } = {}) { const request: any = {} // fake cookies, we don't need them request.cookies = { set: () => {}, get: () => {} } request.config = { jwtSecret: env.JWT_SECRET } - request.user = { tenantId: this.getTenantId() } + if (opts.force) { + request.user = { + tenantId: this.getTenantId(), + admin: { global: true }, + builder: { global: true }, + } + } else if (this.defaultUser) { + request.user = this.defaultUser + } else { + request.user = { + tenantId: this.getTenantId() + } + } request.query = {} request.request = { body: config, @@ -129,7 +141,7 @@ class TestConfiguration { email: "test@test.com", password: "test", }) - this.defaultUser = await this.createUser(user) + this.defaultUser = await this.createUser(user, { force: true }) } async createTenant1User() { @@ -137,15 +149,16 @@ class TestConfiguration { email: "tenant1@test.com", password: "test", }) - this.tenant1User = await this.createUser(user) + this.tenant1User = await this.createUser(user, { force: true }) } async createSession(user: User) { - await sessions.createASession(user._id!, { + const session: any = { sessionId: "sessionid", tenantId: user.tenantId, csrfToken: CSRF_TOKEN, - }) + } + await sessions.createASession(user._id!, session) } cookieHeader(cookies: any) { @@ -185,11 +198,11 @@ class TestConfiguration { }) } - async createUser(user?: User) { + async createUser(user?: User, opts: any = {}) { if (!user) { user = structures.users.user() } - const response = await this._req(user, null, controllers.users.save) + const response = await this._req(user, null, controllers.users.save, opts) const body = response as CreateUserResponse return this.getUser(body.email) } From 89db22858a7a3f7ba4b256a2a6237ec51bb9611e Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Tue, 29 Nov 2022 15:14:29 +0000 Subject: [PATCH 5/9] lint --- packages/worker/src/tests/TestConfiguration.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/packages/worker/src/tests/TestConfiguration.ts b/packages/worker/src/tests/TestConfiguration.ts index b6da8693da..2875cc00e9 100644 --- a/packages/worker/src/tests/TestConfiguration.ts +++ b/packages/worker/src/tests/TestConfiguration.ts @@ -72,7 +72,12 @@ class TestConfiguration { // UTILS - async _req(config: any, params: any, controlFunc: any, opts: { force?: boolean } = {}) { + async _req( + config: any, + params: any, + controlFunc: any, + opts: { force?: boolean } = {} + ) { const request: any = {} // fake cookies, we don't need them request.cookies = { set: () => {}, get: () => {} } @@ -86,8 +91,8 @@ class TestConfiguration { } else if (this.defaultUser) { request.user = this.defaultUser } else { - request.user = { - tenantId: this.getTenantId() + request.user = { + tenantId: this.getTenantId(), } } request.query = {} @@ -153,7 +158,7 @@ class TestConfiguration { } async createSession(user: User) { - const session: any = { + const session: any = { sessionId: "sessionid", tenantId: user.tenantId, csrfToken: CSRF_TOKEN, From d0909392d7f52d3f2f1248a0f1af47eb57a05d47 Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Tue, 29 Nov 2022 16:11:53 +0000 Subject: [PATCH 6/9] Added unit tests --- .../src/api/routes/global/tests/users.spec.ts | 16 ++++++++++++++++ packages/worker/src/tests/api/users.ts | 4 ++-- 2 files changed, 18 insertions(+), 2 deletions(-) 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 218bc60800..8984105c88 100644 --- a/packages/worker/src/api/routes/global/tests/users.spec.ts +++ b/packages/worker/src/api/routes/global/tests/users.spec.ts @@ -262,6 +262,14 @@ describe("/api/global/users", () => { expect(events.user.created).toBeCalledTimes(1) }) + + it("should not allow a non-admin user to create a new user", async () => { + const nonAdmin = await config.createUser(structures.users.builderUser()) + await config.createSession(nonAdmin) + + const newUser = structures.users.user() + await api.users.saveUser(newUser, 403, config.authHeaders(nonAdmin)) + }) }) describe("update", () => { @@ -418,6 +426,14 @@ describe("/api/global/users", () => { expect(user).toStrictEqual(dbUser) expect(response.body.message).toBe("Email address cannot be changed") }) + + it("should allow a non-admin user to update an existing user", async () => { + const existingUser = await config.createUser(structures.users.user()) + const nonAdmin = await config.createUser(structures.users.builderUser()) + await config.createSession(nonAdmin) + + await api.users.saveUser(existingUser, 200, config.authHeaders(nonAdmin)) + }) }) describe("bulk (delete)", () => { diff --git a/packages/worker/src/tests/api/users.ts b/packages/worker/src/tests/api/users.ts index 3677bfffc6..a2f26052bc 100644 --- a/packages/worker/src/tests/api/users.ts +++ b/packages/worker/src/tests/api/users.ts @@ -91,11 +91,11 @@ export class UserAPI { // USER - saveUser = (user: User, status?: number) => { + saveUser = (user: User, status?: number, headers?: any) => { return this.request .post(`/api/global/users`) .send(user) - .set(this.config.defaultHeaders()) + .set(headers ?? this.config.defaultHeaders()) .expect("Content-Type", /json/) .expect(status ? status : 200) } From 197699b2ada5ae8da04ce3ddab63bc0abb701dbe Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Tue, 29 Nov 2022 16:38:44 +0000 Subject: [PATCH 7/9] refactor --- packages/worker/src/api/controllers/global/users.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/worker/src/api/controllers/global/users.ts b/packages/worker/src/api/controllers/global/users.ts index df1797132e..9d7349851b 100644 --- a/packages/worker/src/api/controllers/global/users.ts +++ b/packages/worker/src/api/controllers/global/users.ts @@ -23,11 +23,10 @@ const MAX_USERS_UPLOAD_LIMIT = 1000 export const save = async (ctx: any) => { try { - if ( - !ctx.request.body._id && - !ctx.internal && - (!ctx.user || !ctx.user.admin || !ctx.user.admin.global) - ) { + const body = ctx.request.body + const isCreate = !body._id + const isAdmin = !!ctx.user.admin?.global + if (isCreate && !isAdmin) { ctx.throw(403, "Only admin user can create new user.") } ctx.body = await sdk.users.save(ctx.request.body) From 0e0157c8880c4120bcb4b3ebed29a880b9c64b36 Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Tue, 29 Nov 2022 17:23:54 +0000 Subject: [PATCH 8/9] lint --- packages/worker/src/api/controllers/global/users.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/worker/src/api/controllers/global/users.ts b/packages/worker/src/api/controllers/global/users.ts index 9d7349851b..0d2e5b3c7f 100644 --- a/packages/worker/src/api/controllers/global/users.ts +++ b/packages/worker/src/api/controllers/global/users.ts @@ -23,7 +23,7 @@ const MAX_USERS_UPLOAD_LIMIT = 1000 export const save = async (ctx: any) => { try { - const body = ctx.request.body + const body = ctx.request.body const isCreate = !body._id const isAdmin = !!ctx.user.admin?.global if (isCreate && !isAdmin) { From 6fe2c38bce95dd310965654572591812bf339640 Mon Sep 17 00:00:00 2001 From: Mel O'Hagan Date: Wed, 30 Nov 2022 09:29:56 +0000 Subject: [PATCH 9/9] Move custom rbac from controller to routes --- .../src/api/controllers/global/users.ts | 6 ---- .../worker/src/api/routes/global/users.js | 10 +++++- .../worker/src/tests/TestConfiguration.ts | 34 +++++-------------- 3 files changed, 17 insertions(+), 33 deletions(-) diff --git a/packages/worker/src/api/controllers/global/users.ts b/packages/worker/src/api/controllers/global/users.ts index 0d2e5b3c7f..ea1df5b45a 100644 --- a/packages/worker/src/api/controllers/global/users.ts +++ b/packages/worker/src/api/controllers/global/users.ts @@ -23,12 +23,6 @@ const MAX_USERS_UPLOAD_LIMIT = 1000 export const save = async (ctx: any) => { try { - const body = ctx.request.body - const isCreate = !body._id - const isAdmin = !!ctx.user.admin?.global - if (isCreate && !isAdmin) { - ctx.throw(403, "Only admin user can create new user.") - } ctx.body = await sdk.users.save(ctx.request.body) } catch (err: any) { ctx.throw(err.status || 400, err) diff --git a/packages/worker/src/api/routes/global/users.js b/packages/worker/src/api/routes/global/users.js index 7740276dee..af1fbb0baf 100644 --- a/packages/worker/src/api/routes/global/users.js +++ b/packages/worker/src/api/routes/global/users.js @@ -40,6 +40,14 @@ function buildInviteMultipleValidation() { )) } +const createUserAdminOnly = (ctx, next) => { + if (!ctx.request.body._id) { + return adminOnly(ctx, next) + } else { + return builderOrAdmin(ctx, next) + } +} + function buildInviteAcceptValidation() { // prettier-ignore return joiValidator.body(Joi.object({ @@ -51,7 +59,7 @@ function buildInviteAcceptValidation() { router .post( "/api/global/users", - builderOrAdmin, + createUserAdminOnly, users.buildUserSaveValidation(), controller.save ) diff --git a/packages/worker/src/tests/TestConfiguration.ts b/packages/worker/src/tests/TestConfiguration.ts index 2875cc00e9..746e1ccf1b 100644 --- a/packages/worker/src/tests/TestConfiguration.ts +++ b/packages/worker/src/tests/TestConfiguration.ts @@ -72,29 +72,12 @@ class TestConfiguration { // UTILS - async _req( - config: any, - params: any, - controlFunc: any, - opts: { force?: boolean } = {} - ) { + async _req(config: any, params: any, controlFunc: any) { const request: any = {} // fake cookies, we don't need them request.cookies = { set: () => {}, get: () => {} } request.config = { jwtSecret: env.JWT_SECRET } - if (opts.force) { - request.user = { - tenantId: this.getTenantId(), - admin: { global: true }, - builder: { global: true }, - } - } else if (this.defaultUser) { - request.user = this.defaultUser - } else { - request.user = { - tenantId: this.getTenantId(), - } - } + request.user = { tenantId: this.getTenantId() } request.query = {} request.request = { body: config, @@ -146,7 +129,7 @@ class TestConfiguration { email: "test@test.com", password: "test", }) - this.defaultUser = await this.createUser(user, { force: true }) + this.defaultUser = await this.createUser(user) } async createTenant1User() { @@ -154,16 +137,15 @@ class TestConfiguration { email: "tenant1@test.com", password: "test", }) - this.tenant1User = await this.createUser(user, { force: true }) + this.tenant1User = await this.createUser(user) } async createSession(user: User) { - const session: any = { + await sessions.createASession(user._id!, { sessionId: "sessionid", tenantId: user.tenantId, csrfToken: CSRF_TOKEN, - } - await sessions.createASession(user._id!, session) + }) } cookieHeader(cookies: any) { @@ -203,11 +185,11 @@ class TestConfiguration { }) } - async createUser(user?: User, opts: any = {}) { + async createUser(user?: User) { if (!user) { user = structures.users.user() } - const response = await this._req(user, null, controllers.users.save, opts) + const response = await this._req(user, null, controllers.users.save) const body = response as CreateUserResponse return this.getUser(body.email) }