Fixing an issue discovered in #3385 - RBAC roles worked for applying lower levels of roles, but they didn't revoke access correctly, it would always fallback to the base permissions if higher permissions were set.

packages/auth/src/security/permissions.js

@@ -131,38 +131,14 @@ exports.getBuiltinPermissionByID = id => {
   return perms.find(perm => perm._id === id)
 }
 
-exports.doesHaveResourcePermission = (
-  permissions,
-  permLevel,
-  { resourceId, subResourceId }
-) => {
-  // set foundSub to not subResourceId, incase there is no subResource
-  let foundMain = false,
-    foundSub = false
-  for (let [resource, levels] of Object.entries(permissions)) {
-    if (resource === resourceId && levels.indexOf(permLevel) !== -1) {
-      foundMain = true
-    }
-    if (
-      subResourceId &&
-      resource === subResourceId &&
-      levels.indexOf(permLevel) !== -1
-    ) {
-      foundSub = true
-    }
-    // this will escape if foundMain only when no sub resource
-    if (foundMain && foundSub) {
-      break
-    }
-  }
-  return foundMain || foundSub
-}
-
-exports.doesHaveBasePermission = (permType, permLevel, permissionIds) => {
+exports.doesHaveBasePermission = (permType, permLevel, rolesHierarchy) => {
+  const basePermissions = [
+    ...new Set(rolesHierarchy.map(role => role.permissionId)),
+  ]
   const builtins = Object.values(BUILTIN_PERMISSIONS)
   let permissions = flatten(
     builtins
-      .filter(builtin => permissionIds.indexOf(builtin._id) !== -1)
+      .filter(builtin => basePermissions.indexOf(builtin._id) !== -1)
       .map(builtin => builtin.permissions)
   )
   for (let permission of permissions) {

packages/auth/src/security/roles.js

@@ -170,47 +170,18 @@ async function getAllUserRoles(appId, userRoleId) {
  * to determine if a user can access something that requires a specific role.
  * @param {string} appId The ID of the application from which roles should be obtained.
  * @param {string} userRoleId The user's role ID, this can be found in their access token.
+ * @param {object} opts Various options, such as whether to only retrieve the IDs (default true).
  * @returns {Promise<string[]>} returns an ordered array of the roles, with the first being their
  * highest level of access and the last being the lowest level.
  */
-exports.getUserRoleHierarchy = async (appId, userRoleId) => {
+exports.getUserRoleHierarchy = async (
+  appId,
+  userRoleId,
+  opts = { idOnly: true }
+) => {
   // special case, if they don't have a role then they are a public user
-  return (await getAllUserRoles(appId, userRoleId)).map(role => role._id)
-}
-
-/**
- * Get all of the user permissions which could be found across the role hierarchy
- * @param appId The ID of the application from which roles should be obtained.
- * @param userRoleId The user's role ID, this can be found in their access token.
- * @returns {Promise<{basePermissions: string[], permissions: Object}>} the base
- * permission IDs as well as any custom resource permissions.
- */
-exports.getUserPermissions = async (appId, userRoleId) => {
-  const rolesHierarchy = await getAllUserRoles(appId, userRoleId)
-  const basePermissions = [
-    ...new Set(rolesHierarchy.map(role => role.permissionId)),
-  ]
-  const permissions = {}
-  for (let role of rolesHierarchy) {
-    if (role.permissions) {
-      for (let [resource, levels] of Object.entries(role.permissions)) {
-        if (!permissions[resource]) {
-          permissions[resource] = []
-        }
-        const permsSet = new Set(permissions[resource])
-        if (Array.isArray(levels)) {
-          levels.forEach(level => permsSet.add(level))
-        } else {
-          permsSet.add(levels)
-        }
-        permissions[resource] = [...permsSet]
-      }
-    }
-  }
-  return {
-    basePermissions,
-    permissions,
-  }
+  const roles = await getAllUserRoles(appId, userRoleId)
+  return opts.idOnly ? roles.map(role => role._id) : roles
 }
 
 /**
@@ -246,6 +217,37 @@ exports.getAllRoles = async appId => {
   return roles
 }
 
+/**
+ * This retrieves the required role/
+ * @param appId
+ * @param permLevel
+ * @param resourceId
+ * @param subResourceId
+ * @return {Promise<{permissions}|Object>}
+ */
+exports.getRequiredResourceRole = async (
+  appId,
+  permLevel,
+  { resourceId, subResourceId }
+) => {
+  const roles = await exports.getAllRoles(appId)
+  let main, sub
+  for (let role of roles) {
+    // no permissions, ignore it
+    if (!role.permissions) {
+      continue
+    }
+    const mainRes = role.permissions[resourceId]
+    const subRes = role.permissions[subResourceId]
+    if (mainRes && mainRes.indexOf(permLevel) !== -1) {
+      main = role
+    } else if (subRes && subRes.indexOf(permLevel) !== -1) {
+      sub = role
+    }
+  }
+  return sub ? sub : main
+}
+
 class AccessController {
   constructor(appId) {
     this.appId = appId

packages/server/src/middleware/authorized.js

@@ -1,7 +1,10 @@
-const { getUserPermissions } = require("@budibase/auth/roles")
+const {
+  getUserRoleHierarchy,
+  getRequiredResourceRole,
+  BUILTIN_ROLE_IDS,
+} = require("@budibase/auth/roles")
 const {
   PermissionTypes,
-  doesHaveResourcePermission,
   doesHaveBasePermission,
 } = require("@budibase/auth/permissions")
 const builderMiddleware = require("./builder")
@@ -28,13 +31,7 @@ module.exports =
     await builderMiddleware(ctx, permType)
 
     const isAuthed = ctx.isAuthenticated
-    const { basePermissions, permissions } = await getUserPermissions(
-      ctx.appId,
-      ctx.roleId
-    )
-
     // builders for now have permission to do anything
-    // TODO: in future should consider separating permissions with an require("@budibase/auth").isClient check
     let isBuilder = ctx.user && ctx.user.builder && ctx.user.builder.global
     const isBuilderApi = permType === PermissionTypes.BUILDER
     if (isBuilder) {
@@ -43,20 +40,28 @@ module.exports =
       return ctx.throw(403, "Not Authorized")
     }
 
-    if (
-      hasResource(ctx) &&
-      doesHaveResourcePermission(permissions, permLevel, ctx)
-    ) {
-      return next()
+    // need to check this first, in-case public access, don't check authed until last
+    const roleId = ctx.roleId || BUILTIN_ROLE_IDS.PUBLIC
+    const hierarchy = await getUserRoleHierarchy(ctx.appId, roleId, {
+      idOnly: false,
+    })
+    const permError = "User does not have permission"
+    let requiredRole
+    if (hasResource(ctx)) {
+      requiredRole = await getRequiredResourceRole(ctx.appId, permLevel, ctx)
+    }
+    // check if we found a role, if not fallback to base permissions
+    if (requiredRole) {
+      const found = hierarchy.find(role => role._id === requiredRole._id)
+      return found ? next() : ctx.throw(403, permError)
+    } else if (!doesHaveBasePermission(permType, permLevel, hierarchy)) {
+      ctx.throw(403, permError)
     }
 
+    // if they are not authed, then anything using the authorized middleware will fail
     if (!isAuthed) {
       ctx.throw(403, "Session not authenticated")
     }
 
-    if (!doesHaveBasePermission(permType, permLevel, basePermissions)) {
-      ctx.throw(403, "User does not have permission")
-    }
-
     return next()
   }