From e6844d7bdfa28dada18f718460e1a6d9aa0a236d Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 7 Oct 2021 12:19:23 +0100 Subject: [PATCH 1/7] Fix an issue with current app cookie, get rid of it on login/logout. --- packages/worker/src/api/controllers/global/auth.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/worker/src/api/controllers/global/auth.js b/packages/worker/src/api/controllers/global/auth.js index 4e5603b596..76768d1700 100644 --- a/packages/worker/src/api/controllers/global/auth.js +++ b/packages/worker/src/api/controllers/global/auth.js @@ -54,6 +54,8 @@ async function authInternal(ctx, user, err = null, info = null) { // just store the user ID ctx.cookies.set(Cookies.Auth, user.token, config) + // get rid of any app cookies on login + clearCookie(ctx, Cookies.CurrentApp) } exports.authenticate = async (ctx, next) => { @@ -117,6 +119,7 @@ exports.resetUpdate = async ctx => { exports.logout = async ctx => { clearCookie(ctx, Cookies.Auth) + clearCookie(ctx, Cookies.CurrentApp) ctx.body = { message: "User logged out." } } From 29691637a652cf87cbe38d1a7ffab44f8c001893 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 7 Oct 2021 14:06:28 +0100 Subject: [PATCH 2/7] Fixing an issue with handlebars-helpers --- packages/string-templates/package.json | 2 +- packages/string-templates/yarn.lock | 14 ++++---------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/packages/string-templates/package.json b/packages/string-templates/package.json index ef9c4ae10d..5914389f5c 100644 --- a/packages/string-templates/package.json +++ b/packages/string-templates/package.json @@ -20,7 +20,7 @@ "manifest": "node ./scripts/gen-collection-info.js" }, "dependencies": { - "@budibase/handlebars-helpers": "^0.11.6", + "@budibase/handlebars-helpers": "^0.11.7", "dayjs": "^1.10.4", "handlebars": "^4.7.6", "handlebars-utils": "^1.0.6", diff --git a/packages/string-templates/yarn.lock b/packages/string-templates/yarn.lock index 86592f1cec..b5abb1403a 100644 --- a/packages/string-templates/yarn.lock +++ b/packages/string-templates/yarn.lock @@ -270,15 +270,14 @@ resolved "https://registry.yarnpkg.com/@bcoe/v8-coverage/-/v8-coverage-0.2.3.tgz#75a2e8b51cb758a7553d6804a5932d7aace75c39" integrity sha512-0hYQ8SB4Db5zvZB4axdMHGwEaQjkZzFjQiN9LVYvIFB2nSUHW9tYpxWriPrWDASIxiaXax83REcLxuSdnGPZtw== -"@budibase/handlebars-helpers@^0.11.6": - version "0.11.6" - resolved "https://registry.yarnpkg.com/@budibase/handlebars-helpers/-/handlebars-helpers-0.11.6.tgz#00e924a0142aac41c07e3d104607759635eec952" - integrity sha512-FLtCWkh0cNHC0/X6Pt5Xjmp4/r4tCpv5f5sP1JcZsaSKPyE5gpNu/+fqUxgDTzVS3PVo0KE6hdRPKVWvVqwPEw== +"@budibase/handlebars-helpers@^0.11.7": + version "0.11.7" + resolved "https://registry.yarnpkg.com/@budibase/handlebars-helpers/-/handlebars-helpers-0.11.7.tgz#8e5f9843d7dd10503e9f608555a96ccf4d836c46" + integrity sha512-PvGHAv22cWSFExs1kc0WglwsmCEUEOqWvSp6JCFZwtc3qAAr5yMfLK8WGVQ63ALvyzWZiyxF+yrlzeeaohCMJw== dependencies: array-sort "^1.0.0" define-property "^2.0.2" extend-shallow "^3.0.2" - "falsey" "^1.0.0" for-in "^1.0.2" get-object "^0.2.0" get-value "^3.0.1" @@ -1734,11 +1733,6 @@ extsprintf@^1.2.0: resolved "https://registry.yarnpkg.com/extsprintf/-/extsprintf-1.4.0.tgz#e2689f8f356fad62cca65a3a91c5df5f9551692f" integrity sha1-4mifjzVvrWLMplo6kcXfX5VRaS8= -"falsey@^1.0.0": - version "1.0.0" - resolved "https://registry.yarnpkg.com/falsey/-/falsey-1.0.0.tgz#71bdd775c24edad9f2f5c015ce8be24400bb5d7d" - integrity sha512-zMDNZ/Ipd8MY0+346CPvhzP1AsiVyNfTOayJza4reAIWf72xbkuFUDcJNxSAsQE1b9Bu0wijKb8Ngnh/a7fI5w== - fast-deep-equal@^3.1.1: version "3.1.3" resolved "https://registry.yarnpkg.com/fast-deep-equal/-/fast-deep-equal-3.1.3.tgz#3a7d56b559d6cbc3eb512325244e619a65c6c525" From 183b6e7b50b4996fb0d691244fe6214379b2044e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 7 Oct 2021 15:49:26 +0100 Subject: [PATCH 3/7] Fixing issue with user's being logged in and trying to access other tenants public apps, this work makes sure that users from other tenants will not be 403'd immediately (too aggressive) but instead they will have all other their RBAC roles revoked. --- packages/auth/src/db/constants.js | 15 +++++++ packages/auth/src/db/utils.js | 40 ++----------------- packages/auth/src/tenancy/tenancy.js | 33 ++++++++++++++- packages/server/src/api/controllers/auth.js | 10 ++++- .../server/src/api/routes/tests/user.spec.js | 1 - packages/server/src/middleware/currentapp.js | 35 +++++++++------- packages/server/src/utilities/global.js | 13 ++++-- .../server/src/utilities/workerRequests.js | 34 +--------------- 8 files changed, 91 insertions(+), 90 deletions(-) diff --git a/packages/auth/src/db/constants.js b/packages/auth/src/db/constants.js index ad4f6c9f66..477968975a 100644 --- a/packages/auth/src/db/constants.js +++ b/packages/auth/src/db/constants.js @@ -1,5 +1,20 @@ exports.SEPARATOR = "_" +const PRE_APP = "app" +const PRE_DEV = "dev" + +exports.DocumentTypes = { + USER: "us", + WORKSPACE: "workspace", + CONFIG: "config", + TEMPLATE: "template", + APP: PRE_APP, + DEV: PRE_DEV, + APP_DEV: `${PRE_APP}${exports.SEPARATOR}${PRE_DEV}`, + APP_METADATA: `${PRE_APP}${exports.SEPARATOR}metadata`, + ROLE: "role", +} + exports.StaticDatabases = { GLOBAL: { name: "global-db", diff --git a/packages/auth/src/db/utils.js b/packages/auth/src/db/utils.js index 09e2ff6314..fa162603e6 100644 --- a/packages/auth/src/db/utils.js +++ b/packages/auth/src/db/utils.js @@ -2,8 +2,8 @@ const { newid } = require("../hashing") const Replication = require("./Replication") const { DEFAULT_TENANT_ID } = require("../constants") const env = require("../environment") -const { StaticDatabases, SEPARATOR } = require("./constants") -const { getTenantId } = require("../tenancy") +const { StaticDatabases, SEPARATOR, DocumentTypes } = require("./constants") +const { getTenantId, getTenantIDFromAppID } = require("../tenancy") const fetch = require("node-fetch") const { getCouch } = require("./index") @@ -15,25 +15,11 @@ exports.ViewNames = { exports.StaticDatabases = StaticDatabases -const PRE_APP = "app" -const PRE_DEV = "dev" - -const DocumentTypes = { - USER: "us", - WORKSPACE: "workspace", - CONFIG: "config", - TEMPLATE: "template", - APP: PRE_APP, - DEV: PRE_DEV, - APP_DEV: `${PRE_APP}${SEPARATOR}${PRE_DEV}`, - APP_METADATA: `${PRE_APP}${SEPARATOR}metadata`, - ROLE: "role", -} - exports.DocumentTypes = DocumentTypes exports.APP_PREFIX = DocumentTypes.APP + SEPARATOR exports.APP_DEV = exports.APP_DEV_PREFIX = DocumentTypes.APP_DEV + SEPARATOR exports.SEPARATOR = SEPARATOR +exports.getTenantIDFromAppID = getTenantIDFromAppID /** * If creating DB allDocs/query params with only a single top level ID this can be used, this @@ -70,26 +56,6 @@ function isDevApp(app) { return exports.isDevAppID(app.appId) } -/** - * Given an app ID this will attempt to retrieve the tenant ID from it. - * @return {null|string} The tenant ID found within the app ID. - */ -exports.getTenantIDFromAppID = appId => { - if (!appId) { - return null - } - const split = appId.split(SEPARATOR) - const hasDev = split[1] === DocumentTypes.DEV - if ((hasDev && split.length === 3) || (!hasDev && split.length === 2)) { - return null - } - if (hasDev) { - return split[2] - } else { - return split[1] - } -} - /** * Generates a new workspace ID. * @returns {string} The new workspace ID which the workspace doc can be stored under. diff --git a/packages/auth/src/tenancy/tenancy.js b/packages/auth/src/tenancy/tenancy.js index 67dbfd5619..2cd05ea925 100644 --- a/packages/auth/src/tenancy/tenancy.js +++ b/packages/auth/src/tenancy/tenancy.js @@ -1,5 +1,5 @@ const { getDB } = require("../db") -const { SEPARATOR, StaticDatabases } = require("../db/constants") +const { SEPARATOR, StaticDatabases, DocumentTypes } = require("../db/constants") const { getTenantId, DEFAULT_TENANT_ID, isMultiTenant } = require("./context") const env = require("../environment") @@ -117,3 +117,34 @@ exports.getTenantUser = async identifier => { return null } } + +/** + * Given an app ID this will attempt to retrieve the tenant ID from it. + * @return {null|string} The tenant ID found within the app ID. + */ +exports.getTenantIDFromAppID = appId => { + if (!appId) { + return null + } + const split = appId.split(SEPARATOR) + const hasDev = split[1] === DocumentTypes.DEV + if ((hasDev && split.length === 3) || (!hasDev && split.length === 2)) { + return null + } + if (hasDev) { + return split[2] + } else { + return split[1] + } +} + +exports.isUserInAppTenant = (appId, user = null) => { + let userTenantId + if (user) { + userTenantId = user.tenantId || DEFAULT_TENANT_ID + } else { + userTenantId = getTenantId() + } + const tenantId = exports.getTenantIDFromAppID(appId) || DEFAULT_TENANT_ID + return tenantId === userTenantId +} diff --git a/packages/server/src/api/controllers/auth.js b/packages/server/src/api/controllers/auth.js index 5078218fc7..ac88599713 100644 --- a/packages/server/src/api/controllers/auth.js +++ b/packages/server/src/api/controllers/auth.js @@ -2,6 +2,7 @@ const CouchDB = require("../../db") const { outputProcessing } = require("../../utilities/rowProcessor") const { InternalTables } = require("../../db/utils") const { getFullUser } = require("../../utilities/users") +const { BUILTIN_ROLE_IDS } = require("@budibase/auth/roles") exports.fetchSelf = async ctx => { const appId = ctx.appId @@ -27,7 +28,14 @@ exports.fetchSelf = async ctx => { ...metadata, }) } catch (err) { - ctx.body = user + // user didn't exist in app, don't pretend they do + if (user.roleId === BUILTIN_ROLE_IDS.PUBLIC) { + ctx.body = {} + } + // user has a role of some sort, return them + else { + ctx.body = user + } } } else { ctx.body = user diff --git a/packages/server/src/api/routes/tests/user.spec.js b/packages/server/src/api/routes/tests/user.spec.js index 492ebfff5b..48117fbe4b 100644 --- a/packages/server/src/api/routes/tests/user.spec.js +++ b/packages/server/src/api/routes/tests/user.spec.js @@ -9,7 +9,6 @@ jest.mock("../../../utilities/workerRequests", () => ({ getGlobalSelf: jest.fn(() => { return {} }), - addAppRoleToUser: jest.fn(), deleteGlobalUser: jest.fn(), })) diff --git a/packages/server/src/middleware/currentapp.js b/packages/server/src/middleware/currentapp.js index e523850e1d..062c35a6ca 100644 --- a/packages/server/src/middleware/currentapp.js +++ b/packages/server/src/middleware/currentapp.js @@ -4,14 +4,12 @@ const { Cookies } = require("@budibase/auth").constants const { getRole } = require("@budibase/auth/roles") const { BUILTIN_ROLE_IDS } = require("@budibase/auth/roles") const { generateUserMetadataID } = require("../db/utils") -const { dbExists, getTenantIDFromAppID } = require("@budibase/auth/db") -const { getTenantId } = require("@budibase/auth/tenancy") +const { dbExists } = require("@budibase/auth/db") +const { isUserInAppTenant } = require("@budibase/auth/tenancy") const { getCachedSelf } = require("../utilities/global") const CouchDB = require("../db") const env = require("../environment") -const DEFAULT_TENANT_ID = "default" - module.exports = async (ctx, next) => { // try to get the appID from the request let requestAppId = getAppId(ctx) @@ -55,13 +53,21 @@ module.exports = async (ctx, next) => { return next() } - // If user and app tenant Ids do not match, 403 - if (env.MULTI_TENANCY && ctx.user) { - const userTenantId = getTenantId() - const tenantId = getTenantIDFromAppID(appId) || DEFAULT_TENANT_ID - if (tenantId !== userTenantId) { - ctx.throw(403, "Cannot access application.") - } + let noCookieSet = false + // if the user not in the right tenant then make sure they have no permissions + // need to judge this only based on the request app ID, + if ( + env.MULTI_TENANCY && + ctx.user && + requestAppId && + !isUserInAppTenant(requestAppId) + ) { + // don't error, simply remove the users rights (they are a public user) + delete ctx.user.builder + delete ctx.user.admin + delete ctx.user.roles + roleId = BUILTIN_ROLE_IDS.PUBLIC + noCookieSet = true } ctx.appId = appId @@ -78,9 +84,10 @@ module.exports = async (ctx, next) => { } } if ( - requestAppId !== appId || - appCookie == null || - appCookie.appId !== requestAppId + (requestAppId !== appId || + appCookie == null || + appCookie.appId !== requestAppId) && + !noCookieSet ) { setCookie(ctx, { appId }, Cookies.CurrentApp) } diff --git a/packages/server/src/utilities/global.js b/packages/server/src/utilities/global.js index 8f032df7f0..3637c11eea 100644 --- a/packages/server/src/utilities/global.js +++ b/packages/server/src/utilities/global.js @@ -6,13 +6,20 @@ const { BUILTIN_ROLE_IDS } = require("@budibase/auth/roles") const { getDeployedAppID } = require("@budibase/auth/db") const { getGlobalUserParams } = require("@budibase/auth/db") const { user: userCache } = require("@budibase/auth/cache") -const { getGlobalDB } = require("@budibase/auth/tenancy") +const { getGlobalDB, isUserInAppTenant } = require("@budibase/auth/tenancy") +const env = require("../environment") exports.updateAppRole = (appId, user) => { - if (!user.roles) { + if (!user || !user.roles) { + return user + } + // if in an multi-tenancy environment make sure roles are never updated + if (env.MULTI_TENANCY && !isUserInAppTenant(appId, user)) { + delete user.builder + delete user.admin + user.roleId = BUILTIN_ROLE_IDS.PUBLIC return user } - // always use the deployed app user.roleId = user.roles[getDeployedAppID(appId)] // if a role wasn't found then either set as admin (builder) or public (everyone else) diff --git a/packages/server/src/utilities/workerRequests.js b/packages/server/src/utilities/workerRequests.js index c07c93ba70..6a15e1b243 100644 --- a/packages/server/src/utilities/workerRequests.js +++ b/packages/server/src/utilities/workerRequests.js @@ -2,7 +2,7 @@ const fetch = require("node-fetch") const env = require("../environment") const { checkSlashesInUrl } = require("./index") const { getDeployedAppID } = require("@budibase/auth/db") -const { updateAppRole, getGlobalUser } = require("./global") +const { updateAppRole } = require("./global") const { Headers } = require("@budibase/auth/constants") const { getTenantId, isTenantIdSet } = require("@budibase/auth/tenancy") @@ -98,38 +98,6 @@ exports.getGlobalSelf = async (ctx, appId = null) => { return json } -exports.addAppRoleToUser = async (ctx, appId, roleId, userId = null) => { - let user, - endpoint, - body = {} - if (!userId) { - user = await exports.getGlobalSelf(ctx) - endpoint = `/api/global/users/self` - } else { - user = await getGlobalUser(appId, userId) - body._id = userId - endpoint = `/api/global/users` - } - body = { - ...body, - roles: { - ...user.roles, - [getDeployedAppID(appId)]: roleId, - }, - } - const response = await fetch( - checkSlashesInUrl(env.WORKER_URL + endpoint), - request(ctx, { - method: "POST", - body, - }) - ) - if (response.status !== 200) { - ctx.throw(400, "Unable to save self globally.") - } - return response.json() -} - exports.removeAppFromUserRoles = async (ctx, appId) => { const deployedAppId = getDeployedAppID(appId) const response = await fetch( From fab766adb6ecb7349273439b5f576e47d5d53073 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 7 Oct 2021 16:11:33 +0100 Subject: [PATCH 4/7] Adding functionality to check if ethereal is down, if it is don't fail the test. --- .../worker/src/api/routes/tests/realEmail.spec.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/worker/src/api/routes/tests/realEmail.spec.js b/packages/worker/src/api/routes/tests/realEmail.spec.js index 845e31d911..f61aee830f 100644 --- a/packages/worker/src/api/routes/tests/realEmail.spec.js +++ b/packages/worker/src/api/routes/tests/realEmail.spec.js @@ -34,8 +34,18 @@ describe("/api/global/email", () => { const testUrl = nodemailer.getTestMessageUrl(res.body) console.log(`${purpose} URL: ${testUrl}`) expect(testUrl).toBeDefined() - const response = await fetch(testUrl) - const text = await response.text() + let response, text + try { + response = await fetch(testUrl) + text = await response.text() + } catch (err) { + // ethereal hiccup, can't test right now + if (err.status > 400) { + return + } else { + throw err + } + } let toCheckFor switch (purpose) { case EmailTemplatePurpose.WELCOME: From efb09523a79c0a10662a066dfcefa5ac2cd3bb56 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 7 Oct 2021 16:14:00 +0100 Subject: [PATCH 5/7] Fixing a small issue with email test fix. --- .../src/api/routes/tests/realEmail.spec.js | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/worker/src/api/routes/tests/realEmail.spec.js b/packages/worker/src/api/routes/tests/realEmail.spec.js index f61aee830f..5ca2b54ebc 100644 --- a/packages/worker/src/api/routes/tests/realEmail.spec.js +++ b/packages/worker/src/api/routes/tests/realEmail.spec.js @@ -20,27 +20,27 @@ describe("/api/global/email", () => { await config.saveEtherealSmtpConfig() await config.saveSettingsConfig() const user = await config.getUser("test@test.com") - const res = await request - .post(`/api/global/email/send`) - .send({ - email: "test@test.com", - purpose, - userId: user._id, - }) - .set(config.defaultHeaders()) - .expect("Content-Type", /json/) - .expect(200) - expect(res.body.message).toBeDefined() - const testUrl = nodemailer.getTestMessageUrl(res.body) - console.log(`${purpose} URL: ${testUrl}`) - expect(testUrl).toBeDefined() let response, text try { + const res = await request + .post(`/api/global/email/send`) + .send({ + email: "test@test.com", + purpose, + userId: user._id, + }) + .set(config.defaultHeaders()) + .expect("Content-Type", /json/) + .expect(200) + expect(res.body.message).toBeDefined() + const testUrl = nodemailer.getTestMessageUrl(res.body) + console.log(`${purpose} URL: ${testUrl}`) + expect(testUrl).toBeDefined() response = await fetch(testUrl) text = await response.text() } catch (err) { // ethereal hiccup, can't test right now - if (err.status > 400) { + if (parseInt(err.status) > 400) { return } else { throw err From 52b44736c5434d3ee3c3040162048ecd277897c5 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 7 Oct 2021 16:35:11 +0100 Subject: [PATCH 6/7] Fixing another issue with ethereal test package. --- packages/worker/src/api/routes/tests/realEmail.spec.js | 8 ++++---- .../src/api/routes/tests/utilities/TestConfiguration.js | 3 +++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/worker/src/api/routes/tests/realEmail.spec.js b/packages/worker/src/api/routes/tests/realEmail.spec.js index 5ca2b54ebc..54238e9270 100644 --- a/packages/worker/src/api/routes/tests/realEmail.spec.js +++ b/packages/worker/src/api/routes/tests/realEmail.spec.js @@ -17,11 +17,11 @@ describe("/api/global/email", () => { afterAll(setup.afterAll) async function sendRealEmail(purpose) { - await config.saveEtherealSmtpConfig() - await config.saveSettingsConfig() - const user = await config.getUser("test@test.com") let response, text try { + await config.saveEtherealSmtpConfig() + await config.saveSettingsConfig() + const user = await config.getUser("test@test.com") const res = await request .post(`/api/global/email/send`) .send({ @@ -40,7 +40,7 @@ describe("/api/global/email", () => { text = await response.text() } catch (err) { // ethereal hiccup, can't test right now - if (parseInt(err.status) > 400) { + if (parseInt(err.status) >= 400) { return } else { throw err diff --git a/packages/worker/src/api/routes/tests/utilities/TestConfiguration.js b/packages/worker/src/api/routes/tests/utilities/TestConfiguration.js index 7f84de6b7d..9638e2a2a7 100644 --- a/packages/worker/src/api/routes/tests/utilities/TestConfiguration.js +++ b/packages/worker/src/api/routes/tests/utilities/TestConfiguration.js @@ -38,6 +38,9 @@ class TestConfiguration { request.request = { body: config, } + request.throw = (status, err) => { + throw { status, message: err } + } if (params) { request.params = params } From 4d04a3baf400bb392c4ec1440dd48e7f7c7574f9 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 7 Oct 2021 17:39:44 +0100 Subject: [PATCH 7/7] Adding a check for test environment to not clear app cookie on login as this breaks cypress. --- packages/worker/src/api/controllers/global/auth.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/worker/src/api/controllers/global/auth.js b/packages/worker/src/api/controllers/global/auth.js index 76768d1700..9d7df7ca37 100644 --- a/packages/worker/src/api/controllers/global/auth.js +++ b/packages/worker/src/api/controllers/global/auth.js @@ -55,7 +55,10 @@ async function authInternal(ctx, user, err = null, info = null) { // just store the user ID ctx.cookies.set(Cookies.Auth, user.token, config) // get rid of any app cookies on login - clearCookie(ctx, Cookies.CurrentApp) + // have to check test because this breaks cypress + if (!env.isTest()) { + clearCookie(ctx, Cookies.CurrentApp) + } } exports.authenticate = async (ctx, next) => {