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

@@ -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",

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.

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
+}

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

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(),
 }))
 

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)
   }

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)

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(