From 2057985631e9d6e3f99bfa6109fcfe4663059265 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 1 Jun 2021 15:58:40 +0100 Subject: [PATCH 1/2] Fixing issue with roles not being added correctly to global users and cleaning up roles when an app is deleted. --- .../server/src/api/controllers/application.js | 4 ++- .../src/api/routes/tests/utilities/index.js | 1 + .../server/src/utilities/workerRequests.js | 34 +++++++++++++----- .../worker/src/api/controllers/admin/users.js | 35 +++++++++++++++---- packages/worker/src/api/routes/admin/users.js | 1 + 5 files changed, 58 insertions(+), 17 deletions(-) diff --git a/packages/server/src/api/controllers/application.js b/packages/server/src/api/controllers/application.js index 090de08909..9ea3650b77 100644 --- a/packages/server/src/api/controllers/application.js +++ b/packages/server/src/api/controllers/application.js @@ -27,7 +27,7 @@ const { cloneDeep } = require("lodash/fp") const { processObject } = require("@budibase/string-templates") const { getAllApps } = require("../../utilities") const { USERS_TABLE_SCHEMA } = require("../../constants") -const { getDeployedApps } = require("../../utilities/workerRequests") +const { getDeployedApps, removeAppFromUserRoles } = require("../../utilities/workerRequests") const { clientLibraryPath } = require("../../utilities") const { getAllLocks } = require("../../utilities/redis") @@ -245,6 +245,8 @@ exports.delete = async function (ctx) { if (!env.isTest()) { await deleteApp(ctx.params.appId) } + // make sure the app/role doesn't stick around after the app has been deleted + await removeAppFromUserRoles(ctx.params.appId) ctx.status = 200 ctx.body = result diff --git a/packages/server/src/api/routes/tests/utilities/index.js b/packages/server/src/api/routes/tests/utilities/index.js index 713a3f799e..f28bd5a7af 100644 --- a/packages/server/src/api/routes/tests/utilities/index.js +++ b/packages/server/src/api/routes/tests/utilities/index.js @@ -13,6 +13,7 @@ jest.mock("../../../../utilities/workerRequests", () => ({ _id: "us_uuid1", } }), + removeAppFromUserRoles: jest.fn(), })) exports.delay = ms => new Promise(resolve => setTimeout(resolve, ms)) diff --git a/packages/server/src/utilities/workerRequests.js b/packages/server/src/utilities/workerRequests.js index 639c178e2e..01e5be1791 100644 --- a/packages/server/src/utilities/workerRequests.js +++ b/packages/server/src/utilities/workerRequests.js @@ -57,11 +57,10 @@ exports.sendSmtpEmail = async (to, from, subject, contents) => { }) ) - const json = await response.json() - if (json.status !== 200 && response.status !== 200) { + if (response.status !== 200) { throw "Unable to send email." } - return json + return response.json() } exports.getDeployedApps = async ctx => { @@ -124,10 +123,10 @@ exports.getGlobalSelf = async (ctx, appId = null) => { checkSlashesInUrl(env.WORKER_URL + endpoint), request(ctx, { method: "GET" }) ) - let json = await response.json() - if (json.status !== 200 && response.status !== 200) { + if (response.status !== 200) { ctx.throw(400, "Unable to get self globally.") } + let json = await response.json() if (appId) { json = getAppRole(appId, json) } @@ -151,7 +150,7 @@ exports.addAppRoleToUser = async (ctx, appId, roleId, userId = null) => { ...body, roles: { ...user.roles, - [appId]: roleId, + [getDeployedAppID(appId)]: roleId, }, } const response = await fetch( @@ -161,9 +160,26 @@ exports.addAppRoleToUser = async (ctx, appId, roleId, userId = null) => { body, }) ) - const json = await response.json() - if (json.status !== 200 && response.status !== 200) { + if (response.status !== 200) { ctx.throw(400, "Unable to save self globally.") } - return json + return response.json() } + +exports.removeAppFromUserRoles = async appId => { + const deployedAppId = getDeployedAppID(appId) + const response = await fetch( + checkSlashesInUrl(env.WORKER_URL + `/api/admin/roles/${deployedAppId}`), + request(null, { + method: "DELETE", + headers: { + "x-budibase-api-key": env.INTERNAL_API_KEY, + } + }) + ) + if (response.status !== 200) { + throw "Unable to remove app role" + } + return response.json() +} + diff --git a/packages/worker/src/api/controllers/admin/users.js b/packages/worker/src/api/controllers/admin/users.js index 12208ea3a0..6afd4ecbfd 100644 --- a/packages/worker/src/api/controllers/admin/users.js +++ b/packages/worker/src/api/controllers/admin/users.js @@ -11,6 +11,16 @@ const { sendEmail } = require("../../../utilities/email") const GLOBAL_DB = StaticDatabases.GLOBAL.name +async function allUsers() { + const db = new CouchDB(GLOBAL_DB) + const response = await db.allDocs( + getGlobalUserParams(null, { + include_docs: true, + }) + ) + return response.rows.map(row => row.doc) +} + exports.save = async ctx => { const db = new CouchDB(GLOBAL_DB) const { email, password, _id } = ctx.request.body @@ -105,6 +115,23 @@ exports.destroy = async ctx => { } } +exports.removeAppRole = async ctx => { + const { appId } = ctx.params + const db = new CouchDB(GLOBAL_DB) + const users = await allUsers() + const bulk = [] + for (let user of users) { + if (user.roles[appId]) { + delete user.roles[appId] + bulk.push(user) + } + } + await db.bulkDocs(bulk) + ctx.body = { + message: "App role removed from all users", + } +} + exports.getSelf = async ctx => { ctx.params = { id: ctx.user._id, @@ -134,13 +161,7 @@ exports.updateSelf = async ctx => { // called internally by app server user fetch exports.fetch = async ctx => { - const db = new CouchDB(GLOBAL_DB) - const response = await db.allDocs( - getGlobalUserParams(null, { - include_docs: true, - }) - ) - const users = response.rows.map(row => row.doc) + const users = await allUsers() // user hashed password shouldn't ever be returned for (let user of users) { if (user) { diff --git a/packages/worker/src/api/routes/admin/users.js b/packages/worker/src/api/routes/admin/users.js index 733cee1103..e302725232 100644 --- a/packages/worker/src/api/routes/admin/users.js +++ b/packages/worker/src/api/routes/admin/users.js @@ -67,6 +67,7 @@ router controller.save ) .get("/api/admin/users", adminOnly, controller.fetch) + .delete("/api/admin/roles/:appId", adminOnly, controller.removeAppRole) .delete("/api/admin/users/:id", adminOnly, controller.destroy) .get("/api/admin/roles/:appId") .post( From 6f6b0e5cce15d1947539b5cf415c4764d930cad0 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 1 Jun 2021 16:02:20 +0100 Subject: [PATCH 2/2] Formatting. --- packages/server/src/api/controllers/application.js | 5 ++++- packages/server/src/utilities/workerRequests.js | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/controllers/application.js b/packages/server/src/api/controllers/application.js index 9ea3650b77..70b63c977a 100644 --- a/packages/server/src/api/controllers/application.js +++ b/packages/server/src/api/controllers/application.js @@ -27,7 +27,10 @@ const { cloneDeep } = require("lodash/fp") const { processObject } = require("@budibase/string-templates") const { getAllApps } = require("../../utilities") const { USERS_TABLE_SCHEMA } = require("../../constants") -const { getDeployedApps, removeAppFromUserRoles } = require("../../utilities/workerRequests") +const { + getDeployedApps, + removeAppFromUserRoles, +} = require("../../utilities/workerRequests") const { clientLibraryPath } = require("../../utilities") const { getAllLocks } = require("../../utilities/redis") diff --git a/packages/server/src/utilities/workerRequests.js b/packages/server/src/utilities/workerRequests.js index 01e5be1791..99d9a1c3e2 100644 --- a/packages/server/src/utilities/workerRequests.js +++ b/packages/server/src/utilities/workerRequests.js @@ -174,7 +174,7 @@ exports.removeAppFromUserRoles = async appId => { method: "DELETE", headers: { "x-budibase-api-key": env.INTERNAL_API_KEY, - } + }, }) ) if (response.status !== 200) { @@ -182,4 +182,3 @@ exports.removeAppFromUserRoles = async appId => { } return response.json() } -