From 183b6e7b50b4996fb0d691244fe6214379b2044e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 7 Oct 2021 15:49:26 +0100 Subject: [PATCH] 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(