From 245c92482c11bfcec1984cd3113e611547378c53 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 ad9e5e4334aa78168e3c3baacd2091bc8b41510e 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 380d0e030ec21668f05c4e4c9abcfce66e33348c 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 64b98a6d18419b23d10128aaf1b9bb69ab5baec8 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 a4014c558036f1e8b317890e50566c7ae8f6445b 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 b267698caa5bb719cef4c8a67d64270e6bcf4e58 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 b87559917b59632c8e7e63d34d618b04c27c3fc8 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) => {